Repository navigation
fix: disable GraphiQL in production by default - #386
Jossy-ossy wants to merge 1 commit into
Conversation
|
@Jossy-ossy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
47cb640 to
285cb5d
Compare
|
@Miracle656 I resolved the merge conflicts and updated this branch. Please take another look and merge when ready. |
Miracle656
left a comment
There was a problem hiding this comment.
This is close, and the design judgement is right. Splitting GraphiQL from introspection is the correct call, and keeping introspection on by default with a stated reason — public read API, clients need schema discovery — is the part I would have argued for. Making it injectable through BuildAppOptions.graphqlUi instead of reading process.env deep in the call stack is also the right shape, and it is what let me test this properly.
It also closes the right issue: #362 is "GraphiQL and introspection are served unconditionally", which is exactly this.
Verified before I say anything else:
tsc --noEmitclean- our dropped-test guard:
No test case present on the base is missing from this branch— 877 → 879 cases - GraphiQL genuinely returns 404 under
environment: 'production' - introspection genuinely is blocked when disabled
So the security goal is met. The blocker is that one of your own tests fails, and it fails for a real reason.
The introspection error is masked
allows operators to configure the IDE and introspection independently expects:
expect(response.json().errors?.[0]?.message).toMatch(/introspection/i)and gets:
{"data":null,"errors":[{"message":"Internal server error","extensions":{"code":"INTERNAL_SERVER_ERROR","requestId":"req-1"}}]}I checked what the rule itself produces, and it is correct — two errors, honest messages, no originalError:
GraphQL introspection has been disabled, but the requested query contained the field "__schema".
GraphQL introspection has been disabled, but the requested query contained the field "queryType".
Two errors arrive, one masked error comes out. So they are being wrapped somewhere between the rule and formatGraphQLErrors, in a shape that isUserFacing does not recognise — none of its three escapes match (!original, Array.isArray(original.errors), statusCode < 500), so it falls through to the mask. I did not pin down the exact wrapper mercurius uses here, and you will find that faster than I will.
Two reasons this matters beyond the red test:
-
It tells the caller the wrong thing. An operator who disables introspection wants clients to learn "introspection is off", not "this server is broken". Your test asserts the right behaviour; the formatter is overriding your intent.
-
It will flood the error log.
formatGraphQLErrorslogs every masked error at error level with'unexpected graphql error'. Introspection probes against a public API are constant — any bot sending{ __schema { ... } }would write an ERROR line. With introspection disabled, routine traffic becomes a stream of fake incidents.
isUserFacing's own comment already says the intended policy out loud:
No originalError means graphql-js itself rejected the document (syntax or validation), which only describes the caller's own query.
A disabled-introspection error is precisely that — it describes the caller's query and leaks nothing about the server. It should be user-facing. Either teach isUserFacing about the wrapped validation case, or have the introspection path report an error carrying a code already in USER_FACING_CODES. Your call which; the first is more general and probably fixes the same masking for query-depth rejections.
Two smaller things
parseBoolean only accepts the literal string true. So GRAPHIQL_ENABLED=1, =yes or =on all mean false. For GraphiQL that fails safe. For introspection it does not: GRAPHQL_INTROSPECTION_ENABLED=1 would disable introspection, which is the opposite of what the operator typing it intends, and they would only find out from client reports. Worth accepting the usual truthy spellings, or rejecting an unparseable value loudly instead of silently reading it as false.
src/index.ts:68 now lies in production. It logs unconditionally:
log.info({ url: `http://localhost:${config.api.port}/graphiql` }, 'GraphiQL available')After this PR that line prints on a server where /graphiql is a 404. Gate it on the same resolved value.
Fix the masking and I will merge this.
Summary
Closes #362