-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: prevent multiple redirections to post logout url #3366
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3366 +/- ##
==========================================
- Coverage 76.96% 76.84% -0.13%
==========================================
Files 123 123
Lines 8962 8976 +14
==========================================
Hits 6898 6898
- Misses 1637 1651 +14
Partials 427 427
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Nice, this looks very good to me! Can you share a quick screen cast showing that the feature works? I'm not sure if we have e2e tests for this
a472a7a
to
aed876e
Compare
I'm not sure what kind of screen cast you want, it's a race condition so it's pretty hard to show it But I can show you the embed script works: in chrome devtools, network tab, add a throttling with at least 500ms of latency then check Then run this script in the console (which is the exact same script as in this MR, just with template properties set): var total = 0;
var redir = 'https://tinyurl.com/codingame-test-redirect';
var timeouts = [];
var redirected = false;
// Cancel all pending timeouts to avoid to call the frontchannel multiple times.
window.onbeforeunload = () => {
redirected = true;
for (var i=0; i<timeouts.length; i++) {
clearTimeout(timeouts[i]);
}
timeouts = [];
};
function setAndRegisterTimeout(fct, duration) {
if (redirected) {
return;
}
timeouts.push(setTimeout(fct, duration));
}
function redirect() {
window.location.replace(redir);
// In case replace failed try href
setAndRegisterTimeout(function () {
window.location.href = redir;
}, 250);
}
function done() {
total--;
if (total < 1) {
setAndRegisterTimeout(redirect, 500);
}
}
setAndRegisterTimeout(redirect, 7000); // redirect after 7 seconds if e.g. an iframe doesn't load and you'll get redirected with no issue Then remove the |
Awesome, thank you so much, this is really helpful! One last thing I need to ask of you is to sign the CLI, otherwise I am not authorized to merge the PR :( |
I already did it the first day, I don't know what it says that I didn't If I try to do it again, every field is disabled as it already knows I did it 🤷 Did I miss something? |
It looks like the CLA bot is not properly detecting your signature. To fix this, try the following:
Ensure that
Once that is done, you can force-push your changes (make sure to push to the correct remote and branch!):
|
aed876e
to
e6cbbe4
Compare
Ok thanks it seems ok now! |
Thank you so much for your contribution! :) |
Can I do anything to help it being merged? |
Hello @CGNonofr |
Prevent multiple redirections to post logout url
Related issue(s)
#3342
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments