Skip to content

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