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

Handle improper Accept headers #1795

Merged
merged 5 commits into from
Oct 2, 2018

Conversation

bschmeck
Copy link
Contributor

We have observed situations where our API has received a request from a network scanner that has an improperly formatted Accept header. In these cases, Rack:Accept::Header.parse_media_type returns an empty array (https://github.com/mjackson/rack-accept/blob/master/lib/rack/accept/header.rb#L42) and subtype is set to nil.

This PR checks that we actually received a subtype before checking against the vendor and version regexes.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is good. Fix CHANGELOG.

It would be a good idea to add some unit tests that include actual calls to vendor? and such after parsing an invalid accept header.

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
#### Features

* Your contribution here.
* [#1795](https://github.com/ruby-grape/grape/pull/1795): Fix Accept header parsing bug - [@bschmeck](https://github.com/bschmeck)
Copy link
Member

Choose a reason for hiding this comment

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

Missing a period at the end. It could also be more specific: "Fix: vendor/subtype parsing in an invalid Accept header"

@bschmeck
Copy link
Contributor Author

Pushed a correction to the CHANGELOG, I had missed the comment from grape-bot.

vendor? and version? are private methods, which I generally don't write unit tests for, but am happy to do so if that's a common approach in grape. Do you think they should live in spec/grape/middleware/versioner/header_spec.rb? I ask because these will be the first tests for something other than #call in that spec.

@dblock
Copy link
Member

dblock commented Sep 28, 2018

Thanks, makes sense. I would refactor this and take parsing out of the header middleware implementation, and then we can have tests, but that's beyond the scope of this PR.

@dblock
Copy link
Member

dblock commented Sep 28, 2018

https://travis-ci.org/ruby-grape/grape/jobs/434730968 has a failure, I am not sure what's up with that, care to check it out?

@bschmeck
Copy link
Contributor Author

I will try to find some time to work on extracting the parsing logic and wrapping it in tests.

I'll dig into the failure, too. It appears to be only on rails_edge, related to HashWithIndifferentAccess#except and there was just a PR merged to ActiveSupport around that.

@dblock
Copy link
Member

dblock commented Sep 29, 2018

I am sure it's unrelated, but we do need a green build one way or another.

@bschmeck bschmeck force-pushed the improper-accept-header-handling branch from c576a88 to 50135b0 Compare October 1, 2018 21:28
@bschmeck
Copy link
Contributor Author

bschmeck commented Oct 1, 2018

The upstream problem in ActiveSupport was fixed and the build is now green for this PR.

@dblock dblock merged commit ed0e221 into ruby-grape:master Oct 2, 2018
@dblock
Copy link
Member

dblock commented Oct 2, 2018

Thanks, merged.

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

2 participants