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(error-redirect): use query-safe fragment as recommended in the latest OAuth 2 security best practices #1670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aomader
Copy link

@aomader aomader commented Dec 10, 2024

This PR adjusts the fragment that is added to error redirect responses, which is supposed to prevent browsers from reattaching existing ones.

The previous fragment #_=_ is an invalid query selector and can not be used as is on the receiving end, hence it has been adjusted to the recommended #_.

Additionally, I adjusted the link to the respective latest active internet draft.

This looks like a minor thing, but one can run into issues caused by the browsers behavior of carrying along fragments.

@josephdecock
Copy link
Member

josephdecock commented Dec 13, 2024

The BCP uses #=_ as a non-normative example, so I don't think it is saying that we need to change this. I'm surprised that this is causing a problem for you, as normally authorize redirects are going to be handled by your client's OAuth implementation. At least in asp.net, the middleware that listens for the redirect processes the response and then redirects on to some UI endpoint within the host. So the browser wouldn't end up parsing the fragment and trying to use it for navigation.

@aomader
Copy link
Author

aomader commented Dec 15, 2024

I guess it's a combination of the behavior of the browser to keep the fragment until it reaches the final address and that we do not know what the final address will be. The chain of redirects can be fairly long, with the fragment being added to some URL that eventually assumes something about fragments. Or shortly, the identity server is here eventually causing a final URL that is "invalid".

More precisely, we noticed that a client reacts on errors by performing redirects that end up on a page where fragments are assumed to be valid query selectors. In context of text/html as media type, I guess that this is a valid assumption, since they are document fragment identifiers.

Given that the recommendation was changed from a non-query-selector-safe fragment to a query-selector-safe one, and that following that recommendation is a non-breaking change, I would argue that this would be a generally beneficial adjustment.

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.

2 participants