P2: Verify error masking; make masking explicit (#177)

[dkijania]

What & why

Part of the production-readiness epic (#163). Closes #177.

Yoga masks unexpected errors by default, but nothing in the repo guaranteed it stayed on or proved that internal details don't leak.

Changes

• maskedErrors: true set explicitly in the Yoga config — the production posture is now intentional and can't be silently turned off by a future edit.
• Extracted buildYoga from buildServer so the server's exact config is unit-testable (without binding a port).
• Tests:
  • A db_client whose query throws connection to server failed: password=topsecret → the client receives a generic Unexpected error. and the payload contains no password/topsecret/internal text.
  • An unknown-field query still returns the normal GraphQL validation error verbatim (masking doesn't hide client-facing errors).

Testing

• npm run build / npm run lint / npx prettier --debug-check . — clean
• npm run test:unit — all pass (2 new masking assertions)

🤖 Generated with Claude Code

[SanabriaRusso]

Nice change — making maskedErrors explicit is exactly the right posture, and extracting buildYoga so the config is unit-testable is clean. One small, high-value hardening on the new test, plus a merge heads-up.

Pin the exact string the mina-explorer depends on. The Explorer's graceful-degradation fallbacks key on the literal substring Cannot query field (src/services/api/transactions.ts :488/:660/:909 and src/pages/ZkAppsPage.tsx:181 — errorMessage.includes('Cannot query field')), and on a match it silently falls back to the daemon. Your test currently asserts the field name:

assert.match(body.errors[0].message, /thisFieldDoesNotExist/);

Asserting the contract substring instead turns this into a real regression guard for the Explorer — and would catch it if the yoga-5 bump in #194 ever changed how validation errors surface:

// The mina-explorer keys its field-fallback on this exact prefix.
assert.match(body.errors[0].message, /Cannot query field/);

Good news: maskedErrors: true is already the yoga default, and masking only rewrites non-GraphQLError execution errors — so validation errors (and our intentional GraphQLError codes like ACTION_STATE_NOT_FOUND) already pass through verbatim. This change is a safe no-op at runtime that just locks the posture in. 👍

Heads-up: #193 edits the same createYoga({…}) block in server.ts (the graphiql: / parseBoolean lines), so whichever of the two merges second will hit a conflict — just need to make sure both maskedErrors: true and #193's parseBoolean(...) survive the merge.

[dkijania]

Thanks @SanabriaRusso — done in ea553bd.

Pinned the contract string. Swapped to asserting /Cannot query field/ — the substring the Explorer's fallbacks key on. You're right that this is what makes it a regression guard rather than a tautology: the old assertion on the field name would pass under any wording, including one that silently breaks the client.

Kept the field-name assertion alongside it, since the two check different things: that the error identifies the field, and that it uses the wording the Explorer depends on.

Also pinned the status code. Added a case asserting validation errors return 200 for a client sending no Accept header — the Explorer's exact request shape. Its client throws on any non-2xx before it reads the body, so a 400 would break the fallbacks outright, and the 200 rests on an implicit content-negotiation default rather than anything explicit. (#194 verified this empirically across the 4 → 5 bump; there's an equivalent guard on that branch too, so whichever lands first is covered.)

On the #193 conflict — noted, and that's the plan: #193 is wave 1 and this is wave 2, so I'll reconcile createYoga({…}) by hand and make sure both maskedErrors: true and the parseBoolean(...) calls survive.

[SanabriaRusso]

Verdict: MERGEABLE ✅

Second pass focused on HARD CONSTRAINT #1 (GraphQL error text is a load-bearing API for both consumers). I ran the actual installed stack rather than trusting the description — graphql-yoga@4.0.4 + @envelop/core@4.0.3 + graphql@16.8.1, with this repo's real schema.graphql and maskedErrors: true exactly as src/server/server.ts:30 sets it.

What I checked

• maskedErrors: true is a literal no-op vs. the default, not a widening. node_modules/graphql-yoga/cjs/server.js:113-132: the only branch that changes behaviour is options?.maskedErrors === false; true and undefined build the identical opts object (errorMessage: 'Unexpected error.'). So the runtime posture is unchanged — the PR only locks it in. Confirms @SanabriaRusso's first-pass reading.
• Masking cannot reach validation errors, structurally. @envelop/core's useMaskedErrors only registers onExecute/onSubscribe/context-error hooks (node_modules/@envelop/core/cjs/plugins/use-masked-errors.js), and processRequest returns validation errors before execution (node_modules/graphql-yoga/cjs/process-request.js:39-43 — return { errors }). Parse errors take the handleError path but are GraphQLErrors with no originalError, which yoga's maskError returns untouched (cjs/utils/mask-error.js).
• Verified end-to-end against the real schema (yoga 4.0.4, maskedErrors: true, no Accept header — the Explorer's exact request shape). All HTTP 200, all verbatim:
  • Cannot query field "protocolState" on type "Block". → the Explorer's FULL-tier probe and transactions.ts:488 / ZkAppsPage.tsx:183 includes('Cannot query field') still fire.
  • Field "inBestChainXYZ" is not defined by type "BlockQueryInput". Did you mean "inBestChain"? and Field "inBestChain" is not defined by type "EventFilterOptionsInput". → substring inBestChain survives for bestChainFilter.ts:30 and app/upstream/archive.py:80 / archive_account_txs.py:81.
  • Unknown argument "bogusArg" on field "Query.blocks"., Unknown type "NoSuchInput"., Syntax Error: Unexpected <EOF>. → all three SCHEMA_ERROR_MARKERS (app/upstream/graphql.py:32) intact.
• Repo domain errors pass through. Everything a client is meant to see is a real GraphQLError (src/errors/error.ts:6, used by actions-service.ts:83/94/104/107, events-service.ts:72/75) — masking returns those unchanged, extensions (ACTION_STATE_NOT_FOUND, BLOCK_RANGE_ERROR) included; verified in the harness. The only plain Error throws are internal (archive-node-adapter.ts:40/84/91, database-row-adapters.ts:154) and nothing downstream matches them. Note archive-node-adapter.ts:84 interpolates the raw pg failure (Reason: ${e}) — masking that is exactly right. transition frontier / Could not find block come from the daemon endpoint (mina-explorer/src/services/api/daemon.ts:101-102), not this service, so UNAVAILABLE_ERROR_MARKERS are unaffected.
• No leak in the other direction. A resolver throwing connection to server at "10.0.0.5", port 5432 failed: password=topsecret + stack yields exactly {"errors":[{"message":"Unexpected error."}]} — no host, port, password, stack, or node_modules path anywhere in the body. And the new test's leak assertions are not vacuous: setSpanNameFromGraphQLContext (src/tracing/tracer.ts:43-51) doesn't throw without a tracer, so the sensitive error really does reach the wire path.
• CORS/preflight untouched: OPTIONS with Origin + Access-Control-Request-Headers: content-type still → 204 with access-control-allow-methods: GET, POST and allow-headers: content-type. Constraint Add a Dockerfile to build and run the server #2 unaffected by this diff.

The PR does ship the validation-text regression test asked for (tests/unit/error-masking.test.ts:41-77, both the Cannot query field contract string and the HTTP-200 pin), and it's picked up by the test:unit glob, so the silent-regression concern is already covered.

Non-blocking nits

1. Masking is NODE_ENV-gated, and nothing in this repo pins NODE_ENV. @envelop/core computes isDev at module load from NODE_ENV === 'development' and, when true, attaches the original message and stack to extensions.originalError. I reproduced it against this config: with NODE_ENV=development the same request returns "message":"Unexpected error." but extensions.originalError.stack containing password=topsecret and the pg internals. There is no NODE_ENV reference anywhere in the repo (no Dockerfile/compose/manifest sets it), so today this is a latent operator footgun rather than a live leak — but since the PR's whole point is "the posture can't be silently turned off", closing this makes it airtight:

    // Mask unexpected (non-GraphQLError) errors so internal details — SQL,
    // connection strings, stack traces — never reach clients. `isDev: false`
    // pins it: envelop otherwise attaches the original message + stack to
    // `extensions.originalError` whenever NODE_ENV === 'development'.
    maskedErrors: { isDev: false },

Verified this keeps the message Unexpected error. and drops extensions entirely even under NODE_ENV=development (yoga merges it over the defaults at cjs/server.js:115-132, so errorMessage is unchanged).

2. Pin the other two markers while you're here. The test pins Cannot query field, but mina-explorer-api keys on three (app/upstream/graphql.py:32), and the unknown-input-field wording is what inBestChain detection rides on. A table turns one guard into four for ~15 lines — drop-in after the existing tests in tests/unit/error-masking.test.ts (messages below are the literal graphql-16.8.1 output I captured against this schema):

  // Every error marker the downstream consumers string-match must survive
  // masking verbatim:
  //   mina-explorer-api  app/upstream/graphql.py:32  SCHEMA_ERROR_MARKERS
  //   mina-explorer      src/services/api/bestChainFilter.ts:30  ('inBestChain')
  //   mina-explorer      src/services/api/transactions.ts:488, ZkAppsPage.tsx:183
  const VALIDATION_CASES: Array<[string, string, RegExp]> = [
    [
      'Cannot query field',
      '{ blocks(query: { blockHeight_lt: 10 }, limit: 1) { protocolState { consensusState { epoch } } } }',
      /^Cannot query field "protocolState" on type "Block"\./,
    ],
    [
      'unknown input field (inBestChain detection)',
      '{ blocks(query: { inBestChainX: true }, limit: 1) { blockHeight } }',
      /Field "inBestChainX" is not defined by type "BlockQueryInput"\./,
    ],
    [
      'Unknown argument',
      '{ blocks(query: { blockHeight_lt: 10 }, limit: 1, bogus: 3) { blockHeight } }',
      /^Unknown argument "bogus" on field "Query\.blocks"\./,
    ],
    [
      'Unknown type',
      'query Q($x: NoSuchInput!) { blocks(query: { blockHeight_lt: 10 }, limit: 1) { blockHeight } }',
      /^Unknown type "NoSuchInput"\./,
    ],
  ];

  for (const [name, query, expected] of VALIDATION_CASES) {
    test(`validation error reaches the client verbatim: ${name}`, async () => {
      const yoga = buildYoga(throwingContext(), []);
      const response = await yoga.fetch('http://localhost/', {
        method: 'POST',
        // no Accept header — the Explorer's exact request shape
        headers: { 'content-type': 'application/json' },
        body: JSON.stringify({ query }),
      });
      assert.strictEqual(response.status, 200);
      const body = await response.json();
      assert.match(body.errors[0].message, expected);
    });
  }

Both nits are optional; neither blocks. The one thing worth carrying into #194 is nit 2 — a yoga-5 bump is precisely the change that could reword or re-route these, and four pinned markers catch more of it than one.

Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.

[SanabriaRusso]

Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.

Two things this approval does not mean:

• It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
• It does not by itself mean the branch is ready to merge. main requires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.

Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.

[SanabriaRusso]

Approving — and this one is stronger than "no regressions". It closes a live secret leak and it is now the regression guard for the error-text contract mina-explorer depends on.

The contract is byte-identical. I A/B'd main's default maskedErrors against this PR's maskedErrors: { isDev: false } on real graphql-yoga@4.0.4:

case	main	#195
Cannot query field "protocolState" on type "Block".	200	200, identical
Cannot query field "thisFieldDoesNotExist" on type "Query".	200	200, identical
Unknown argument "bogus" on field "Query.blocks".	200	200, identical
Unknown type "NoSuchInput".	200	200, identical
throwing resolver carrying a DSN	200 / Unexpected error.	200 / Unexpected error., identical

Diff with NODE_ENV unset: IDENTICAL on message, status, and extensions.

The mechanism backs this up rather than it being a lucky outcome. graphql-yoga/esm/server.js:109-128 merges an object maskedErrors over the default { errorMessage: 'Unexpected error.' }, so { isDev: false } preserves the default message and only pins isDev. And utils/mask-error.js returns the error unchanged when isGraphQLError(error) && !error.originalError — precisely what a graphql-js validation error is. Validation errors structurally cannot be masked here.

Worth noting those four strings are exactly the contract: mina-explorer-api matches Cannot query field, Unknown argument, and Unknown type (app/upstream/graphql.py:33-42) to drive tier fallback and poison its capability cache, and mina-explorer keys on Cannot query field in transactions.ts:488,556,698,977 and ZkAppsPage.tsx:183. All three markers are covered.

Statuses unchanged too, across all four Accept negotiations: no header → 200, */* → 200, application/json → 200, application/graphql-response+json → 400. Identical in both configs (error.js:getResponseInitByRespectingErrors skips the spec 400 when isApplicationJson). Neither consumer sends the spec type, so both keep their 200 — now pinned by the test at tests/unit/error-masking.test.ts:133-144.

The tests assert the literal strings, not a vague "an error occurred": anchored literals at :10-31 for all four validation shapes, plus :118-131 asserting both /Cannot query field/ and /thisFieldDoesNotExist/. That is a real guard on the Explorer's tier detection.

It fixes a live leak. With NODE_ENV=development — which the Compose/dev flow sets — main leaks the full connection string including the password into extensions.originalError. Observed verbatim on main:

"originalError": {
  "message": "connection to server failed: postgres://user:topsecret@db:5432/archive",
  "stack":   "Error: connection to server failed: postgres://user:topsecret@db:5432/archive\n    at ..."
}

With isDev: false that extensions object is undefined. This is the only behavioural delta versus main, and it's strictly an improvement. Masking cannot leak a PG error generally either: the masked branch builds a fresh createGraphQLError carrying only nodes/source/positions/path/extensions, dropping the original message and stack — proven by the test at :47-67, which asserts neither topsecret nor password appears anywhere in the raw response text.

#183 (graphql-armor) cross-check — noting for that PR, not blocking here. This PR neither improves nor worsens the interaction. Armor rules throw a GraphQLError out of validate(), and isGraphQLError && !originalError ⇒ unmasked. Confirmed A/B: both configs return ["Syntax Error: Query depth limit of 6 exceeded, found 7."] at 200; a rule throwing a plain Error gives ["Unexpected error."] at 500 under both. So round 1's concern is unchanged and still belongs on #183: when a rule trips, only armor's message comes back, so a probe that is both too deep and references an unknown field loses its Cannot query field marker. #183's shipped headroom (depth 12 vs a depth-7 worst case) keeps that unreachable in practice.

Non-blocking nits

1. isDev: false is unconditional, so a dev running NODE_ENV=development loses extensions.originalError in the response body. That's the intent, and the original is still on the server side (yoga's logger.error(error), plus #190's structured error line) — one doc line pointing devs there would smooth the DX.
2. buildYoga (server.ts:7-10) is exported purely for testability. Fine, but it widens the module's public surface; a /** @internal */ marker would say so.

Merge note — read this before resolving. src/server/server.ts conflicts with #190, and a naive resolution silently loses something. #190 replaces logging: LOG_LEVEL with a YogaLogger object (that's its fix for LOG_LEVEL=fatal emitting debug); this PR keeps logging: LOG_LEVEL, splits out buildYoga, and adds maskedErrors. Taking either side wholesale re-breaks the other. The merged file must keep all three — buildYoga, maskedErrors: { isDev: false }, and #190's logging: { debug, info, warn, error }.

Dropping maskedErrors in the resolution would be silent: tests/unit/error-masking.test.ts would still pass, because with NODE_ENV unset the behaviour is identical to main — only the NODE_ENV=development leak would come back, and nothing in CI sets that. Either add an assertion that maskedErrors is explicitly configured, or merge this PR before #190.

Downstream: zero impact, verified. The Cannot query field string driving mina-explorer's fallback chains is unchanged at unchanged HTTP 200 and now pinned by five assertions; mina-explorer-api's { __typename } probe path is untouched (maskedErrors is a config value, not a plugin, so no new per-request work against its 20 s timeout).

[SanabriaRusso]

Re-approving after the rebase (the previous approval was dismissed by the force-push).

Re-verified: the PR content at 23fbe7a is byte-identical to the commit I approved (4e17023) once both are diffed against their respective merge bases. The rebase onto 7361526 was mechanical.

Carrying forward the round-2 verification unchanged:

• Stronger than "no regressions". A/B against main across five cases: identical message, status, and extensions with NODE_ENV unset.
• Mechanism confirmed: mask-error.js returns the error unchanged when isGraphQLError && !originalError, so validation errors structurally cannot be masked. Statuses identical across all four Accept negotiations.
• Tests assert the literal strings, anchored.
• It fixes a live leak: with NODE_ENV=development — which the Compose flow sets — main leaks the full connection string including the password into extensions.originalError. This PR suppresses it.

Merge-train note — this is the silent one. #190 and this PR both rewrite src/server/server.ts. #190 replaces logging: LOG_LEVEL with a YogaLogger object; this PR keeps logging: LOG_LEVEL, splits out buildYoga, and adds maskedErrors: { isDev: false }. Dropping this PR's maskedErrors during conflict resolution is invisible: its own test suite still passes, because with NODE_ENV unset the behaviour is identical to main. Only the NODE_ENV=development password leak comes back, and nothing in CI sets that. Merge this before #190.