Rails 7 router can raise `NoMethodError (undefined method `names' for nil:NilClass)`

[engwan]

Steps to reproduce

When having a route with a regex constraint with a negative lookahead, we get exceptions when matching some routes.

I simplified the regex and created a minimal example below:

# frozen_string_literal: true

require "bundler/inline"

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

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

  # Activate the gem you are reporting the issue against.
  gem "rails", "~> 7.0.0"
end

require "rack/test"
require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.session_store :cookie_store, key: "cookie_store_key"
  secrets.secret_key_base = "secret_key_base"

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

  routes.draw do
    get '*namespace_id/:project_id/bar', to: 'test#index', constraints: { namespace_id: /(?!api\/)[a-zA-Z0-9_\\]+/, project_id: /[a-zA-Z0-9]+/ }

    # This version works:
    #get '*namespace_id/:project_id/bar', to: 'test#index', constraints: lambda { |req| req.params[:namespace_id].match?(/\A(?!api\/)[a-zA-Z0-9_\\]+\z/) && req.params[:project_id].match?(/\A[a-zA-Z0-9]+\z/) }

    get '/api/foo/bar', to: 'test#index'
  end
end

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

  def index
    render plain: "Home"
  end
end

require "minitest/autorun"

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

  def test_returns_success
    get "/api/foo/bar"
    assert last_response.ok?
  end

  private
    def app
      Rails.application
    end
end

Expected behavior

It should match the second route and render the page

Actual behavior

It raises an exception:

Started GET "/api/foo/bar/" for 127.0.0.1 at 2023-02-03 14:04:19 +0800

NoMethodError (undefined method `names' for nil:NilClass):

actionpack (7.0.4.2) lib/action_dispatch/journey/router.rb:126:in `block in find_routes'
actionpack (7.0.4.2) lib/action_dispatch/journey/router.rb:123:in `map!'
actionpack (7.0.4.2) lib/action_dispatch/journey/router.rb:123:in `find_routes'
actionpack (7.0.4.2) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (7.0.4.2) lib/action_dispatch/routing/route_set.rb:852:in `call'

System configuration

Rails version: 7.0.4.2

Ruby version: 3.0.5

[engwan]

@theojulienne I suspect this is due to 16a8088 because it works fine in Rails 6. And when I change the constraint to a lambda in Rails 7, it's also working fine.

I tried patching this locally to ignore the routes when match_data is nil:

diff --git a/router.rb b/router.rb
index 27671b6..19f2491 100644
--- a/router.rb
+++ b/router.rb
@@ -123,12 +123,15 @@ module ActionDispatch
           routes.map! { |r|
             match_data = r.path.match(path_info)
             path_parameters = {}
+
+            next if match_data.nil?
+
             match_data.names.each_with_index { |name, i|
               val = match_data[i + 1]
               path_parameters[name.to_sym] = Utils.unescape_uri(val) if val
             }
             [match_data, path_parameters, r]
-          }
+          }.compact!
+          routes
         end

         def match_head_routes(routes, req)

It works but I'm not sure if it's the correct fix. Should the FSM not match this route?

[pedro108]

Hi @engwan ! I was able to add a regression test to replicate your issue, in actionpack/test/controller/routing_test.rb :

def test_route_constraints_with_negative_lookahead_regex_not_matched_must_not_error
  set.draw do
    get "*namespace_id/:project_id/bar",
      to: "pages#show",
      constraints: {
        namespace_id: /(?!api\/)[a-zA-Z0-9_\\]+/,
        project_id: /[a-zA-Z0-9]+/
      }

    get "/api/foo/bar", to: "pages#index"
  end

  params = set.recognize_path("/api/foo/bar", method: :get)
  assert_equal("pages", params[:controller])
  assert_equal("index", params[:action])
end

I am working through why this route does not get any matches and will get back if I find some results

[pedro108]

From what I understood, the optimized implementation of transition_table, when filtering for acceptable routes, does not take into account RegEx lookahead syntax, and treats /(?!api\/)[a-zA-Z0-9_\\]+/ as /(api\/)[a-zA-Z0-9_\\]+/, thus considering the first unacceptable route as acceptable to match /api/foo/bar. That's why it is considered a valid route when executing the routes.map! part, which actually performs a RegEx match, causing the bug.

I believe that it would be best to fix Journey's route filtering algorithm to consider lookahead and similar RegEx syntax instead of just doing nil checks on the last part 🤔 . But I don't think that I can do it by myself 😅

[jesse-shopify]

@pedro108 I came to the same conclusions and I'm working on a PR now.

[jesse-shopify]

As a new contributor, I'm feeling this is an area I should not be making changes to. :(

[engwan]

I believe that it would be best to fix Journey's route filtering algorithm to consider lookahead and similar RegEx syntax instead of just doing nil checks on the last part

Yes, I agree.

Another possible fix is to exclude regexes with lookarounds from the transition table. Effectively reverting them to Rails 6 behavior. This is simpler and similar to doing the nil checks but it's still better if the transition table could handle these lookarounds.

[pedro108]

I believe that it would be best to fix Journey's route filtering algorithm to consider lookahead and similar RegEx syntax instead of just doing nil checks on the last part

Yes, I agree.

Another possible fix is to exclude regexes with lookarounds from the transition table. Effectively reverting them to Rails 6 behavior. This is simpler and similar to doing the nil checks but it's still better if the transition table could handle these lookarounds.

That's a great middle of the ground approach! Let me give it a try 🚀

[rails-bot]

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]

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.