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.

Sign up for GitHub

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

Jump to bottom

Add validation of HTTP status line, header keys and values #5452

Merged
merged 9 commits into from
Sep 4, 2021
Merged

Add validation of HTTP status line, header keys and values #5452

merged 9 commits into from
Sep 4, 2021

Conversation

franekmagiera
Copy link
Contributor

@franekmagiera franekmagiera commented Jan 29, 2021 •
edited
Loading

What do these changes do?

HTTP status line, header keys and values are checked in order to prevent custom header injection. If any of those contain a carriage return or newline character a ValueError is raised. The checks are done in _serialize_headers.

Changes where made and tested for pure python version and cython version.

Also imported the _serialize_headers function to test_web_response. I wanted to prevent code duplication and make sure that future changes to the way the headers are created are reflected in the tests. Let me know what you think about it.

The documentation was not updated since I could not find a suitable place to find describe the changes made. Suggestions are welcome.

Are there changes in behavior for the user?

No significant changes, except an exception may be raised.

Related issue number

#4818

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 29, 2021
@codecov
Copy link

codecov bot commented Jan 29, 2021 •
edited
Loading

Codecov Report

Merging #5452 (66ec8d5) into master (10a51ae) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5452   +/-   ##
=======================================
  Coverage   96.75%   96.75%           
=======================================
  Files          44       44           
  Lines        9852     9857    +5     
  Branches     1591     1592    +1     
=======================================
+ Hits         9532     9537    +5     
  Misses        182      182           
  Partials      138      138
Flag Coverage Δ
unit 96.65% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiohttp/http_writer.py 99.15% <100.00%> (+0.03%) ⬆️
aiohttp/web_protocol.py 86.41% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10a51ae...66ec8d5. Read the comment docs.

@franekmagiera
Copy link
Contributor Author

@asvetlov @webknjaz what do you think about this?

@franekmagiera
Copy link
Contributor Author

@asvetlov @webknjaz would you be interested in merging something like this?

Dreamsorcerer
Dreamsorcerer requested changes Aug 13, 2021
View reviewed changes
aiohttp/http_writer.py Outdated Show resolved Hide resolved
aiohttp/http_writer.py Outdated Show resolved Hide resolved
aiohttp/http_writer.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

Tests without extensions are stuck. Looks like they get stuck on tests/test_client_functional.py::test_keepalive_two_requests_success[pyloop]

@franekmagiera
Copy link
Contributor Author

franekmagiera commented Aug 16, 2021 •
edited
Loading

Sorry, missed that, forgot to run the code without extensions. There should be two \r\n signs after the headers.

@Dreamsorcerer
Copy link
Member

Sorry, missed that, forgot to run the code without extensions. There should be two \r\n signs after the headers.

Ah, yes, I missed that change.

Dreamsorcerer
Dreamsorcerer approved these changes Aug 16, 2021
View reviewed changes
Copy link
Member

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

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

@webknjaz Unless you've got an idea on a different place to include the check, I think this should be merged.

@aio-libs aio-libs locked and limited conversation to collaborators Aug 17, 2021
@aio-libs aio-libs unlocked this conversation Aug 21, 2021
@Dreamsorcerer
Copy link
Member

If there's no response from @webknjaz, I'll merge this at the weekend.

@Dreamsorcerer Dreamsorcerer merged commit a1158c5 into aio-libs:master Sep 4, 2021
@patchback
Copy link
Contributor

patchback bot commented Sep 4, 2021 •
edited
Loading

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/a1158c5389854f9885c30e08b0020e2d7d8a290f/pr-5452

Backported as #5984

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 4, 2021
@franekmagiera
Add validation of HTTP status line, header keys and values (#5452)
d1195a9 
* Add validation of HTTP status line, header keys and values

* Apply review comments

* Rename _check_string to _safe_header and remove validation for the status_line

* Update aiohttp/http_writer.py

Co-authored-by: Sam Bull <[email protected]>

* Modify changelog message

* Refactor headers join

* Refactor headers serialization back to the broken down version and add the second CRLF sign after the headers

* Update aiohttp/http_writer.py

Co-authored-by: Sam Bull <[email protected]>
(cherry picked from commit a1158c5)
@patchback patchback bot mentioned this pull request Sep 4, 2021
Merged
5 tasks
Dreamsorcerer pushed a commit that referenced this pull request Sep 4, 2021
@patchback @franekmagiera
Add validation of HTTP status line, header keys and values (#5452) (#…
3cae27a 
…5984)

* Add validation of HTTP status line, header keys and values

* Apply review comments

* Rename _check_string to _safe_header and remove validation for the status_line

* Update aiohttp/http_writer.py

Co-authored-by: Sam Bull <[email protected]>

* Modify changelog message

* Refactor headers join

* Refactor headers serialization back to the broken down version and add the second CRLF sign after the headers

* Update aiohttp/http_writer.py

Co-authored-by: Sam Bull <[email protected]>
(cherry picked from commit a1158c5)

Co-authored-by: Franek Magiera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dreamsorcerer Dreamsorcerer Dreamsorcerer approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@webknjaz webknjaz Awaiting requested review from webknjaz webknjaz is a code owner

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@franekmagiera @Dreamsorcerer