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: double-decoding of client credentials in request body #434

Merged
merged 5 commits into from
May 23, 2020

Conversation

pjcdawkins
Copy link
Contributor

@pjcdawkins pjcdawkins commented May 22, 2020

I noticed that client credentials are URL-decoded after being extracted from the POST body form, which was already URL-decoded by Go. The accompanying error message suggests this was copied and pasted from the HTTP basic authorization header handling, which is the only place where the extra URL-decoding was needed (as per the OAuth 2.0 spec). The result is that client credentials containing %-prefixed sequences, whether valid sequences or not, are going to fail validation.

Proposed changes

Remove the extra URL decoding. Add tests that ensure client credentials work with special characters in both the HTTP basic auth header and in the request body.

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability.
  • 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)

@pjcdawkins pjcdawkins force-pushed the client-credentials-double-decode branch from 0f5b3e2 to 692b038 Compare May 22, 2020 10:02
@pjcdawkins pjcdawkins marked this pull request as draft May 22, 2020 10:03
@pjcdawkins pjcdawkins marked this pull request as ready for review May 22, 2020 10:07
@pjcdawkins
Copy link
Contributor Author

pjcdawkins commented May 22, 2020

I separated the commits just to demonstrate the test works - happy to squash if you need.

@aeneasr
Copy link
Member

aeneasr commented May 23, 2020

Awesome, thank you!

@aeneasr aeneasr merged commit 48c9b41 into ory:master May 23, 2020
@pjcdawkins pjcdawkins deleted the client-credentials-double-decode branch May 23, 2020 16:39
@aeneasr
Copy link
Member

aeneasr commented May 28, 2020

@pjcdawkins
Copy link
Contributor Author

@aeneasr thanks for releasing, just a note the release refers to "double-encoding", that should be double-decoding

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