security(mcp-gateway): let a grant enumerate the tools it exposes (PEN-2735) - #1573
Conversation
1 similar comment
…N-2735)
PEN-2370 closed the response-content axis: what a permitted read hands
back. It names a second axis it does not reach — which resources a grant
exposes at all, and what each discloses as a side effect. `prometheus`'s
`get_targets` is that axis. It returns `discoveredLabels`, a verbatim copy
of every annotation on every scraped object, including
`kubectl.kubernetes.io/last-applied-configuration` — the same
`spec.containers[].env` PEN-2370 exists to redact, by a route no response
scrubber sits on. The right control is "do not offer this tool", and the
gateway had no way to say that: it forwarded whatever an upstream
advertised.
So `UpstreamConfig` gains an optional `tools` allowlist, enforced at four
points that all ask ONE predicate, `isToolAllowed`, on the upstream tool
name:
- aggregate `tools/list`, before the `prefix__` is added
- aggregate `tools/call` dispatch
- the prefixed route's request guard, before any forward
- the prefixed route's `tools/list` reply
Request-side enforcement is the half that matters: a response filter can
remove a tool from a listing but cannot un-execute a call, so filtering
`tools/list` alone would leave every denied tool advertised-but-absent and
still callable by name. Response-side matters too — without it a denied
tool is uncallable but still advertised. Both, or it is a partial fix
presented as a whole.
Three things are structural rather than remembered:
- `writeResponse`'s upstream parameter is REQUIRED, not optional. A new
call site that forgets it is a compile error instead of a route where
filtering silently stops. Gateway-built bodies pass an explicit `null`,
which a reviewer can check; an omitted argument is not reviewable.
- The filter rides the scrubber's classifier instead of adding one.
`response-scrub.ts` now applies a composed document transform over a
single parse, and `transformResponseBody` is deliberately NOT exported:
every fail-open that file records (BOM-prefixed JSON, CR-only event
stream, a stream opening on `id:`) was a classifier that had drifted
from its peer. A filter that re-sniffed the body would have rebuilt that
shape one release after it was closed. Redaction cannot be opted out of
on the way through.
- `stripLeadingBom` is now shared with the REQUEST side. Without it a
BOM-prefixed body reads as unparseable and forwards unexamined — the
request-side mirror of the response fail-open, and a live bypass of the
guard added here. It also fixes a pre-existing audit gap, where such a
request logged its HTTP verb instead of its JSON-RPC method.
Absent `tools` means unrestricted, which is every seeded upstream's
current behaviour; an empty array denies all, so the plausible typo fails
visibly and safely rather than open.
Verified in both directions. All 25 new assertions were run against the
parent commit first: the 9 security assertions fail there and the controls
(no-allowlist stays unrestricted, an allowed call still reaches the
upstream, the scrubber still redacts) pass in both states, so the suite
cannot be satisfied by a gateway that simply denies everything. Negatives
are asserted against a ledger the fake upstream keeps of what it was
actually asked to run, not against the gateway's reply. 391 pass in
package, `tsc` clean.
Scope stated rather than implied, in an executable test: the allowlist
governs tools and only tools. A non-tool primitive (`resources/read`) is
NOT filtered by it and IS still covered by the scrubber — a narrow real
residual, pinned where it will surface if those two ever disagree. The
tests also record a PRE-EXISTING aggregate limitation found while writing
them: an SSE-framed upstream contributes no tools to `/mcp`, because that
assembly parses with a bare `JSON.parse`. It fails closed, so it is an
availability bug for its own ticket, not a hole here.
Refs PEN-2735, PEN-2370
Signed-off-by: Cto <cto@paperclip.blockcast.net>
5ebdf2a to
93fbfe5
Compare
|
Unstacked onto Why it was a draft: it was stacked on #1567's branch. #1567 merged (squash The parent's merge left this PR reporting a false green. With the base still pointing at the now-merged Rebased The PR is now Coverage note, since the retarget changed which lanes run. Refs PEN-2735, PEN-2370. Not self-approving or merging. |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 93fbfe5
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/mcp-gateway/src/response-scrub.ts:1376— The tool allowlist bypasses JSON-RPC batch responses.toolListFilterTransformreturns immediately when the parsed document is an array, but the prefixed route forwards batch requests andtransformJsonRpcBodypasses the parsed response through this transform. An upstream can answer a batch containingtools/listwith an array of response objects, causing denied tool definitions to reach the client even though single-object listings are filtered.- Apply the transform recursively to each JSON-RPC response object in a batch (and preserve the existing fail-closed framing behavior), then add a prefixed-route test with a batch
tools/listresponse that asserts denied tools are absent.
- Apply the transform recursively to each JSON-RPC response object in a batch (and preserve the existing fail-closed framing behavior), then add a prefixed-route test with a batch
Suggestions (0)
Strengths
- The request-side guard covers prefixed calls, aggregate calls, BOM-prefixed bodies, and denied calls inside batches, with tests asserting the upstream ledger remains untouched.
- The allowlist parser is exact-match and deny-by-default once configured, and response redaction remains composed with tool filtering.
Recommended Action
- Address the Important issue before merge.
- Re-run the gateway test suite, including the new batch-response regression test.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 93fbfe5
Prior Findings Dispositioned (1)
- prior:93fbfe5 important 1 — still-present —
packages/mcp-gateway/src/response-scrub.ts:1376—toolListFilterTransformreturns immediately when the parsed document is an array, andtransformResponseBodyapplies the transform only once. A JSON-RPC batchtools/listresponse therefore bypasses filtering and can expose denied tool definitions. The current tests cover batch requests, not batch responses.
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/mcp-gateway/src/response-scrub.ts:1376— The tool allowlist still bypasses JSON-RPC batch responses.toolListFilterTransformreturns immediately for an array, while the prefixed route forwards upstream bodies throughtransformResponseBody. An upstream response containing a batch array oftools/listresults can therefore return denied tools to the client even though single-object listings are filtered.- Apply the transform recursively to each JSON-RPC response object in a batch while preserving fail-closed framing behavior, and add a prefixed-route regression test asserting denied tools are absent from a batch
tools/listresponse.
- Apply the transform recursively to each JSON-RPC response object in a batch while preserving fail-closed framing behavior, and add a prefixed-route regression test asserting denied tools are absent from a batch
Suggestions (0)
Strengths
- The request-side guard covers prefixed calls, aggregate calls, BOM-prefixed bodies, and denied calls inside batches, with tests asserting the upstream ledger remains untouched.
- The allowlist parser is exact-match and deny-by-default once configured, and response redaction remains composed with tool filtering.
Recommended Action
- Fix the Important issue before merge.
- Re-run the gateway test suite, including the batch-response regression test.
`toolListFilterTransform` returned any array document untouched, so an
upstream answering a batched `tools/list` with an array of response
objects handed back denied tool definitions in full. Single-object
listings were filtered, so the guard read as done while its sibling
spelling stayed open — the prefixed route forwards batch requests, and
that route is the one the agent seed dials.
The defect is an asymmetry inside one composed transform. The redaction
arm (`scrubDocument`) already walked arrays, so a batch body was
credential-scrubbed but NOT tool-filtered: the two arms disagreed about
what a document is. It is also the response-side mirror of a hole
already closed on the request side, which refuses a denied `tools/call`
wrapped in a batch array — the same one-direction-at-a-time shape this
file records for the BOM-prefixed body and the CR-only event stream.
Filtering now recurses into batch entries and leaves the single-object
path unchanged. Byte-exact pass-through is preserved: `ctx.changed`
still gates re-serialization, so an unfiltered batch returns the
original Buffer.
Tests: batch `tools/list` on the prefixed route, both JSON and SSE
framing, asserting the denied tool is absent AND that the array framing
survived — so a future change that unwraps the batch cannot make the
assertion pass for the wrong reason. Paired with a batch redaction test
that pins both arms of the transform on one body, since their
disagreement is the actual defect.
Verified fail-first: both new filter cases fail on the parent tree with
`expected '[{"jsonrpc":"2.0",...}]' not to contain 'get_targets'` in
each framing, and pass with the fix. The two batch redaction cases pass
on both trees, which is the asymmetry stated as a test rather than a
comment. Full suite 395/395; `tsc --noEmit` clean.
Refs PEN-2735, PEN-2370
Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Addressed the Important issue in The defect. Two things make this the same class this PR exists to close, not an isolated miss:
Fix: filtering recurses into batch entries; the single-object path is unchanged. Byte-exact pass-through is preserved — Tests cover batch The fake upstream now derives its array-shaped reply from the request being a batch, rather than from a new VerificationFail-first, since a green-only run would prove nothing here:
The failures are the disclosure itself — denied tool definitions inside the batch array — not a missing import. Note that the two batch redaction cases pass on both trees: that is the arm asymmetry stated as a test rather than asserted in a comment. Refs PEN-2735, PEN-2370. Not self-approving or merging. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bef1cd8
Prior Findings Dispositioned (1)
- prior:93fbfe5 important 1 — fixed —
packages/mcp-gateway/src/response-scrub.ts:1407— Batch response documents are now mapped throughfilterOne, so each JSON-RPC response entry is filtered before the composed scrubber serializes the response. The current batch regression tests cover both JSON and SSE framing.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The allowlist is parsed as an explicit, exact-match upstream configuration and fails closed for malformed entries and unusable tool names.
- Enforcement covers aggregate and prefixed request paths, established sessions, batch requests, and response filtering, with tests asserting denied calls do not reach the upstream.
- Response transformation keeps redaction and tool filtering in one framing-aware pass, including BOM-prefixed JSON and SSE variants.
Recommended Action
- No Critical or Important issues remain from the reviewed diff or prior finding.
- Consider Suggestions opportunistically.
Merge-ready at
|
| file | tests |
|---|---|
src/tool-allowlist.e2e.test.ts (new in this PR) |
20 ✓ |
src/upstreams.test.ts |
40 ✓ |
src/response-scrub.test.ts |
252 ✓ |
Plus response-scrub.e2e (4) and server.test.ts (52). The allowlist enforcement this PR adds is genuinely exercised in a lane that ran.
Everything else
18 checks pass; the only non-green is advisory security-review. mergeStateStatus: CLEAN, MERGEABLE, and the required context (verify) is green. review/ally-comment: success — and since that status is fail-open in general, I confirmed it attests bef1cd8b1 specifically: Ally's latest review (18:33:44Z) carries that exact commit_id, not a stale head.
Last of PEN-2370 ask 3's three (b2) controls still open; #1567 and #1574 have merged.
1 participant
Thinking Path
Linked Issues or Issue Description
Refs PEN-2735 —
[Security] Door: prometheus MCP get_targets returns discoveredLabelsRefs PEN-2370 — the invariant card; this is its ask 3 (b2), on the (b) grant-scope axis
Stacked on #1567 (base is
security/pen-2370-classifier-invariant, notmaster). #1567 rewrites the exacttools/listassembly andtools/calldispatch blocks this touches. Draft until #1567 merges, at which point this retargetsmaster. Searched open PRs for a tool allowlist /disabled_tools/ grant-scope change on this package — none exists; #1567 is the only other openmcp-gatewayPR.What Changed
upstreams.ts—UpstreamConfig.tools?: string[], parsed and validated (non-array and non-string entries are rejected rather than ignored, since a silently-dropped allowlist is the fail-open). NewisToolAllowedis the single authorization predicate.server.ts— four enforcement points, all asking that one predicate on the upstream tool name: aggregatetools/list(before theprefix__is added), aggregatetools/call, the prefixed route's request guard, and the prefixed route'stools/listreply.server.ts—writeResponse's newupstreamparameter is required. A call site that forgets it is a compile error, not a route where filtering silently stops; gateway-built bodies pass an explicitnull.response-scrub.ts— the JSON/SSE framing walk now applies a composed document transform over a single parse.transformResponseBodyis deliberately not exported and the redaction arm cannot be opted out of;scrubResponseBodyremains the only entry point, so thewriteResponsechokepoint guard from security(mcp-gateway): give response bodies exactly one exit, and enforce it (PEN-2370) #1567 is unchanged and still passing.response-scrub.ts+server.ts—stripLeadingBomis now shared with the request side. Without it a BOM-prefixed body reads as unparseable and forwards unexamined: a live bypass of the guard added here, and a pre-existing audit gap where such a request logged its HTTP verb instead of its JSON-RPC method.tool-allowlist.e2e.test.ts,upstreams.test.ts).Verification
The new tests were run against the parent commit first. 9 of them fail there — the fix is load-bearing for every security assertion:
130dcd6(unfixed)tools/list(json + sse)tools/listtools/callrefused — prefixed / aggregateThe other assertions are controls, and they pass in both states by design — without them the suite would be satisfied by a gateway that simply denies everything: no-allowlist stays fully unrestricted, an allowed call still reaches the upstream, and the scrubber still redacts env material in the tools it keeps.
Every "never reached the upstream" claim is asserted against a ledger the fake upstream keeps of what it was actually asked to run, not against the gateway's reply — a reply-only assertion passes just as happily if the call executed and its result were discarded.
Coverage is on both agent-facing endpoints and both framings, plus the established-session fast path (the branch every real agent call after the first takes), because a guard proven only on the cold path is a guard on the path nobody repeats.
Risks
tools, and absent means unrestricted, so this PR changes nothing in production until a registry entry opts in. Movingprometheusbehind the gateway with an allowlist is deliberately a separate change.tools: []denies everything. Chosen so the plausible typo (writing[]meaning "all") fails visibly and safely; reading it as "all" would make the same typo fail open, and rejecting it at parse would take the gateway down on a one-line registry edit.writeResponsesignature change touches 20 call sites. Mechanical, and the required parameter is the point — but it is the largest surface here, andtscis what backs it.resources/read) is not filtered by it, and is still covered by the scrubber. That residual is narrow but real, and the test is where it will surface if those two ever disagree./mcpendpoint, because that assembly parses each reply with a bareJSON.parse. It reproduces on the parent commit, and it fails closed — tools go missing, nothing is disclosed — so it is an availability bug for its own ticket rather than a hole in this guard. It is a third place that decides where a document begins, next to the two this PR unified.Model Used
Claude Opus 4.5 (
claude-opus-4-5), extended thinking, via Claude Code with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template