Skip to content

Raise 400 Bad Request on server-side await request.json() if incorrect content-type received#3889

Merged
asvetlov merged 9 commits intoaio-libs:masterfrom
atemate:ay/json-content-type
Jul 13, 2019
Merged

Raise 400 Bad Request on server-side await request.json() if incorrect content-type received#3889
asvetlov merged 9 commits intoaio-libs:masterfrom
atemate:ay/json-content-type

Conversation

@atemate
Copy link
Copy Markdown

@atemate atemate commented Jul 5, 2019 •
edited
Loading

What do these changes do?

make server-side request fail with HTTPBadRequest on await request.json() with wrong content-type for json.

This PR is an alternative implementation of #2177.

Are there changes in behavior for the user?

The following code now raises HTTP 400:

async def handler(request):
    await request.json()  # raises HTTP 400

Related issue number

closes #2174

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

@atemate atemate changed the title Raise 400 Bad Request on server-side await request.json() for incorrect content-type Raise 400 Bad Request on server-side await request.json() if incorrect content-type received Jul 5, 2019
@atemate atemate marked this pull request as ready for review July 5, 2019 17:13
@atemate atemate requested review from asvetlov and webknjaz as code owners July 5, 2019 17:13
asvetlov
asvetlov approved these changes Jul 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I'm curious why aiohttp.svg is changed?

asvetlov
asvetlov requested changes Jul 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM except for logo file

webknjaz
webknjaz requested changes Jul 12, 2019
View reviewed changes
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 12, 2019

Codecov Report

Merging #3889 into master will decrease coverage by 0.03%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3889      +/-   ##
==========================================
- Coverage   97.81%   97.78%   -0.04%     
==========================================
  Files          43       43              
  Lines        8666     8813     +147     
  Branches     1386     1446      +60     
==========================================
+ Hits         8477     8618     +141     
- Misses         82       84       +2     
- Partials      107      111       +4
Impacted Files Coverage Δ
aiohttp/client_reqrep.py 97% <100%> (-0.03%) ⬇️
aiohttp/helpers.py 97.49% <100%> (+0.03%) ⬆️
aiohttp/web_request.py 96.77% <85.71%> (-0.22%) ⬇️
aiohttp/web_fileresponse.py 96.55% <0%> (-1.15%) ⬇️
aiohttp/http_parser.py 97.7% <0%> (-0.23%) ⬇️
aiohttp/web_protocol.py 95.04% <0%> (+1.58%) ⬆️

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 aeb01ce...e2becfd. Read the comment docs.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 12, 2019 •
edited by codecov bot
Loading

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.78%. Comparing base (aeb01ce) to head (e2becfd).
⚠️ Report is 6020 commits behind head on master.

Files with missing lines Patch % Lines
aiohttp/web_request.py 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3889      +/-   ##
==========================================
- Coverage   97.81%   97.78%   -0.04%     
==========================================
  Files          43       43              
  Lines        8666     8813     +147     
  Branches     1386     1446      +60     
==========================================
+ Hits         8477     8618     +141     
- Misses         82       84       +2     
- Partials      107      111       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Jul 12, 2019

@webknjaz please review again

@asvetlov asvetlov merged commit 637db70 into aio-libs:master Jul 13, 2019
@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Jul 13, 2019

thanks!

@asvetlov asvetlov mentioned this pull request Jul 24, 2019
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov asvetlov approved these changes

@webknjaz webknjaz webknjaz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Insecure way to handle json requests

4 participants

@atemate @codecov-io @asvetlov @webknjaz