Narrow go-sdk cacheScope:public to private on projected results

[JAORMX]

Follow-up to #187: replaces its deferred cacheScope TODO with the actual fix.

go-sdk's setDefaultCacheableValues (unexported, mcp/protocol.go) unconditionally stamps cacheScope:"public" on the in-body Cacheable.CacheScope of tools/list, resources/list, resources/templates/list, prompts/list, server/discover, and resources/read results — with no awareness that this shim serves them per-identity. A "public" hint on a per-identity result is a cross-identity cache-leak signal: it tells any MCP-aware cache/gateway it MAY serve one identity's result to another. cacheScope is a body field, so it must be corrected in the body — an HTTP Cache-Control header can't neutralize it.

sessionDispatchMiddleware now calls a stripPublicCacheScope helper after dispatch (on success) that rewrites public→private on the six result types that carry Cacheable. It is downgrade-only (never widens cache-sharing) and a no-op for empty/already-private/non-cacheable results.

Applies to every serving mode, not just stateless (corrected in review — thanks @jhrozek): stateful serving is per-identity more than stateless — registerAndSync installs the SessionWithTools/Resources/ResourceTemplates/Prompts overlays onto that session's own go-sdk server (syncSession*), and that path runs only when !stateless (stateless projects tools-only). The identity bridge (valueBridgeContext) is likewise not gated on serving mode, which is the same reason resources/read is covered. So "public" is equally wrong on the stateful path, and the rewrite is unconditional. Not exploitable today on either path (nothing sets ttlMs, and ttlMs<=0 means never-cacheable per the 2026-07-28 caching spec and go-sdk's own client cache) — this is defense-in-depth, and the stateless/stateful split was not a real boundary.

Forward-compat backstop: go-sdk is pinned to a pre-release, so a future bump could embed Cacheable into a new result type the type switch doesn't cover — silently reintroducing the leak. Every Cacheable embedder satisfies the exported gosdk.CacheableResult, so the switch's default arm warns (never panics; logs the type only, never the body) when it sees an uncovered cacheable result reporting "public", so the switch gets updated on the next bump.

Same pre-release constraint as #187 (the release.yml guard blocks tagging until go-sdk v1.7.0 final).

Test plan

• task test (race) — blackbox: stateless tools/list/server/discover/resources/read and stateful tools/list all assert cacheScope:"private"; unit: stripPublicCacheScope over all six covered types + already-private no-op + unset-left-alone + non-cacheable no-op + untyped-nil no-op + the unhandled-cacheable warn path (with and without a logger)
• task lint — 0 issues
• task license-check — clean
• go build ./... — clean

🤖 Generated with Claude Code

[jhrozek]

Reviewed this and pushed a follow-up commit (8500ded) rather than leaving comments, since one of the findings needed the test inverted and that's easier to read as a diff than as prose. Your 24cab4d is untouched. Happy to drop the commit if you'd rather take it differently.

Everything the PR asserts about go-sdk checks out — I verified it against the pinned v1.7.0-pre.3 rather than taking the comments at face value:

• setDefaultCacheableValues really does stamp unconditionally (mcp/protocol.go:1195-1197 — it assigns CacheScope = "public" with no emptiness check), so a handler genuinely cannot express "private" itself and the shim is the only seam.
• Exactly six types embed Cacheable (mcp/protocol.go:1141,1205,1236,1267,1334,1599) and exactly six setDefaultCacheableValues() call sites match them 1:1 (mcp/server.go:854,908,944,992,1012,1033). The switch is exhaustive, and GetPromptResult/CompleteResult/CallToolResult really are excluded.
• The draft caching spec agrees: CacheableResult is extended by exactly those six, cacheScope is a closed "public" | "private" union, and "private" is the spec's own named case for "filtered list results that vary per user". So the direction of the fix and the six-type set are both right.
• The middleware is installed on every go-sdk *Server the shim builds (server.go:559) and every transport funnels through buildServer, so nothing bypasses it. The one non-go-sdk path (request_handler.go's HandleMessage) uses the shim's own mark3labs-shaped mcp.*Result types, which don't embed Cacheable at all — no gap there.
• No aliasing or race: all six are freshly allocated per request, and go-sdk's only result cache (mcp/cache.go) is client-side.

Three things I did change.

1. The && stateless guard was gated on an inverted premise (the substantive one)

The comment said the stateful path is exempt because there's "no per-identity projection there". It's the other way round — stateful is projected more, not less:

• registerAndSync installs the SessionWithTools/Resources/ResourceTemplates/Prompts overlays onto that session's own go-sdk server via syncSession* (session.go:356-367), and it is called only when !stateless (server.go:798). SessionWithTools' own doc says "ToolHive's vMCP layer uses this to project a per-session tool set."
• Meanwhile WithStateless' KNOWN LIMITATION block (transports.go:235-242) records that stateless projection is tools-only — resources/list, resources/templates/list and prompts/list come from the global set there.

So as written, the PR marked "private" on three list types that aren't projected in the mode the guard applies to, and left "public" on four that are projected in the mode it skips.

The stripPublicCacheScope doc comment's own justification for including resources/read closes the argument: it's the identity bridge — "its handler runs with the caller's bridged identity and can return per-identity content". That bridge (valueBridgeContext, server.go:755-761) has no stateless condition on it, so it applies verbatim to stateful resources/read. The reason given for widening across result types is exactly the reason to widen across serving modes.

Fix is a one-line deletion: strip unconditionally. Per the PR's own stated invariant ("only narrows the SDK's default; never widens cache-sharing") that's the simpler option as well as the safer one — it removes the need to reason about which sessions happen to carry overlays.

Worth being explicit that this is not exploitable today, on either path. Nothing in the repo ever sets TTLMs, setDefaultCacheableValues leaves it 0, and both the spec ("if 0, the response SHOULD be considered immediately stale") and go-sdk's own client cache (mcp/cache.go:37, TTLMs <= 0 ⇒ evict) mean no conformant cache retains any body this shim emits. That caveat applies equally to the stateless fix the PR ships — both are defense-in-depth. The point isn't a live leak, it's that the stateless/stateful split wasn't a real security boundary, just an accident of what's true today.

Consequence for the tests: TestStateful_ListToolsCacheScopeUntouched was pinning the leaky value as intended behavior. It's now TestStateful_ListToolsCacheScopePrivate and asserts the opposite — it fails against 24cab4d, so it's a genuine regression test rather than a restatement of current output.

2. A go-sdk bump could silently re-open the leak

default: return meant that if upstream embeds Cacheable into a new result type — CallToolResult being the obvious candidate — it'd be missed with no signal. That's precisely the failure the removed TODO(cacheScope) was worried about, and go-sdk is pinned to a pre-release with a standing note to re-bump at v1.7.0 final, so a version bump is scheduled rather than hypothetical.

Cheap to guard: anything embedding Cacheable satisfies the exported gosdk.CacheableResult for free, because GetCacheScope/GetTTLMs are promoted from the embedded struct. The default: arm now warns when it sees a cacheable result reporting "public" that the switch doesn't cover.

Warn rather than panic, deliberately: a missed cache hint shouldn't take down a request on a goroutine go-sdk never recovers (this file notes that property in several places already). It logs %T only, never the body. stripPublicCacheScope became a method on *MCPServer to reach s.logger, matching the existing pattern at session.go:391-399.

3. The nil-safety claim in the doc comment

"A nil res ... is a no-op" holds for an untyped-nil gosdk.Result (matches no case arm, falls to default), but not for a typed-nil pointer of a covered type — &r.Cacheable is fine, it's address arithmetic, but the subsequent c.CacheScope read panics.

I checked whether it's reachable and it isn't: go-sdk's own pointer-receiver setDefaultCacheableValues would have panicked on such a value before returning it, paginateList only returns its zero value paired with a non-nil error, and wrapResourceHandler (server.go:1004) always allocates a non-nil out regardless of what the inner handler returns. So I corrected the comment rather than adding per-case nil guards for a path that can't be hit — didn't seem worth ~18 lines of dead defensive code.

Smaller things

• Named the two cacheScope union members as constants. go-sdk exports none (checked — CacheScope is a bare string with the values only in a doc comment), so the literals were unavoidable, but they're now in one place and shared with the tests.
• Added TestStripPublicCacheScope in server_internal_test.go covering all six types directly, plus already-private, unset-scope, non-cacheable, and untyped-nil no-ops. Three arms (prompts/list, resources/list, resources/templates/list) had no coverage at all, and stateless_test.go is package server_test so it structurally can't reach the unexported helper — which is presumably why the existing tests go through HTTP. Those three round-trip tests stay useful as integration-level confirmation, they just no longer carry the mapping-completeness burden.

Deliberately left alone

• The long comment blocks. They're doing real work — the in-body-vs-HTTP-header distinction and the GetPromptResult/CompleteResult exclusion both invite a regression if cut. I kept the full explanation and consolidated it onto the helper, so it lives in one place instead of being restated in the middleware (the restated copy was the one carrying the incorrect stateful claim). The middleware comment is now a three-line pointer.
• The server/discover subtest's hardcoded Mcp-Method header and 2026-07-28 version. No exported constants exist to use — this shim's mcp.LATEST_PROTOCOL_VERSION is still 2025-11-25 and go-sdk's methodHeader is unexported. Inherent to blackbox-testing an unadopted revision, not worth exporting something new for. Your comment explaining why postRPC can't be used is accurate, and I confirmed go-sdk really does enforce both requirements (streamable_headers.go:355, streamable.go:1545).

One upstream bug, not actionable here

go-sdk calls res.setDefaultCacheableValues() before its resultType == resultTypeInputRequired early return (mcp/server.go:1033-1034), so it stamps cacheScope onto MRTR interim results. The draft spec says interim input_required results are never cacheable and carry no hints at all — so neither "public" nor "private" is correct there, and the shim can't remove the field (no omitempty). Our rewrite at least makes it non-shareable. Probably worth an upstream issue alongside the v1.7.0 final bump.

Verification

task lint (0 issues), task test (race, all packages), task license-check, go build ./... — all clean on the pushed commit.

🤖 Reviewed and fixed with Claude Code

[jhrozek]

Addendum — I put my own commit through the same review, and it had a gap

Having asked 24cab4d for untested switch arms, I shipped 8500ded without reviewing it to the same standard. Corrections below, plus one more commit (f8df7d6). Two are self-criticism; one is a correction to my reasoning that doesn't change the code.

1. I repeated the exact gap I flagged

8500ded added the default: arm specifically so that a future go-sdk embedding Cacheable into a new result type is loud rather than silent — and then shipped that arm with zero coverage on the one branch it exists for. The existing "non-cacheable result is a no-op" subtest only passes CallToolResult/GetPromptResult/CompleteResult, none of which satisfy gosdk.CacheableResult, so none of them ever reach the arm's GetCacheScope check.

That's worse than ordinary missing coverage here. The covered map in TestStripPublicCacheScope is hand-maintained against the switch, so a seventh Cacheable type slips past both silently — the runtime warning is the only real backstop, and an untested warning is close to no backstop.

f8df7d6 fixes it with a fakeCacheableResult standing in for exactly that future type: gosdk.ResultBase (documented as embeddable for this purpose at mcp/shared.go:790-802) supplies the unexported marker making it a gosdk.Result, and the embedded Cacheable promotes GetCacheScope/GetTTLMs so it satisfies gosdk.CacheableResult the same way a real new SDK type would. Two subtests: the arm reports it and leaves the body untouched (asserting the warning names the type but carries no result body fields), and it tolerates a nil logger since WithLogger is optional.

2. A claim I asserted without checking, now checked

My earlier comment said the inverted stateful test "fails against the previous commit". I hadn't actually run that. I have now: restoring && stateless makes TestStateful_ListToolsCacheScopePrivate fail with public vs expected private, while TestStripPublicCacheScope still passes — it calls the helper directly and bypasses the middleware entirely. So the two tests guard different things, gate-correctness and switch-completeness, and neither covers the other. The claim holds, but it shouldn't have been stated before it was verified.

3. My justification was partly wrong — the conclusion isn't

I argued the rewrite should apply to stateful because stateful sessions carry per-session overlays. Checking that against the draft caching spec: session bookkeeping is not the spec's criterion. The spec ties cacheScope to whether response content varies by authorization context — "a different access token requires a different cache" — and 2026-07-28 removed protocol-level sessions from Streamable HTTP outright, requiring servers to ignore Mcp-Session-Id entirely. So internal session state is invisible to the caching model. "Stateful ⇒ per-session ⇒ private" isn't a spec-backed inference; it's evidence of per-caller variance, not the criterion itself.

The correct framing, which reaches the same place: ToolHive's overlays are how content comes to vary per caller, and the identity bridge makes that variance real. The response content varies by authorization context — that's the spec's test, and it's satisfied.

Two things worth putting on the record that my first comment glossed:

The trade-off is real. An embedder with no per-session overlays and no auth genuinely does serve identical bodies to every caller. "public" is correct for them, and I've unconditionally narrowed it. Per the spec that costs them legitimate cross-context gateway sharing — though "private" still permits their own client cache and intermediary caching within one authorization context; the only prohibition is cross-context reuse. It's a missed optimization, never a conformance issue: nothing in the spec ever requires "public", and the Security Considerations name only the opposite direction as a hazard. And with ttlMs pinned at 0 the cost is exactly zero today, since nothing is ever fresh long enough to be reused from any cache. It becomes real only if this shim starts emitting a positive ttlMs.

The obvious "more precise" fix would be worse. Conditioning on "does this session carry an overlay" sounds tighter and isn't: valueBridgeContext bridges caller identity into every handler's context with no gating, so a handler — resources/read most obviously — can vary its response by identity with zero overlay present. An overlay check would silently leave exactly those responses "public". Unconditional narrowing is both the simpler and the safer condition; the thing to revisit is not the condition but ttlMs.

Verification

task lint (0 issues), task test (race, all packages), task license-check, go build ./... — clean on f8df7d6.

Net: no code change from this pass beyond the added test. The gate removal, the constants, the default: arm, the testify usage, and the stateful premise all held up under adversarial re-checking — the premise independently re-verified against session.go:336-367 and server.go:788. What needed fixing was my coverage and my reasoning, not the behavior.

🤖 Reviewed and fixed with Claude Code