Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails 7: allow redirections to other/external hosts after logout. #5462

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ammancilla
Copy link

@ammancilla ammancilla commented Jan 27, 2022

Context

When using different authentication strategies, devise_saml_autheticatable in my case, it's common to redirect the user to an external host after logout.

Rails 7 requires to explicitly pass allow_other_host: true to redirect_to to allow such redirections. Otherwise, an ugly exception is raised.

  • Devise 4.8.1
  • Rails 7.0.1

Expected behaviour.

When using devise with Rails 7 and setting the after_sign_out_path_for to an external host, the user is sucessfully redirected.

Current behaviour.

When using devise with Rails 7 and setting the after_sign_out_path_for to an external host, a ActionController::Redirecting::UnsafeRedirectError is raised.

@ammancilla ammancilla changed the title Allows redirections to other/external hosts after logout, for Rails 7 Allow redirections to other/external hosts after logout, for Rails 7 Jan 27, 2022

def redirect_to_after_sign_out_path
redirect_to after_sign_out_path_for(resource_name), allow_other_host: true
end
Copy link
Author

@ammancilla ammancilla Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about what's the best place to write tests for this. If someone can point me in a direction I'll be happy to add tests 😃.

Copy link
Author

@ammancilla ammancilla Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about the proposed solution a bit longer, I wonder if it actually is the best one 😅.

Different use cases might require different options to be passed to the redirect_to. It's probably not possible to cover all such cases by hard coding options here. So, the question that comes to my mind now is: What's the best way to override the options passed to the redirect_to? For example, to also change the redirection status code etc.

  • Override the destroy action in the controller. Then, having full control over the redirection (already possible).

  • Provide a after_sign_out_redirect_options_for method or similar where users can define the custom options for the redirection?

  • Other...

Copy link

@allanclloyds allanclloyds Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or set the default in Devise in the meantime to allow_other_host: true and put a warning in a comment in after_sign_up_path_for about overriding it with a user provided URLs? You have to go out of your way to override the method, so it doesn't seem like a less secure default.

@daslicious
Copy link

There are other places than logout that redirect_to would need allow_other_host: true.

For instance, if I'm on me.site.com and load site.com/sign_in, I'll get redirected back to me.site.com which would raise the exception here...
https://github.com/heartcombo/devise/blob/v4.8.1/app/controllers/devise_controller.rb#L116

I don't know how many instances like this there are

@ammancilla ammancilla changed the title Allow redirections to other/external hosts after logout, for Rails 7 Rails 7: allow redirections to other/external hosts after logout. Feb 25, 2022
@kreintjes
Copy link

kreintjes commented Mar 14, 2022

There are other places than logout that redirect_to would need allow_other_host: true.

Yes indeed. For instance we need to be able to set it for the registrations_controller: https://github.com/heartcombo/devise/blob/v4.8.1/app/controllers/devise/registrations_controller.rb#L25-L29

@nehagupta93
Copy link

nehagupta93 commented May 16, 2022

Any chance this will be looked into soon? The same issue happens if after_sign_in_path_for is overridden to return an external host.
For now I'm making do by overriding Devise::SessionsController#create.

@trueinviso
Copy link

trueinviso commented Nov 26, 2022

I'm also having this issue because I want to redirect to an external Stripe checkout page after a user signs up. For now I am overriding the respond_with method:

def respond_with(resource, location: nil)
  if !resource.persisted? || resource.active_for_authentication?
    super
  else
    co_session = Stripe::Checkout::Session.create(...)
    redirect_to co_session.url, allow_other_host: true
  end
end

@seanarnold
Copy link

Seems like the best way to approach this is add a alow_directs_to_other_hosts configuration under Devise.configure, and then use that everywhere redirect_to is currently used.

Should be relatively straightforward I think

@richardonrails
Copy link

richardonrails commented Dec 9, 2022

+1 on improvements here, my login broke after a Rails 7 upgrade because I am routing users to a per-user custom external URLs in after_sign_in_path_for

@sealabcore
Copy link

+1 to improvements here, for now we will have to set this new config to false to have our app to still work with devise.

@fonji
Copy link

fonji commented May 2, 2023

+1 Haven't found another way than setting action_controller.raise_on_open_redirects = false, more than a year later

Copy link

@fashberg fashberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fa

@ivan-kocienski-gfsc
Copy link

I just ran in to this problem in project and fixed it across the entirety of devise with

class DeviseController
  # monkey patching devise so it can handle the new default behaviour in rails 7
  #   redirects

  original_redirect_to = instance_method(:redirect_to)
  define_method(:redirect_to) do |options, response_options = {}|
    if options.is_a?(Hash)
      options[:allow_other_host] = true unless options.key?(:allow_other_host)

    elsif response_options.is_a?(Hash)
      response_options[:allow_other_host] = true unless response_options.key?(:allow_other_host)
    end

    original_redirect_to
      .bind_call(self, options, response_options)
  end
end

But please can this be fixed properly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet