fix(api-gateway): enforce meta scope on /v1/graphql-to-json - #11939
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
github-actions
Bot
added
docs
|
Claude finished @paveltiunov's task in 2m 39s —— View job Approve with nits — 0 high, 0 medium, 3 lowFull review
VerdictThe core fix is correct.
Mock-level check of the new assertion: Findings
Findings 1 and 2 are posted inline. Finding 3 — the jsonParser claim. Three more POST routes read
They share exactly the latent failure this PR fixes on Notes, not findingsBehaviour change for a narrow configuration. A deployment granting Docs table completeness. Adding Visibility asymmetry. TestsNew coverage is well targeted — denied without I could not execute the suite: this is a fresh checkout with no Review threadsThe listing returned 0 threads of my own (empty, single page) — no prior threads. 0 resolved, 0 re-affirmed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // the data model metadata and executes nothing, so it belongs to the `meta` | ||
| // scope rather than `graphql` - the surface it exposes is the same one | ||
| // `/v1/meta` guards. | ||
| app.post(`${this.basePath}/v1/graphql-to-json`, jsonParser, userMiddlewares, userAsyncHandler(async (req: any, res) => { |
There was a problem hiding this comment.
Not fixing this in this PR, and flagging that the CodeQL check is consequently red — reasoning below so a maintainer can dismiss the alert if they agree.
The alert fires because of this PR's fix, not because of a new exposure. js/missing-rate-limiting flags route handlers that perform authorization; this handler previously performed none, which is the bug being fixed. Adding assertApiScope is what made CodeQL classify it as an authorization route, so the alert is a direct artifact of adding the security check. The endpoint is no more reachable than before — strictly less so.
It is also not specific to this route. There are 13 inline assertApiScope call sites in gateway.ts, including /graphql (:343) with the identical shape, plus /v1/cubesql (:557) and /v1/pre-aggregations/can-use (:590) — all on master, none rate-limited. CodeQL only surfaces alerts in code a PR changed, which is why this one appeared and the others didn't.
And there is nothing to be consistent with: grep for rate-limit/rateLimit across cubejs-api-gateway, cubejs-server and cubejs-server-core returns nothing, and express-rate-limit is not a dependency of any of them. Rate limiting is handled ahead of the gateway rather than inside it.
So the available fix would be to introduce a rate-limiting dependency and apply it to exactly one endpoint out of ~20 — inconsistent with every sibling, a new runtime behaviour (throttling a legitimate Playground call path) and well outside an authorization fix. Doing it properly means a gateway-wide policy with configuration, which is its own change.
I have no means to dismiss the alert myself. Happy to open a follow-up issue for gateway-wide rate limiting if that's wanted; otherwise this needs a maintainer to dismiss as "won't fix" so the check can go green.
Generated by Claude Code
574fab0 to
f21bbe8
Compare
|
Claude Code is working… I'll analyze this and get back to you. |
`initApp()` registered `/v1/graphql-to-json` under the "graphql scope"
comment block but never asserted any scope, so a token that an operator
had explicitly denied access could still reach it. Every sibling endpoint
asserts its scope; this one was missed.
The correct scope is `meta`, not `graphql`. The handler only reads the
data model metadata and translates a GraphQL query string into a Cube JSON
query - it executes nothing and returns no data, so the surface it exposes
is the one `/v1/meta` guards. Requiring `graphql` would also have been
wrong in practice: a user holding `meta` + `data` but not `graphql` has a
legitimate reason to convert a GraphQL query into JSON they then run via
`/v1/load`. Both directions are covered by tests.
While here, removes a dead block from the handler:
let schema = compilerApi.getGraphQLSchema();
if (!schema) {
schema = makeSchema(metaConfig);
compilerApi.setGraphQLSchema(schema);
}
`schema` is never read - `getJsonQueryFromGraphQLQuery()` takes
`metaConfig`, not a schema. Its only effect was writing the compiler API's
shared GraphQL schema cache, and because it built the schema without
`skipVisibilityPatch` it could prime that cache with a narrower schema that
`/graphql` then served to other users in the same compiler context.
Also mounts `jsonParser` on the route. It was the only POST endpoint
without one, silently relying on the host app to mount a body parser
app-wide; without one the handler threw on destructuring `req.body`.
`userAsyncHandler` is needed so the scope rejection reaches the error
middleware instead of leaving the request hanging.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EsEm9cas3XTPrjcQeRQBdP
f21bbe8 to
cba01db
Compare
|
Claude finished @paveltiunov's task in 2m 14s —— View job Approve — no defects found. 0 high, 0 medium, 1 low (pre-existing, out of scope).Full review
What I verifiedScope choice. Route placement after the relocation. Dead-code removal is a real fix, not just cleanup. The removed block built the schema without
Tests. Consumer check. The only in-repo caller is Docs. The Low — pre-existing, out of scopeThe PR description says CommentsThe two-line comment at Not actionableThe CodeQL "Missing rate limiting" alert applies to every authenticated route in this file, not to anything this diff introduced. Awaiting maintainer dismissal as noted on that thread. Review threadsThe listing returned 1 unresolved thread, 0 of them mine — it was opened by Not run
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11939 +/- ##
==========================================
+ Coverage 61.23% 61.32% +0.08%
==========================================
Files 247 247
Lines 19824 19821 -3
Branches 4043 4042 -1
==========================================
+ Hits 12140 12156 +16
+ Misses 7112 7092 -20
- Partials 572 573 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch was successfully deployed
4 participants
Check List
Description of Changes Made
initApp()registered/v1/graphql-to-jsonunder thegraphql scopecomment block but never asserted any scope, so a token an operator had explicitly denied could still reach it. Every sibling endpoint asserts its scope; this one was missed.Reproduced against the gateway wired the way
@cubejs-backend/serverwires it (NODE_ENV=production, app-widebodyParser.json(),CUBEJS_DEFAULT_API_SCOPES=data), same token to both endpoints:Why
metaand notgraphqlThe handler only reads the data model metadata and translates a GraphQL query string into a Cube JSON query. It executes nothing and returns no data, so the surface it exposes is the one
/v1/metaguards.Requiring
graphqlwould also have been wrong in practice: a user holdingmeta+databut notgraphqlhas a legitimate reason to convert a GraphQL query into JSON they then run via/v1/load. Both directions are pinned by tests — denied withoutmeta, still allowed withmetabut nographql.The route is registered in the existing
meta scopesection next to/v1/meta.Dead code removed
schemais never read —getJsonQueryFromGraphQLQuery(query, metaConfig, variables)takesmetaConfig, not a schema. The block's only effect was writing the compiler API's shared GraphQL schema cache, and because it built the schema withoutskipVisibilityPatchit could prime that cache with a narrower schema that/graphqlthen served to other users in the same compiler context. Verified before/after:setGraphQLSchemacalls from this path drop to 0, and/graphqlnow serves the full unfiltered schema regardless of call ordering.This is also what made the endpoint look like a GraphQL endpoint; with it gone,
metais unambiguous.Supporting changes
userAsyncHandlerso the scope rejection reaches the error middleware. Without it the rejected promise is unhandled and the request hangs rather than returning 403 — this showed up as a 5s test timeout.jsonParseron the route. It previously relied on the host app mounting a body parser app-wide, and threw aTypeErroron destructuringreq.bodywithout one. Note this does not close the class:/v1/cubesql,/v1/pre-aggregations/can-useand/v1/pre-aggregations/jobsreadreq.bodywith no parser mounted either. Left for a follow-up rather than widened into an authorization fix./v1/pre-aggregations/can-useundermetaand/v1/dry-run,/v1/convert-query,/v1/subscribeunderdata— so this adds the endpoint at issue without claiming to fix the table.Impact
Operators who restrict
meta(Cube's docs use "restrict access to the/v1/metaendpoint to service accounts only" as the worked example) could not enforce it — denied users still reached data-model structure through this endpoint. Requires a valid token, and default scopes are['graphql', 'meta', 'data', 'sql'], so stock installs were not affected by the authorization bypass. The schema cache write did apply to default configurations.Behaviour change worth a release note: a deployment granting
graphql+databut notmetacurrently gets working GraphQL→JSON conversion in Playground's Query Builder v2 and will now get a 403. That is the intended effect of the fix.Testing
packages/cubejs-api-gateway: 288/288 passing, including 4 new/extended assertions intest/permissions.test.ts.["data"]→ 403,["graphql","data"]→ 403,["meta"]→ 200,["meta","graphql"]→ 200.oxlintclean on changed files.🤖 Generated with Claude Code
https://claude.ai/code/session_01EsEm9cas3XTPrjcQeRQBdP