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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Check Content-Type request header before assuming JSON #2118

Merged
merged 12 commits into from
Jun 7, 2021

Conversation

patrickkwang
Copy link
Contributor

@patrickkwang patrickkwang commented Oct 2, 2020

馃悰 Check Content-Type request header before assuming JSON

Related to #1018.


Edit by @tiangolo: Updated tests to include extra corner cases, and updated implementation to parse content-type headers using Python's standard library email module to cover more corner cases.

@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #2118 (5729e8a) into master (60918d2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2118   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          246       246           
  Lines         7556      7591   +35     
=========================================
+ Hits          7556      7591   +35     
Impacted Files Coverage 螖
fastapi/routing.py 100.00% <100.00%> (酶)
tests/test_tutorial/test_body/test_tutorial001.py 100.00% <100.00%> (酶)
.../test_custom_request_and_route/test_tutorial001.py 100.00% <100.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 90120dd...5729e8a. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2020

馃摑 Docs preview for commit fa54629 at: https://5f76a9473885317380dee0a8--fastapi.netlify.app

fastapi/routing.py Outdated Show resolved Hide resolved
"msg": "Expecting value: line 1 column 1 (char 0)",
"type": "value_error.jsondecode",
"loc": ["body"],
"msg": "value is not a valid dict",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be considered a breaking change. Think even my work API has a test similar to this.

Copy link
Contributor

@ArcLightSlavik ArcLightSlavik left a comment

Choose a reason for hiding this comment

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

Find the formatting a bit odd but if that's how flake8(?) did it then okay.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2020

馃摑 Docs preview for commit 49e5945 at: https://5f77437a90b2834b8fa103c0--fastapi.netlify.app

@Mause
Copy link
Contributor

Mause commented Oct 2, 2020

I think this needs to be thought about a bit more - there are other mimetypes that are JSON but which don't use the application/json mimetype. application/hal+json is one such common example of this

@patrickkwang
Copy link
Contributor Author

Good point, @Mause. Based on this list of registered mimetypes, it seems safe to interpret as JSON any mimetype that ends with "json". Notably, this excludes the "json-seq" type, which is not itself a subset of JSON.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2020

馃摑 Docs preview for commit 47a1d9f97c0e187b9ba151a8315549c0bbe2aa93 at: https://5f775f837658b75dc0ecf011--fastapi.netlify.app

@Mause
Copy link
Contributor

Mause commented Oct 3, 2020

Another thing - you're also breaking content-type headers with charset values, such as application/json; charset=UTF-8

@patrickkwang
Copy link
Contributor Author

Perhaps some new tests are called-for at this point.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2020

馃摑 Docs preview for commit 332cbd47c607765bf144ca0fa1536810f22f1713 at: https://5f78bf70b71464b14bcbcfc0--fastapi.netlify.app

@patrickkwang
Copy link
Contributor Author

How do we like conflicts to be handled? I can rebase and force-push to my feature branch...

@rdpravin1895
Copy link

Hi @patrickkwang. Are you planning to merge this?

@github-actions
Copy link
Contributor

馃摑 Docs preview for commit d752efd at: https://6070fdb40418353f9ff17368--fastapi.netlify.app

@patrickkwang
Copy link
Contributor Author

I've rebased and everything is green, but I do not have the necessary permissions to merge.

@rdpravin1895
Copy link

Hi @tiangolo, sorry to disturb you, but can you have a look at this? We've been using FastAPI, so far has been an excellent framework, but only because of strict json application type, we are facing troubles in documenting the examples.

@tiangolo tiangolo changed the title Check Content-Type before assuming JSON 馃悰 Check Content-Type request header before assuming JSON Jun 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

馃摑 Docs preview for commit 29257d0 at: https://60bdf67acdd67667c44ad652--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

馃摑 Docs preview for commit 5729e8a at: https://60bdf86fcdd6766d184ad619--fastapi.netlify.app

@tiangolo
Copy link
Owner

tiangolo commented Jun 7, 2021

Thanks @patrickkwang ! 馃殌 馃

And thanks for the thorough reviews @ArcLightSlavik and @Mause 馃檱 馃嵃

I updated the tests to include extra corner cases and updated the implementation to parse the content-type headers using Python's standard library email module to cover more corner cases.

@tiangolo tiangolo merged commit fa7e3c9 into tiangolo:master Jun 7, 2021
@jolynch
Copy link

jolynch commented Jun 9, 2021

For what it's worth this appears to be a pretty big breaking change for clients who were previously not sending Content-Type headers. For example, some of our production clients started receiving a validation error from a route where previously it was working fine:

{"message":"Error processing request","error":"RequestValidationError","details":[{"loc":["body"],"msg":"value is not a valid dict","type":"type_error.dict"}]}

I understand the original default was incorrect but this fix breaks functional code and I believe the CVE would be resolved by assuming json in any case that is not exempted from CORS preflight (so don't assume json when the content-type is explicitly set to something not application/json)? Alternatively could the server author configure the default assumed content type?

@kpflugshaupt
Copy link

This just cost me two days of debugging. Not so pleased right now.

I fully see and appreciate the reasoning behind stricter checks, mind you. Go ahead with that!
But the error message is rather misleading. 'value is not a valid dict' does not make me suspect my HTTP headers!

Is it possible to implement an exception that's more to the point here? I have the feeling I'm not the only one tearing their hair out over this...

Sorry for the venting -- FastAPI is and remains my favourite framework. Just feeling a bit betrayed by that error message?

@hademircii
Copy link

hademircii commented Jun 29, 2021

For what it's worth this appears to be a pretty big breaking change for clients who were previously not sending Content-Type headers.

experiencing a similar problem right now (content-type is plain text). Bumping Fast API to 0.65.2 to breaks their integration, and we are not able to update all clients right away. So we either need to hold off on this and future FastAPI updates, or find a server side fix to go around it.

clwgg added a commit to clwgg/anaconda-mode that referenced this pull request Jul 1, 2021
FastAPI introduced Content-Type checking in v0.65.2
(see tiangolo/fastapi#2118)
@tiangolo
Copy link
Owner

tiangolo commented Jul 3, 2021

That's a very good point @jolynch! Thanks for idea.

I just released version 0.65.3 that does exactly that. If there's no Content-Type header, it is now assumed as JSON.

This still provides protection from CSRF because browsers automatically set Content-Type headers. And if an attacker removes the header explicitly, it triggers the CORS preflight, which provides the main protection using CORS.

So, you should all be able to upgrade to 0.65.3 more easily now. 馃帀

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

8 participants