Authentication in routes breaks URL helpers

[peterzhu2118]

Environment

• Ruby 2.5.1
• Rails 5.2.2
• Devise 4.5.0

Current behavior

In my routes.rb, I have a snippet that looks like this:

authenticate :user, ->(_u) { current_user&.admin? } do
  mount Sidekiq::Web => '/sidekiq'
end

In a test, I have a snippet that looks like this:

p new_user_session_path
get '/sidekiq'
p new_user_session_path

However, the two print statements gives an output like this:

"/users/sign_in"
"/sidekiq/users/sign_in"

If I remove the authenticate line in routes.rb, I get the expected output.

Expected behavior

I expect the path to never change, i.e. output should look like this:

"/users/sign_in"
"/users/sign_in"

[peterzhu2118]

I've created a project to demo this. The test that demonstrates this can be found here.

[feliperenan]

Hi @peterzhu2118, thanks for the issue report.

It's not a Devise issue because Rails builds this path internally. I found this issue https://github.com/rails/rails/issues/34452 that should address it.

Thanks.

[feliperenan]

Looking again to the issue that I've mentioned, I'm not sure if solving that issue will fix this one.

Digging a little into the Rails code, we can see where it happens:

options = {
  :host=>"www.example.com",
  :protocol=>"http",
  :controller=>"devise/sessions",
  :action=>"new",
  :path=>"/users/sign_in",
  :script_name=>""
}

url_strategy.call options
=> "/users/sign_in"

url_strategy.call options.merge(script_name: "sidekiq")
=> "sidekiq/users/sign_in"

Basically, when there is a script_name within options, the path will be concatenated with the string 'Sidekiq' here. I don't know whether if it's working as intended or not, but it's definitely a Rails issue.

[peterzhu2118]

Hey @feliperenan,

Thanks for looking into this! Since this isn't a devise issue and since that issue you referenced in Rails doesn't fix this issue, should I raise this issue to the Rails repo?

[feliperenan]

I think so. I just recommend you search for a similar issue before, maybe someone else has already faced this issue in the past.

[peterzhu2118]

@feliperenan I dug a little deeper into this, it seems the problem is devise's authenticate method in the routes. The problem disappears if I change:

authenticate :user, ->(_u) { false } do
  mount Sidekiq::Web => '/sidekiq'
end

in routes.rb to:

mount Sidekiq::Web => '/sidekiq'

So I wonder if something is up with the Devise constraints?

[feliperenan]

@peterzhu2118 thanks for pointing this out. I will reopen this issue then since in fact it maybe is a Devise issue. Also, I have more findings to share.

When we do a request to another path after we did one to the /sidekiq, the path helpers start to return the correct path again.

get '/sidekiq'
=> 302
new_user_session_path
=> "/sidekiq/users/sign_in"

get '/'
=> 200
new_user_session_path
=> "/users/sign_in"

The issue doesn't happen when the user is authenticated.

user = User.create(email: "[email protected]", password: "123456")

sign_in user

get '/sidekiq'
=> 200
new_user_session_path
=> "/users/sign_in"

Maybe the bug is happening after the redirect? 🤔

[peterzhu2118]

@feliperenan That's a really interesting observation. Following the redirect seems to be a temporary workaround:

p "new_user_session_path before request: #{new_user_session_path}"
p "root_path before request: #{root_path}"

get '/sidekiq'

follow_redirect!

p "new_user_session_path after request: #{new_user_session_path}"
p "root_path after request: #{root_path}"

With output:

"new_user_session_path before request: /users/sign_in"
"root_path before request: /"
"new_user_session_path after request: /users/sign_in"
"root_path after request: /"

However, adding follow_redirect! is only a temporary workaround and the paths shouldn't change regardless of whether the follow_redirect! is there or not.

[feliperenan]

I've more finds to share 🙈.

When we do a request to "/sidekiq" that needs authentication, Devise redirects the user to the /users/sign_in with the FailureApp that behaves as a controller. Once the request is finished, the ActionDispatch put the FailureApp into the session controller instance. Also, notice that in this request process, ActionDispatch sets the @url_options to nil.

So, when we ask for a path using some path helper, ActionDispatch uses the session cached through memoization to build the correct path. When url_options method is called, as @url_options is nil, ActionDispatch is going to merge the controller#url_options with the session#url_options.

As I said before, FailureApp behaves as a controller, so it has the request information. When we inspect this controller asking for a path after a request for "/sidekiq", we got:

From: /Users/feliperenan/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/testing/integration.rb @ line 126 ActionDispatch::Integration::Session#url_options:

    123: def url_options
    124:   @url_options ||= default_url_options.dup.tap do |url_options|
    125:     require 'pry'; binding.pry
 => 126:     url_options.reverse_merge!(controller.url_options) if controller
    127:
    128:     if @app.respond_to?(:routes)
    129:       url_options.reverse_merge!(@app.routes.default_url_options)
    130:     end
    131:
    132:     url_options.reverse_merge!(host: host, protocol: https? ? "https" : "http")
    133:   end
    134: end

[1] pry(#<#<Class:0x00007fc99adab108>>)> controller.class.name
=> "Devise::FailureApp"
[2] pry(#<#<Class:0x00007fc99adab108>>)> controller.url_options
=> {:host=>"www.example.com", :port=>nil, :protocol=>"http://", :_recall=>{}, :script_name=>"/sidekiq"}

This controller#url_options isn't a FailureApp method. It's included by ActionController::UrlFor. https://github.com/plataformatec/devise/blob/20e299bce0307d79895b05b13530f3c74a0ca0e0/lib/devise/failure_app.rb#L11

Then, as the script_name is there, the path will be concatenated with it as I said in this previous comment https://github.com/plataformatec/devise/issues/5019#issuecomment-463264252

It doesn't happen in a get request that give 200 or when we use a follow_redirect! (that makes another get under the hood), because it makes another request that cleans up the @url_options and sets another controller into sessions instance.

From: /Users/feliperenan/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/testing/integration.rb @ line 126 ActionDispatch::Integration::Session#url_options:

    123: def url_options
    124:   @url_options ||= default_url_options.dup.tap do |url_options|
    125:     require 'pry'; binding.pry
 => 126:     url_options.reverse_merge!(controller.url_options) if controller
    127:
    128:     if @app.respond_to?(:routes)
    129:       url_options.reverse_merge!(@app.routes.default_url_options)
    130:     end
    131:
    132:     url_options.reverse_merge!(host: host, protocol: https? ? "https" : "http")
    133:   end
    134: end

[1] pry(#<#<Class:0x00007fc99b43c7b0>>)> controller.class.name
=> "DemoController"
[2] pry(#<#<Class:0x00007fc99b43c7b0>>)> controller.url_options
=> {:host=>"www.example.com", :port=>nil, :protocol=>"http://", :_recall=>{:controller=>"demo", :action=>"index"}, :script_name=>""}

Now that we have all this information, I'm not sure if it's a Devise issue. Maybe we should do something in the FailureApp, but I don't know what. Maybe the issue is with ActionDispatch since they memoize the session and relies on it to build paths in the test environment.

I'm open for suggestions on how to proceed with this issue.

[peterzhu2118]

@feliperenan Wow that's a really detailed report! Thanks for taking the time to look into this! I'll try to do more digging into this to determine the source of the issue (in both devise and in Rails). I'll post an update here if I find anything interesting.

[tegon]

@feliperenan Great work on this!

My 2 cents here is that since this only happens in the test environment - e.g. because the session is memoized - I believe there's not much we can do. I'm not even sure if this should be addressed in Rails or not in some way. I'd go with following the redirect in the test in order to make it more close to a real request/response flow.

[feliperenan]

@tegon I was thinking about the same thing. As far as I understood, a new session is created on every single test, so developers could use follow_redirect! approach only in the test that they want to use a path returned by the path helper after a Devise redirect in this specific case.

If we don't do a patch for it, we may need to document this behavior at some place 🤔.

[glennfu]

Just wanted to follow up and mention that as of Rails 5.2.5 and Devise 4.7.1 this is still an active bug.

[carlosantoniodasilva]

I've come across a similar problem with mounting sidekiq myself, and had to basically make sure script_name is cleared in devise redirect URL inside the failure app to ensure the redirect works properly... that was a bit custom to our use case though, I'm not sure that's the right solution for everyon, I'll have to dig some more here to investigate this (as well as #5076 which seems to be related).

[kimasenbeck]

I'm also running into this issue. Rails 6.0.3.6, Devise 4.7. @carlosantoniodasilva is this something that you're still looking into?