Skip to content

fix(audits): inconsistency between D6D5 and 6A70 #147

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

Merged
merged 1 commit into from
Jun 6, 2025

Conversation

DoctorJohn
Copy link
Contributor

D6D5 and 6A70 essentially perform the same audit check. However, while 6A70 checks that the execution result has no errors, D6D5 does not. This PR updates D6D5 to also check that the execution result has no errors (i.e, make both audit functions do the same assertions).

Relevant code:

audit(
'D6D5',
'MAY allow URL-encoded JSON string {variables} parameter in GETs when accepting application/graphql-response+json',
async () => {
const url = new URL(await getUrl(opts.url));
url.searchParams.set(
'query',
'query Type($name: String!) { __type(name: $name) { name } }',
);
url.searchParams.set('variables', JSON.stringify({ name: 'sometype' }));
const res = await fetchFn(url.toString(), {
method: 'GET',
headers: {
accept: 'application/graphql-response+json',
},
});
ressert(res).status.toBe(200);
},
),
audit(
'6A70',
'MAY allow URL-encoded JSON string {variables} parameter in GETs when accepting application/json',
async () => {
const url = new URL(await getUrl(opts.url));
url.searchParams.set(
'query',
'query Type($name: String!) { __type(name: $name) { name } }',
);
url.searchParams.set('variables', JSON.stringify({ name: 'sometype' }));
const res = await fetchFn(url.toString(), {
method: 'GET',
headers: {
accept: 'application/json',
},
});
ressert(res).status.toBe(200);
await ressert(res).bodyAsExecutionResult.notToHaveProperty('errors');
},
),

The check that's only present in 6A70 but not in D6D5:

await ressert(res).bodyAsExecutionResult.notToHaveProperty('errors');

@enisdenjo
Copy link
Member

enisdenjo commented Jun 6, 2025

There is a fine detail here: application/json uses 200 almost always (true legacy servers truly always) and the only way to detect an error there is by looking at the errors entry - which is what the test checks; on the other hand, application/graphql-response+json will return a 400 in case of invalid parameters, not needing to check the errors entry.

But nevertheless, the extra assertion wont hurt!

@enisdenjo enisdenjo merged commit eac6dc8 into graphql:main Jun 6, 2025
20 of 21 checks passed
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