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

oauth2: Fix for #345 #346

Merged
merged 1 commit into from
Dec 23, 2018
Merged

oauth2: Fix for #345 #346

merged 1 commit into from
Dec 23, 2018

Conversation

ngrigoriev
Copy link
Contributor

@ngrigoriev ngrigoriev commented Dec 17, 2018

Modify URL generation with a fragment

Signed-off-by: Grigoriev, Nikolai nikolai.grigoriev@nuance.com

#345, ory/hydra#1201

Proposed changes

Go's URL class does not seem (to me) to be well designed for constructing the URLs. It seems to be impossible to avoid double-encoding of the characters if one used url.Fragment populated using url.Values.Encode().

The proposed change encodes the fragment separately using url.Values.Encode(), ensures that the original redirect URL does not have a fragment (it must not have one by RFC). Then it constructs the URL string and, if the fragment is not empty, adds the raw encoded fragment to the URL.

Checklist

  • I have read the contributing guidelines
  • 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 hi@ory.sh) from the maintainers to push the changes.
  • I signed the Developer's Certificate of Origin
    by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
    amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
    force push.
  • I have added tests that prove my fix is effective or that my feature works
  • [n/a] I have added necessary documentation within the code base (if appropriate)

Further comments

This is my first Go experience so I will appreciate any comments and suggestions

Modify URL generation with a fragment

Signed-off-by: Grigoriev, Nikolai <nikolai.grigoriev@nuance.com>
@aeneasr
Copy link
Member

aeneasr commented Dec 18, 2018

Awesome, this looks good to me! Rerunning the tests which failed due to a GitHub issue.

@aeneasr aeneasr merged commit 1f41934 into ory:master Dec 23, 2018
budougumi0617 added a commit to budougumi0617/fosite that referenced this pull request May 10, 2019
Closes ory#345

Signed-off-by: Grigoriev, Nikolai <nikolai.grigoriev@nuance.com>
@mitar mitar mentioned this pull request Apr 3, 2020
5 tasks
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