Skip to content
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

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

CGNonofr
Copy link
Contributor

Prevent multiple redirections to post logout url

Related issue(s)

#3342

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    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.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2022

CLA assistant check
All committers have signed the CLA.

@CGNonofr CGNonofr changed the title Prevent multiple redirections to post logout url fix: prevent multiple redirections to post logout url Nov 16, 2022
@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #3366 (5abb516) into master (f9f0337) will decrease coverage by 0.12%.
The diff coverage is 5.88%.

❗ Current head 5abb516 differs from pull request most recent head e6cbbe4. Consider uploading reports for the commit e6cbbe4 to get more accurate results

@@            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              
Impacted Files Coverage Δ
oauth2/handler.go 66.50% <0.00%> (-2.02%) ⬇️
cmd/cmd_introspect_token.go 78.94% <100.00%> (ø)
health/doc.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@aeneasr aeneasr left a 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

@CGNonofr CGNonofr force-pushed the fix-logout-page-redirection branch 2 times, most recently from a472a7a to aed876e Compare November 16, 2022 13:12
@CGNonofr
Copy link
Contributor Author

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

image

then check Disable cache

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 onbeforeunload part and try again, you'll notice:

image

@aeneasr
Copy link
Member

aeneasr commented Nov 21, 2022

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 :(

@CGNonofr
Copy link
Contributor Author

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?

@aeneasr
Copy link
Member

aeneasr commented Nov 21, 2022

It looks like the CLA bot is not properly detecting your signature. To fix this, try the following:

$ git commit  --amend --author="Author Name <email@address.com>"

Ensure that Author Name is replaced with your GitHub username (e.g. aeneasr) and that the email address is replaced with the email address you have set up in GitHub (e.g. 3372410+aeneasr@users.noreply.github.com):

$ git commit  --amend --author="aeneasr <3372410+aeneasr@users.noreply.github.com>"

Once that is done, you can force-push your changes (make sure to push to the correct remote and branch!):

$ git push --force <remote> HEAD:<branch>

@CGNonofr
Copy link
Contributor Author

Ok thanks it seems ok now!

@aeneasr
Copy link
Member

aeneasr commented Nov 21, 2022

Thank you so much for your contribution! :)

@CGNonofr
Copy link
Contributor Author

CGNonofr commented Dec 5, 2022

Can I do anything to help it being merged?

@aeneasr aeneasr merged commit 50666b9 into ory:master Dec 5, 2022
@vinckr
Copy link
Member

vinckr commented Dec 5, 2022

Hello @CGNonofr
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

harnash pushed a commit to Wikia/ory-hydra that referenced this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants