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 spaces encoding in parameters #88

Closed
wants to merge 1 commit into from

Conversation

Xowap
Copy link

@Xowap Xowap commented Oct 8, 2013

OAuth requires spaces to be encoded as %20 instead of + in order to generate a valid signature.

Explained in http://troy.yort.com/2-common-problems-with-oauth-client-libraries/

Before this patch, I was unable to use Yahoo!'s BOSS Placefinder API, which now seems to be responding correctly to me, instead of what it used to say:

<?xml version='1.0' encoding='UTF-8'?>
<yahoo:error xmlns:yahoo='http://yahooapis.com/v1/base.rng'
  xml:lang='en-US'>
  <yahoo:description>Please provide valid credentials. OAuth oauth_problem="signature_invalid", realm="yahooapis.com"</yahoo:description>
</yahoo:error>

OAuth requires spaces to be encoded as %20 instead of + in order to generate a valid signature.
@Xowap
Copy link
Author

Xowap commented Oct 8, 2013

Now the question being: shouldn't this be instead fixed in oauthlib?

I'd say it should probably, but their code is more complex and I needed to fix that ASAP for my project. I'll let you guys handle this one ;)

@Lukasa
Copy link
Member

Lukasa commented Oct 9, 2013

In principle this is fine. In practice, I've got two concerns:

  1. This is probably oauthlib's problem, as you've stated. =)
  2. I worry that a blanket replace of + with %20 has a few unexpected side-effects.

@Xowap
Copy link
Author

Xowap commented Oct 9, 2013

Regarding your second item, this is what I first thought, but then I had the following reasoning: since the URL-encoded string must be unambiguous, then a "+" should have only one meaning, which is "space" and thus can safely be replaced by a "%20"

And this is especially true since any "+" will be replaced by a "%2B".

But anyway, we should open an issue in oauthlib's repository instead. I don't really have time to dig into this right now, let me know if you do it, I'll send it later otherwise.

@Lukasa
Copy link
Member

Lukasa commented Oct 9, 2013

@Xowap Sure thing. =)

Just for future reference, a urlencoded string must be unambiguous, but that doesn't mean "+" can have only one meaning. In particular, different characters may be used unencoded in different parts of the URL. It's not immediately obvious to me that the '+' is special in any situation, but it might be and I'm just not sure.

@ib-lundgren
Copy link
Member

@Xowap Just took a look at this and the problem is neither here nor in oauthlib but with Yahoo!. As far as OAuth 1 RFC is concerned, + and %20 is the same. oauthlib produce the same signing string for both but that is not the what Yahoo does and therefore you get the invalid signature. Not sure why but they probably have a reason.

I don't think replacing + with %20 would cause much problems so we can take three approaches here

  1. Replace for everyone
  2. Disable replacement by default and enable with constructor flag
  3. Ignore the issue

@Lukasa @Xowap what are your thoughts on these approaches?

Regarding the PR code if you just want to replace you can utilize the order in which requests prepare a request (auth being last) and simply add the line below at oauth1_auth.py#L59

r.url = r.url.replace('+', '%20')

This does not address the issue when passing urlencoded data as a dict in the request but neither does this PR :) Of course it could easily be added after L63 with

r.body = r.body.replace('+', '%20')

since r.body is always a string at this point :)

@ib-lundgren
Copy link
Member

Or we could add a compliance fix framework similar to what we have for OAuth 2.

@Lukasa
Copy link
Member

Lukasa commented Nov 1, 2013

Ugh, goddamn it Yahoo. How confident are we that a blanket change will be safe?

@Xowap
Copy link
Author

Xowap commented Nov 1, 2013

@ib-lundgren another solution would be to only affect oauth's sign method, which would have to decode the URL and re-encode it in an oauth-compliant way before signing it

@ib-lundgren
Copy link
Member

@Xowap I rather not do a blanket change in oauthlib for this as it is specific to a provider.

@Lukasa I think it is fairly safe in the scope of making the actual request (doubt proxies would be trouble?) but we could go with the safe approach of a compliance fix / flag for it rather than enable for all.

@Xowap
Copy link
Author

Xowap commented Nov 3, 2013

@ib-lundgren apparently, this is not yahoo-specific, but rather an oauth requirement that is being respected more or less often, so it would indeed fit in oauthlib

@ib-lundgren
Copy link
Member

@Xowap sorry for the late reply.

this is not yahoo-specific, but rather an oauth requirement that is being respected more or less often

I am not sure I follow. OAuth requires the base signing string to use %20 to represent space characters, regardless of whether the parameter was supplied with + or %20 originally. Which is indeed what OAuthLib does. OAuthlib does change the original %20 parameters to + as that happens to be the python default in urllib.urlencode. While you can argue %20 might be more correct in the scope of URIs they are considered equivalent in the eyes of OAuth 1.

As mentioned I think the fix would fit better here but could see some small value in being able to choose between + and %20 in oauthlib as well. The latter would require working around pythons urlencode as it does not provide this option but would not refuse a PR ;)

ib-lundgren added a commit that referenced this pull request Sep 12, 2014
Apparently (#88) some providers see + and %20 as different beings
when verifying OAuth signatures. This is a test to see if the
providers work when blanket changing all + to %20.
@ib-lundgren
Copy link
Member

@Xowap, I thought I'd revisit this issue after a long busy period. If you have time it would be awesome if you could test the changes in 19a9e10 and see if they make the Placefinder API happy. If so we can add those into master under a toggle/if-statement perhaps.

@laughinghan
Copy link

Hi, I'm running into the same problem (with the exact same Yahoo BOSS Placefinder API, fancy that), and I'm trying to understand the workarounds. This PR forks requests_oauthlib, this other blogpost author forked requests; what's your recommended fix "above the fold", so to speak? Right now I'm calling urllib.urlencode(params).replace('+', '%20') myself and passing that in as params to requests, because requests is awesome and will accept params as a str rather than dict, but it's a pain because urllib is Unicode-unaware and instead expects UTF-8 encoded str()s: http://stackoverflow.com/a/6481120/362030

@singingwolfboy
Copy link
Member

There's been no activity on this pull request for over two years, and it has conflicts with the master branch. As a result, I'm closing this pull request. If we determine that we still want this change after all, we can open a new pull request for it, based on the latest version of master.

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.

None yet

5 participants