P0: Add GraphQL query-cost controls via graphql-armor (#164) - #183
P0: Add GraphQL query-cost controls via graphql-armor (#164)#183dkijania wants to merge 2 commits into
Conversation
|
Verdict: MERGEABLE ✅ I measured every configured limit against the actual queries the two downstream consumers send, running the real What I checked1. Plugins enabled at head (
2. Measured metrics for the real consumer queries (depth/alias/cost taken from the plugins' own
The 3. // @escape.tech/graphql-armor-block-field-suggestions@3.0.1
error.message = error.message.replace(/Did you mean ".+"\?/g, mask).trim();Driven end-to-end through Yoga 4 with So 4. Rejection messages can't be confused with the markers, and rejections don't retry-amplify. All four limits reject with 5. Introspection / GraphiQL unchanged. 6. No hidden parser-cache bypass. Yoga adds All four limits are env-tunable with documented defaults ( Non-blocking nits
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
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.
mainrequires 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.
2ea7cec to
c53ddeb
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving. The round-1 carry-forward is fully addressed, and I verified the numbers by execution against this repo's real schema and resolved dependency versions rather than by inspection.
Verified fixed
maxDepth: 12atsrc/server/graphql-armor.ts:34, with the floor documented in the config table: "Do not set below8: known clients send depth-7 probes, and a depth rejection replaces theCannot query fielderror those clients rely on for schema-tier fallback." Mirrored in both.env.example.*files.- Armor's own default of 6 is unreachable.
buildArmorPluginsalways passes{ n: config.maxDepth }explicitly (:72), andresolveArmorConfigfalls back toARMOR_DEFAULTS(12), never to the library default, for unset / blank / non-integer /<1values (:50,:56-59). - Headroom, measured. Minimum
maxDepththat passes each real consumer query: searchTransactionFull 7, analytics 4, blocksFullPaginatedBestChain 4,{ __typename }1 — 5 levels of headroom at 12, and 1 at the documented floor of 8. Measured cost of the heaviest query: 160.671875 vsmaxCost: 5000(31×). Tokens 89 vs 1000; aliases 0 vs 15. - Round 1's concern was real and is now closed: at
n: 6the depth-7 query is rejected —Syntax Error: Query depth limit of 6 exceeded, found 7. costLimit's multiplier still only fires onfirst/last— re-confirmed by direct experiment on the same query shape:l1(limit: 2000)→ cost 12.875;l1(first: 2000)→ cost 25750.limit: 2000contributes nothing.- All four limits are env-configurable and present in
.env.example.compose,.env.example.lightnet,src/envionment.d.ts, and the docs table.
On the silent-tier-degradation risk — the main downstream question, stated plainly
A tripped armor rule still aborts validation and therefore still suppresses Cannot query field. What has changed is that it is now only reachable by misconfiguration (12 vs a depth-7 worst case, floor documented at 8). And when it does trip, the failure is loud, not silent: armor emits Syntax Error: Query depth limit … / Aliases limit … / Token limit … / Query Cost limit …, and none of those contain any of mina-explorer-api's SCHEMA_ERROR_MARKERS (app/upstream/graphql.py:31-35 — "Cannot query field", "Unknown argument", "Unknown type") or its UNAVAILABLE_ERROR_MARKERS. It classifies as a generic UpstreamError, so it will not poison the capability cache or trigger a bogus tier downgrade. That is the right side to fail on.
One consequence worth knowing for capacity planning: in mina-explorer-api, a generic UpstreamError maps to outcome="unavailable" → breaker.record_failure() (app/upstream/graphql.py, query()). So repeated armor rejections drive that consumer's per-endpoint circuit breaker toward OPEN rather than merely failing the individual query. With 5× depth and 31× cost headroom this should never fire in practice — it is an argument for keeping the headroom, not against the PR.
Non-blocking
- The floor is documented but not enforced.
graphql-armor.ts:50accepts any integer>= 1, soGRAPHQL_MAX_DEPTH=6is silently honoured and reinstates exactly what the doc warns about. Consider making it real:
/** Below this, known downstream depth-7 probes break — see docs/getting-started.md. */
const MIN_SAFE_DEPTH = 8;
const maxDepth = intFromEnv(env.GRAPHQL_MAX_DEPTH, ARMOR_DEFAULTS.maxDepth);
if (maxDepth < MIN_SAFE_DEPTH) {
console.warn(
`[armor] GRAPHQL_MAX_DEPTH=${maxDepth} is below the safe floor of ${MIN_SAFE_DEPTH}: ` +
'known clients send depth-7 queries and a depth rejection replaces the ' +
'"Cannot query field" error they use for schema-tier fallback.'
);
}- The floor is pinned only indirectly.
resolveArmorConfig({}) deepStrictEqual ARMOR_DEFAULTSis tautological and would pass at 6. What actually catches a regression is thesearchTransactionFull is not rejected by any armor limitcase — that query needsmaxDepth >= 7, and I confirmed its strict regex (/Syntax Error: (Query depth limit|Query Cost limit|Token limit|Aliases limit)/) matches armor's real message strings byte-for-byte, so it is not passing vacuously. A regression to 8–11 would still slip through. One explicit line closes it:
test('the default depth stays above the documented floor for downstream clients', () => {
// Deepest known real query is 7 (mina-explorer SearchTransaction FULL tier);
// armor's own default of 6 would reject it.
assert.ok(ARMOR_DEFAULTS.maxDepth >= 8, `maxDepth floor breached: ${ARMOR_DEFAULTS.maxDepth}`);
});maxCost: 5000is safe only because this schema useslimit:. The same query written withfirst: 2000costs 25750 — 5× over. Worth a comment atgraphql-armor.ts:37so whoever adds afirst/lastpagination argument (#162 is a live candidate) revisitsGRAPHQL_MAX_COSTin the same PR.- The lockfile bumps
graphql16.8.1 → 16.14.2 (transitively — armor requires^16.10.0). Both consumers treat graphql-js error text as a contract. The text is stable across 16.x and yourstartsWith('Cannot query field "protocolState" on type "Block".')assertion pins it, so this is fine — but please call it out in the PR description rather than leaving it buried in the lockfile diff. - Merge-order note:
src/server/plugins.tsis touched by #183, #185, #190 and #191, all prepending to the same array. No functional conflict for this PR specifically (armor hooks validation, notonRequest), but expect a textual one.
Downstream: none at the shipped defaults, verified by execution. All three real consumer queries and the { __typename } probe pass every limit with 5× depth, 31× cost and 11× token headroom. blockFieldSuggestions preserves Cannot query field "X" on type "Y". verbatim, so explorer-api's tier fallback and the explorer's unsupported-filter detection are untouched. The only way to break a consumer is to set GRAPHQL_MAX_DEPTH below 8, which the docs explicitly warn against.
c53ddeb to
8cb9a2d
Compare
The public GraphQL endpoint had no query-cost controls. The list fields (`events`, `actions`, `blocks`) return unbounded lists, so a deeply-nested or heavily-aliased query is a trivial denial-of-service against the backing Postgres — it can be made arbitrarily expensive before any limit applies. Add graphql-armor validation plugins, wired ahead of execution in `buildPlugins`, with conservative, env-tunable limits: - GRAPHQL_MAX_DEPTH (selection-set nesting, default 10) - GRAPHQL_MAX_ALIASES (aliases per operation, default 15) - GRAPHQL_MAX_TOKENS (lexical tokens per document, default 1000) - GRAPHQL_MAX_COST (depth/field cost heuristic, default 5000) Field suggestions are always blocked so error messages don't leak schema shape (complementing `useDisableIntrospection`); introspection is ignored by the depth/cost rules so GraphiQL still works when explicitly enabled. The deepest query this API legitimately serves is ~5 levels, well within the defaults. Malformed env values fall back to the safe default rather than disabling a protection. The individual `@escape.tech/graphql-armor-*` plugins are used (not the meta package) because the meta package requires @envelop/core v5 while this stack is pinned to v4 (Yoga 4); the sub-plugins have no conflicting peer deps. The full graphql-armor meta integration can follow the Yoga 5 upgrade (#176). Unit tests cover config parsing/fallbacks and prove end-to-end through Yoga that an over-depth query is rejected before execution. Docs, env example, and env type declarations updated. Closes #164. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
8cb9a2d to
441a52d
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Re-approving after the rebase (the previous approval was dismissed by the force-push).
Re-verified: the PR content at 441a52d differs from the commit I approved (c53ddeb) by one blank line in src/server/plugins.ts — absorbed while rebasing over #191's metrics plugin, which now sits in the same function. No semantic change. The lockfile still carries full resolved/integrity metadata for all 1602 entries.
Carrying forward the round-2 verification unchanged:
maxDepth12, floor documented at 8, armor's own default of 6 unreachable.- Headroom measured against the real schema: the deepest real consumer query is depth 7; cost 160.67 vs 5000 (31x); tokens 89 vs 1000. Confirmed
n:6does reject the depth-7 query, so the round-1 concern was real. costLimitmultiplier re-confirmed to fire only onfirst/last:(limit:2000)→ 12.875,(first:2000)→ 25750.
Non-blocking:
- The depth floor is documented but not enforced in code.
maxCost5000 is only safe while the schema avoidsfirst/last— relevant to #162.- The lockfile bumps
graphql16.8.1 → 16.14.2; worth naming in the PR description. - Keep the current 5x/31x headroom: mina-explorer-api maps a non-retried 4xx without a GraphQL
errors[]array tooutcome="unavailable"→breaker.record_failure(), so sustained armor rejections open its per-endpoint circuit breaker and the archive tier goes dark as a step function rather than degrading gradually.
What & why
Part of the production-readiness epic (#163). Closes #164.
The public GraphQL endpoint had no query-cost controls. The list fields (
events,actions,blocks) return unbounded lists, so a deeply-nested or heavily-aliased query is a trivial DoS against the backing Postgres — it can be made arbitrarily expensive before any limit applies. This is the highest-impact P0: the "crafted query = outage" mitigation.Changes
src/server/graphql-armor.ts— builds graphql-armor validation plugins from conservative, env-tunable limits (isolated and unit-testable).buildPluginsruns them ahead of execution, so abusive shapes are rejected during validation.GRAPHQL_MAX_DEPTH10GRAPHQL_MAX_ALIASES15GRAPHQL_MAX_TOKENS1000GRAPHQL_MAX_COST5000useDisableIntrospection). Introspection is ignored by the depth/cost rules so GraphiQL still works when explicitly enabled.Why the individual plugins, not the meta package
@escape.tech/graphql-armor(meta) requires@envelop/corev5, but this stack is pinned to v4 (Yoga 4). The individual@escape.tech/graphql-armor-*sub-plugins have no conflicting peer deps and work on the v4 stack. Switching to the meta integration can follow the Yoga 5 upgrade (#176).Testing
npm run build— cleannpm run test:unit— all pass, including an end-to-end test through Yoga asserting an over-depth query is rejected before execution, plus config parsing/fallback testsnpm run lint— cleannpx prettier --debug-check .— exit 0🤖 Generated with Claude Code