Skip to content

Fix parsing of 'Access-Control-Request-Headers' header#422

Merged
leplatrem merged 1 commit intoCornices:masterfrom
spoof:master
Dec 14, 2016
Merged

Fix parsing of 'Access-Control-Request-Headers' header#422
leplatrem merged 1 commit intoCornices:masterfrom
spoof:master

Conversation

@spoof
Copy link
Contributor

@spoof spoof commented Dec 10, 2016

Cornice parses this header by using the non flexible way as HTTP specification
required. Specifications says Access-Control-Request-Headers header
can have any number of LWS (Linear White Spaces) between commas.
But Cornice strictly waits for 'comma with space' delimiter.
It leads to an error for header values like
Access-Control-Request-Headers: header1,header2,header3 when
cors_expose_all_headers=True, even if any of these headers (or all of them)
persist in service.cors_headers attribute.

Is it necessary to write tests for this situation? (I see no such tests for other headers)

@coveralls
Copy link

coveralls commented Dec 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling f727a3d on spoof:master into 33de169 on Cornices:master.

@leplatrem
Copy link
Contributor

Thanks for fixing this!

Is it necessary to write tests for this situation? (I see no such tests for other headers)

If you can, it would be excellent! Especially to make the spec explicit with a simple test (e.g. reproduce with the example you gave in the description). Some parts of Cornice are not tested with a lot of precision indeed, but let's improve!

Also, could you add a line in the CHANGELOG and add yourself to the contributors please?

@coveralls
Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling df581dc on spoof:master into 33de169 on Cornices:master.

@spoof
Copy link
Contributor Author

spoof commented Dec 13, 2016

I've updated my PR: added line to CHANGES.txt and CONTRIBUTORS.txt
I also added testing case to
the tests/test_cors.py:TestCORS.test_preflight_request_headers_isnt_too_permissive test.
By doing this I wanted to point that if your if your service is configured with cors_expose_all_headers=False, as eggs service did, and if you make request with valid headers you get OK and error if they are not valid.
Now we have two cases in test (valid and invalid headers) for service with hiding headers (cors_expose_all_headers=False flag) and it's good place to test 'Access-Control-Request-Headers' header value parsing and also check either this flag affects parsing or not.
Is it okay?

Cornice parses this header using non flexible way as HTTP specification
required. Specifications says Access-Control-Request-Headers header
can have any number of LWS (Linear White Spaces) between commas.
@coveralls
Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling aa7f7ff on spoof:master into 33de169 on Cornices:master.

Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@leplatrem leplatrem merged commit 3b4c0ae into Cornices:master Dec 14, 2016
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