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

Fixing the way signed parameters are encoded #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rubenfonseca
Copy link

Hi. I was having a problem using node-oauth with Twitter SiteStreams.

I found the bug to be happening when I had a comma on the URL parameters, like "follow=123,324,45".

Checked the code and verified it was a double encoding bug. I think I've fixed it on my branch, here is the patch. Tell me what you think!

Thank you!

Fixed a bug where using a comma on a query parameter
caused the generation of a wrong Oauth signature
(result was being double encoded)
@ciaranj
Copy link
Owner

ciaranj commented Oct 6, 2010

This is really weird :( The 1.0 spec ( http://tools.ietf.org/html/rfc5849#section-3.4.1 ) and the 1.0a reference (http://oauth.net/core/1.0a/#sig_norm_param) seem to be pretty clear on their approach, which matches what I've implemented! If I apply your changes they break several of the tests I can do :( I.e. other OAuth Providers do not behave as Twitter describe :( arggh OAuth can be such a PitA!

@rubenfonseca
Copy link
Author

Indeed it can be a pita :( The exact problem with your code was that:

follow=1,2 became follow=1%2C2

and then on the final encodeData

follow=1%2C2 became follow%3D1%25C2 (ie, %2C got double escaped)

again, I think there is a problem on this specific case, but I'm still not sure if my solution is generic enough! I tried to run the tests running "make" on the console, but it always said "ran 0 tests" :/

@ciaranj
Copy link
Owner

ciaranj commented Oct 6, 2010

I know :( The tests broke when I swapped to npm.. Its on my urgent todo list to fix them Sigh (I desperately want to) I've commented on your question in the mailing list as far as I can tell twitter are generating their signature incorrectly. The specification as far as I can interpret it, expects this behaviour! I confirmed this (by yet another source) at http://hueniverse.com/2008/10/beginners-guide-to-oauth-part-iv-signing-requests/

:(

@rubenfonseca
Copy link
Author

Thank you ciaranj! We'll wait for twitter's response then :) Let's keep this on hold for now!

@ciaranj
Copy link
Owner

ciaranj commented Oct 6, 2010

no dude, thank you. I had no idea of this issue (I searched and searched the twitter APIs and couldn't find another example where they used commas as argument values.. but I imagine the issue exists with any argument value that requires encoding such as UTF8) :(

@ciaranj
Copy link
Owner

ciaranj commented Oct 17, 2010

For anyone else reading this, Twitter still haven't responded to me :( In the meantime just encode any parameters before passing them in for signing it will work with Twitter :(

@bmeck
Copy link

bmeck commented Oct 28, 2010

Link to thread about this with response from twitter. http://groups.google.com/group/twitter-development-talk/browse_thread/thread/0970d03e4e3528df

@ciaranj
Copy link
Owner

ciaranj commented Oct 28, 2010

Thanks ;) I'm not sure if I"m going insane or not, but given the silence i keep getting :( I may put in an 'if(twitter) { sign_like_this } section in the client !

@bmeck
Copy link

bmeck commented Oct 28, 2010

Yea, seems a bit odd the way they interpreted it, but valid. I think having an option instead new OAuth({encoding:"twitter"}) or something would be more apt, just cant think of a good name. "inside-loop" "outside-loop" maybe?

@ciaranj
Copy link
Owner

ciaranj commented Oct 28, 2010

I genuinely don't think it is valid, not spec wise, but there's nothing stopping people form implementing OAuth however they want (it isn't true standard after all). The standard basically means the ',' will be encoded twice, once when the value is encoded, and then again when the whole concatentated string is encoded. In their scheme this re-encode of the comma never happens :(

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.

3 participants