-
Notifications
You must be signed in to change notification settings - Fork 179
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
[Core] fix timeout redirect location #9534
Conversation
@maximemulder @driusan I'd appreciate a quick look at this one |
@kongtiaowang not sure why the tests are failing here, is the branch broken ? |
@ridz1208 A GitHub update broke CI during the new year, the fix has been committed to main but not 26, so the branch is indeed broken. @kongtiaowang can you push the fix to 26 as well ? |
@maximemulder I agree someone else should look into it but on your end can you just see, on 26, the redirect works for you? I just want to know if its still working on your environment but not on mine |
@ridz1208 @maximemulder #9536 will fix the CI failure issue. |
@kongtiaowang I think this PR was assigned to you for testing. Best way I found to test it is to login, go to a page with a complex URL, copy the URL, logout, try to go to the URL again (as if you timed out), you get a 403 error, then choose the option to try signing in and see if it redirects you to your original URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Brief summary of changes
The feature introduced in #9041 seems to be broken on 26. The changes I'm proposing are the hack that i'm temporarily using on CBIG but there might be a better way of doing it.
One cause of the bug might actually be another PR submitted by maxime and merged #9055. not sure if the changes in that PR broke the chnages in #9041
Below are a couple examples of the existing redirect for URLs
Testing instructions (if applicable)
Link(s) to related issue(s)