rails icon indicating copy to clipboard operation
rails copied to clipboard

`ActionDispatch::Routing::RoutesProxy#merge_script_names` throws exceptions given certain arguments

Open kenaniah opened this issue 1 year ago • 2 comments

Steps to reproduce

  1. Mount an engine in the routes file inside of a namespace (or sub path)
  2. Attempt to call a top-level route from the engine's route helper
  3. A runtime exception occurs -- NoMethodError: undefined method `join' for nil:NilClass

Test Case

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"
require "rails"

class BugTest < Minitest::Test
  def test_stuff
    proxy = ActionDispatch::Routing::RoutesProxy.new(nil, nil, nil)
    assert_equal "/foo/bar", proxy.send(:merge_script_names, "", "/foo/bar") #=> NoMethodError: undefined method `join' for nil:NilClass
  end
end

Expected behavior

The engine's route helper (which is an instance of ActionDispatch::Routing::RoutesProxy) should return the requested url or path without throwing an exception.

Actual behavior

https://github.com/rails/rails/blob/18707ab17fa492eb25ad2e8f9818a320dc20b823/actionpack/lib/action_dispatch/routing/routes_proxy.rb#L58-L66

  • #merge_script_names is invoked with two arguments, "" and "/foo/bar"
  • resolved_parts evaluates to 2
  • previous_parts evaluates to 0
  • context_parts evaluates to -1 (!!!)
  • previous_script_name.split("/") evaluates to an empty array
  • Array#slice(0, -1) is called, which evaluates to nil (but an array was expected)
  • #join is invoked on an instance of NilClass, because #slice did not return an array due to the negative value of the second argument

Suggested Remediation

context_parts should probably be guarded to ensure a negative value isn't generated -- maybe something like:

context_parts = [0, previous_parts - resolved_parts + 1].max

System configuration

Rails version: 7.0.3

Ruby version: ruby 3.0.4p208 (2022-04-12 revision 3fa771dded) [arm64-darwin21]

kenaniah avatar Jul 09 '22 07:07 kenaniah

Hi @kenaniah , thanks for opening this issue!

It looks like your reproduction script is using some private Rails APIs like ActionDispatch::Routing::RoutesProxy. I'm not that familiar with the internals of ActionDispatch so it would be nice to get a reproduction script using public APIs from a user's standpoint.

I started a reproduction script here, but I wasn't sure what you meant in step 2 of the steps to reproduce

  1. Attempt to call a top-level route from the engine's route helper

Are you able to expand this script to show the issue?

Reproduction script
# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "action_controller/railtie"

module Blorgh
  class Engine < ::Rails::Engine
    # Patch to make repro script work
    def self.find_root(from)
      Pathname.new(from).expand_path
    end

    isolate_namespace Blorgh
    routes.draw do
      get "/" => "posts#index"
    end
  end

  class PostsController < ActionController::Base
    def index
      render plain: "Hello, world!"
    end
  end
end


class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    namespace :example do
      mount Blorgh::Engine => "/blorgh"
    end
    get "/" => "test#index"
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    render plain: "Home"
  end
end

require "minitest/autorun"
require "rack/test"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_returns_success
    get "/example/blorgh"
    assert last_response.ok?
  end

  private
    def app
      Rails.application
    end
end

luanzeba avatar Jul 12 '22 16:07 luanzeba

@luanzeba I looked into creating a test case using just the public APIs, but frankly that's kind of irrelevant here. This report comes from a backtrace from one of my Rails 7 applications that happens to have a wild and wonderful routing tree (using multiple engines for subroutes that are dynamically generated via meta-programming).

Reducing the test case in the public API would depend on understanding how script names are inferred, which is not something I was able to figure out in the time I had allocated to this bug report. Regardless, my initial report should be sufficient for demonstrating the issue.

kenaniah avatar Jul 29 '22 19:07 kenaniah

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

rails-bot[bot] avatar Oct 27 '22 19:10 rails-bot[bot]