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: url-encode the fragment of the redirect URL of the authorize response #649

Merged
merged 1 commit into from
Mar 21, 2022
Merged

fix: url-encode the fragment of the redirect URL of the authorize response #649

merged 1 commit into from
Mar 21, 2022

Conversation

ste93cry
Copy link
Contributor

@ste93cry ste93cry commented Feb 3, 2022

This patch reverts the encoding logic for the fragment of the redirect URL returned as part of the authorize response to what was the one before version 0.36.0. In that version, the code was refactored and the keys and values of the fragment ceased to be url-encoded. This in turn reflected on all Ory Hydra versions starting from 1.9.0 and provoked a breaking change that made the parsing of the fragment impossible if any of the params contain a character like & or = because they get treated as separators instead of as text

Related Issue or Design Document

Fixes #648

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • 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 green light (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 necessary documentation within the code base (if
    appropriate).

Further comments

These changes reflect on Ory Hydra, partially restoring the behaviour related to the fragment encoding to the one that was before version 1.9. The OAuth specs mandate the fragment to be encoded using the application/x-www-form-urlencoded format, which is slightly different from the usual query string format: in fact, the spaces are not encoded as %20 but rather as a +. For this reason, decoding the value in Javascript must first replace all + signs with spaces and only then call decodeURIComponent(). Since before the spaces were encoded wrongly, this patch strictly speaking is still breaking the frontends. Let me know if we want to completely restore the logic or if the current implementation is fine as-is: in that case, a note should probably be added to the CHANGELOG

@ste93cry
Copy link
Contributor Author

@aeneasr can you please review this PR? At work we're unable to update to a more recent version of Hydra because of this bug affecting all newer versions

@aeneasr
Copy link
Member

aeneasr commented Mar 21, 2022

Thank you, not sure how this previous bug sneaked in 😓

@aeneasr aeneasr self-assigned this Mar 21, 2022
@aeneasr aeneasr merged commit beec138 into ory:master Mar 21, 2022
@ste93cry ste93cry deleted the fix-authorize-response-redirect-url-fragment-encoding branch March 21, 2022 17:20
@ste93cry
Copy link
Contributor Author

Beware of the notes in the Further comments section: the CHANGELOG description for this bug fix does not mention anything about the fact that the encoding is still slightly different than version 0.36, although according to the specs it is the right one now

@ste93cry
Copy link
Contributor Author

ste93cry commented Apr 8, 2022

@aeneasr 🙏 can you share an ETA on when this fix will be released? As I said, at work we can't update Hydra because of this bug, and it's now 2 months that this PR has been opened. Don't want to sound rude of course, I'm just wondering when I can finally unblock things downstream

@aeneasr
Copy link
Member

aeneasr commented Apr 9, 2022

Hey there, I’m currently on vacation so we’ll get it tagged once I’m back. Regarding release of Hydra, we’re planning to release Hydra 2.0 and have (kind of) frozen any new releases until that’s done. So I can’t give you an estimate right now

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.

Fragment is not escaped properly when computing the authorize response redirect URL
2 participants