Skip to content

Conversation

@Andersond11
Copy link

Fixes an issue that was introduced when we moved to Rack 2.0.4 where a POST request without a content-type caused Rack to attempt to url decode the body. When this happened to a body that included % in any of it's contents, the request would blow up with ArgumentError - invalid %-encoding. The following two curl commands demonstrate the issue and solution locally:

This command without the content header will blow up with the above error:

curl -d'{"order":{"recipients":[{"line_items":[{"item_name":"NA","product_options":{"select-quantity":"%"}}]}]}}' http://localhost:5000/api/stores/9a5c1fafa3e1d7d45a9a3775a17e2622/orders?api_key=f9a7c8ebdfd34beaf260d9b0296c7059&api_timestamp=1525899406&api_signature=eeafc156e1ecf6fc02f49d5f9c17ce5f916f4a90390c072376943e5df05904a0

Adding the Content-type: application/json header results in the appropriate order validation errors, since the payload doesn't contain complete order data:

curl -H "Content-Type: application/json" -d'{"order":{"recipients":[{"line_items":[{"item_name":"NA","product_options":{"select-quantity":"%"}}]}]}}' http://localhost:5000/api/stores/9a5c1fafa3e1d7d45a9a3775a17e2622/orders?api_key=f9a7c8ebdfd34beaf260d9b0296c7059&api_timestamp=1525899406&api_signature=eeafc156e1ecf6fc02f49d5f9c17ce5f916f4a90390c072376943e5df05904a0

Copy link
Contributor

@brasic brasic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice sleuthing!

Copy link
Contributor

@brasic brasic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, think it's in the wrong place. shouldn't it be in post? GETs should not have bodies.

@brasic
Copy link
Contributor

brasic commented May 9, 2018

I think I recall that our GETs do sometimes have bodies, which is odd, but at any rate, the header should also be added to def post.

@Andersond11
Copy link
Author

Yes, I fixed that in the bundled copy then put it in the wrong spot when I cloned the repo for the PR

@Andersond11 Andersond11 force-pushed the da/add-content-type-header branch from cdd545e to fad9d8b Compare May 9, 2018 21:24
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