fix(audits): incorrect status code range

[DoctorJohn]

This PR fixes the status code range assertion of audit check B6DC, which did not match the audit name/description.

graphql-http/src/audits/server.ts

Lines 559 to 572 in a49c45b

	audit(
	'B6DC',
	'MAY use 4xx or 5xx status codes on JSON parsing failure',
	async () => {
	const res = await fetchFn(await getUrl(opts.url), {
	method: 'POST',
	headers: {
	'content-type': 'application/json',
	},
	body: '{ "not a JSON',
	});
	ressert(res).status.toBeBetween(400, 499);
	},
	),

The name/description suggests 5xx status codes may be used. However, the audit function currently requires the status code to be between 400 and 499.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: DoctorJohn / name: Jonathan Ehwald (4e9b409, e1d314c)

[enisdenjo]

Actually, as per the spec, the expected status code should be 400 for both content-types. Would you like to make the necessary changes?

application/json
https://graphql.github.io/graphql-over-http/draft/#sec-application-json.Examples.JSON-parsing-failure

application/graphql-response+json
https://graphql.github.io/graphql-over-http/draft/#sec-application-graphql-response-json.Examples.JSON-parsing-failure

[DoctorJohn]

Good point!

For application/graphql-response+json, 865D could then be removed because 556A already checks that the status code should be 400. I just checked the spec and found no reason why anything other than 400 would be allowed.

Code for 865D and 556A

graphql-http/src/audits/server.ts

Lines 672 to 689 in a49c45b

	audit(
	// TODO: convert to MUST after watershed
	'865D',
	'SHOULD use 4xx or 5xx status codes on document parsing failure when accepting application/graphql-response+json',
	async () => {
	const res = await fetchFn(await getUrl(opts.url), {
	method: 'POST',
	headers: {
	'content-type': 'application/json',
	accept: 'application/graphql-response+json',
	},
	body: JSON.stringify({
	query: '{',
	}),
	});
	ressert(res).status.toBeBetween(400, 599);
	},
	),

graphql-http/src/audits/server.ts

Lines 690 to 706 in a49c45b

	audit(
	'556A',
	'SHOULD use 400 status code on document parsing failure when accepting application/graphql-response+json',
	async () => {
	const res = await fetchFn(await getUrl(opts.url), {
	method: 'POST',
	headers: {
	'content-type': 'application/json',
	accept: 'application/graphql-response+json',
	},
	body: JSON.stringify({
	query: '{',
	}),
	});
	ressert(res).status.toBe(400);
	},
	),

For application/json, there's this note which would technically allow 2xx and 5xx in addition to 400 on JSON parsing failure, if I understand correctly. I believe that's what B6DC is trying to check, while BCF8 covers that the status code should be 400.

Code for B6DC and BCF8

graphql-http/src/audits/server.ts

Lines 559 to 572 in a49c45b

	audit(
	'B6DC',
	'MAY use 4xx or 5xx status codes on JSON parsing failure',
	async () => {
	const res = await fetchFn(await getUrl(opts.url), {
	method: 'POST',
	headers: {
	'content-type': 'application/json',
	},
	body: '{ "not a JSON',
	});
	ressert(res).status.toBeBetween(400, 499);
	},
	),

graphql-http/src/audits/server.ts

Lines 573 to 586 in a49c45b

	audit(
	'BCF8',
	'MAY use 400 status code on JSON parsing failure',
	async () => {
	const res = await fetchFn(await getUrl(opts.url), {
	method: 'POST',
	headers: {
	'content-type': 'application/json',
	},
	body: '{ "not a JSON',
	});
	ressert(res).status.toBe(400);
	},
	),

I would proceed like this:
1. Remove 865D
2. Update B6DC to allow 2xx, 400, and 5xx for compatibility with legacy servers
3. Update 8CF8 from MAY to SHOULD to match section 6.4.1.1.1

[enisdenjo]

Historically we allowed 4XX which is why those cases exist, but are now obsolete.

2. Update B6DC to allow 2xx, 400, and 5xx for compatibility with legacy servers

Note that it should allow 2XX only when accepting application/json, not the new content type.

Other than that, we're looking good. Thanks!

[DoctorJohn]

(woops, accidentally closed this by resetting my changes to keep working on them. Going to reopen in a few minutes).

[DoctorJohn]

Finally found time to wrap things up. Out of the three proposed changes, I ultimately implemented numbers 2 and 3. Number 1 turned out to be unrelated to the other changes, because I mixed up document/query parsing failures and request body parsing failures.

Here's a summary of the changes:

• I updated B6DC to check the extra status codes, and clarified that the check is for the application/json media type
• I updated BCF8 from MAY to SHOULD to match the spec, and also clarified that the check is for the application/json media type

It's two separate commits, each referencing the relevant parts in the specs in the commit message body.

Regarding BCF8, the specification is a bit unclear regarding 400 versus 4xx:
Example 6.4.1.1.1 JSON parsing failure says the server SHOULD respond with the 400 status code. In contrast, section 6.4.1 says the server SHOULD respond with a 4xx status code. The audit check was, and still is, checking 400 (not 4xx).

[enisdenjo]

Great work, this is looking good! 🙏