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

In check_oauth_signature, normalize_parameters strips encoding for parameters #91

Open
coderkevin opened this issue Oct 27, 2015 · 1 comment · May be fixed by #92
Open

In check_oauth_signature, normalize_parameters strips encoding for parameters #91

coderkevin opened this issue Oct 27, 2015 · 1 comment · May be fixed by #92
Labels

Comments

@coderkevin
Copy link

The best example of this is the oauth_callback. If you set a URI, JSON, or something else that is already encoded as a parameter to your oauth_callback, then you have trouble.

For example (extraneous parameters removed for clarity):

Sent from client:

http://mywpsite.org/oauth/request?oauth_callback=http%3A%2F%2Fmyapp.com%2Fredirect.html%3Fstate%3D%257B%2522client_id%2522%253A%2522myClientID%2522%257D

In the plugin, $_GET decodes all parameters, so $_GET['oauth_callback'] gets you:

http://myapp.com/redirect.html?state=%7B%22client_id%22%3A%22myClientID%22%7D

Then, in check_oauth_signature, the processing includes a call to normalize_parameters, which decodes, then re-encodes each parameter key and value. That ends up doing this:

After decode:

http://myapp.com/redirect.html?state={"client_id":"myClientID"}   <--- This is the problem!

After re-encode:

http%3A//myapp.com/redirect.html%3Fstate%3D%7B%22client_id%22%3A%22myClientID%22%7D

So, as you can see, the oauth_callback was stripped of a layer of URI encoding. This means the signatures will never match if you include anything that needs to be encoded in a callback parameter.

I'm willing to help fix this, but I'm unsure why it's being decoded and re-encoded in the first place. I'm sure it was for a good reason, so if a project maintainer can provide some insight, I'd be happy to help.

@coderkevin
Copy link
Author

I have created a PR which merges a couple commits and fixes this issue along with two others. #92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants