Skip to content

fix(security): authorize aggregated prompts by canonical owner and stop profile enumeration on a deleted pin - #1227

Open
Dumbris wants to merge 3 commits into
mainfrom
claude/eager-kirch-10f6c2
Open

fix(security): authorize aggregated prompts by canonical owner and stop profile enumeration on a deleted pin#1227
Dumbris wants to merge 3 commits into
mainfrom
claude/eager-kirch-10f6c2

Conversation

@Dumbris

@Dumbris Dumbris commented Sep 8, 2026

Copy link
Copy Markdown
Member

Part of Spec 105 (agent-token scope hardening).

Summary

Two agent-token scope leaks on the HTTP MCP surfaces, both fail-closed now.

Aggregated prompts (Spec 105 FR-006). filterAggregatedPromptsForAuth re-parsed each published server__prompt display name on its first __ to find the owner, while the registered handler dispatched to the server captured at publication. With servers a and a__b both serving greeting, a__b's prompt is published as a__b__greeting; the re-parse claims owner a, so an agent token scoped to a alone could list and fetch a prompt that dispatches to a__b. The fix:

  • buildAggregatedServerPrompts stamps the canonical owner into the registered prompt's _meta (app.mcpproxy/server). The stamp is a private struct carrying the owner and the upstream's original _meta pointer, so an upstream-supplied string under that key is never accepted as a stamp. The filter authorizes list and get against that stamp, read from the same registered prompt mcp-go hands it, so owner and prompt cannot skew across a refresh. The stamp is stripped from every prompt returned (admins included) by restoring the upstream _meta verbatim (nilnil, {}{}, progress token and foreign keys untouched), so client-visible _meta is exactly what the upstream sent.
  • An upstream prompt with no stamp is dropped for scoped callers — the display-name guess is gone (fail closed).
  • Every aggregated prompt handler runs authorizeAggregatedPromptServer against its own server before getPrompt, returning mcp-go's exact unregistered-name wording (prompt '<name>' not found: prompt not found), so a scope change between the filter and the handler inside one request still cannot dispatch.
  • Upstream prompts are sorted by qualified name before collision resolution, so the display-name winner no longer depends on map iteration order.
  • The prompt filter is bound to all four routing-mode servers unconditionally, not only under construction-time enable_prompts. RefreshPrompts publishes from the live config snapshot on every servers.changed / config.reloaded / prompts-changed event and mcp-go registers the prompts capability implicitly on the first SetPrompts, so a core started with prompts off and hot-reloaded to on would otherwise serve every upstream prompt unfiltered, with the internal stamp on the wire (cross-review round 2).

Profile enumeration on a deleted pin (Spec 105 FR-004). When the profile an agent token is pinned to has been deleted, both the profile-URL middleware (/mcp/p/<slug> 404 with "available": [...]) and the set_profile tool (unknown profile 'x' (available: ...)) fell into the generic unknown-slug branch and enumerated every remaining profile — profiles the resolver treats as unselectable for that token (a deleted pin is deny-all). Both now omit the list for pinned callers; unpinned callers and administrators are unchanged.

What changed

File Change
internal/server/mcp_routing.go buildAggregatedServerPrompts gains an authorize hook consulted by every upstream prompt handler before dispatch; stamps the canonical owner into _meta as a private aggregatedPromptStamp{server, upstream}; sorts upstream prompts by qualified name (slices.SortStableFunc) before collision resolution. Helpers stampAggregatedPromptServer / aggregatedPromptStampOf / aggregatedPromptServer / stripAggregatedPromptServer (strip restores the upstream _meta pointer verbatim). RefreshPrompts wires p.authorizeAggregatedPromptServer; initRoutingModeServers binds WithPromptFilter on the direct / code / call servers regardless of EnablePrompts.
internal/server/mcp.go NewMCPProxyServer binds WithPromptFilter(filterAggregatedPromptsForAuth) on the default server unconditionally (was inside if config.EnablePrompts); registerPrompts stays gated.
internal/server/mcp_direct_scope.go filterAggregatedPromptsForAuth authorizes against the _meta stamp instead of re-parsing the display name; unstamped upstream prompts dropped for scoped callers; stamp stripped from all output. New promptServerAllowed (single predicate shared by filter and handler gate), authorizeAggregatedPromptServer, errPromptNotFound (= mcpserver.ErrPromptNotFound).
internal/server/server.go profileMiddleware unknown-slug 404 omits available when the request carries a profile pin.
internal/server/profile_tool.go handleSetProfile unknown-slug error omits (available: ...) when the caller is pinned.
internal/server/mcp_prompt_scope_test.go Base fixtures now stamped like production; new TestAggregatedPrompt_ScopeUsesCanonicalOwner (with refusal-shape parity + direct handler invocation), TestAggregatedPrompt_LateEnableStillFiltered, TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed, table-driven TestStripAggregatedPromptServer_PreservesUpstreamMeta; removed ..._KeepsUnparseableName (it encoded the leak).
internal/server/mcp_routing_test.go Signature update; owner-stamp assertions; new TestBuildAggregatedServerPrompts_HandlerAuthorizesCanonicalServer; _CollisionKeepsFirst runs both input orders.
internal/server/mcp_block_tools_test.go createTestProxyWithRuntimeCfgcreateTestProxyWithRuntime with a pre-construction config hook (used by the late-enable test).
internal/server/profile_integration_test.go New TestProfile_DeletedPinDoesNotEnumerateProfiles (HTTP: pinned token gets no list, admin still does).
internal/server/profile_tool_test.go New TestHandleSetProfile_DeletedPinDoesNotEnumerateProfiles (pinned: no list; unpinned agent token and no-auth admin: list unchanged).

No config, API-surface, or tool-definition golden changes.

Tests

  • TestAggregatedPrompt_ScopeUsesCanonicalOwner — real a and a__b upstreams (streamable-http test servers), prompts/list + prompts/get through HandleMessage as an a-only token: a__greeting visible/fetchable, a__b__greeting hidden and denied; admin sees both; no _meta stamp on the wire for either caller. Asserts JSON-RPC code + message parity (modulo the echoed name) between the hidden a__b__greeting get and a nonexistent a__nonexistent get, and invokes the registered handler directly (bypassing mcp-go's filter) requiring errPromptNotFound for the a-only context while the admin context is served.
  • TestAggregatedPrompt_LateEnableStillFiltered — proxy built with enable_prompts=false, both prompt flags flipped in the live snapshot, RefreshPrompts, then prompts/list as an a-only agent on all four servers: hidden prompt absent, no _meta stamp on any entry.
  • TestBuildAggregatedServerPrompts_HandlerAuthorizesCanonicalServer — each handler asks about its canonical server (a__b, not a); a denied handler never calls getPrompt.
  • TestBuildAggregatedServerPrompts_CollisionKeepsFirst — both input orders yield the same winner/owner/description and the same dropped-collision log fields.
  • TestStripAggregatedPromptServer_PreservesUpstreamMeta — table: nil / {} / fields / progress token / upstream value under our key, each comparing json.Marshal of stripped vs as-sent; plus a forged-string stamp case.
  • TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed, stamp assertions in TestBuildAggregatedServerPrompts.
  • TestProfile_DeletedPinDoesNotEnumerateProfiles, TestHandleSetProfile_DeletedPinDoesNotEnumerateProfiles.

Bite checks (each restored afterwards): PR-head stripAggregatedPromptServer → strip empty and our_key subtests fail; SortStableFunc removed → collision reversed_order fails; authorize passed as nil in RefreshPrompts → integration wiring assertion fails; filter bound only under EnablePromptsLateEnableStillFiltered fails on all four servers; a guard hiding the profile list from every agent → profile_tool_test.go:138 fails.

Gates run locally (CI invocation), green on every commit:

go build ./cmd/mcpproxy && go build -tags server -o /dev/null ./cmd/mcpproxy
go test -race -count=1 -skip 'E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint' ./internal/server/...
/opt/homebrew/bin/golangci-lint run --config .github/.golangci.yml ./internal/server/...   # 0 issues

Frozen tool-surface goldens under internal/server/testdata pass unregenerated.

Verification

Live verification on an isolated personal-edition instance (127.0.0.1:18201, fresh --data-dir + scratch --config, socket and Web UI disabled). Branch da05678f (the original fix commit; the two review commits below change the _meta restore and the filter binding, neither of which alters the probes shown) vs baseline origin/main c93f79423, same config on both binaries.

Rig. Upstreams a and a__b are both npx -y @modelcontextprotocol/server-everything (stdio; 4 prompts each: args-prompt, completable-prompt, resource-prompt, simple-prompt). Profiles p-doomed["a"], p-other["a__b"]. enable_prompts: true, aggregate_upstream_prompts: true, quarantine_enabled: false, both servers quarantined: false, both connected: true before any probe. Tokens minted via POST /api/v1/tokens (X-API-Key): tok-a-only (allowed_servers: ["a"], read), tok-wild (["*"]), tok-pinned (["*"], profile_pin: "p-doomed").

Request shapeinitialize (protocolVersion 2025-03-26) → capture Mcp-Session-Id → second POST with the probe method:

curl -s -D hdrs -X POST http://127.0.0.1:18201/mcp -H 'Content-Type: application/json' \
  -H 'Accept: application/json, text/event-stream' -H "Authorization: Bearer $TOKEN" \
  -d '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","capabilities":{},"clientInfo":{"name":"verify","version":"1"}}}'
curl -s -X POST http://127.0.0.1:18201/mcp … -H "Mcp-Session-Id: $SID" \
  -d '{"jsonrpc":"2.0","id":2,"method":"prompts/get","params":{"name":"a__b__args-prompt","arguments":{"city":"Vilnius"}}}'

Scenario 1 — a-only agent token vs prompts owned by a__b

probe baseline (origin/main) branch (#1227)
tok-a-only prompts/list leak — all 10 names, incl. a__b__args-prompt, a__b__completable-prompt, a__b__resource-prompt, a__b__simple-prompt a__args-prompt, a__completable-prompt, a__resource-prompt, a__simple-prompt, setup-new-mcp-server, troubleshoot-mcp-server — no a__b__*, no _meta
tok-a-only prompts/get a__b__args-prompt leakresult.messages[0].content.text = "What's weather in Vilnius?" (upstream a__b was contacted) {"error":{"code":-32602,"message":"prompt 'a__b__args-prompt' not found: prompt not found"}}
tok-a-only prompts/get totally-unknown-prompt (reference) -32602 "prompt 'totally-unknown-prompt' not found: prompt not found" same
tok-a-only prompts/get a__args-prompt (own server) 1 message 1 message, "What's weather in Vilnius?"
tok-wild prompts/get a__b__args-prompt (positive control) 1 message 1 message, "What's weather in Vilnius?"
tok-wild / admin prompts/list (positive control) 10 names 10 names, no _meta on any prompt

Refusal-shape parity on the branch (request id stripped, caller-supplied name placeholdered):

diff <(jq -c 'del(.id)|.error.message|=sub("a__b__args-prompt";"<NAME>")' branch-aonly-get-ab.json) \
     <(jq -c 'del(.id)|.error.message|=sub("totally-unknown-prompt";"<NAME>")' branch-aonly-get-unknown.json)
→ identical: {"jsonrpc":"2.0","error":{"code":-32602,"message":"prompt '<NAME>' not found: prompt not found"}}

An a__b prompt is indistinguishable from an unregistered name to the a-only caller; the only name in the refusal is the one the caller itself sent.

Scenario 2 — pinned token whose pinned profile was deleted

Deletion = remove p-doomed from profiles[] in the scratch config; the watcher hot-reloaded within 2 s (GET /api/v1/profiles["p-other"]) on both binaries.

probe (tok-pinned, pin p-doomed) baseline branch
initialize via /mcp/p/p-doomed after deletion HTTP 404 {"available":["p-other"],"error":"unknown profile 'p-doomed'"} — leak HTTP 404 {"error":"unknown profile 'p-doomed'"}
tools/call set_profile {"profile":"p-doomed"} on /mcp isError "unknown profile 'p-doomed' (available: p-other)" — leak isError "unknown profile 'p-doomed'"
set_profile {"profile":"nope-nope"} "agent token is pinned to profile 'p-doomed' and cannot switch to 'nope-nope'" (pin-mismatch check fires first on both) same
set_profile {"profile":""} {"active_profile":"p-doomed","servers":[]} — deny-all, no server names
/mcp/p/p-other (foreign profile) HTTP 403 "agent token is pinned to profile 'p-doomed' and cannot access profile 'p-other'"
/mcp/p/p-doomed before deletion (positive control) 200; prompts/list = a__* ×4 + built-ins

Unpinned admin references are unchanged by design on both binaries: /mcp/p/nope-nope → 404 with available: ["p-other"]; set_profile nope-nope"unknown profile 'nope-nope' (available: p-other)"; set_profile p-other{"active_profile":"p-other","servers":["a__b"]}. grep -c -E 'p-other|available|a__b' over every branch pinned-token body → 0.

Caveats from the run:

  • The branch prompts/get refusal does name the requested prompt, but only by echoing the caller's own input; it is byte-identical in shape to the refusal for a never-registered name. Read "without naming it" as "without confirming it exists".
  • The baseline set_profile enumeration is only reachable when the pinned token names its own deleted slug; a foreign bad name is short-circuited by the pin-mismatch check on both binaries. The own-slug path is the one shown closed above.
  • ./cmd/mcpfixture exposes no prompts, so both upstreams are server-everything — identical prompt names on both servers, which is exactly the first-__ parse trap.
  • Unscoped admin / API-key callers still receive every aggregated prompt and the available list (Spec 105 admin-withholding point, listed under follow-ups).
  • Live-instance verification only; unit tests and lint were run separately (see Tests above and the review record below). Both instances were killed by pattern, port 18201 confirmed free, no orphaned server-everything children; 8080 / ~/.mcpproxy untouched.

Cross-model review

Reviewer: opencode 1.18.29, model github-copilot/gpt-6-astra. Rounds run: 3 (one attempt each, every round returned a verdict of FINDINGS). Fix rounds: 2 (round 1 → 8466860ec, round 2 → b1a4864b5, both pushed); round 3 produced no genuine defect in the diff, so nothing was pushed and the PR stands at b1a4864b5. Every finding was verified against the code before being applied or declined. The reviewer was given Spec 105 (staged under the worktree's uncommitted .review-tmp/) as the acceptance contract.

Round 1 — 6 findings (all P3); fixed in 8466860ec

id file claim disposition
R1-1 mcp_routing.go stripAggregatedPromptServer changed admin-visible upstream _meta: an upstream _meta: {} (non-nil empty map) went absent on the wire, and an upstream value under app.mcpproxy/server was overwritten at stamp time and deleted at strip time — contradicting the "exactly what the upstream sent" claim. Applied. Confirmed (mcp.Meta.MarshalJSON emits {} for a non-nil empty map). Stamp is now a private aggregatedPromptStamp{server, upstream *mcp.Meta}; strip restores stamp.upstream verbatim (exact pre-PR pointer parity); an upstream string under our key is no longer accepted as a stamp. TestStripAggregatedPromptServer_PreservesUpstreamMeta rewritten table-driven; empty and our_key subtests fail on the PR-head strip.
R1-2 mcp_direct_scope.go Unstamped non-built-in prompts are dropped only when enforce is true; admin / API-key callers still list and fetch them; FR-006 + SC-005 require withholding from everyone. Declined. Unreachable in production — every upstream prompt registered by buildAggregatedServerPrompts is stamped by construction and the only unstamped prompts are built-ins. Admin withholding is the FR-006/SC-005 contract point already listed as a follow-up; the test at mcp_prompt_scope_test.go:150 documents current behaviour for that follow-up to flip.
R1-3 mcp_routing.go strings.Cut("a:", ":") succeeds, so an empty upstream prompt name is stamped with owner a and registered as a__; FR-006/SC-005 require withholding it from everyone. Declined. The !ok gate on strings.Cut and the manager's name + ":" + prompt qualification predate the PR; the diff only stamps the (correct) owner onto what the gate admits. An a-authorized caller listing a__ is not a scope leak; withholding empty-name registrations is the first FR-006 follow-up.
R1-4 mcp_direct_scope.go Handler-side denial surfaces as -32603 (INTERNAL_ERROR) while the unregistered-name / filter-hidden paths return -32602 (INVALID_PARAMS); message identical. Declined. Verified against mcp-go v1.0.0 handleGetPrompt: any finalHandler error is unconditionally wrapped as INTERNAL_ERROR — no handler-controllable code. Reachable only via a scope change inside one request; already documented as a residual in the code and under follow-ups. The reachable path is now pinned to -32602 + identical message by the new parity assertion.
R1-5 mcp_prompt_scope_test.go, mcp_routing_test.go, profile_tool_test.go Test-strength gaps: (a) UnstampedFailsClosed is vacuous for the explicit !stamped branch; (b) CollisionKeepsFirst is vacuous for the new SortStableFunc (input already lexically ordered); (c) the "unpinned" comparison in HandleSetProfile_DeletedPin… runs with a nil auth context; (d) ScopeUsesCanonicalOwner asserts only NotNil(denied["error"]) and passes with the authorize hook removed. (b), (d) applied. Collision test runs both input orders (removing SortStableFunc fails reversed_order); ScopeUsesCanonicalOwner asserts code + message parity vs a nonexistent name and invokes the registered handler directly with the a-only context (passing nil for authorize fails). (a) declined: auth.CanAccessServer("") and ProfileScope.Allows("") both return false for every enforcing caller, so the !stamped branch is redundant defence in depth and no fixture can make it deciding; the test still bites for the contract it names (it fails if the filter re-parses the display name). (c) declined in round 1 (an unpinned-agent comparison would codify the (available: …) enumeration that the FR-003/FR-004 follow-up removes) — then reconsidered and applied in round 2 as R2-3, since the comparison pins the admin/unpinned affordance unchanged by this diff, which is what the PR claims.
R1-6 server.go, profile_tool.go Enumeration suppression keys on profilePinFromContext(...) != "" only, so an unpinned restricted token on an unknown slug still receives every profile name and can initialize through a profile disjoint from its grant. Reviewer rated P1 against FR-003/FR-004. Declined. Pre-existing and outside the diff (the reviewer's own note downgrades it); the diff closes only the deleted-pin branch. The selectable-profile predicate is the FR-003/FR-004 follow-up and needs a new authorization predicate, not a review-round fix.

Round 2 — 4 findings (1 P2, 3 P3); fixed in b1a4864b5

id sev file claim disposition
R2-1 P2 mcp_routing.go, mcp.go Prompts enabled after startup are published onto servers carrying no prompt filter: the filter was bound only under construction-time EnablePrompts, but RefreshPrompts gates on the live snapshot and runs on every config.reloaded; mcp-go AddPrompts implicitly registers the capability. Result: unfiltered prompts/list for every caller, with "_meta":{"app.mcpproxy/server":{}} on the wire. The filter-less listing hole predates the diff; the diff added the stamp to that path. Applied. Reproduced with a failing test (enable_prompts=false at construction, both flags flipped live, RefreshPrompts, prompts/list as an a-only agent on all four servers → hidden prompt listed + stamp serialized). Filter now bound unconditionally on all four servers (no-op while nothing is registered; capability advertisement stays gated). TestAggregatedPrompt_LateEnableStillFiltered fails before, passes after; helper createTestProxyWithRuntimeCfg added. The handler-side authorize hook already blocked prompts/get on this path, so the diff never widened fetch.
R2-2 P3 mcp_routing.go Stamp doc comment claimed owner and prompt are "published in ONE SetPrompts step and filtered from ONE snapshot"; mcp-go v1.0.0 SetPrompts clears under promptsMu, unlocks, then AddPrompts re-locks per batch, so a concurrent list can observe an empty/partial set. No cross-owner bypass follows (each prompt carries its own owner). Applied. Verified (server.go:914-919); comment rewritten to state the per-prompt binding the guard relies on and to acknowledge the non-atomic clear/re-add. No behaviour change.
R2-3 P3 profile_tool_test.go R1-5(c) marked applied but unchanged: setProfileCtx installs no auth context for pin "", so the "unpinned" comparison was administrator-shaped. Applied. The unpinned call now runs with an AuthTypeAgent context (ProfilePin "", AllowedServers ["*"]) and separately with no auth context. A mutant guard hiding the list from every agent fails the new assertion and passed the old fixture vacuously.
R2-4 P3 (reviewer P2) server.go A deleted-pin token can still tell whether other profiles exist: zero profiles → 404 no profiles configured (checked before the pin check), another profile present → 404 unknown profile '<pin>'; empty-slug /mcp/p gets 404 vs 403. Declined. Pre-existing (the no profiles configured ordering is identical on main); the diff only removes the name list. Already the FR-004 status/body-parity follow-up.

Round 3 — 2 findings (both P2, both recommended as follow-ups by the reviewer); no code change

id file claim disposition
R3-1 server.go FR-004 selectable-profile predicate absent from profileMiddleware: once a slug exists and passes the pin check, the ProfileScope is built without intersecting the profile's servers with the agent's allowed servers, so an unpinned a-only token initializes through a b-only (or empty) profile while a nonexistent slug 404s — an existence oracle. Downstream filtering still blocks b content. Declined. Pre-existing, outside the diff: git diff origin/main -- internal/server/server.go has exactly two hunks (doc comment + the FR-009 404 body); the ProfileScope build is byte-identical on main and handleSetProfile equally lacks the intersection. Reframing of declined R1-6; listed as follow-up per the reviewer's own recommendation.
R3-2 mcp_prompt_scope_test.go Mandatory FR-006 fixtures missing: no __a server / review prompt fixture, no retained-old-registration-across-replacement test, no overlapping-refresh test; collision-owner change is tested at builder level only. Implementation appears correct. Declined as a test-coverage gap, not a runtime defect. Verified with a scratch test (run, then deleted, never committed) through the real path — upstream __a serving review over streamable-http, published by RefreshPrompts, JSON-RPC prompts/list + prompts/get via HandleMessage: registered as __a__review; listed and fetchable for an unrestricted agent and an admin; withheld from an a-only token on list, on get (-32602, same shape as an absent prompt) and at the registered handler (errPromptNotFound). ParseDirectToolName("__a__review") returns ok=false, so main's "unidentifiable owner → keep" rule would have leaked it; the canonical-owner stamp is what makes it pass. Recorded under follow-ups.

Gates per fix round (identical invocation to the Tests section): build both editions OK; go test -race -count=1 …/internal/server/... ok (227 s / 226 s / 246 s); golangci-lint v2 0 issues; goldens unregenerated (git status clean under internal/server/testdata after each run); gofmt / go vet clean; pre-commit and pre-push hooks passed. Commits made by explicit path only; .review-tmp/ never staged.

Follow-ups / Spec 105 gaps (not in this PR)

  • FR-006 / SC-005: prompts with no registration identity are still served to unscoped admin / API-key callers — an empty upstream prompt name (a: → registered a__, mcp_routing.go stamp path), :x stamping an empty owner, and any unstamped display entry (mcp_direct_scope.go, unreachable in production today but the filter only drops it under enforce). Spec names both as administrator exceptions that must be withheld from everyone. (R1-2, R1-3.)
  • FR-006: add a fixture for server __a with prompt review (unparseable display name __a__review) — listed+fetchable for an unrestricted agent and admin, withheld from an a-only token. Verified by hand in review round 3 (R3-2); the committed test is still missing.
  • FR-006: controlled tests for retained old registrations across a SetPrompts replacement and for interleaved/overlapping RefreshPrompts (edge case "Prompt refresh interleaving") — mcp-go v1.0.0 SetPrompts clears under lock then AddPrompts re-locks per batch, and the four routing-mode servers are updated sequentially, so empty/mixed windows exist (R2-2); plus collision-owner change exercised through authorization + dispatch, not only at builder level (both input orders at builder level and the refusal-shape parity assertion in TestAggregatedPrompt_ScopeUsesCanonicalOwner landed in 8466860ec). (R3-2.)
  • FR-006 with FR-013/FR-014: credential-authenticated HTTP prompts/list + prompts/get matrix across /mcp, /mcp/all, /mcp/code, /mcp/call, /mcp/p/<slug> with the differential sentinel fixtures — the committed prompt tests inject the auth context directly into proxy.server.HandleMessage, and TestAggregatedPrompt_LateEnableStillFiltered covers list on all four server instances but not get.
  • FR-004: make the agent-facing status/body identical across "profile missing" (404), "pin mismatch" (403 body naming the pin, server.go:2335), "no profiles configured" (distinct 404, evaluated before the pin check at server.go:2317 — so a deleted-pin caller can still tell an empty fleet from a non-empty one, and on empty-slug /mcp/p gets 404 vs 403; R2-4), and "profile exists but not selectable" (200/proceeds); only the deleted-pin case stops enumerating here.
  • FR-004/FR-003: implement the selectable-profile predicate (server-set intersection with the token's allowed servers) for unpinned tokens on /mcp/p/<slug> and set_profile; today an unpinned restricted token on an unknown slug still receives the full available list (server.go:2358, profile_tool.go:98), set_profile still says (available: ...), and /mcp/p/<slug> initializes through an existing profile whose server set is disjoint from (or empty against) the token's grant while a nonexistent slug 404s — an existence oracle (R1-6, R3-1). Add the required fixture pair: initialize through a selectable AND a non-selectable existing profile URL with an unpinned restricted token (the new deleted-pin test covers only a matching deleted slug with another profile remaining).
  • FR-004: cover /mcp/p and /mcp/p/ (empty slug) with tests; for an unpinned token the empty slug falls into the enumerating unknown-profile branch.
  • Residual (FR-006 / FR-010): handler-side denial surfaces as mcp-go INTERNAL_ERROR (-32603) vs INVALID_PARAMS (-32602) for an unregistered name — reachable only inside the filter→handler window (scope resolved per check, not per request); message identical; no handler-controllable code in mcp-go v1.0.0 (R1-4). The reachable path is pinned to -32602 + message parity by TestAggregatedPrompt_ScopeUsesCanonicalOwner.
  • Process: re-run the opencode gpt-6-astra round to completion so the PR carries a real cross-review verdict — done, three rounds with verdicts (see Cross-model review).

Notes

  • Branches claude/eager-kirch-10f6c2 (this one) and claude/xenodochial-lumiere-5abe75 both edit internal/server/profile_tool.go (handleSetProfile); expect a merge conflict in that function with whichever lands second.
  • The Spec 105 document is the acceptance contract on branch 105-agent-scope-hardening and is not yet on main; code comments cite the Spec 104 FR-016g / FR-016b lineage the fix was built against, which Spec 105 FR-006 / FR-004 restate.

…op profile enumeration on a deleted pin

Two agent-token scope leaks on the HTTP MCP surfaces.

Aggregated prompts (Spec 105 FR-006). filterAggregatedPromptsForAuth
re-parsed each published "server__prompt" display name on its first "__"
to find the owning server, while the registered handler dispatched to the
server captured at publication. With servers "a" and "a__b" both serving
"greeting", "a__b"'s prompt is published as "a__b__greeting", the re-parse
claims owner "a", and an agent token scoped to "a" alone could both list
and fetch a prompt that dispatches to "a__b". The fix records the
canonical owner inside the registered prompt's _meta at publication
("app.mcpproxy/server", stamped by buildAggregatedServerPrompts) and
authorizes list and get against that stamp, read from the same snapshot
mcp-go hands the filter, so owner and prompt can never skew across a
refresh. An upstream prompt with no stamp is dropped for scoped callers
(fail closed). The stamp is stripped from every prompt returned so the
client-visible _meta is exactly what the upstream sent. Every aggregated
prompt handler additionally runs authorizeAggregatedPromptServer against
its OWN server before contacting the upstream, returning mcp-go's exact
unregistered-name wording, so a scope change between the filter and the
handler within one request still cannot dispatch. Upstream prompts are
sorted by qualified name before collision resolution so the display-name
winner no longer depends on map iteration order.

Profile enumeration (Spec 105 FR-004). When the profile an agent token is
pinned to has been deleted, both the profile-URL middleware
(/mcp/p/<slug>, 404 body "available": [...]) and the set_profile tool
("unknown profile 'x' (available: ...)") fell into the generic unknown-slug
branch and enumerated every remaining profile — profiles the resolver
treats as unselectable for that token (a deleted pin is deny-all). Both
now omit the list for pinned callers; unpinned callers and administrators
keep the discovery affordance unchanged.

Tests: TestAggregatedPrompt_ScopeUsesCanonicalOwner (real "a"/"a__b"
upstreams over streamable-http, prompts/list and prompts/get through
HandleMessage, admin parity, no stamp on the wire);
TestBuildAggregatedServerPrompts_HandlerAuthorizesCanonicalServer;
TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed;
TestStripAggregatedPromptServer_PreservesUpstreamMeta; owner-stamp
assertions in the existing buildAggregatedServerPrompts tests;
TestProfile_DeletedPinDoesNotEnumerateProfiles (HTTP, pinned vs admin);
TestHandleSetProfile_DeletedPinDoesNotEnumerateProfiles. The replaced
TestFilterAggregatedPromptsForAuth_KeepsUnparseableName encoded the
leak (keep-on-unparseable) and is superseded by the fail-closed test.

Spec 105 (agent-token scope hardening) is the acceptance contract for
this change; it lives on branch 105-agent-scope-hardening and is not yet
merged. The code comments cite the Spec 104 FR-016g / FR-016b lineage the
fix was built against; Spec 105 FR-006 / FR-004 restate those points.
Frozen tool-surface goldens are untouched.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 8, 2026

Copy link
Copy Markdown

Deploying mcpproxy-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: b1a4864
Status: ✅  Deploy successful!
Preview URL: https://3775fc9a.mcpproxy-docs.pages.dev
Branch Preview URL: https://claude-eager-kirch-10f6c2.mcpproxy-docs.pages.dev

View logs

@codecov-commenter

codecov-commenter commented Sep 8, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 95.77465% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/server/mcp_direct_scope.go 89.28% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📦 Build Artifacts

Workflow Run: View Run
Branch: claude/eager-kirch-10f6c2

Available Artifacts

  • archive-darwin-amd64 (29 MB)
  • archive-darwin-arm64 (26 MB)
  • archive-linux-amd64 (17 MB)
  • archive-linux-arm64 (16 MB)
  • archive-windows-amd64 (29 MB)
  • archive-windows-arm64 (26 MB)
  • frontend-dist-pr (0 MB)
  • installer-dmg-darwin-amd64 (23 MB)
  • installer-dmg-darwin-arm64 (21 MB)

How to Download

Option 1: GitHub Web UI (easiest)

  1. Go to the workflow run page linked above
  2. Scroll to the bottom "Artifacts" section
  3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 34234236233 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

…dler gate is wired

Round-1 review of the aggregated-prompt owner stamp (PR #1227):

- stripAggregatedPromptServer rebuilt the client-visible _meta from the
  stamped copy, so an upstream `_meta: {}` came back absent and an
  upstream value under our own key was lost — an administrator-visible
  wire change the code comment denied. The stamp is now a private struct
  that carries the upstream's own *Meta, and strip hands it back
  unchanged (nil stays nil, `{}` stays `{}`, progress tokens and foreign
  values survive). A string an upstream sends under our key is no longer
  mistaken for a stamp. Table-driven test compares marshalled output
  against the unstamped prompt for every shape.

- TestBuildAggregatedServerPrompts_CollisionKeepsFirst fed the builder an
  input that was already in sorted order, so the new deterministic sort
  was never exercised; both input orders now assert the same winner,
  owner and log fields.

- TestAggregatedPrompt_ScopeUsesCanonicalOwner passed with the production
  authorize hook unwired because mcp-go's filter alone denied the get. It
  now invokes the registered handler directly (bypassing the filter) and
  asserts the scoped caller gets the not-found sentinel while an admin is
  served, and asserts code + message parity between a hidden and a
  nonexistent prompt on prompts/get.
…tion-time enable_prompts

Cross-review round 2 on the prompt-scope change.

R2-1 (P2, applied): the prompt filter was installed only when enable_prompts
was true at CONSTRUCTION (mcp.go for p.server, initRoutingModeServers for
the routing-mode servers), while RefreshPrompts publishes from the LIVE
snapshot on every servers.changed / config.reloaded / prompts-changed event
and mcp-go registers the prompts capability implicitly on the first
SetPrompts. Boot with prompts off, enable them at runtime, and every
routing-mode server served the aggregated prompts with no scope filter and
with the internal owner stamp serialized as "_meta":{"app.mcpproxy/server":{}}
for every caller. The filter is now bound unconditionally on all four
servers; it is a no-op while nothing is registered. The capability
advertisement stays gated as before. Regression:
TestAggregatedPrompt_LateEnableStillFiltered (fails before, passes after,
on all four servers).

R2-2 (P3, applied): the stamp doc no longer claims SetPrompts publishes and
filters from ONE atomic snapshot; mcp-go clears and re-adds under separate
lock acquisitions. The per-prompt binding is what the guard relies on and
that is what the comment now states.

R2-3 (P3, applied): the "unpinned callers keep the discovery affordance"
half of TestHandleSetProfile_DeletedPinDoesNotEnumerateProfiles ran with no
auth context (administrator-shaped). It now runs with an unpinned agent
identity (ProfilePin "", AllowedServers ["*"]) and, separately, with no
auth context. Verified to bite: a guard that hides the list from every
agent fails the new assertion and passed the old one vacuously.

R2-4 (pre-existing, outside the diff, already a PR follow-up): declined.

Test helper: createTestProxyWithRuntimeCfg adds a pre-construction config
hook; createTestProxyWithRuntime is unchanged in behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants