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
Stricter JSON body validation; run graphql-over-HTTP test suite #7171
Conversation
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 56684d9:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into the codecov funkiness but otherwise LGTM. Cool addition and glad they made this into a package we can run locally!
@@ -36,6 +36,7 @@ | |||
"@josephg/resolvable": "^1.0.1", | |||
"body-parser": "^1.20.0", | |||
"express": "^4.18.1", | |||
"graphql-http": "1.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs some renovate config to enforce the pin due to:
https://github.com/apollographql/renovate-config-apollo-open-source/blob/master/package.json#L15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thiiiink this might actually be ok (after all it doesn't un-pin @apollo/server
) but we can also find out the hard way?
This PR adds the spec audit suite from `graphql-http` (designed to validate compatibility with the "GraphQL over HTTP" specification) to the integration test suite. It expects all required (MUST) audits to pass, and for all optional (MAY/SHOULD) audits to pass that we haven't explicitly excepted. All required audits already passed. Failing optional audits fell into three categories: - Ignoring `operationName`, `variables`, and `extensions` when provided with incorrect types instead of returning a 400 error. The SHOULD here seemed reasonable and so this PR changes our POST handler to return 400s if incorrect types are provided here. As described in the changeset, this is backwards incompatible but we are comfortable making this choice today. - Returning 400 errors for various error conditions like GraphQL parse errors, for the original `application/json` response type. The one aspect of the GoH spec that is prescriptive rather than descriptive is the invention of the `application/graphql-response+json` response MIME type; the theory is that you can't really "trust" that a non-2xx response with MIME type `application/json` is actually generated by the GraphQL server rather than by some other proxy, so it has you use this new type along with 4xx for these errors, and then SHOULDs that these errors come with 200 for the original MIME type. The main reason that this is a SHOULD is because many servers like Apollo Server already return 400s in this case, and we're not interested in changing that. So we ignore these particular errors. - There's a sorta buggy audit that has opinions about how bad JSON errors should look. See graphql/graphql-http#25 Note that we deliberately make a strict dependency on a single version of graphql-http, so that the tests run by a given version of `@apollo/server-integration-test-suite` are well defined. Fixes #7158.
To be honest I'm not sure why this is valid (TS has no way of knowing that the beforeAll callback gets called before the others?) but hey, it works and if it's ever not valid tests would fail.
a20c2cc
to
56684d9
Compare
I don't understand the codecov result: those lines are definitely being run by the new tests... |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-integration-testsuite@4.2.0 ### Minor Changes - [#7171](#7171) [`37b3b7fb5`](37b3b7f) Thanks [@glasser](https://github.com/glasser)! - If a POST body contains a non-string `operationName` or a non-object `variables` or `extensions`, fail with status code 400 instead of ignoring the field. In addition to being a reasonable idea, this provides more compliance with the "GraphQL over HTTP" spec. This is a backwards incompatible change, but we are still early in the Apollo Server 4 adoption cycle and this is in line with the change already made in Apollo Server 4 to reject requests providing `variables` or `extensions` as strings. If this causes major problems for users who have already upgraded to Apollo Server 4 in production, we can consider reverting or partially reverting this change. ### Patch Changes - [#7170](#7170) [`4ce738193`](4ce7381) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update @apollo/utils packages to v2 (dropping node 12 support) - [#7179](#7179) [`c8129c23f`](c8129c2) Thanks [@renovate](https://github.com/apps/renovate)! - Fix a few tests to support (but not require) TypeScript 4.9. - [#7171](#7171) [`37b3b7fb5`](37b3b7f) Thanks [@glasser](https://github.com/glasser)! - The integration test suite now incorporates the `graphql-http` package's audit suite for the "GraphQL over HTTP" specification. - [#7183](#7183) [`46af8255c`](46af825) Thanks [@glasser](https://github.com/glasser)! - Apollo Server tries to detect if execution errors are variable coercion errors in order to give them a `code` extension of `BAD_USER_INPUT` rather than `INTERNAL_SERVER_ERROR`. Previously this would unconditionally set the `code`; now, it only sets the `code` if no `code` is already set, so that (for example) custom scalar `parseValue` methods can throw errors with specific `code`s. (Note that a separate graphql-js bug can lead to these extensions being lost; see <graphql/graphql-js#3785> for details.) - Updated dependencies \[[`4ce738193`](4ce7381), [`37b3b7fb5`](37b3b7f), [`b1548c1d6`](b1548c1), [`7ff96f533`](7ff96f5), [`46af8255c`](46af825)]: - @apollo/server@4.2.0 ## @apollo/server@4.2.0 ### Minor Changes - [#7171](#7171) [`37b3b7fb5`](37b3b7f) Thanks [@glasser](https://github.com/glasser)! - If a POST body contains a non-string `operationName` or a non-object `variables` or `extensions`, fail with status code 400 instead of ignoring the field. In addition to being a reasonable idea, this provides more compliance with the "GraphQL over HTTP" spec. This is a backwards incompatible change, but we are still early in the Apollo Server 4 adoption cycle and this is in line with the change already made in Apollo Server 4 to reject requests providing `variables` or `extensions` as strings. If this causes major problems for users who have already upgraded to Apollo Server 4 in production, we can consider reverting or partially reverting this change. - [#7184](#7184) [`b1548c1d6`](b1548c1) Thanks [@glasser](https://github.com/glasser)! - Don't automatically install the usage reporting plugin in servers that appear to be hosting a federated subgraph (based on the existence of a field `_Service.sdl: String`). This is generally a misconfiguration. If an API key and graph ref are provided to the subgraph, log a warning and do not enable the usage reporting plugin. If the usage reporting plugin is explicitly installed in a subgraph, log a warning but keep it enabled. ### Patch Changes - [#7170](#7170) [`4ce738193`](4ce7381) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update @apollo/utils packages to v2 (dropping node 12 support) - [#7172](#7172) [`7ff96f533`](7ff96f5) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - startStandaloneServer: Restore body-parser request limit to 50mb (as it was in the `apollo-server` package in Apollo Server 3) - [#7183](#7183) [`46af8255c`](46af825) Thanks [@glasser](https://github.com/glasser)! - Apollo Server tries to detect if execution errors are variable coercion errors in order to give them a `code` extension of `BAD_USER_INPUT` rather than `INTERNAL_SERVER_ERROR`. Previously this would unconditionally set the `code`; now, it only sets the `code` if no `code` is already set, so that (for example) custom scalar `parseValue` methods can throw errors with specific `code`s. (Note that a separate graphql-js bug can lead to these extensions being lost; see <graphql/graphql-js#3785> for details.) - Updated dependencies \[[`4ce738193`](4ce7381), [`45856e1dd`](45856e1)]: - @apollo/server-gateway-interface@1.0.6 ## @apollo/server-gateway-interface@1.0.6 ### Patch Changes - [#7170](#7170) [`4ce738193`](4ce7381) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update @apollo/utils packages to v2 (dropping node 12 support) - [#7173](#7173) [`45856e1dd`](45856e1) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Remove unnecessary engines constraint on types-only package ## @apollo/server-plugin-response-cache@4.0.2 ### Patch Changes - [#7170](#7170) [`4ce738193`](4ce7381) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update @apollo/utils packages to v2 (dropping node 12 support) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
In v4.2.0 (#7171) we changed POST handling to be stricter if `operationName`, `variables`, or `extensions` were provided with a surprising data type. This was intended to pass more of the optional recommendations of the GraphQL Over HTTP spec as tested by the graphql-http audit suite. However, we were overzealous and also banned providing these parameters as an explicit `null`, which is documented by the spec as legitimate. (And some clients, such as FIXME, actually send `variables: null` in practice.) We added explicit tests for this to the `graphql-http` test suite (graphql/graphql-http#28) and this commit allows these `null`s again. Fixes #7200.
In v4.2.0 (#7171) we changed POST handling to be stricter if `operationName`, `variables`, or `extensions` were provided with a surprising data type. This was intended to pass more of the optional recommendations of the GraphQL Over HTTP spec as tested by the graphql-http audit suite. However, we were overzealous and also banned providing these parameters as an explicit `null`, which is documented by the spec as legitimate. (And some clients, such as FIXME, actually send `variables: null` in practice.) We added explicit tests for this to the `graphql-http` test suite (graphql/graphql-http#28) and this commit allows these `null`s again. Fixes #7200.
In v4.2.0 (#7171) we changed POST handling to be stricter if `operationName`, `variables`, or `extensions` were provided with a surprising data type. This was intended to pass more of the optional recommendations of the GraphQL Over HTTP spec as tested by the graphql-http audit suite. However, we were overzealous and also banned providing these parameters as an explicit `null`, which is documented by the spec as legitimate. (And some clients, such as FIXME, actually send `variables: null` in practice.) We added explicit tests for this to the `graphql-http` test suite (graphql/graphql-http#28) and this commit allows these `null`s again. Fixes #7200.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-integration-testsuite@4.2.2 ### Patch Changes - [#7203](#7203) [`2042ee761`](2042ee7) Thanks [@glasser](https://github.com/glasser)! - Fix v4.2.0 (#7171) regression where `"operationName": null`, `"variables": null`, and `"extensions": null` in POST bodies were improperly rejected. - Updated dependencies \[[`2042ee761`](2042ee7)]: - @apollo/server@4.2.2 ## @apollo/server@4.2.2 ### Patch Changes - [#7203](#7203) [`2042ee761`](2042ee7) Thanks [@glasser](https://github.com/glasser)! - Fix v4.2.0 (#7171) regression where `"operationName": null`, `"variables": null`, and `"extensions": null` in POST bodies were improperly rejected. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This PR adds the spec audit suite from
graphql-http
(designed to validate compatibility with the "GraphQL over HTTP" specification) to the integration test suite. It expects all required (MUST) audits to pass, and for all optional (MAY/SHOULD) audits to pass that we haven't explicitly excepted.All required audits already passed. Failing optional audits fell into three categories:
operationName
,variables
, andextensions
when provided with incorrect types instead of returning a 400 error. The SHOULD here seemed reasonable and so this PR changes our POST handler to return 400s if incorrect types are provided here. As described in the changeset, this is backwards incompatible but we are comfortable making this choice today.application/json
response type. The one aspect of the GoH spec that is prescriptive rather than descriptive is the invention of theapplication/graphql-response+json
response MIME type; the theory is that you can't really "trust" that a non-2xx response with MIME typeapplication/json
is actually generated by the GraphQL server rather than by some other proxy, so it has you use this new type along with 4xx for these errors, and then SHOULDs that these errors come with 200 for the original MIME type. The main reason that this is a SHOULD is because many servers like Apollo Server already return 400s in this case, and we're not interested in changing that. So we ignore these particular errors.SHOULD not contain the data entry on JSON parsing failure when accepting application/graphql-response+json
also fails on non-JSON responses graphql/graphql-http#25Note that we deliberately make a strict dependency on a single version of graphql-http, so that the tests run by a given version of
@apollo/server-integration-test-suite
are well defined.Fixes #7158.