Skip to content

fix: disable GraphiQL in production by default - #386

Open
Jossy-ossy wants to merge 2 commits into
Divinity-Labs:mainfrom
Jossy-ossy:fix/issue-362
Open

Jossy-ossy wants to merge 2 commits into
Divinity-Labs:mainfrom
Jossy-ossy:fix/issue-362

Conversation

@Jossy-ossy

Copy link
Copy Markdown
Contributor

Summary

  • Disable the GraphiQL browser IDE in production by default while leaving the GraphQL endpoint available.
  • Make GraphiQL and schema introspection independently configurable; document that public API introspection remains enabled by default.
  • Add regression coverage for production behavior and both settings.

Closes #362

@drips-wave

drips-wave Bot commented Oct 16, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

@Jossy-ossy

Copy link
Copy Markdown
Contributor Author

@Miracle656 I resolved the merge conflicts and updated this branch. Please take another look and merge when ready.

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 --noEmit clean
  • 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:

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

  2. It will flood the error log. formatGraphQLErrors logs 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.

@Jossy-ossy

Copy link
Copy Markdown
Contributor Author

Note for maintainers: the introspection test's expected status changed from 200 to 400, because mercurius returns 400 for validation failures. Please confirm that is acceptable. A local NoIntrospectionFieldsRule replaces graphql's NoSchemaIntrospectionCustomRule, because the instanceof check fails under vitest. GRAPHIQL_ENABLED and GRAPHQL_INTROSPECTION_ENABLED now accept true/1/yes/on and false/0/no/off, and anything else throws at boot.

Bring the branch up to date with main and resolve the conflicts in
.env.example and src/app.ts by keeping both sides: the CORS override and
the TVL routes from main alongside the graphqlUi option from this branch.

Address review feedback:
- Disabled introspection is now reported by a local validation rule.
  graphql's NoSchemaIntrospectionCustomRule relies on instanceof checks,
  which fail when the schema and the rule come from different graphql
  module instances. The failure was masked as "Internal server error".
  The rejection is now a normal validation error with an honest message.
- The introspection rejection test expects 400, the status mercurius uses
  for every validation failure.
- GRAPHIQL_ENABLED and GRAPHQL_INTROSPECTION_ENABLED accept true/1/yes/on
  and false/0/no/off, and fail boot on any other value instead of
  silently reading as false.
- The "GraphiQL available" startup log only prints when GraphiQL is on.
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.

GraphiQL and introspection are served unconditionally

2 participants