fix(scope): scope-first refusal precedence, hidden equals nonexistent on every surface (Spec 105 PR G, FR-010) - #1328
Merged
Merged
Conversation
Deploying mcpproxy-docs with
|
| Latest commit: |
62fb994
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://15a166e1.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://scope-refusal-shapes.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 35524053671 --repo smart-mcp-proxy/mcpproxy-go
|
… on every surface (Spec 105 PR G, FR-010) Closes T094-T106. Every fixed MCP surface now answers a hidden server/tool and a nonexistent one with the same refusal shape, and a tier denial on an authorized server reaches the handler instead of being swallowed into a generic "not found" - closing gaps FR010-G1, G2, G3, G4, G6 and G7 (G5 was closed in a prior PR). ## Changes - G1: the "no client found" hint's `Available servers:` list is filtered through the caller's effective scope (profile ∩ token), not the raw fleet (mcp.go). - G2: describe_tool's toolVisibleToSession checks scope before index presence for scoped callers, and its case-correction suggestion is attempted for the same reason set - so a hidden document occupying the exact (server, tool) pair a caller asked about no longer silently swallows the did-you-mean it would otherwise get (mcp_visibility.go, mcp_describe_tool.go). - G3: the direct-mode catalog gained a canonicalShadow map and LookupCanonicalForAuth, so an authorized canonical id resolves at request time even when a hidden display-name entry collides with it on the same string - resolveDirectDescribeIDIn now falls through to that shadow resolution, filtered to full visibility (scope + tier + callability), not scope alone (mcp_direct_catalog.go, mcp_describe_direct.go). - G4: the direct surface's case-correction resolver (suggestDirectToolID) no longer gives up at the first case-fold match that happens to be invisible - it keeps scanning for a later, authorized one. - G6: at call time, mcp-go's WithToolFilter chain (which re-runs unchanged at both tools/list and tools/call) now lets an over-tier-but-authorized tool through instead of excluding it into an unregistered-name -32602 - the registered handler's own scope->tier->callability precedence answers instead. The mechanism is a mutable directRequestKindBox installed on every request's ctx by mcpAuthMiddleware and written by BeforeListTools/BeforeCallTool hooks, letting the filter chain tell a listing from a call-time re-evaluation for the first time (mcp_direct_scope.go, mcp_direct_callability.go, mcp_routing.go, server.go). - G7: the sandbox (jsruntime) and the retrieve surface (call_tool_*) each used to run the profile-derived allow-list check and the token's own server-scope check independently, answering two different refusal bodies depending on which one excluded a server - a server inside the pin but outside the token was distinguishable from a nonexistent one. Both are now one evaluation, one body, for agent callers (mcp.go, internal/jsruntime/runtime.go). ## Caller-kind correctness (found by adversarial review, fixed in this PR) Cross-model review caught two classes of real bug across 4 rounds: - Several fixes above initially used `authInfo/authCtx != nil` where "is this actually a scoped/restricted caller" was meant, silently treating a profile-scoped ADMINISTRATOR (and, separately, a server-edition AuthTypeUser caller) as unrestricted. Introduced isAdmin() on jsruntime.AuthInfo and isScopeRestrictedCaller() on auth.AuthContext-consuming code in internal/server, and gated every new G1-G7 behavior change on the correct caller-kind predicate, preserving SC-005 byte-parity for administrators and closing the same scope gaps for AuthTypeUser callers that FR-010 closes for agent tokens. - The canonical-shadow disambiguation predicate (G3) initially filtered candidates by scope alone; a candidate that was scope-authorized but callability-locked (pending/changed/disabled) still counted as "authorized," making a genuinely reachable candidate look ambiguous against it. Fixed to use full visibility (directEntryVisibleToSession). A related instance of the AuthTypeUser pattern was found in internal/server/mcp_direct_callability.go's callability-hiding gates and the aggregated-prompt filters (internal/server/mcp_direct_scope.go) - deliberately left unfixed here per the reviewer's own confirmation that the agent-vs-operator callability split is intentional design, not a scope bug, and flagged as a separate follow-up. ## Testing T094-T099 written first (failing), then T100-T104a implementation: mcp_auth_scope_test.go, mcp_describe_tool_scope_test.go (new), mcp_describe_direct_test.go, mcp_direct_scope_test.go, mcp_code_execution_scope_test.go, plus jsruntime/runtime_test.go and scope_cache_fixtures_test.go (one inverted pinned-reversal assertion). - go build ./..., go vet ./...: clean - go test ./internal/server/... ./internal/jsruntime/... (full packages, non-race and -race): all green - go test -race -tags server (serveredition/config/oauth/server/httpapi/storage, CLAUDE.md's -skip regex): all green - golangci-lint (bare + --build-tags server), gofmt: zero new findings - Cross-model review: opencode (terra/sol/astra) quota-exhausted, confirmed; fell back to codex exec -m gpt-5.6-sol per CLAUDE.md. 4 rounds, VERDICT: clean (7 real findings total across rounds 1-3, all fixed and covered by new regression tests). ROADMAP.md regenerated (scripts/gen-roadmap.py) to reflect the tasks.md checkbox updates; roadmap.yaml's scope-refusal-shapes status is left untouched for the maintainer, per this repo's convention for these security PRs. Related #105 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nary noise (Spec 105 PR G follow-up) CI's Server Edition job flagged TestProfileIndex_SelectableAllocatesNothing with 13 allocations for "scoped, absent slug" over the 4096-server fleet (#1328), but the identical commit's other CI run passed with zero everywhere, and running the full internal/server suite locally under -race -tags server never reproduces it. profileIndex.selectable/reach (profile_tool.go) are unchanged by PR G and remain allocation-free in isolation. testing.AllocsPerRun counts process-wide mallocs, so a goroutine still winding down from an earlier test in this package's shared test binary (an SSE/HTTP client closing against an already-stopped httptest server, a runtime fixture's shutdown) inflates whichever case's measurement window it happens to overlap. This is the exact, already-diagnosed failure mode documented on the neighboring TestSelectableProfileNames_PinOutcomesDoSameWork, which takes the minimum AllocsPerRun over several samples per case because noise only ever adds allocations. TestProfileIndex_SelectableAllocatesNothing predates that hardening and took a single sample, making it the one allocation test in this file still vulnerable to the same known scheduler noise. Apply the identical minimum-of-7-samples pattern here. The assertion stays require.Zero — this does not widen the allocation budget, it only filters transient noise from unrelated goroutines the way its sibling test already does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
force-pushed
the
scope-refusal-shapes
branch
from
September 20, 2026 16:52
f0fbf3a to
62fb994
Compare
This was referenced Sep 20, 2026
Dumbris
added a commit
that referenced
this pull request
Sep 20, 2026
Spec 105 PR G (#1328) merged; roadmap.yaml still read status: todo. 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
5 tasks
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 G —
scope-refusal-shapes(FR-010). Closes tasks T094-T106.Every fixed MCP surface now answers a hidden server/tool and a
nonexistent one with the same refusal shape, and a tier denial on an
authorized server reaches the handler (insufficient-permission) instead
of being swallowed into a generic "not found" — closing gaps FR010-G1, G2,
G3, G4, G6 and G7 (G5 was closed in a prior PR).
Changes
Available servers:list is nowfiltered through the caller's effective scope (profile ∩ token), not the
raw fleet (
mcp.go).describe_tool'stoolVisibleToSessionchecks scope before indexpresence for scoped callers, and its case-correction suggestion is
attempted for the same reason set — so a hidden document occupying the
exact
(server, tool)pair a caller asked about no longer silentlyswallows the did-you-mean it would otherwise get (
mcp_visibility.go,mcp_describe_tool.go).canonicalShadowmap andLookupCanonicalForAuth, so an authorized canonical id resolves atrequest time even when a hidden display-name entry collides with it on
the same string —
resolveDirectDescribeIDInfalls through to thatshadow resolution, filtered to full visibility (scope + tier +
callability), not scope alone (
mcp_direct_catalog.go,mcp_describe_direct.go).(
suggestDirectToolID) no longer gives up at the first case-fold matchthat happens to be invisible — it keeps scanning for a later, authorized
one.
WithToolFilterchain (which re-runsunchanged at both
tools/listandtools/call) now lets anover-tier-but-authorized tool through instead of excluding it into an
unregistered-name
-32602— the registered handler's ownscope→tier→callability precedence answers instead. Mechanism: a mutable
directRequestKindBoxinstalled on every request'sctxbymcpAuthMiddleware, written byBeforeListTools/BeforeCallToolhooks,letting the filter chain tell a listing from a call-time re-evaluation for
the first time (
mcp_direct_scope.go,mcp_direct_callability.go,mcp_routing.go,server.go).call_tool_*)each used to run the profile-derived allow-list check and the token's own
server-scope check independently, answering two different refusal bodies
depending on which one excluded a server — a server inside the pin but
outside the token was distinguishable from a nonexistent one. Both are
now one evaluation, one body, for agent callers (
mcp.go,internal/jsruntime/runtime.go).Caller-kind correctness (found by adversarial review, fixed in this PR)
Cross-model review caught two classes of real bug across 4 rounds:
authInfo/authCtx != nilwhere "isthis actually a scoped/restricted caller" was meant, silently treating a
profile-scoped administrator (and, separately, a server-edition
AuthTypeUsercaller) as unrestricted. IntroducedisAdmin()onjsruntime.AuthInfoandisScopeRestrictedCaller()ininternal/server,and gated every new G1–G7 behavior change on the correct caller-kind
predicate — preserving SC-005 byte-parity for administrators and closing
the same scope gaps for
AuthTypeUsercallers that FR-010 closes foragent tokens.
candidates by scope alone; a candidate that was scope-authorized but
callability-locked (pending/changed/disabled) still counted as
"authorized," making a genuinely reachable candidate look ambiguous
against it. Fixed to use full visibility
(
directEntryVisibleToSession).A related instance of the
AuthTypeUserpattern was found inmcp_direct_callability.go's callability-hiding gates and theaggregated-prompt filters (
mcp_direct_scope.go) — deliberately leftunfixed here per the reviewer's own confirmation that the agent-vs-operator
callability split is intentional design, not a scope bug. Flagged as a
separate follow-up task.
Test plan
go build ./...,go vet ./...: cleango test ./internal/server/... ./internal/jsruntime/...(fullpackages, non-race and
-race): all greengo test -race -tags serveracrossserveredition/config/oauth/server/httpapi/storage (CLAUDE.md's
-skipregex): all green
golangci-lint(bare +--build-tags server),gofmt: zero newfindings
confirmed; fell back to
codex exec -m gpt-5.6-solper CLAUDE.md. 4rounds,
VERDICT: clean(7 real findings total across rounds 1–3, allfixed and covered by new regression tests)
roadmap.yaml'sscope-refusal-shapesstatus is intentionally leftuntouched — for the maintainer to flip, per this repo's convention for
these security PRs. Not merged, per instruction.
🤖 Generated with Claude Code