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

Not percent encoding colons in query parameters #246

Closed
thomas-holmes opened this issue Aug 13, 2015 · 12 comments
Closed

Not percent encoding colons in query parameters #246

thomas-holmes opened this issue Aug 13, 2015 · 12 comments
Assignees

Comments

@thomas-holmes
Copy link

Colons in query parameters are not getting properly encoded. I ran into this while passing ISO8601 formatted timestamps in query strings with the twitter API.

While their documentation does not specify needing to encode the colon for oauth signing, simple_oauth appears to (probably correctly, since the URI specification says the colon should be percent encoded) use the encoded value when generating the oauth signature value.

The resulting behavior is an oauth hmac signature mismatch due to the final URL that is used does not have the query param colons percent encoded.

This behavior changed starting with version 0.8.

ref: sferik/twitter-ruby#687

@ixti ixti self-assigned this Aug 13, 2015
@ixti
Copy link
Member

ixti commented Aug 13, 2015

Fix was applied to 0-8-stable branch. Will try to fix tests (fails on travis but not locally and not related to fix itself) and will cut 0.8.13 release. Thanks for report.

@tarcieri
Copy link
Member

What about 0.9.0?

@ixti
Copy link
Member

ixti commented Aug 13, 2015

@tarcieri Stupid me. By some reason I wanted to apply patch to 0.8.x and then add it to 0.9.x; Will apply patch to 0.9 now as specs will pass so it will become available as 0.9.1 release.

@ixti ixti closed this as completed in a931f50 Aug 13, 2015
ixti added a commit that referenced this issue Aug 13, 2015
@ixti
Copy link
Member

ixti commented Aug 14, 2015

0.9.1 and 0.8.13 fixes issue now.

@thomas-holmes
Copy link
Author

I am just testing this out now and I don't actually get encoded colons in my real web request.

[13] pry(#<Twitter::REST::Request>)> HTTP.public_send(:get, "http://google.com", params: {:t => "1970-01-01T00:00:00Z"})
=> #<HTTP::Response/1.1 301 Moved Permanently {"Location"=>"http://www.google.com/?t=1970-01-01T00:00:00Z", "Content-Type"=>"text/html; charset=UTF-8", "Date"=>"Fri, 14 Aug 2015 15:14:15 GMT", "Expires"=>"Sun, 13 Sep 2015 15:14:15 GMT", "Cache-Control"=>"public, max-age=2592000", "Server"=>"gws", "Content-Length"=>"242", "X-Xss-Protection"=>"1; mode=block", "X-Frame-Options"=>"SAMEORIGIN", "Connection"=>"close"}>
[14] pry(#<Twitter::REST::Request>)> HTTP::VERSION
=> "0.8.13"


# Output from httpry:
2015-08-14 11:14:50.560 192.168.1.130   74.125.21.139   >   GET google.com  /?t=1970-01-01T00:00:00Z    HTTP/1.1    -   -

@thomas-holmes
Copy link
Author

@ixti Thanks for the quick response. Have you seen my comment above?

@ixti
Copy link
Member

ixti commented Aug 18, 2015

@thomas-holmes Sorry. Overlooked. Will check ASAP.

@ixti
Copy link
Member

ixti commented Aug 18, 2015

Hm. Found the issue:

# https://github.com/httprb/http/blob/master/lib/http/request.rb#L69

query = HTTP::URI.form_encode(:t => Time.now.iso8601)
# => "t=2015-08-19T00%3A21%3A20%2B02%3A00"

uri = HTTP::URI.parse("http://google.com/?#{query}")
# => #<Addressable::URI:0x1802c48 URI:http://google.com/?t=2015-08-19T00%3A21%3A20%2B02%3A00>

uri.normalize
# => #<Addressable::URI:0x181206c URI:http://google.com/?t=2015-08-19T00:21:20%2B02:00>

I remember I have added normalize for some reason. Something related to international support in path/domain. Will fix this issue. Again. Thanks for pinging me!

@ixti ixti reopened this Aug 18, 2015
@thomas-holmes
Copy link
Author

Thanks again :)

ixti added a commit that referenced this issue Aug 19, 2015
@ixti ixti closed this as completed in 8c8486c Aug 19, 2015
@ixti
Copy link
Member

ixti commented Aug 19, 2015

@thomas-holmes Please check with 0.8.14. I have changed the way we normalize URIs in request so that:

  • query part is not normalized anymore and kept as is. (this is most arguable part, but I think we are safe to keep it like this unless issues will arise with such behavior)
  • fragment part is not sent to server anymore

@thomas-holmes
Copy link
Author

@ixti It looks like it works for my use case! Thanks so much for your help :)

@ixti
Copy link
Member

ixti commented Aug 19, 2015

@thomas-holmes thanks for report! :D

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

No branches or pull requests

3 participants