varvet / pundit

Minimal authorization through OO design and pure Ruby classes
MIT License
8.22k stars 625 forks source link

`redirect_to(request.referrer || root_path)` on Rails 7 #727

Closed joemasilotti closed 2 years ago

joemasilotti commented 2 years ago

In the README and docs the following method is recommended to recover from unauthorized access.

def user_not_authorized
  flash[:alert] = I18n.t("pundit.errors.unauthorized")
  redirect_to(request.referrer || root_path)
end

I just upgraded to Rails 7 and this tried to redirect the user to google.com. I only know because I received an error that I should be using allow_other_host: true to the redirection!


So, my question is if request.referrer is the best approach here. Should instead it be recommended to `redirect_back(fallback_location: root_path)? Or something else? Or maybe I'm missing something entirely!

dgmstuart commented 2 years ago

Hi - thanks for the question.

I'm not sure I understand your use-case: why would you redirect the user to an external site (like google.com) when authorization fails? This will mean that they never see the flash message.

The expected use case is that you redirect the user back to the page from which they tried to log in, and show the flash message there. If the documentation is unclear, maybe we can make it clearer?

redirect_to request.referer is just a wrapper around redirect_to request.referer, so you'd still have to pass allow_other_host: true

In conclusion: yes we should probably be suggesting redirect_back(fallback_location: root_path) in the README, but if you want to redirect to an external site, you'll need to explicitly allow that.

joemasilotti commented 2 years ago

I'm not sure I understand your use-case: why would you redirect the user to an external site (like google.com) when authorization fails? This will mean that they never see the flash message.

Ha, agreed! I 100% do not want to redirect them back to an external site. Which is why I was thinking that the redirect_back(fallback_location: root_path) is a better solution. Happy to open a PR if that's what you'd like in the README!

dgmstuart commented 2 years ago

@joemasilotti still not sure we're on the same page:

These two are entirely identical in terms of what they do, right?

redirect_to(request.referrer || root_path)
redirect_back(fallback_location: root_path)

If you look at the source code, it's just syntactic sugar: the implementation of the second line is basically the first line?

I do think the second line is clearer and what we should say in the README - I just want to be clear between us that it's exactly the same behaviour.

joemasilotti commented 2 years ago

Ah, got it. I thought redirect_back did something fancy.

So, even though this issue isn't related to pundit, do you have any advice on how to not redirect if the referrer is external?

dgmstuart commented 2 years ago

@joemasilotti I'm not sure I understand the question - isn't the ActionController::Redirecting::UnsafeRedirectError (which you described in your original post) the mechanism which prevents you from redirecting to external referrers?

joemasilotti commented 2 years ago

It is! But it raises an exception. I'd rather not raise the error and redirect to root_path in that case.

Or is the recommended approach to rescue ActionController::Redirecting::UnsafeRedirectError?

Burgestrand commented 2 years ago

It is! But it raises an exception. I'd rather not raise the error and redirect to root_path in that case.

Or is the recommended approach to rescue ActionController::Redirecting::UnsafeRedirectError?

It looks like it from the commit in rails, and I think that's the approach I'd take too.

joemasilotti commented 2 years ago

Ah, perfect! Thanks so much for the help with this.