Skip to content

Commit 37b3b7f

Browse files
glasserenisdenjo
andauthored
Stricter JSON body validation; run graphql-over-HTTP test suite (#7171)
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. Co-authored-by: Denis Badurina <[email protected]>
1 parent 4ce7381 commit 37b3b7f

File tree

8 files changed

+150
-0
lines changed

8 files changed

+150
-0
lines changed

.changeset/good-pets-begin.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@apollo/server-integration-testsuite': minor
3+
'@apollo/server': minor
4+
---
5+
6+
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.
7+
8+
In addition to being a reasonable idea, this provides more compliance with the "GraphQL over HTTP" spec.
9+
10+
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.

.changeset/tidy-eggs-turn.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@apollo/server-integration-testsuite': patch
3+
---
4+
5+
The integration test suite now incorporates the `graphql-http` package's audit suite for the "GraphQL over HTTP" specification.

docs/source/migration.mdx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,6 +1070,8 @@ Whereas this query would be invalid:
10701070
}
10711071
```
10721072

1073+
(Moreover, Apollo Server 4 responds with a 400 status code if `variables` and `extensions` are provided in a `POST` body with any type other than object, such as array, boolean, or null. Similarly, it responds with a 400 status code if `operationName` is provided in a `POST` body with any type other than string.)
1074+
10731075

10741076
## Changed features
10751077

package-lock.json

Lines changed: 19 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/integration-testsuite/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
"@josephg/resolvable": "^1.0.1",
3737
"body-parser": "^1.20.0",
3838
"express": "^4.18.1",
39+
"graphql-http": "1.8.0",
3940
"graphql-tag": "^2.12.6",
4041
"loglevel": "^1.8.0",
4142
"node-fetch": "^2.6.7",
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import type {
2+
CreateServerForIntegrationTests,
3+
CreateServerForIntegrationTestsResult,
4+
} from './index.js';
5+
import { afterAll, beforeAll, describe, test } from '@jest/globals';
6+
import { serverAudits } from 'graphql-http';
7+
import fetch from 'node-fetch';
8+
9+
export function defineIntegrationTestSuiteHttpSpecTests(
10+
createServer: CreateServerForIntegrationTests,
11+
) {
12+
describe('httpSpecTests.ts', () => {
13+
let createServerResult: CreateServerForIntegrationTestsResult;
14+
15+
beforeAll(async () => {
16+
createServerResult = await createServer({
17+
// Any schema will do (the tests just run `{__typename}`).
18+
typeDefs: 'type Query { x: ID }',
19+
// The test doesn't know we should send apollo-require-preflight along
20+
// with GETs. We could override `fetchFn` to add it but this seems simple enough.
21+
csrfPrevention: false,
22+
});
23+
});
24+
25+
afterAll(async () => {
26+
await createServerResult.server.stop();
27+
await createServerResult.extraCleanup?.();
28+
});
29+
30+
for (const audit of serverAudits({
31+
url: () => createServerResult.url,
32+
fetchFn: fetch,
33+
})) {
34+
test(audit.name, async () => {
35+
const result = await audit.fn();
36+
37+
if (result.status === 'ok') {
38+
return;
39+
}
40+
if (result.status === 'error') {
41+
throw new Error(result.reason);
42+
}
43+
44+
if (result.status !== 'warn') {
45+
throw new Error(`unknown status ${result.status}`);
46+
}
47+
48+
// We failed an optional audit. That's OK, but let's make sure it's
49+
// one of the ones we expect to fail!
50+
51+
// The spec has a bunch of optional suggestions which say that you
52+
// should use 200 rather than 400 for various errors unless opting in to
53+
// the new application/graphql-response+json response type. That's based
54+
// on the theory that "400 + application/json" might come from some
55+
// random proxy layer rather than an actual GraphQL processor and so it
56+
// shouldn't be relied on. (It *does* expect you to use 400 for these
57+
// errors when returning `application/graphql-response+json`, and we
58+
// pass those tests.) But Apollo Server has used non-200 status codes
59+
// for a long time, and in fact a major reason these are merely SHOULDs
60+
// in the spec is so that AS can pass without backwards-incompatible
61+
// changes here. So we ignore these particular SHOULD failures.
62+
if (
63+
audit.name.startsWith('SHOULD use 200 status code') &&
64+
audit.name.endsWith('when accepting application/json') &&
65+
result.reason === 'Status code 400 is not 200'
66+
) {
67+
return;
68+
}
69+
70+
// This is a bit weird: this issue is not actually that we include the 'data'
71+
// entry, but that JSON parse errors aren't delivered as JSON responses at all.
72+
// See https://github.com/graphql/graphql-http/issues/25
73+
if (
74+
audit.name ===
75+
'SHOULD not contain the data entry on JSON parsing failure when accepting application/graphql-response+json'
76+
) {
77+
return;
78+
}
79+
80+
throw new Error(result.reason);
81+
});
82+
}
83+
});
84+
}

packages/integration-testsuite/src/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import type {
77
import { describe } from '@jest/globals';
88
import { defineIntegrationTestSuiteApolloServerTests } from './apolloServerTests.js';
99
import { defineIntegrationTestSuiteHttpServerTests } from './httpServerTests.js';
10+
import { defineIntegrationTestSuiteHttpSpecTests } from './httpSpecTests.js';
1011

1112
export interface CreateServerForIntegrationTestsResult {
1213
server: ApolloServer<BaseContext>;
@@ -33,5 +34,6 @@ export function defineIntegrationTestSuite(
3334
describe('integration tests', () => {
3435
defineIntegrationTestSuiteApolloServerTests(createServer, options);
3536
defineIntegrationTestSuiteHttpServerTests(createServer, options);
37+
defineIntegrationTestSuiteHttpSpecTests(createServer);
3638
});
3739
}

packages/server/src/runHttpQuery.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,33 @@ export async function runHttpQuery<TContext extends BaseContext>({
153153
);
154154
}
155155

156+
if (
157+
'extensions' in httpRequest.body &&
158+
!isStringRecord(httpRequest.body.extensions)
159+
) {
160+
throw new BadRequestError(
161+
'`extensions` in a POST body must be an object if provided.',
162+
);
163+
}
164+
165+
if (
166+
'variables' in httpRequest.body &&
167+
!isStringRecord(httpRequest.body.variables)
168+
) {
169+
throw new BadRequestError(
170+
'`variables` in a POST body must be an object if provided.',
171+
);
172+
}
173+
174+
if (
175+
'operationName' in httpRequest.body &&
176+
typeof httpRequest.body.operationName !== 'string'
177+
) {
178+
throw new BadRequestError(
179+
'`operationName` in a POST body must be a string if provided.',
180+
);
181+
}
182+
156183
graphQLRequest = {
157184
query: fieldIfString(httpRequest.body, 'query'),
158185
operationName: fieldIfString(httpRequest.body, 'operationName'),

0 commit comments

Comments
 (0)