Enable exceptions on open redirects #469
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Raising an exception on open redirects is the default behaviour in Rails 7[1]. We disabled it in ad01e1c (15 Mar 2022) with a comment saying: > allow open redirects, necessary for SSO My guess is that this was required for the `/auth/gds/sign_out` path provided by gds-sso as that path redirects to Signon (i.e. a different host). The gds-sso Gem was updated in PR 250[2] (merged on 29 Mar 2022) to explicitly allow redirecting to a different host in `/auth/gds/sign_out`. I believe that change in gds-sso means we can safely remove this line. I've checked that signing in and out continues to work as expected with this line removed. I think the only risk of something breaking with this change is if `session["redirect_to"]` somehow contained a URL and not just the requested path that's stored in `GDS::SSO::FailureApp#store_location`[3]. I don't think we'd expect that to happen so it seems reasonable to raise an UnsafeRedirectError if it does. [1]: https://guides.rubyonrails.org/configuring.html#config-action-controller-raise-on-open-redirects [2]: alphagov/gds-sso#250 [3]: https://github.com/alphagov/gds-sso/blob/v18.1.0/lib/gds-sso/failure_app.rb#L47
|
Thanks @chrislo! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Raising an exception on open redirects is the default behaviour in Rails 7. We disabled it in ad01e1c (15 Mar 2022) with a comment saying:
My guess is that this was required for the
/auth/gds/sign_outpath provided by gds-sso as that path redirects to Signon (i.e. a different host). The gds-sso Gem was updated in PR 250 (merged on 29 Mar 2022) to explicitly allow redirecting to a different host in/auth/gds/sign_out. I believe that change in gds-sso means we can safely remove this line.I've checked that signing in and out continues to work as expected with this line removed.
I think the only risk of something breaking with this change is if
session["redirect_to"]somehow contained a URL and not just the requested path that's stored inGDS::SSO::FailureApp#store_location. I don't think we'd expect that to happen so it seems reasonable to raise an UnsafeRedirectError if it does.