Return 400, not 500, when POST body missing #4165
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a user error, not a server error, so a 400 status would be more appropriate, and consistent with how mistakes in GET requests are handled. The current behaviour is making it difficult for my team to write monitoring that is triggered by real server errors; if just a single user persistently sends requests missing a body it's enough to trigger alarms.
|
@wheresrhys: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
|
@abernix Could this be released as an option that can be passed into the library to force it down a path of returning 400s? I'd be happy to submit a new PR implementing such an option? |
|
Somewhat related to #1841. |
glasser
added a commit
that referenced
this pull request
Jun 15, 2021
There are a lot of reasons that a supposedly GraphQL request might not
make it to requestPipeline: missing body, bad JSON, bad content-type,
etc. Previously, many of these outcomes resulted in 5xx errors despite
them being client errors (which should be 4xx), and their handling was
pretty inconsistent across the integrations.
Part of the confusion was a particular error message which combined "you
seem to have set up your server wrong by not setting up body parsing
properly" with "there was a problem parsing this request". The former is
reasonable to be 500 but the latter is much more likely to be what's
actually happening (you have to actively disable body parsing to make
that happen!). And the recommendation was ungrammatical and
express-specific despite being in apollo-server-core. But the good news
is that for some reason the Express body-parser unconditionally sets
`req.body` to at least `{}` before checking `content-type` (see
https://github.com/expressjs/body-parser/blob/480b1cfe29af19c070f4ae96e0d598c099f42a12/lib/types/json.js#L105-L117)
so we can move the "500 if body not parsed" from `apollo-server-core` to
`apollo-server-express` and directly detect whether `req.body` is empty.
This is a backwards-incompatible change, so we can finally implement it
as v3 is imminent.
The PR consists of:
- Core
- Make an error about needing a query be clear that it needs to be a
non-empty query (the error could fire before if `query` was provided
yet empty).
- Make "POST body missing" error 400 instead of 500, include that it
could be bad content-type or empty JSON object, and don't suggest
(with poor grammar) the use of body-parser (as described above).
Also show this error if httpRequest.query is a string or Buffer
(this can happen with Azure Functions on JSON parse failures).
- Express: Check for a sign that body-parser hasn't been applied here
instead of in Core and provide a more targeted body-parser-suggesting
500.
- Micro: Only parse JSON for `content-type: application/json` (like all
other integrations).
- Azure Functions
- 400 instead of 500 for missing body
- Make test more realistic on parser failures; see Core change for
where this matters.
Fixes #1841 (thanks to @nihalgonsalves for a good set of test cases).
Fixes #4165.
|
Fixed on |
glasser
added a commit
that referenced
this pull request
Jun 15, 2021
There are a lot of reasons that a supposedly GraphQL request might not
make it to requestPipeline: missing body, bad JSON, bad content-type,
etc. Previously, many of these outcomes resulted in 5xx errors despite
them being client errors (which should be 4xx), and their handling was
pretty inconsistent across the integrations.
Part of the confusion was a particular error message which combined "you
seem to have set up your server wrong by not setting up body parsing
properly" with "there was a problem parsing this request". The former is
reasonable to be 500 but the latter is much more likely to be what's
actually happening (you have to actively disable body parsing to make
that happen!). And the recommendation was ungrammatical and
express-specific despite being in apollo-server-core. But the good news
is that for some reason the Express body-parser unconditionally sets
`req.body` to at least `{}` before checking `content-type` (see
https://github.com/expressjs/body-parser/blob/480b1cfe29af19c070f4ae96e0d598c099f42a12/lib/types/json.js#L105-L117)
so we can move the "500 if body not parsed" from `apollo-server-core` to
`apollo-server-express` and directly detect whether `req.body` is empty.
This is a backwards-incompatible change, so we can finally implement it
as v3 is imminent.
The PR consists of:
- Core
- Make an error about needing a query be clear that it needs to be a
non-empty query (the error could fire before if `query` was provided
yet empty).
- Make "POST body missing" error 400 instead of 500, include that it
could be bad content-type or empty JSON object, and don't suggest
(with poor grammar) the use of body-parser (as described above).
Also show this error if httpRequest.query is a string or Buffer
(this can happen with Azure Functions on JSON parse failures).
- Express: Check for a sign that body-parser hasn't been applied here
instead of in Core and provide a more targeted body-parser-suggesting
500.
- Micro: Only parse JSON for `content-type: application/json` (like all
other integrations).
- Azure Functions
- 400 instead of 500 for missing body
- Make test more realistic on parser failures; see Core change for
where this matters.
Fixes #1841 (thanks to @nihalgonsalves for a good set of test cases).
Fixes #4165.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a user error, not a server error, so a 400 status would be more appropriate, and consistent with how mistakes in GET requests are handled. The current behaviour is making it difficult for my team to write monitoring that is triggered by real server errors; if just a single user persistently sends requests missing a body it's enough to trigger alarms.
** Leaving as draft until I fix tests **