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] Allowing raw request body when Content-Type is already specified #60

Merged
merged 1 commit into from
Jun 8, 2015
Merged

[fix] Allowing raw request body when Content-Type is already specified #60

merged 1 commit into from
Jun 8, 2015

Conversation

brndnblck
Copy link
Contributor

Currently, when you use the -d option twurl calls request.set_form_data which URL encodes the request body and overrides any previously set Content-Type value by forcibly setting it to application/x-www-form-urlencoded.

https://github.com/twitter/twurl/blob/master/lib/twurl/oauth_client.rb#L112

Example without Modification (Unexpected Behavior):

twurl -t -X POST -A "Content-Type: application/json" "/1.1/some/endpoint" -d '{ "foo": "bar" }'

opening connection to api.twitter.com:443...
opened
starting SSL for api.twitter.com:443...
SSL established
<- "POST /1.1/some/endpoint HTTP/1.1\r\nContent-Type: application/x-www-form-urlencoded\r\nAccept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3\r\nAccept: */*\r\nUser-Agent: OAuth gem v0.4.7\r\nAuthorization: OAuth oauth_consumer_key=\"OIDntXZULiYrCzSBq23BQ\", oauth_nonce=\"oYU432ViX4OSSBLyWvThQ7jvxor5j84kN2Xy75SHDw\", oauth_signature=\"8v%2B4YEeV8gItbM100oyzFV5NwoU%3D\", oauth_signature_method=\"HMAC-SHA1\", oauth_timestamp=\"1433577823\", oauth_token=\"15678855-Q9DtXWXni83LwkqPcUl3dBkhtr9VgHu02nLKueKMI\", oauth_version=\"1.0\"\r\nConnection: close\r\nHost: api.twitter.com\r\nContent-Length: 30\r\n\r\n"
<- "%7B+%22foo%22%3A+%22bar%22+%7D"

This behavior prevents users from ever being able to send a request with Content-Type: application/json and a JSON payload in the request body.

The change I'm making here is simple, non-breaking and preserves the default behavior while allowing the expected behavior you'd see in curl for the scenario described. If the request has Content-Type set to any value, we set request.body to the raw value of options.data and leave the content header untouched.

Example with Modification (Expected Behavior):

twurl -t -X POST -A "Content-Type: application/json" "/1.1/some/endpoint" -d '{ "foo": "bar" }'

opening connection to api.twitter.com:443...
opened
starting SSL for api.twitter.com:443...
SSL established
<- "POST /1.1/some/endpoint HTTP/1.1\r\nContent-Type: application/json\r\nAccept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3\r\nAccept: */*\r\nUser-Agent: OAuth gem v0.4.7\r\nAuthorization: OAuth oauth_body_hash=\"Tj2XHfH2CojbvcRl2SoNiHqmQgM%3D\", oauth_consumer_key=\"OIDntXZULiYrCzSBq23BQ\", oauth_nonce=\"Kq6NBzJqGI5afVJVEbvOxMG9VuqeLcctiaSsQkDX2Q\", oauth_signature=\"a%2BGF7v4ZdUH%2FYk0f0gsKoL3b%2BAo%3D\", oauth_signature_method=\"HMAC-SHA1\", oauth_timestamp=\"1433578200\", oauth_token=\"15678855-Q9DtXWXni83LwkqPcUl3dBkhtr9VgHu02nLKueKMI\", oauth_version=\"1.0\"\r\nConnection: close\r\nHost: api.twitter.com\r\nContent-Length: 16\r\n\r\n"
<- "{ \"foo\": \"bar\" }"

@brndnblck brndnblck changed the title [fix] allowing raw request body when content-type is already specified [fix] Allowing raw request body when Content-Type is already specified Jun 6, 2015
@sferik
Copy link
Contributor

sferik commented Jun 6, 2015

Would you mind adding a test for this?

@brndnblck
Copy link
Contributor Author

@sferik done. apologies for not including that from the get go.

@sferik
Copy link
Contributor

sferik commented Jun 8, 2015

No need to apologize. Thanks for the patch!

sferik added a commit that referenced this pull request Jun 8, 2015
[fix] Allowing raw request body when Content-Type is already specified
@sferik sferik merged commit 9faa220 into twitter:master Jun 8, 2015
@brndnblck
Copy link
Contributor Author

Just my own guilt ;-) Thanks Erik.

@brndnblck brndnblck deleted the post_body_fix branch June 16, 2015 17:29
@brndnblck
Copy link
Contributor Author

@sferik got a few people asking when this fix will be released. Ads API in particular has a new feature launching this week which twurl won't work with unless this fix is present.

Do you have an ETA on when we'll be cutting a new gem? I hate to ask for a release just for this, but I also don't see much else in motion on here at the moment.

@brndnblck
Copy link
Contributor Author

@sferik Ping.

@sferik
Copy link
Contributor

sferik commented Jun 22, 2015

Hey. Thanks for the ping. I was on vacation last week. I’ll try to get to this later today. I’ll also add you as a gem owner so you can push releases yourself in the future.

@sferik
Copy link
Contributor

sferik commented Jun 22, 2015

:shipit:

@brndnblck
Copy link
Contributor Author

Thanks a bunch @sferik!

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

Successfully merging this pull request may close these issues.

2 participants