Preserve parameters during interstitial redirect #2004
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
The InterstitialController show route is public so we should not use html_safe. For the redirect to work correctly, window.location.href needs unencoded ampersands.
corylown
requested changes
Feb 23, 2024
There was a problem hiding this comment.
@taylor-steve I believe the issue has been fixed upstream in Rails (I am still looking for what exactly got fixed between 7.1.1 and 7.1.3.2). I can duplicate the problem on requests-dev with this request https://requests-dev.stanford.edu/requests/new?item_id=in00000001563&origin=SAL3&origin_location=SAL3-STACKS with Rails 7.1.1 but 7.1.3.2 works fine. I think we should close this PR and open a new PR that updates Rails to latest version of 7.1 to fix #1934
Amendment: the issue appears to be in Rack and not Rails and the behavior persists if we update rack from 2 to 3.
|
Here's a draft PR to update Rails to 7.1.3: #2022 |
corylown
approved these changes
Feb 23, 2024
Merged
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.
Fixes #1934.
This seems to occur in both Rails 7.0 and 7.1:
https://app.honeybadger.io/projects/49963/faults/101591418/01HDEW0GXPZ486GB2K9862T6WT
https://app.honeybadger.io/projects/49963/faults/101723263/01HE0QQMPMVWHHTNX50KW496GQ
As suggested in #1934 it appears this was happening when going through
bounce_request_through_sso, when the interstitial show ERB was escaping the URL in the script tag. This led to the parameter rewriting behavior. All previous tests passed because none had parameters that would be escaped.As for safety, the only uses of interstitial use internally created
redirect_topaths. This is my first time dealing with this in rails, if there is a better way, I am happy to hear it.