Skip to content

fix(audits): incorrect status code range #145

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DoctorJohn
Copy link
Contributor

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

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.

Copy link

linux-foundation-easycla bot commented Jun 6, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

DoctorJohn commented Jun 6, 2025

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

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);
},
),

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

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);
},
),

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
Copy link
Member

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

  1. 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 DoctorJohn closed this Jun 25, 2025
@DoctorJohn DoctorJohn force-pushed the fix-B6DC-status-code-range branch from 9a425d7 to 5587c03 Compare June 25, 2025 14:43
@DoctorJohn
Copy link
Contributor Author

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

@DoctorJohn DoctorJohn reopened this Jun 25, 2025
@DoctorJohn
Copy link
Contributor Author

DoctorJohn commented Jun 25, 2025

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).

@DoctorJohn DoctorJohn force-pushed the fix-B6DC-status-code-range branch from 426cc00 to ec5c8c2 Compare June 25, 2025 15:39
@DoctorJohn DoctorJohn requested a review from enisdenjo June 25, 2025 15:46
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.

2 participants