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

[PR #5452/a1158c53 backport][3.8] Add validation of HTTP status line, header keys and values #5984

Conversation

patchback[bot]
Copy link
Contributor

@patchback patchback bot commented Sep 4, 2021

This is a backport of PR #5452 as merged into master (a1158c5).

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."

* 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 <aa6bs0@sambull.org>

* 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 <aa6bs0@sambull.org>
(cherry picked from commit a1158c5)
@codecov
Copy link

codecov bot commented Sep 4, 2021

Codecov Report

Merging #5984 (a1158c5) into 3.8 (66e281f) will decrease coverage by 0.77%.
The diff coverage is 96.32%.

❗ Current head a1158c5 differs from pull request most recent head bb19323. Consider uploading reports for the commit bb19323 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              3.8    #5984      +/-   ##
==========================================
- Coverage   97.52%   96.75%   -0.78%     
==========================================
  Files          44       44              
  Lines        8865     9857     +992     
  Branches     1429     1592     +163     
==========================================
+ Hits         8646     9537     +891     
- Misses        103      182      +79     
- Partials      116      138      +22     
Flag Coverage Δ
unit 96.65% <96.09%> (-0.70%) ⬇️

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

Impacted Files Coverage Δ
aiohttp/web_middlewares.py 100.00% <ø> (ø)
aiohttp/web_protocol.py 86.41% <ø> (-4.20%) ⬇️
aiohttp/web_request.py 95.99% <ø> (-1.57%) ⬇️
aiohttp/web_response.py 98.21% <ø> (-0.06%) ⬇️
aiohttp/web_routedef.py 98.11% <ø> (+0.01%) ⬆️
aiohttp/web_runner.py 97.74% <ø> (+0.11%) ⬆️
aiohttp/web_server.py 94.28% <ø> (-5.72%) ⬇️
aiohttp/web_urldispatcher.py 97.83% <ø> (+0.23%) ⬆️
aiohttp/web_ws.py 91.53% <ø> (ø)
aiohttp/worker.py 94.91% <ø> (-1.81%) ⬇️
... and 75 more

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 02db846...bb19323. Read the comment docs.

@Dreamsorcerer Dreamsorcerer merged commit 3cae27a into 3.8 Sep 4, 2021
@Dreamsorcerer Dreamsorcerer deleted the patchback/backports/3.8/a1158c5389854f9885c30e08b0020e2d7d8a290f/pr-5452 branch September 4, 2021 16:14
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