fix(scope): direct-surface definitions authorized by producing identity at every seam (Spec 105 PR F, FR-008) - #1326
Merged
Merged
Conversation
…ty at every seam (Spec 105 PR F, FR-008)
Closes T080-T093. Every direct-mode (/mcp/all) tools/list and tools/call
now authorizes a rendered tool against the identity of the SAME
publication that produced it, never against whatever directCatalog
snapshot happens to be live when the scope/callability filter runs -
closing the publication-skew window an origin flip could exploit (e.g.
display name "a__b__c" meaning server "a" tool "b__c" in one generation
and server "a__b" tool "c" in the next).
## Changes
- renderDirectTools now stamps each rendered mcp.Tool with a private
directToolStamp{owner, rawName, tier} carrying the identity of the
catalog entry that produced it and its dispatch handler
(mcp_direct_scope.go, mcp_routing.go). filterDirectModeToolsForAuth and
filterDirectToolsForAgentCallability read the stamp first and authorize
against it directly; only an unstamped tool (a built-in, or a bare
mcp.Tool a test constructs directly) falls back to the pre-105
catalog/builtin resolution unchanged.
- A terminal WithToolFilter(stripDirectToolStampFilter), registered last,
removes the stamp before any tool reaches a client, for every caller
including administrators - the admin early-return that used to skip the
scope filter's per-tool loop entirely is removed, so the "no identity ->
withheld" rule applies to everyone (SC-005 exception), not only scoped
callers.
- buildDirectCatalog refuses to admit a tool with an empty raw name at
all (FR008-G7); resolveDirectTool's "no `__` separator -> built-in"
structural inference is removed, replaced by the explicit
builtinDirectToolNames set populated from the surface's own
constructors (FR008-G2, positive identification).
- makeDirectModeHandler's two scope-refusal branches no longer name the
server they denied (D12); the message now echoes only the
caller-supplied display name, matching what mcp-go's own call-time
filter re-evaluation already answers for an unregistered name before
the handler ever runs in real dispatch - this branch is defense in
depth for direct handler invocation that bypasses that re-evaluation.
- buildAggregatedServerPrompts drops an upstream prompt with an empty raw
name; filterAggregatedPromptsForAuth withholds an unstamped prompt for
every caller, not only when scope enforcement was already active.
## Tests
T080-T086a written first (mcp_direct_publication_identity_test.go,
mcp_direct_underscore_test.go, mcp_direct_protocol_test.go - all new),
covering the origin-flip fixture in both serialization modes with full
definition assertions, reverse-flip/plain-addition visibility, the
tier-change seam in deferred mode, the protocol-level -32602 envelope
parity proof (byte-equal to an unregistered name, no owner metadata), the
`__a` server steady-state and seam fixtures, and the prompt-side empty-name
and unstamped-withholding cases.
Inverted the pinned tests that predated this fix, including several not
explicitly enumerated by the task list but discovered red by the full
suite run: mcp_direct_skew_test.go (added-name, origin-flip,
annotations-only-change scenarios), mcp_direct_catalog_test.go /
mcp_direct_catalog_publish_test.go (empty-raw-name admission, structural
builtin inference), mcp_routing_test.go (TestDirectModeHandler_
ServerAccessDenied's disclosing message; TestFilterDirectModeToolsForAuth_
KeepsNonDirectTools renamed to _DropsNonBuiltinSeparatorlessNames),
profile_pin_enforcement_test.go, and mcp_prompt_scope_test.go
(TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed's
unscoped-caller assertion).
direct_full_prefeature.golden.json and every toolslist golden are
unregenerated and byte-identical.
## Verification
- go build ./..., go vet ./... clean
- go test ./internal/server/... (full package): ok, 278s
- go test -race ./internal/server/...: ok, 383s
- golangci-lint (bare + --build-tags server): clean on every touched file
(pre-existing issues elsewhere in the repo are unrelated to this diff)
- Cross-model review: opencode quota exhausted on both terra and sol
(confirmed); fell back to codex exec -m gpt-5.6-sol per the CLAUDE.md
ladder. 3 rounds, VERDICT: clean.
- ROADMAP.md regenerated (scripts/gen-roadmap.py) to reflect the tasks.md
checkbox updates and pass the pre-commit roadmap-verify hook; it was
already drifted from an unrelated prior change, so this also fixes
that drift.
Related #105
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
d61a1f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://339e4fe5.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://105-f-direct-publication.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35512156453 --repo smart-mcp-proxy/mcpproxy-go
|
# Conflicts: # ROADMAP.md
…sal shape (Spec 105 PR F follow-up) Adversarial re-review of PR #1326 (codex exec, gpt-5.6-sol) found two real issues in the direct-publication scope hardening, both fixed here, plus test-hardening gaps the same pass flagged: 1. directBlockReasonKey (telemetry) and directToolCallabilityResult (the response body) disagreed on which gate "won" when a tool was BOTH config-denied and pending/changed-approval at once. Both now share one classifyDirectRefusal helper with a single precedence order (quarantine -> approval-lock -> config-denied/generic), matching the order every other dispatch path already uses (toolGate.lockStatus in mcp.go's handleCallToolVariant/handleCallTool). 2. makeDirectModeHandler's own D12 defense-in-depth scope refusal built a mcp.NewToolResultError (a successful result with isError:true) for a case mcp-go's own call-time filter refuses with a protocol-level -32602 error — a different envelope KIND, not just different wording. It now returns the handler's own error, wrapping mcp-go's ErrToolNotFound, so the envelope kind and text converge (the JSON-RPC error CODE cannot be matched from a ToolHandlerFunc — mcp-go always maps a handler error to INTERNAL_ERROR — documented as an accepted residual, mirroring the already-accepted prompt-side twin in authorizeAggregatedPromptServer). 3. Test hardening: a forged-stamp regression test for directToolStamp (the tool-side analogue of the existing prompt-side test), a positive control on the unstamped-prompt withholding test, an exact-match (not Contains) assertion on the profile-pin non-disclosure check, and full JSON-RPC envelope equality (not just error code + substring) in TestDirectProtocol_StampNeverOnWire_FilterReEvaluatedAtCallTime. go build/vet, race suite (internal/server, internal/server/tokens, internal/serveredition/..., internal/config, internal/oauth, internal/httpapi, internal/storage), and golangci-lint (bare + --build-tags server) all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 20, 2026
…tion to done (#1327) Spec 105 PR C (#1325) and PR F (#1326) both merged; roadmap.yaml still read status: todo for both since the implementing agents intentionally left them unflipped pending merge review, per this repo's "no merge without instruction" convention for security PRs. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 21, 2026
…ng holds (Spec 105 PR H1) (#1332) Spec 105 PR H1 — `scope-regression-suite` part 2 (FR-011/013/014, SC-007, gaps FR01x-G4..G7). Closes tasks T107-T116. This is the FINAL phase of the nine-PR agent-scope-hardening epic (A, B, D, E, H0, C, F, G already merged). ## Changes - **Two-fixture differential oracle** (`internal/server/scope_differential_test.go`, new): `newScopeFixture`/`runScopeScenario`/`normalizeScopeResponse` per contracts/differential-oracle.md, plus `TestScopeCoverage_EveryUserStoryScenario` registering every US1.1-1.8/US2.1-2.6/US3.1-3.4 scenario by id — some directly via the general three-server harness, most by re-running each prior PR's own dedicated differential fixture (plan.md: "H1 re-registers them by User Story id"). - **Pinned-reversal grep guard** (`internal/server/scope_pinned_reversal_guard_test.go`, new): `TestScopePinnedReversalsStayInverted` proves none of the four pre-105 insecure assertions gap-map.md's FR01x-G7 entry names have crept back to their original form, traced to their exact pre/post-105 diffs. - **Retained-effect fixtures** (`internal/server/scope_retained_effects_test.go`, new): real reproductions of 5 of 7 SC-001-excluded effects (shared-limiter contention, cross-server scan admission via the actual shadowing.cross_server impersonation-clone mechanism, prompt-name collision, global prompt cap, direct display-name collision), 1 via a companion package (`internal/upstream/manager_prompts_deadline_test.go`, new — shared vs independent prompt-refresh deadline, two discriminating regimes), 1 cited cross-package (log rotation). - **HTTP credential matrix** (`internal/server/scope_http_matrix_test.go`, new): real minted tokens over real loopback HTTP through mcpAuthMiddleware, covering every FR-014 applicability-matrix row at least once plus a content-level (raw wire-text) non-disclosure proof for retrieve_tools. - **Latency harness finalized** (`internal/server/scope_latency_test.go`): all four FR-011 operations (retrieve_tools, read_cache, prompts/list, tools/list) with real workloads, plus `.github/workflows/scope-latency.yml` (new) and `cmd/scope-latency-compare` (new, stdlib-only) for the merge-base p95 regression gate. - **Bug fix found via this suite**: `internal/logs.ReadUpstreamServerLogTail`/ `ReadUpstreamServerLogTailAttributed` dereferenced a nil `*config.LogConfig` — fixed with a TDD red/green pair (`internal/logs/nil_log_config_test.go`). - Docs and roadmap sync: `docs/features/agent-tokens.md` invariant wording finalized; `roadmap.yaml`'s `scope-refusal-shapes` task (stale `todo` despite PR G #1328 already merged) flipped to `done`. `scope-regression-suite` and the epic-level status are deliberately left for the maintainer to flip on merge. ## Testing - `go build`/`go vet` (bare + `-tags server`): clean - Full `-race` suite across internal/server, internal/upstream, internal/cache, internal/logs, cmd/scope-latency-compare: green - `golangci-lint --new-from-rev=origin/main` (bare + `--build-tags server`): 0 new issues - `./scripts/test-api-e2e.sh`: 70/70 passed - Cross-model review: opencode terra/sol/astra confirmed quota-exhausted; fell back to `codex exec --model gpt-5.6-sol`. 4 rounds, every finding verified genuine before fixing (non-vacuous confirmed by temporarily reverting the underlying guard and observing red). `VERDICT: clean` (round 4) — full history in tasks.md T116. Related #1223 #1224 #1225 #1226 #1227 #1279 #1282 #1283 #1284 #1285 #1325 #1326 #1328
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Spec 105 PR F —
scope-direct-publication(FR-008, gaps FR008-G1…G7), tasks T080-T093.Every direct-mode (
/mcp/all)tools/listandtools/callnow authorizes a rendered tool against the identity of the same publication that produced it, never against whateverdirectCatalogsnapshot happens to be live when the scope/callability filter runs. Before this fix, both filters resolved a renderedmcp.Tool's owning server by looking its display name up inp.loadDirectCatalog()at filter-evaluation time — unsound during a publication rebuild, sinceSetToolslands the new registry before the catalog pointer swaps. Sharpest in an "origin flip": display namea__b__ccan mean (servera, toolb__c) in one generation and (servera__b, toolc) in the next; a token scoped to old-onlyacould see the new owner's definition still attributed to the old one, and a subsequent dispatch (bound to the new registration) would be refused with a message naming the new server — a disclosure.renderDirectToolsstamps each rendered tool with a privatedirectToolStamp{owner, rawName, tier}carrying the identity of the catalog entry that produced it and its dispatch handler.filterDirectModeToolsForAuth/filterDirectToolsForAgentCallabilityread the stamp first; only an unstamped tool (a built-in, or a baremcp.Toola test constructs directly) falls back to the pre-105 catalog/builtin resolution.WithToolFilter(stripDirectToolStampFilter), registered last, removes the stamp before any tool reaches a client, for every caller including administrators — the admin early-return that used to skip the scope filter's per-tool loop is gone, so "no identity → withheld" applies to everyone (SC-005 exception).buildDirectCatalogrefuses to admit a tool with an empty raw name at all (FR008-G7); the "no__separator → built-in" structural inference is removed, replaced by an explicitbuiltinDirectToolNamesset populated from the surface's own constructors (FR008-G2, positive identification).makeDirectModeHandler's two scope-refusal branches no longer name the server they denied (D12) — defense-in-depth for direct handler invocation that bypasses mcp-go's call-time filter re-evaluation, or for a narrow live profile/config race (see review round 2 below).Test plan
mcp_direct_publication_identity_test.go,mcp_direct_underscore_test.go,mcp_direct_protocol_test.go) covering the origin-flip fixture (both serialization modes, full definition assertions), reverse-flip/plain-addition visibility, deferred-mode tier-change seam, mcp-go's own call-time filter refusal envelope (byte-equal to an unregistered name, no owner metadata), the__aserver fixture, and the prompt-side empty-name/unstamped casesmcp_routing_test.go,profile_pin_enforcement_test.go,mcp_prompt_scope_test.go) — documented in the commit messagego build ./...,go vet ./...cleango test ./internal/server/...(full package, no skip beyond the standard CI regex): okgo test -race ./internal/server/...: okgolangci-lint(bare +--build-tags server): clean on every touched file*.golden.json,toolslist_goldens/) unregenerated and byte-identicalterraandsol(confirmed, not assumed); fell back tocodex exec -m gpt-5.6-sol --sandbox read-onlyper the CLAUDE.md ladder — 3 rounds,VERDICT: cleanReview round 2 follow-up (adversarial re-review, 2026-09-20)
A fresh
codex exec --model gpt-5.6-solpass found two real issues in the original PR, both fixed here, plus test-hardening gaps:Block-reason precedence mismatch (
internal/server/mcp_direct_callability.go):directBlockReasonKey(telemetry) anddirectToolCallabilityResult(the response body) classified a tool that is BOTH config-denied AND pending/changed-approval differently — the response said config-denied, the counted reason said pending/changed. Both now share oneclassifyDirectRefusalhelper with a single precedence order (quarantine → approval-lock → config-denied/generic), matching the order every other dispatch path already uses (toolGate.lockStatusinmcp.go'shandleCallToolVariant/handleCallTool).D12 refusal-shape correction (
internal/server/mcp_routing.go):makeDirectModeHandler's own defense-in-depth scope refusal (directScopeRefusalError, formerlydirectScopeRefusalMessage) used to build amcp.NewToolResultError— a successful JSON-RPC result withisError:true— for a case mcp-go's own call-time filter refuses with a protocol-level-32602error. That was a different envelope KIND, not just different wording, and the original PR's "byte-equal to an unregistered name" claim (below) did not hold for this branch, only for mcp-go's own filter-level refusal.Corrected claim: the handler-level branch now returns the handler's own
error(fmt.Errorf("tool '%s' not found: %w", name, mcpserver.ErrToolNotFound)), which:"tool '<name>' not found: tool not found").INTERNAL_ERROR(-32603), a code aToolHandlerFunccannot override, while the filter path answersINVALID_PARAMS(-32602) for the identical case. This is a structural limit ofmcp-gov1.0.0's dispatch (server.go handleToolCall), not something this PR's code controls, and mirrors an already-accepted, already-documented residual on the prompt side (authorizeAggregatedPromptServer's doc comment inmcp_direct_scope.go). No owner name or scope reason is ever disclosed either way — the residual is the numeric code alone, observable only by a caller that can time a request inside the narrow live-profile/config race this defense-in-depth branch exists for.TestDirectProtocol_StampNeverOnWire_FilterReEvaluatedAtCallTimenow asserts full envelope equality (code + message + exact key set) between a hidden-but-registered call and a genuinely unregistered one, not just error code + substring.Test hardening (chunk D/E findings, all closed):
TestReadDirectToolStamp_RejectsForgedValue/_AcceptsGenuineStamp(mcp_direct_scope_test.go) — the tool-side forged-stamp regression the prompt side already had.TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgentnow registers a properly-stamped sibling prompt as a positive control and asserts it IS listed and IS retrievable, so the test can no longer pass vacuously if prompt list/get were broken outright.TestDirectModeHonorsTokenProfilePin's non-disclosure assertion switched from a looseContainsto an exact string match against the corrected refusal text, closing the gap where a message that appended extra scope-reason wording after the expected prefix would have still passed.Not merging — leaving for review per this repo's convention for the Spec 105 security PRs.
🤖 Generated with Claude Code