diff --git a/ROADMAP.md b/ROADMAP.md index 60f62983d..2eadc71fe 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -888,7 +888,7 @@ graph LR | Web UI + macOS app UX audit | In progress | P0 | — | | | | Release qualification gate (auto-QA matrix blocks the tag) | In progress | P0 | — | [081-release-qa-gate](./specs/081-release-qa-gate/) | | | Action log / transparency — info at a glance | In progress | P1 | — | | | -| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 31/109 (28%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | | +| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 42/109 (39%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | | | Token-efficiency benchmark: measured savings, published results | In progress | P1 | 62/64 (97%) | [103-token-bench](./specs/103-token-bench/) | | | Telemetry identity & data quality (machine_id + CI-filter hardening) | In progress | P1 | — | | | | Telemetry v7: honest funnel + churn instrumentation | In progress | P1 | — | [080-telemetry-v7-churn](./specs/080-telemetry-v7-churn/) | | @@ -1034,6 +1034,6 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [102-schema-deferred](./specs/102-schema-deferred/) | `shipped` | 89/89 (100%) | | [103-token-bench](./specs/103-token-bench/) | `shipped` | 62/64 (97%) | | [104-auto-routing-mode](./specs/104-auto-routing-mode/) | — | — | -| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 31/109 (28%) | +| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 42/109 (39%) | | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `in-flight` | 102/126 (81%) | diff --git a/docs/features/agent-tokens.md b/docs/features/agent-tokens.md index 798b604c0..d68f8bf65 100644 --- a/docs/features/agent-tokens.md +++ b/docs/features/agent-tokens.md @@ -534,8 +534,8 @@ mcpproxy token create \ Server-side enforcement (no client cooperation required): -- **`set_profile("other")` is rejected** — a pinned token cannot switch its session to a different profile (switching to its own pinned profile, or clearing, is allowed). -- **`/mcp/p/` returns `403`** — connecting to any profile URL other than the pinned one is forbidden; the pinned profile's own URL works. +- **`set_profile("other")` is rejected** — a pinned token cannot switch its session to a different profile (switching to its own pinned profile while it still has reach, or clearing, is allowed; clearing reports `active_profile: ""` and the pin's servers). +- **`/mcp/p/` returns `404`** — connecting to any profile URL other than the pinned one is refused with the same non-disclosing `unknown profile` body every other non-selectable slug produces (see [404 responses](./profiles.md#404-responses)); the pinned profile's own URL works while the pin has reach. - **The pin is the highest-precedence resolver source**, above an explicit `/mcp/p/` URL scope and above a session `set_profile` selection. - **Every dispatch surface resolves it** — `retrieve_tools`, `describe_tool`, `call_tool_*`, the `code_execution` sandbox, direct-routing mode (`server__tool`) and [preflight](./tools-preflight.md) all bound themselves by the pin, so no routing mode is a way around it. @@ -548,7 +548,7 @@ Resolution precedence (highest wins): 4. none (no profile filtering — all allowed servers) ``` -**Validation & config changes**: the pinned slug must name a configured profile at creation time (creation is rejected otherwise). If the profile is **later removed** from the configuration, the pin resolves to a **deny-all scope**: the token sees no upstream servers and no tools, on the MCP session path and in [preflight](./tools-preflight.md#disclosure-tiers) alike. The request is logged with a warning naming the removed profile, not hard-failed at the transport. The pin is a restriction the operator applied, so losing the profile it names must never hand the token a wider view than it had the day before — re-create the profile, or re-mint the token against a live one, to restore it. Pinning composes with server scoping and permission tiers: a request must satisfy **all** of them. +**Validation & config changes**: the pinned slug must name a configured profile at creation time (creation is rejected otherwise). If the profile is **later removed** from the configuration, the pin resolves to a **deny-all scope**: the token sees no upstream servers and no tools, on the MCP session path and in [preflight](./tools-preflight.md#disclosure-tiers) alike. A pin with **zero reach** — the profile still exists but is empty, names only unconfigured servers, or no longer overlaps the token's `allowed_servers` — is treated exactly like a deleted one on `set_profile` and `/mcp/p/`, so the token cannot tell whether its own pin still exists. A request under a **deleted** pin is logged with a warning naming the removed profile, not hard-failed at the transport; a refused `/mcp/p/` initialization (deleted or zero-reach alike) is logged as `profile URL refused for scoped caller`. The pin is a restriction the operator applied, so losing the profile it names must never hand the token a wider view than it had the day before — re-create the profile, or re-mint the token against a live one, to restore it. Pinning composes with server scoping and permission tiers: a request must satisfy **all** of them. The pin is shown by `token list` (PROFILE PIN column) and `token show` (Profile Pin field), and is preserved across `token regenerate`. diff --git a/docs/features/profiles.md b/docs/features/profiles.md index 0ba58115c..166195062 100644 --- a/docs/features/profiles.md +++ b/docs/features/profiles.md @@ -67,9 +67,9 @@ The `set_profile` MCP tool switches the active profile **inside a live session** - The selection is keyed by the MCP session id (stable per streamable-HTTP / SSE connection) and persists for the lifetime of that session. - It applies to subsequent `retrieve_tools`, `call_tool_*`, `code_execution` and direct-mode (`server__tool`) calls on the base `/mcp` endpoint — `retrieve_tools` searches the profile's per-profile index directly. -- Passing an empty string (`""`) clears the selection and returns to all servers (the result lists every configured server). A token with a [`profile_pin`](./agent-tokens.md#profile-pinning) keeps its pin — the result then lists the pinned profile's servers, since that is what the session can still reach. -- The `servers` list is always bounded by the caller's credential, using the same rule that scopes `retrieve_tools`: for an [agent token](./agent-tokens.md) scoped to specific servers it is the intersection of the selection (all servers, the chosen profile, or the pin) with the token's `allowed_servers`, so a token restricted to one server is never told about the others. API-key and socket callers see the full lists. -- An unknown slug is rejected: `unknown profile '' (available: research, deploy)`. For an agent token the `available:` list names only the profiles that token may select — its pin, or the profiles overlapping its `allowed_servers` — not the whole catalogue, and a profile entirely outside the token's reach is rejected with that same error rather than confirmed as existing. +- Passing an empty string (`""`) clears the selection and returns to all servers. `active_profile` always reports the **stored session selection** — `""` after a clear, even for a token with a [`profile_pin`](./agent-tokens.md#profile-pinning) — while `servers` reports the **effective scope** the session can actually reach after the update: the pin's servers for a pinned token (nothing once the pinned profile has been deleted), the URL profile on a `/mcp/p/` endpoint, otherwise the selection or every configured server. +- The `servers` list is always bounded by the caller's credential, using the same rule that scopes `retrieve_tools`: for an [agent token](./agent-tokens.md) scoped to specific servers it is the intersection of the effective profile (resolved pin > URL > session, see [Resolution precedence](#resolution-precedence)) with the token's `allowed_servers`, so a token restricted to one server is never told about the others. On a `/mcp/p/` endpoint the URL still governs the request, so `set_profile("other")` there stores `other` as `active_profile` but reports ` ∩ allowed_servers` in `servers`. API-key and socket callers see the full lists. +- An unknown slug is rejected. An administrator (API key, socket, anonymous back-compat) gets the discovery affordance: `unknown profile '' (available: research, deploy)`. An agent token gets `unknown profile ''` with no list at all: it may select only the profiles overlapping its `allowed_servers` (or its pin while the pin still has reach), and a profile entirely outside its reach (an empty profile, a profile whose servers are all outside `allowed_servers`, or the token's own pin once it no longer exists or no longer overlaps the token's servers) is rejected with that same error rather than confirmed as existing. A pinned token asking for any profile OTHER than its pin (see [profile pinning](./agent-tokens.md#profile-pinning)) is rejected with that same `unknown profile ''` error too — never a distinct "pinned to..." message, which would let the token confirm from the wording alone that it is pinned, and to what, from a refusal aimed at a different slug. The check looks only at the requested slug (and the token's pin) and tests the token's own `allowed_servers` against that profile's precomputed server set — its cost does not depend on how many other profiles are configured, on how many servers the requested profile declares or on how many servers are configured at all, only on the size of the token's own grant — so a token cannot learn which profiles or servers exist, or whether it is pinned, from `set_profile`, by body or by timing. - Session state is cleared automatically on session close. `set_profile` is available on the default `/mcp` server and the `call_tool` / `code_execution` routing-mode servers. @@ -136,7 +136,11 @@ Profile changes take effect for new connections on the next config reload. In-fl ## 404 responses +For API-key, socket and (when `require_mcp_auth` is off) unauthenticated callers: + | Condition | Body | |-----------|------| | No profiles configured | `{"error":"no profiles configured"}` | | Unknown slug | `{"error":"unknown profile ''","available":["research","deploy"]}` | + +An [agent token](./agent-tokens.md) may initialize through `/mcp/p/` only when that profile is one it could select with `set_profile` — its servers overlap the token's `allowed_servers`, or it is the token's pin and the pin still has reach. Every other request — a missing or deleted slug, a configured profile outside the token's reach, an empty profile, a pin mismatch, the slug-less `/mcp/p` and `/mcp/p/`, and an empty fleet — receives one and the same `404 {"error":"unknown profile ''"}` with no `available` list, and the check itself looks only at the requested slug (and the token's pin), testing the token's own `allowed_servers` against that profile's precomputed server set — its cost does not depend on how many other profiles are configured, on how many servers the requested profile declares, on how many servers are configured at all, or on whether this is the first request after a reload (the profile index is rebuilt when the configuration changes, not on demand); it scales only with the size of the token's own grant — so a scoped caller cannot learn which profiles or servers exist from the profile URL, by body or by timing. The refusal is silent towards the agent only: each one is logged (`profile URL refused for scoped caller`, with the token name, the requested slug and the remote address) so an operator can spot a token probing the slug space. diff --git a/internal/runtime/configsvc/service.go b/internal/runtime/configsvc/service.go index 0aa97daaf..b2b286e3f 100644 --- a/internal/runtime/configsvc/service.go +++ b/internal/runtime/configsvc/service.go @@ -3,6 +3,7 @@ package configsvc import ( "context" "fmt" + "slices" "sync" "sync/atomic" "time" @@ -54,6 +55,13 @@ type Service struct { // #937 admission gate so a poisoned server can never be reconciled and // indexed in the window between "config parsed" and "config gated". prePublish atomic.Value // func(*config.Config) *config.Config + + // prePublishObservers run after prePublish, on the exact *Config that is + // about to be stored — while nothing else can observe it. Read-only: they + // build state keyed on that pointer (Spec 105's profile index) so it is + // ready before the first reader can capture the snapshot. + observersMu sync.RWMutex + prePublishObservers []func(*config.Config) } // NewService creates a new configuration service with the given initial config. @@ -106,6 +114,46 @@ func (s *Service) SetPrePublishHook(hook func(*config.Config) *config.Config) { s.prePublish.Store(hook) } +// AddPrePublishObserver registers a read-only observer of every configuration +// on its way into the snapshot. Observers run inside the update, after the +// pre-publish hook has produced the final config and before the snapshot is +// stored or any subscriber is notified — so they see the exact *Config that +// will be published, at a moment nothing else can. They must not mutate it, +// and because they run under the update mutex they must be cheap (an index +// build of one insertion per entry, not I/O). Distinct from the single +// SetPrePublishHook slot, which the #937 admission gate owns. +// +// Observers run with the update mutex held and MUST NOT publish: a call to +// Update, UpdateIfCurrent or ReloadFromFile from inside an observer is a +// re-entrant publish and deadlocks by design, exactly like the pre-publish +// hook. Registering a further observer from inside one is allowed — the +// list is not locked while observers run — and that observer first runs on +// the next publication. +// +// Nil observers and a nil service are ignored. Safe to call at any time, +// including from an observer. +func (s *Service) AddPrePublishObserver(observe func(*config.Config)) { + if s == nil || observe == nil { + return + } + s.observersMu.Lock() + defer s.observersMu.Unlock() + s.prePublishObservers = append(s.prePublishObservers, observe) +} + +// runPrePublishObservers runs every observer registered when it is called on +// cfg (called with updateMu held, before the snapshot is stored). The list is +// copied under the read lock and released before any observer runs, so an +// observer may register another without deadlocking. +func (s *Service) runPrePublishObservers(cfg *config.Config) { + s.observersMu.RLock() + observers := slices.Clone(s.prePublishObservers) + s.observersMu.RUnlock() + for _, observe := range observers { + observe(cfg) + } +} + // runPrePublishHook applies the installed hook, if any. func (s *Service) runPrePublishHook(cfg *config.Config) *config.Config { v := s.prePublish.Load() @@ -148,6 +196,7 @@ func (s *Service) UpdateIfCurrent(expected, newConfig *config.Config, updateType func (s *Service) updateLocked(newConfig *config.Config, updateType UpdateType, source string) { newConfig = s.runPrePublishHook(newConfig) + s.runPrePublishObservers(newConfig) current := s.Current() s.version++ diff --git a/internal/runtime/configsvc/service_test.go b/internal/runtime/configsvc/service_test.go index a3f72c416..a957c1a27 100644 --- a/internal/runtime/configsvc/service_test.go +++ b/internal/runtime/configsvc/service_test.go @@ -399,3 +399,110 @@ func TestService_Close(t *testing.T) { t.Error("Subscriber channel not closed after service close") } } + +// TestService_PrePublishObserverSeesTheConfigBeforePublication pins the +// contract AddPrePublishObserver offers a derived-index builder (Spec 105 +// PR D, the profile index): the observer runs on the exact *config.Config +// about to be published — after the pre-publish hook has produced it — +// while Current() still answers the previous snapshot and before any +// subscriber has been notified. A builder keyed on that pointer therefore +// has its index ready before a single reader can capture the snapshot. +func TestService_PrePublishObserverSeesTheConfigBeforePublication(t *testing.T) { + initial := &config.Config{Listen: "127.0.0.1:8080"} + svc := NewService(initial, "/tmp/config.json", zap.NewNop()) + + // The #937 admission gate replaces the incoming config; the observer must + // see the gated one, never the caller's. + gated := &config.Config{Listen: "127.0.0.1:9090"} + svc.SetPrePublishHook(func(*config.Config) *config.Config { return gated }) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + updates := svc.Subscribe(ctx) + <-updates // initial snapshot + + type observation struct { + seen, current *config.Config + notified bool + } + observed := make(chan observation, 1) + svc.AddPrePublishObserver(func(cfg *config.Config) { + o := observation{seen: cfg, current: svc.Current().Config} + select { + case <-updates: + o.notified = true + default: + } + observed <- o + }) + + require.NoError(t, svc.Update(&config.Config{Listen: "incoming"}, UpdateTypeModify, "test")) + + select { + case o := <-observed: + require.Same(t, gated, o.seen, "the observer must see the config the gate hook produced") + require.Same(t, initial, o.current, "the observer must run before Current() moves to the new snapshot") + require.False(t, o.notified, "the observer must run before subscribers are notified") + default: + t.Fatal("the observer must run synchronously inside Update") + } + require.Same(t, gated, svc.Current().Config) + select { + case u := <-updates: + require.Same(t, gated, u.Snapshot.Config) + case <-time.After(time.Second): + t.Fatal("subscribers must still be notified after the observer ran") + } +} + +// TestService_PrePublishObserverNilSafe: a nil observer and a nil service +// are both ignored, and every registered observer runs on every update. +func TestService_PrePublishObserverNilSafe(t *testing.T) { + var none *Service + none.AddPrePublishObserver(func(*config.Config) {}) + + svc := NewService(&config.Config{Listen: "127.0.0.1:8080"}, "/tmp/config.json", zap.NewNop()) + svc.AddPrePublishObserver(nil) + runs := 0 + svc.AddPrePublishObserver(func(*config.Config) { runs++ }) + svc.AddPrePublishObserver(func(*config.Config) { runs++ }) + + require.NoError(t, svc.Update(&config.Config{Listen: "a"}, UpdateTypeModify, "one")) + require.NoError(t, svc.Update(&config.Config{Listen: "b"}, UpdateTypeModify, "two")) + require.Equal(t, 4, runs, "each observer runs once per update; nil ones are skipped") +} + +// TestService_PrePublishObserverMayRegisterAnObserver (Spec 105 PR D codex +// round 6, finding 2): observers run under the update mutex, so the observer +// list must not be held locked while they run — an observer that registers a +// follow-up observer deadlocked on observersMu. The list is copied under the +// read lock and released before any observer is invoked; an observer added +// during a publication first runs on the NEXT one. +func TestService_PrePublishObserverMayRegisterAnObserver(t *testing.T) { + svc := NewService(&config.Config{Listen: "127.0.0.1:8080"}, "/tmp/config.json", zap.NewNop()) + + var registered sync.Once + innerRuns := 0 + svc.AddPrePublishObserver(func(*config.Config) { + registered.Do(func() { + svc.AddPrePublishObserver(func(*config.Config) { innerRuns++ }) + }) + }) + + update := func(listen string) { + t.Helper() + done := make(chan error, 1) + go func() { done <- svc.Update(&config.Config{Listen: listen}, UpdateTypeModify, "test") }() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Update deadlocked: an observer registering another observer must not block publication") + } + } + + update("one") + require.Equal(t, 0, innerRuns, "an observer registered during a publication runs from the next one on") + update("two") + require.Equal(t, 1, innerRuns, "the observer registered by another observer must run on the next publication") +} diff --git a/internal/server/cache_authz.go b/internal/server/cache_authz.go index 326861e65..a73c083bc 100644 --- a/internal/server/cache_authz.go +++ b/internal/server/cache_authz.go @@ -22,18 +22,22 @@ import ( // which the auth middleware would otherwise have handed an anonymous admin // context — so it is recorded as such rather than as an agent with nothing. func (p *MCPProxyServer) cacheAuthorization(ctx context.Context) cache.Authorization { - name, scope := p.resolveActiveProfile(ctx) - return p.cacheAuthorizationWith(ctx, name, scope) + name, scope, idx := p.resolveActiveProfileWithIndex(ctx) + return p.cacheAuthorizationWith(ctx, name, scope, idx) } // cacheAuthorizationWith is cacheAuthorization for a handler that has already -// resolved the request's effective profile — the same (name, scope) pair it -// authorized the call against. Handlers capture the producer stamp HERE, before -// the upstream call, not when the response comes back to be truncated: a -// profile deleted or narrowed while the call is in flight must not re-stamp a +// resolved the request's effective profile — the same (name, scope, idx) +// triple it authorized the call against (idx is the (index, snapshot) pair +// resolveActiveProfileWithIndex resolved that (name, scope) pair from — see +// its doc comment). Handlers capture the producer stamp HERE, before the +// upstream call, not when the response comes back to be truncated: a profile +// deleted or narrowed while the call is in flight must not re-stamp a // response that was authorized under the wider scope. -func (p *MCPProxyServer) cacheAuthorizationWith(ctx context.Context, profileName string, scope *profile.ProfileScope) cache.Authorization { +func (p *MCPProxyServer) cacheAuthorizationWith(ctx context.Context, profileName string, scope *profile.ProfileScope, idx *profileIndex) cache.Authorization { a := cache.Authorization{CallerKind: cache.CallerKindAnonymous} + var callerAllowed []string + callerBounded := false if ac := auth.AuthContextFromContext(ctx); ac != nil { switch { case ac.Anonymous: @@ -44,6 +48,8 @@ func (p *MCPProxyServer) cacheAuthorizationWith(ctx context.Context, profileName a.AllowedServers = append([]string(nil), ac.AllowedServers...) a.Permissions = append([]string(nil), ac.Permissions...) a.ProfilePin = ac.ProfilePin + callerAllowed = ac.AllowedServers + callerBounded = true case ac.Type == auth.AuthTypeUser: // A server-edition user is bounded by the SAME dispatch gates // as an agent token — CanAccessServer, HasPermission and the @@ -53,12 +59,22 @@ func (p *MCPProxyServer) cacheAuthorizationWith(ctx context.Context, profileName // digest (codex round 4: a snapshot of the user id alone let a // user narrowed to {b} redeem the {a} entry it produced // earlier; research D16: digest equality is the only user - // admission). + // admission). Spec 107 PR-C (IdP-group server grants, #1293, + // already merged) gives a plain OAuth user its own restricted + // AllowedServers via CanAccessServer's exact rule (nil/empty is + // deny-all, same as an agent token) — so a User is caller- + // bounded exactly like an Agent, not "no AllowedServers of its + // own" as the ProfileServers comment below used to assume + // (cross-model review, PR D: that assumption was already false + // for this type, reopening round 17's cache side-channel for a + // restricted OAuth user). a.CallerKind = cache.CallerKindUser a.Principal = ac.UserID a.AllowedServers = append([]string(nil), ac.AllowedServers...) a.Permissions = append([]string(nil), ac.Permissions...) a.ProfilePin = ac.ProfilePin + callerAllowed = ac.AllowedServers + callerBounded = true case ac.Type == auth.AuthTypeAdminUser: a.CallerKind = cache.CallerKindAdminUser a.Principal = ac.UserID @@ -69,7 +85,33 @@ func (p *MCPProxyServer) cacheAuthorizationWith(ctx context.Context, profileName a.Profile = profileName if scope != nil { a.ProfileScoped = true - a.ProfileServers = scope.AllowedServerNames() + // Spec 105 PR D review round 17 MUST-FIX (widened by cross-model + // review to cover AuthTypeUser, not only AuthTypeAgent — see the + // AuthTypeUser case above): a caller-bounded token's stamp is the + // CALLER-INTERSECTED profile membership — the same + // EffectiveServersFor helper handleSetProfile's own scoped-visible + // path already renders through (profile_tool.go), O(len(callerAllowed)) + // via idx's precomputed serverPos/members data, never a fleet- or + // profile-declared-size walk. A profile member entirely outside the + // token's own grant (the token never had, and never will have, + // access to it) must not appear in the stamp: left in, its later + // removal narrows what THIS token's cache read resolves to on the + // next request and fails the redemption set-covering comparison + // (internal/cache/authorization.go CouldHaveProduced/coversAll) for + // an entry produced from a server the token remains fully authorized + // for — an unrelated, never-authorized server's continued existence + // becoming an observable side-channel through the cache layer + // (SC-005-class disclosure). Only truly unbounded scoped callers + // (admin/anonymous reading through a profile URL, which carry no + // AllowedServers of their own to intersect against) keep the + // resolver's full profile membership — exactly resolveActiveProfileIn's + // documented wildcard/profile's-own-membership semantic, untouched + // here. + if callerBounded && idx != nil { + a.ProfileServers = idx.EffectiveServersFor(profileName, callerAllowed) + } else { + a.ProfileServers = scope.AllowedServerNames() + } sort.Strings(a.ProfileServers) } return a diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 95e3be08a..85b7708a5 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -158,6 +158,14 @@ type MCPProxyServer struct { // whole Runtime (mirrors workSessionResolver). preflightRecorder func(runtime.PreflightActivity) error + // profileIndexes is the slug → profile index cache set_profile consults + // when no mainServer stands behind this proxy (tests build a bare + // MCPProxyServer). In production profileIndexCurrent takes the main + // Server's warm index — the (index, snapshot) pair — so one index per + // config snapshot serves both the /mcp/p/ gate and set_profile + // and no request builds one (Spec 105 FR-003/FR-004). + profileIndexes profileIndexCache + // preflightStateSource overrides the connection-state snapshot the // preflight glue reads (Spec 099). Nil in production, where // preflightSnapshot resolves it from the supervisor's StateView; tests @@ -1804,10 +1812,14 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques // that is allowed to see nothing leave a new index directory behind — for a // profile that may no longer exist. The post-filter below returns the same // empty result set from the shared index. - profileName, profileScope := p.resolveActiveProfile(ctx) + profileName, profileScope, profileIdx := p.resolveActiveProfileWithIndex(ctx) // Spec 104 FR-016a: the cache stamp is the authorization THIS search runs // under, captured now rather than re-resolved when the response is cut. - producer := p.cacheAuthorizationWith(ctx, profileName, profileScope) + // The index/snapshot pair is threaded through too (Spec 105 PR D review + // round 17 MUST-FIX): cacheAuthorizationWith derives the stamped + // ProfileServers from this exact pair, never a second, independently + // resolved one. + producer := p.cacheAuthorizationWith(ctx, profileName, profileScope, profileIdx) searchIndex := p.index if profileName != "" && !profileScope.DeniesAll() { if pIdx, perr := p.index.ForProfile(profileName); perr == nil && pIdx != nil { @@ -2329,8 +2341,14 @@ func (p *MCPProxyServer) handleCallToolVariant(ctx context.Context, request mcp. // Spec 057 / Profiles v2: the active profile (token pin > URL > session // set_profile). Resolved once, up here, because the audit attempt stamps // it; the profile-scope GATE itself still runs below, after the intent - // gates, exactly where it always did. - profileSlug, profileScope := p.resolveActiveProfile(ctx) + // gates, exactly where it always did. WithIndex (not plain + // resolveActiveProfile — a pure wrapper around this that discards the + // index) so the SAME (name, scope, idx) triple resolved here also feeds + // cacheAuthorizationWith's caller-intersected ProfileServers stamp below + // (Spec 105 PR D review round 17) — one resolution for the whole call, + // never a second, independent one that could pair a decision made + // against this snapshot with an index built from a later one. + profileSlug, profileScope, profileIdx := p.resolveActiveProfileWithIndex(ctx) // Spec 107 T103: the audit attempt, installed BEFORE the first gate so // every refusal below — the intent gates included — writes its `authz @@ -2429,8 +2447,11 @@ func (p *MCPProxyServer) handleCallToolVariant(ctx context.Context, request mcp. // attempt) is the read_cache producer stamp (Spec 104 FR-016a), captured // here — at authorization time, before the upstream call — so a profile // deleted or narrowed while the call is in flight cannot re-stamp a - // response that was authorized under the wider scope. - producer := p.cacheAuthorizationWith(ctx, profileSlug, profileScope) + // response that was authorized under the wider scope. profileIdx is the + // same index that (name, scope) triple was resolved against (see above), + // never re-derived here — cacheAuthorizationWith's caller-intersected + // ProfileServers stamp needs it (Spec 105 PR D review round 17). + producer := p.cacheAuthorizationWith(ctx, profileSlug, profileScope, profileIdx) if profileScope != nil && !profileScope.Allows(serverName) { errMsg := fmt.Sprintf("server '%s' is not in profile '%s'", serverName, profileScope.Name) p.emitActivityPolicyDecision(ctx, serverName, actualToolName, getSessionID(), requestID, "blocked", errMsg, telemetry.BlockReasonProfileScope) diff --git a/internal/server/mcp_auth_anonymous_test.go b/internal/server/mcp_auth_anonymous_test.go index 418dd946c..8d5891b77 100644 --- a/internal/server/mcp_auth_anonymous_test.go +++ b/internal/server/mcp_auth_anonymous_test.go @@ -10,6 +10,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" "github.com/smart-mcp-proxy/mcpproxy-go/internal/transport" ) @@ -113,3 +114,33 @@ func TestStdioAuthContext_IsRealAdmin(t *testing.T) { require.False(t, authCtx.Anonymous) require.True(t, authCtx.CanRevealSecrets(), "stdio is a local, OS-authenticated transport") } + +// TestMCPAuthMiddleware_AgentTokenWithoutConfigIsRefused (Spec 105 PR D +// critique round 1): an agent-token request that arrives while the runtime +// has NO published configuration must be refused, never forwarded. Before +// this guard the middleware passed such a request through with no +// AuthContext at all, and every scope predicate downstream reads an absent +// context as an administrator (auth.IsScopedCaller → false) — the one path +// where "no identity" widened into "full identity". A 503 keeps the +// fail-closed shape of the sibling storage-unavailable branch. +func TestMCPAuthMiddleware_AgentTokenWithoutConfigIsRefused(t *testing.T) { + // A zero Runtime publishes no configuration (no config service, no legacy + // config). The middleware reads only s.runtime and s.logger on this path, + // so a bare Server is enough — publishing a nil snapshot through a real + // runtime's config service would wake the supervisor's reconcile loop on + // a nil config instead. + srv := &Server{runtime: &runtime.Runtime{}, logger: zap.NewNop()} + require.Nil(t, srv.runtime.Config(), "fixture: the runtime must publish no configuration") + + reached := false + handler := srv.mcpAuthMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + reached = true + })) + req := httptest.NewRequest(http.MethodPost, "/mcp/p/research", http.NoBody) + req.Header.Set("Authorization", "Bearer "+auth.TokenPrefixStr+"not-validated-without-config") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.Equal(t, http.StatusServiceUnavailable, rec.Code, "an agent token cannot be validated without a configuration: %s", rec.Body.String()) + require.False(t, reached, "the request must not reach the MCP handler without an AuthContext") +} diff --git a/internal/server/mcp_read_cache_authz_test.go b/internal/server/mcp_read_cache_authz_test.go index 1b63b9d06..ce7a6bfcb 100644 --- a/internal/server/mcp_read_cache_authz_test.go +++ b/internal/server/mcp_read_cache_authz_test.go @@ -217,3 +217,133 @@ func TestReadCache_BroaderReaderMayReadNarrowerEntry(t *testing.T) { result := readCacheAs(t, proxy, admin, match[1], 0) assert.False(t, result.IsError, "an unrestricted admin could have produced the entry, so it may read it") } + +// Spec 105 PR D review round 17 MUST-FIX: cacheAuthorizationWith must stamp +// ProfileServers with the CALLER-INTERSECTED profile membership +// (profiles.EffectiveServersFor(profileName, ac.AllowedServers)), not the +// resolver's wildcard-derived ProfileScope.AllowedServerNames() — a server +// entirely outside the token's own grant must not affect this token's own +// cache behavior for a server it IS, and remains, authorized to reach. +// +// Scenario (traced by the round-16 reviewer): token AllowedServers={github} +// pinned to profile research={github, weather} — weather sits outside this +// token's own grant, and the token never had, and never will have, access to +// it. An operator later removes weather from the profile (an event this +// token has no authorization to observe or care about). The SAME token's +// cache read for its own authorized server (github) must not be revoked by +// that unrelated removal: before the fix, the cache stamp carried the +// resolver's wildcard-derived full profile membership {github, weather}, so +// the reader's now-narrower resolved set {github} failed the redemption +// set-covering comparison — an unrelated, never-authorized server's presence +// became an observable side-channel through the cache layer (SC-005-class). +func TestReadCache_CacheAuthzIntersectsCallerGrant_HiddenProfileMemberRemovalDoesNotRevoke(t *testing.T) { + proxy := createTestMCPProxyServer(t) + seedEntryBuilderFixture(t, proxy) + proxy.config.Servers = []*config.ServerConfig{{Name: "github", Enabled: true}, {Name: "weather", Enabled: true}} + proxy.config.Profiles = []config.ProfileConfig{{Name: "research", Servers: []string{"github", "weather"}}} + pIdx, err := proxy.index.ForProfile("research") + require.NoError(t, err) + for _, name := range []string{"github:create_issue", "github:list_issues", "github:get_repo", "weather:get_forecast", "weather:search_city"} { + require.NoError(t, pIdx.IndexTool(&config.ToolMetadata{ + Name: name, ServerName: name[:strings.Index(name, ":")], + Description: "manage things with " + name, ParamsJSON: `{"type":"object","properties":{"a":{"type":"string"},"b":{"type":"string"}}}`, + Hash: "hash-" + name, + })) + } + + // Token is granted github ONLY — weather sits in the profile, but the + // token never had, and never will have, access to it. + pinned := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "pinned", TokenPrefix: "mcp_agt_pinne", + AllowedServers: []string{"github"}, Permissions: []string{auth.PermRead}, + ProfilePin: "research", + }) + + args := map[string]interface{}{"query": "manage", "limit": float64(10)} + req := mcp.CallToolRequest{} + req.Params.Arguments = args + full, err := proxy.handleRetrieveTools(pinned, req) + require.NoError(t, err) + require.Contains(t, resultText(t, full), "github:", "premise: the token's own grant surfaces github") + require.NotContains(t, resultText(t, full), "weather:", "premise: weather sits outside the token's own grant and must not be discoverable at all") + setTruncateLimit(proxy, len(resultText(t, full))/2) + truncated, err := proxy.handleRetrieveTools(pinned, req) + require.NoError(t, err) + match := cacheKeyRE.FindStringSubmatch(resultText(t, truncated)) + require.Len(t, match, 2, "premise: the github-only response must still be large enough to truncate into a keyed page") + setTruncateLimit(proxy, 1_000_000) + + before := readCacheAs(t, proxy, pinned, match[1], 0) + require.False(t, before.IsError, "premise: the producing token reads its own entry") + + // Operator removes weather from the profile — a server this token never + // had, and never will have, access to. This token's OWN cache behavior + // for its own authorized server (github) must not change. + proxy.config.Profiles = []config.ProfileConfig{{Name: "research", Servers: []string{"github"}}} + + after := readCacheAs(t, proxy, pinned, match[1], 0) + assert.False(t, after.IsError, "removing an out-of-grant profile member must not revoke this token's own cached access to its own authorized server") + assert.Contains(t, resultText(t, after), `"records"`) +} + +// Companion to the fix above: narrowing a profile to drop a server the token +// WAS itself authorized for (through the profile) must still revoke access to +// entries produced under the wider scope — the caller-intersection fix must +// not weaken this pre-existing guarantee (mirrors +// TestReadCache_DeletedPinnedProfileRevokesCachedAccess at a finer grain: a +// partial narrowing of the token's own reach, not a full profile deletion). +func TestReadCache_CacheAuthzIntersectsCallerGrant_OwnGrantMemberRemovalStillRevokes(t *testing.T) { + proxy := createTestMCPProxyServer(t) + seedEntryBuilderFixture(t, proxy) + proxy.config.Servers = []*config.ServerConfig{{Name: "github", Enabled: true}, {Name: "weather", Enabled: true}} + proxy.config.Profiles = []config.ProfileConfig{{Name: "research", Servers: []string{"github", "weather"}}} + pIdx, err := proxy.index.ForProfile("research") + require.NoError(t, err) + for _, name := range []string{"github:create_issue", "github:list_issues", "github:get_repo", "weather:get_forecast", "weather:search_city"} { + require.NoError(t, pIdx.IndexTool(&config.ToolMetadata{ + Name: name, ServerName: name[:strings.Index(name, ":")], + Description: "manage things with " + name, ParamsJSON: `{"type":"object","properties":{"a":{"type":"string"},"b":{"type":"string"}}}`, + Hash: "hash-" + name, + })) + } + + // Token is granted BOTH servers the profile declares. + pinned := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "pinned", TokenPrefix: "mcp_agt_pinne", + AllowedServers: []string{"github", "weather"}, Permissions: []string{auth.PermRead}, + ProfilePin: "research", + }) + + args := map[string]interface{}{"query": "manage", "limit": float64(10)} + req := mcp.CallToolRequest{} + req.Params.Arguments = args + full, err := proxy.handleRetrieveTools(pinned, req) + require.NoError(t, err) + require.Contains(t, resultText(t, full), "github:") + setTruncateLimit(proxy, len(resultText(t, full))/2) + truncated, err := proxy.handleRetrieveTools(pinned, req) + require.NoError(t, err) + match := cacheKeyRE.FindStringSubmatch(resultText(t, truncated)) + require.Len(t, match, 2) + setTruncateLimit(proxy, 1_000_000) + + before := readCacheAs(t, proxy, pinned, match[1], 0) + require.False(t, before.IsError, "premise: the producing token reads its own entry") + + // Operator narrows the profile to drop weather — a server the token WAS, + // itself, authorized to reach through this profile. That must still + // revoke this entry, even though the entry's own content came from + // github (Spec 104 FR-016a compares server SETS, not entry content). + proxy.config.Profiles = []config.ProfileConfig{{Name: "research", Servers: []string{"github"}}} + + after := readCacheAs(t, proxy, pinned, match[1], 0) + assert.True(t, after.IsError, "narrowing a server the token itself was authorized for must still revoke cached access") + // The reader is a scoped agent, not an administrator: readCacheRefusal + // remaps ErrUnauthorizedRead to the uniform ErrKeyNotFound body for any + // non-administrator caller kind (FR-001 "refusal is non-disclosing") — + // "not readable with this credential" is the administrator-only wording + // (cache_authz.go readCacheRefusal, reached only when + // reader.IsAdministrator()); this pinned agent must see the SAME body a + // missing key produces, not the reason. + assert.Contains(t, resultText(t, after), "cache key not found") +} diff --git a/internal/server/mcp_session_never_reaches_test.go b/internal/server/mcp_session_never_reaches_test.go index 58085741e..863f8213d 100644 --- a/internal/server/mcp_session_never_reaches_test.go +++ b/internal/server/mcp_session_never_reaches_test.go @@ -131,7 +131,7 @@ func TestCacheAuthorizationCallerKindUser_NeverProducedOnMCPPath(t *testing.T) { if ac != nil { ctx = auth.WithAuthContext(ctx, ac) } - got := p.cacheAuthorizationWith(ctx, "", nil) + got := p.cacheAuthorizationWith(ctx, "", nil, nil) if got.CallerKind == cache.CallerKindUser { callerKindUserCount++ } @@ -145,7 +145,7 @@ func TestCacheAuthorizationCallerKindUser_NeverProducedOnMCPPath(t *testing.T) { // map to CallerKindUser. If this assertion ever failed, the zero count // above would be meaningless (the switch itself broken, not merely unfed). sessionCtx := auth.WithAuthContext(context.Background(), auth.UserContext("u-t068", "alice@example.com", "Alice", "google")) - got := p.cacheAuthorizationWith(sessionCtx, "", nil) + got := p.cacheAuthorizationWith(sessionCtx, "", nil, nil) require.Equal(t, cache.CallerKindUser, got.CallerKind, "fixture: auth.UserContext must map to CallerKindUser, or the negative assertions above are vacuous") } diff --git a/internal/server/profile_integration_test.go b/internal/server/profile_integration_test.go index ad29124cc..5e14d8635 100644 --- a/internal/server/profile_integration_test.go +++ b/internal/server/profile_integration_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "strings" "testing" @@ -621,29 +622,49 @@ func TestProfile_SetProfileUnknown(t *testing.T) { // Profiles v2 (T3): per-agent-token profile_pin — server-side URL enforcement // --------------------------------------------------------------------------- -// mintPinnedToken creates a stored agent token pinned to the given profile and -// returns its raw secret. It uses the same HMAC key path the auth middleware -// reads, so the minted token validates end-to-end. -func (e *profileTestEnv) mintPinnedToken(name, pin string) string { - e.t.Helper() - cfg := e.proxyServer.runtime.Config() +// mintProfileAgentToken creates a stored agent token with the given +// allowed-server list, permission set and profile pin, and returns its raw +// secret. It uses the same HMAC key path the auth middleware reads, so the +// minted token validates end-to-end (Spec 105 T035 — generalised from the +// pin-only minter so the FR-004 fixtures can mint a RESTRICTED unpinned +// token; PR H1 reuses it). Named distinctly from mcp_auth_forced_test.go's +// server-tagged mintAgentToken(t, *Server, name) helper (Spec 107 PR-C, +// merged in #1293) to avoid a same-package redeclaration under -tags server. +// +// Fixture semantics mirror internal/server/scope_fixture_test.go: an EMPTY +// allowed list is deny-all under CanAccessServer, so an unrestricted token must +// pass []string{"*"}; HasPermission is exact membership, so pass every tier +// the token holds; an empty pin means unpinned. +func mintProfileAgentToken(t *testing.T, env *profileTestEnv, name string, allowed, perms []string, pin string) string { + t.Helper() + cfg := env.proxyServer.runtime.Config() hmacKey, err := auth.GetOrCreateHMACKey(cfg.DataDir) - require.NoError(e.t, err) + require.NoError(t, err) rawToken, err := auth.GenerateToken() - require.NoError(e.t, err) - require.NoError(e.t, e.proxyServer.runtime.StorageManager().CreateAgentToken(auth.AgentToken{ + require.NoError(t, err) + require.NoError(t, env.proxyServer.runtime.StorageManager().CreateAgentToken(auth.AgentToken{ Name: name, - AllowedServers: []string{"*"}, - Permissions: []string{"read"}, + AllowedServers: allowed, + Permissions: perms, ExpiresAt: time.Now().Add(24 * time.Hour), ProfilePin: pin, }, rawToken, hmacKey)) return rawToken } +// mintPinnedToken mints an unrestricted ("*", read-only) agent token pinned to +// the given profile — the shape the pre-105 pin tests were written against. +func (e *profileTestEnv) mintPinnedToken(name, pin string) string { + e.t.Helper() + return mintProfileAgentToken(e.t, e, name, []string{"*"}, []string{auth.PermRead}, pin) +} + // TestProfile_PinnedTokenURLEnforcement verifies the T3 server-side guard: an -// agent token pinned to "research" is rejected with 403 at /mcp/p/deploy, but -// reaches its own /mcp/p/research endpoint. +// agent token pinned to "research" is refused at /mcp/p/deploy, but reaches +// its own /mcp/p/research endpoint. Inverted for Spec 105 FR-004 (FR003-G3): +// the pre-105 refusal was a 403 whose body named the pin; a pin mismatch is +// now the same uniform 404 every non-selectable slug produces, and the body +// must not name the pin (TestProfile_PinnedRefusalUniform proves the ≡). func TestProfile_PinnedTokenURLEnforcement(t *testing.T) { env := newProfileTestEnv(t) rawToken := env.mintPinnedToken("pinned-research", "research") @@ -661,14 +682,16 @@ func TestProfile_PinnedTokenURLEnforcement(t *testing.T) { return resp } - // Different profile → 403 with a pin-naming error. + // Different profile → the uniform 404, without the pin in the body. resp := post("deploy") defer resp.Body.Close() - require.Equal(t, http.StatusForbidden, resp.StatusCode, "pinned token must be 403 on a non-pinned profile URL") + require.Equal(t, http.StatusNotFound, resp.StatusCode, "pinned token must get the uniform 404 on a non-pinned profile URL") var body map[string]interface{} require.NoError(t, json.NewDecoder(resp.Body).Decode(&body)) errMsg, _ := body["error"].(string) - assert.Contains(t, errMsg, "pinned to profile 'research'", "403 error must name the pin: %s", errMsg) + assert.NotContains(t, errMsg, "pinned", "the refusal must not name the pin: %s", errMsg) + _, enumerated := body["available"] + assert.False(t, enumerated, "the refusal must not enumerate profiles: %v", body) // Its own pinned profile → route matched, not forbidden. resp2 := post("research") @@ -677,8 +700,10 @@ func TestProfile_PinnedTokenURLEnforcement(t *testing.T) { "pinned token must reach its own profile URL; got %d", resp2.StatusCode) } -// TestProfile_UnpinnedTokenUnaffected verifies an unpinned agent token can reach -// any profile URL (no T3 enforcement applied). +// TestProfile_UnpinnedTokenUnaffected verifies an unpinned, unrestricted ("*") +// agent token can reach any profile URL (no T3 enforcement applied; every +// configured profile intersects its grant, so the FR-004 predicate admits it — +// a RESTRICTED unpinned token is covered by TestProfile_ScopedUnpinnedRefusalUniform). func TestProfile_UnpinnedTokenUnaffected(t *testing.T) { env := newProfileTestEnv(t) rawToken := env.mintPinnedToken("free-agent", "") // empty pin = unpinned @@ -771,7 +796,9 @@ func TestProfile_EndpointReachability(t *testing.T) { // to is deleted, a request to /mcp/p/ passes the pin check and fell into // the generic "unknown profile" branch, whose "available" list enumerated // every remaining profile — profiles the token may never select (the resolver -// treats a deleted pin as deny-all). The error must not list them. +// treats a deleted pin as deny-all). The error must not list them. Under +// Spec 105 FR-004 the deleted pin takes the single scoped refusal +// (profileNotSelectable); the anonymous administrator control keeps the list. func TestProfile_DeletedPinDoesNotEnumerateProfiles(t *testing.T) { env := newProfileTestEnv(t) rawToken := env.mintPinnedToken("pinned-research", "research") @@ -811,3 +838,210 @@ func TestProfile_DeletedPinDoesNotEnumerateProfiles(t *testing.T) { available, _ := adminBody["available"].([]interface{}) assert.Equal(t, []interface{}{"deploy"}, available, "admin still sees the available list") } + +// --------------------------------------------------------------------------- +// Spec 105 PR D (FR-004, gaps FR003-G1…G5, G8/D1): the profile URL applies the +// selectable-profile predicate for every scoped caller and refuses with ONE +// status+body — no `available` list — whether the slug is missing, deleted, +// configured-but-not-selectable, a pin mismatch, or the fleet is empty. +// --------------------------------------------------------------------------- + +// profileInitRequest POSTs an MCP initialize to baseURL+path with the given +// agent token (empty token = unauthenticated / anonymous admin-shaped caller) +// and returns the status code and the raw body. +func profileInitRequest(t *testing.T, baseURL, path, rawToken string) (int, string) { + t.Helper() + const initBody = `{"jsonrpc":"2.0","method":"initialize","id":1,"params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"test","version":"1.0"}}}` + req, err := http.NewRequest(http.MethodPost, baseURL+path, strings.NewReader(initBody)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if rawToken != "" { + req.Header.Set("Authorization", "Bearer "+rawToken) + } + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return resp.StatusCode, strings.TrimSpace(string(body)) +} + +// profileRefusal captures one refusal so it can be compared with another +// after slug normalisation: the body may echo the caller's own slug (that is +// not disclosure), so `''` is replaced by a placeholder before the +// byte comparison. An implementation that emits a slug-free constant body +// passes the same assertion unchanged. +type profileRefusal struct { + path string + status int + body string +} + +func captureProfileRefusal(t *testing.T, baseURL, path, slug, rawToken string) profileRefusal { + t.Helper() + status, body := profileInitRequest(t, baseURL, path, rawToken) + return profileRefusal{ + path: path, + status: status, + body: strings.ReplaceAll(body, "'"+slug+"'", "''"), + } +} + +// assertUniformProfileRefusal checks that every captured refusal is the same +// 404 (status and slug-normalised body), that its `error` is the documented +// `unknown profile ''` text (contracts/refusals.md — uniformity alone +// would also accept a uniform `forbidden`), and that none of them carries an +// `available` list. +func assertUniformProfileRefusal(t *testing.T, refusals []profileRefusal) { + t.Helper() + require.NotEmpty(t, refusals) + for _, r := range refusals { + assert.Equal(t, http.StatusNotFound, r.status, "%s: a scoped caller must get the uniform 404, got %d %s", r.path, r.status, r.body) + var decoded map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(r.body), &decoded), "%s: body must be JSON: %s", r.path, r.body) + assert.Equal(t, "unknown profile ''", decoded["error"], "%s: the refusal must be the documented unknown-profile text", r.path) + _, enumerated := decoded["available"] + assert.False(t, enumerated, "%s: the refusal must not enumerate profiles: %s", r.path, r.body) + assert.Equal(t, refusals[0].body, r.body, "%s must be byte-identical (slug-normalised) to %s", r.path, refusals[0].path) + } +} + +// TestProfile_ScopedUnpinnedRefusalUniform (FR003-G1/G2/G5): an unpinned +// token restricted to research-srv initializes through /mcp/p/research (its +// only selectable profile) and is refused with ONE status+body — no +// `available` list — through the disjoint /mcp/p/deploy, the nonexistent +// /mcp/p/nonexistent, the slug-less /mcp/p and /mcp/p/, and /mcp/p/deploy +// after the deploy profile has been deleted. +func TestProfile_ScopedUnpinnedRefusalUniform(t *testing.T) { + env := newProfileTestEnv(t) + rawToken := mintProfileAgentToken(t, env, "a-only", []string{"research-srv"}, []string{auth.PermRead}, "") + + // Positive control: the selectable profile initializes. + status, body := profileInitRequest(t, env.baseURL, "/mcp/p/research", rawToken) + require.Equal(t, http.StatusOK, status, "a selectable profile URL must initialize for the restricted token: %s", body) + + refusals := []profileRefusal{ + captureProfileRefusal(t, env.baseURL, "/mcp/p/deploy", "deploy", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p/nonexistent", "nonexistent", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p", "", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p/", "", rawToken), + } + + // Delete the disjoint profile; the same slug must be refused identically. + old := env.proxyServer.runtime.Config() + cfgCopy := *old + cfg := &cfgCopy + cfg.Profiles = []config.ProfileConfig{{Name: "research", Servers: []string{"research-srv"}}} + env.proxyServer.runtime.UpdateConfig(cfg, "") + refusals = append(refusals, captureProfileRefusal(t, env.baseURL, "/mcp/p/deploy", "deploy", rawToken)) + + // Byte-equality with the nonexistent-slug refusal is the disclosure + // oracle: a slug that names no profile has no servers to leak. + assertUniformProfileRefusal(t, refusals) +} + +// TestProfile_PinnedRefusalUniform (FR003-G3): a token pinned to research is +// refused identically — status and slug-normalised body — through a pin +// mismatch on an existing profile (/mcp/p/deploy), a pin mismatch on a +// nonexistent one (/mcp/p/nope), and its own pin's URL after the pinned +// profile has been deleted. HEAD answers 403 / 403 / 404. +func TestProfile_PinnedRefusalUniform(t *testing.T) { + env := newProfileTestEnv(t) + rawToken := env.mintPinnedToken("pinned-research", "research") + + status, body := profileInitRequest(t, env.baseURL, "/mcp/p/research", rawToken) + require.Equal(t, http.StatusOK, status, "the pin's own URL must initialize while the pin has reach: %s", body) + + refusals := []profileRefusal{ + captureProfileRefusal(t, env.baseURL, "/mcp/p/deploy", "deploy", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p/nope", "nope", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p", "", rawToken), + } + + old := env.proxyServer.runtime.Config() + cfgCopy := *old + cfg := &cfgCopy + cfg.Profiles = []config.ProfileConfig{{Name: "deploy", Servers: []string{"deploy-srv"}}} + env.proxyServer.runtime.UpdateConfig(cfg, "") + refusals = append(refusals, captureProfileRefusal(t, env.baseURL, "/mcp/p/research", "research", rawToken)) + + assertUniformProfileRefusal(t, refusals) + for _, r := range refusals { + assert.NotContains(t, r.body, "pinned", "%s: the refusal must not name the pin: %s", r.path, r.body) + } +} + +// TestProfile_PinnedRefusalIndependentOfFleet (FR003-G4): pin mismatch and +// deleted-pin refusals are evaluated independently of fleet population — the +// "no profiles configured" branch must not run before the gate for a scoped +// caller. For a token pinned to research, /mcp/p/research and /mcp/p/deploy +// answer byte-identically whether the fleet is [deploy] or empty. The +// anonymous (admin-shaped) caller keeps today's distinct branches. +func TestProfile_PinnedRefusalIndependentOfFleet(t *testing.T) { + env := newProfileTestEnv(t) + rawToken := env.mintPinnedToken("pinned-research", "research") + + setFleet := func(profiles []config.ProfileConfig) { + old := env.proxyServer.runtime.Config() + cfgCopy := *old + cfg := &cfgCopy + cfg.Profiles = profiles + env.proxyServer.runtime.UpdateConfig(cfg, "") + } + + type fleetRefusals struct { + research profileRefusal + deploy profileRefusal + } + capture := func() fleetRefusals { + return fleetRefusals{ + research: captureProfileRefusal(t, env.baseURL, "/mcp/p/research", "research", rawToken), + deploy: captureProfileRefusal(t, env.baseURL, "/mcp/p/deploy", "deploy", rawToken), + } + } + + setFleet([]config.ProfileConfig{{Name: "deploy", Servers: []string{"deploy-srv"}}}) + withDeploy := capture() + setFleet(nil) + emptyFleet := capture() + + assertUniformProfileRefusal(t, []profileRefusal{withDeploy.research, emptyFleet.research, withDeploy.deploy, emptyFleet.deploy}) + + // Admin control: the anonymous caller still distinguishes an empty fleet. + status, body := profileInitRequest(t, env.baseURL, "/mcp/p/research", "") + require.Equal(t, http.StatusNotFound, status) + assert.Contains(t, body, "no profiles configured", "the anonymous caller keeps today's no-profiles branch: %s", body) +} + +// TestProfile_PinnedZeroReachURLRefusedUniformly (FR003-G8, research D1): a +// token pinned to a profile that exists but has zero reach — an empty profile +// — is refused through /mcp/p/ with the same uniform body a pin mismatch +// or a deleted pin produces, so the token cannot observe whether its own pin +// still exists. HEAD initializes 200 through the empty pin. +func TestProfile_PinnedZeroReachURLRefusedUniformly(t *testing.T) { + env := newProfileTestEnv(t) + + old := env.proxyServer.runtime.Config() + cfgCopy := *old + cfg := &cfgCopy + cfg.Profiles = append(append([]config.ProfileConfig{}, old.Profiles...), config.ProfileConfig{Name: "empty"}) + env.proxyServer.runtime.UpdateConfig(cfg, "") + + // Unrestricted grant: only the profile's emptiness removes its reach. + rawToken := mintProfileAgentToken(t, env, "pinned-empty", []string{"*"}, []string{auth.PermRead}, "empty") + + refusals := []profileRefusal{ + captureProfileRefusal(t, env.baseURL, "/mcp/p/empty", "empty", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p/nope", "nope", rawToken), + captureProfileRefusal(t, env.baseURL, "/mcp/p/research", "research", rawToken), + } + assertUniformProfileRefusal(t, refusals) + + // A disjoint grant is zero reach too: pinned to deploy, allowed research-srv only. + disjoint := mintProfileAgentToken(t, env, "pinned-disjoint", []string{"research-srv"}, []string{auth.PermRead}, "deploy") + assertUniformProfileRefusal(t, []profileRefusal{ + captureProfileRefusal(t, env.baseURL, "/mcp/p/deploy", "deploy", disjoint), + captureProfileRefusal(t, env.baseURL, "/mcp/p/nope", "nope", disjoint), + }) +} diff --git a/internal/server/profile_pin_enforcement_test.go b/internal/server/profile_pin_enforcement_test.go index d59205d2f..c3016646e 100644 --- a/internal/server/profile_pin_enforcement_test.go +++ b/internal/server/profile_pin_enforcement_test.go @@ -132,13 +132,19 @@ func TestDirectModeHonorsTokenProfilePin(t *testing.T) { } // set_profile("") clears the session tier, which a pin outranks anyway. The -// response must describe the scope that remains, not the full server list. +// response must describe the scope that remains, not the full server list — +// and, since Spec 105 FR-003 (G7), report the STORED selection ("") as +// `active_profile` rather than echoing the pin (inverted from the pre-105 +// expectation; the pin is a credential restriction, not a session selection). +// A prior selection is seeded so "cleared" is an observable store change, +// not the fixture's initial state. func TestSetProfileClearReportsPinnedScope(t *testing.T) { proxy, cfg := pinnedProxy(t, []config.ProfileConfig{ {Name: "research", Servers: []string{"research-srv"}}, }) helper := mcpserver.NewMCPServer("test", "1.0.0") ctx := helper.WithContext(pinnedAgentContext("research"), &fakeClientSession{id: "sess-pin-clear"}) + proxy.sessionStore.SetActiveProfile("sess-pin-clear", "research") request := mcp.CallToolRequest{} request.Params.Arguments = map[string]interface{}{"profile": ""} @@ -147,7 +153,8 @@ func TestSetProfileClearReportsPinnedScope(t *testing.T) { require.NoError(t, err) require.False(t, result.IsError) payload := decodeSetProfilePayload(t, result) - assert.Equal(t, "research", payload["active_profile"]) + assert.Equal(t, "", payload["active_profile"], "the pin is not a stored selection") + assert.Equal(t, "", proxy.sessionStore.GetActiveProfile("sess-pin-clear"), "the stored selection must be cleared") assert.Equal(t, []interface{}{"research-srv"}, payload["servers"], "clearing must not advertise servers the pin still denies") @@ -157,7 +164,7 @@ func TestSetProfileClearReportsPinnedScope(t *testing.T) { require.NoError(t, err) require.False(t, result.IsError) payload = decodeSetProfilePayload(t, result) - assert.Equal(t, "research", payload["active_profile"]) + assert.Equal(t, "", payload["active_profile"]) assert.Empty(t, payload["servers"]) } diff --git a/internal/server/profile_resolver.go b/internal/server/profile_resolver.go index 498e9e6ab..cbc1b6d67 100644 --- a/internal/server/profile_resolver.go +++ b/internal/server/profile_resolver.go @@ -35,6 +35,40 @@ func profilePinFromContext(ctx context.Context) string { return "" } +// profileRequestIndexKey is an unexported context key for the (index, +// snapshot) PAIR a /mcp/p/ request was admitted and scoped against +// (profileMiddleware / serveProfileURL). It is a package-private companion +// to profile.WithProfileScope, not exported through the profile package, +// because both the writer (Server) and the reader (MCPProxyServer) already +// live in this package. +// +// Carrying the pair — not merely its cfg — lets every downstream profile +// decision on this request, set_profile's admission included, reuse the +// index the gate already built rather than re-resolving cfg's own index +// through a second Published() lookup: a request that paused between +// admission and here must still decide with the exact index it was +// admitted against, never one a later publication's Published() would hand +// back for the live runtime.Config() read at that later moment (Spec 105 PR +// D review round 9, MUST-FIX 1 — set_profile on /mcp/p/ read its own +// independent runtime.Config() and could therefore admit or scope against a +// config one reload ahead of, or behind, the one the URL gate used). +type profileRequestIndexKey struct{} + +// withProfileRequestIndex returns a context carrying idx as the exact +// (index, snapshot) pair downstream profile resolution on this request must +// decide over. +func withProfileRequestIndex(ctx context.Context, idx *profileIndex) context.Context { + return context.WithValue(ctx, profileRequestIndexKey{}, idx) +} + +// profileRequestIndexFromContext returns the pair withProfileRequestIndex +// injected, or (nil, false) when the request did not enter through a path +// that pins one (e.g. the base /mcp endpoint, or a bare test server). +func profileRequestIndexFromContext(ctx context.Context) (*profileIndex, bool) { + idx, ok := ctx.Value(profileRequestIndexKey{}).(*profileIndex) + return idx, ok +} + // currentConfig returns the live configuration snapshot (hot-reload safe), // falling back to the construction-time config if the runtime is unavailable. func (p *MCPProxyServer) currentConfig() *config.Config { @@ -88,11 +122,14 @@ func (p *MCPProxyServer) effectiveDirectToolResponseMode() string { // profileScopeForSlug builds a ProfileScope for the named profile from the live // config, or returns nil when the slug does not match a configured profile. func (p *MCPProxyServer) profileScopeForSlug(slug string) *profile.ProfileScope { - if slug == "" { - return nil - } - cfg := p.currentConfig() - if cfg == nil { + return profileScopeForSlugIn(p.currentConfig(), slug) +} + +// profileScopeForSlugIn is profileScopeForSlug against an explicit config +// snapshot, for callers that must not re-read the live config between a +// check and the scope they build from it. +func profileScopeForSlugIn(cfg *config.Config, slug string) *profile.ProfileScope { + if slug == "" || cfg == nil { return nil } for i := range cfg.Profiles { @@ -114,8 +151,92 @@ func (p *MCPProxyServer) profileScopeForSlug(slug string) *profile.ProfileScope // It returns the resolved profile slug ("" when none) and the matching // ProfileScope ("" ⇒ nil). A session selection that no longer matches any // configured profile is treated as stale: it is cleared and resolution falls -// through to "none". +// through to "none". Resolution reads the live config snapshot once — unless +// the request came in through /mcp/p/, in which case it decides with the +// exact (index, snapshot) PAIR profileMiddleware already admitted the request +// against (profileRequestIndexFromContext), consumed directly via +// resolveActiveProfileFromIndex — never a fresh runtime.Config() read, and +// never a second, independent index lookup of its own. Extracting only the +// pair's cfg and handing it to resolveActiveProfileIn (which resolves the +// index again through profileIndexFor(cfg): an O(1) Published(cfg) match that +// falls back to a fleet-sized For(cfg) build on a miss) would let a request +// that paused across two publications between admission and this call land on +// a snapshot neither of profileIndexFor's two warmed slots covers any more — +// exactly the pair-acquisition bypass rounds 11/13 closed on the admission +// path (profileIndexCurrent/Acquire), reopened here on the downstream +// resolution path a paused request reaches next (round 15 MUST-FIX). Using +// the already-resolved pair outright cannot miss: there is no lookup to fall +// back from. Callers that hold only a snapshot — never an admitted pair — +// use resolveActiveProfileIn, which still resolves the index via +// profileIndexFor(cfg). func (p *MCPProxyServer) resolveActiveProfile(ctx context.Context) (string, *profile.ProfileScope) { + name, scope, _ := p.resolveActiveProfileWithIndex(ctx) + return name, scope +} + +// resolveActiveProfileWithIndex is resolveActiveProfile, but also returns the +// (index, snapshot) the (name, scope) pair was resolved against — the SAME +// pair, never a second, independent lookup. A caller that must derive +// something else from that identical pair — cacheAuthorizationWith's +// caller-intersected ProfileServers stamp (Spec 105 PR D review round 17 +// MUST-FIX) — uses this seam instead of re-resolving the index on its own, +// which could pair a decision made against one published snapshot with an +// index built from a later one on a request that pauses in between, exactly +// the class of bug rounds 9/11/14/15 closed on the admission and resolution +// paths. +func (p *MCPProxyServer) resolveActiveProfileWithIndex(ctx context.Context) (string, *profile.ProfileScope, *profileIndex) { + idx, ok := profileRequestIndexFromContext(ctx) + if !ok { + idx = p.profileIndexFor(p.currentConfig()) + } + name, scope := p.resolveActiveProfileFromIndex(ctx, idx) + return name, scope, idx +} + +// resolveActiveProfileIn is resolveActiveProfile against an explicit config +// snapshot. handleSetProfile admits a selection against one snapshot and must +// report the effective scope from that same snapshot: re-reading the live +// config here would let a hot reload between the two hand back a payload whose +// `active_profile` and `servers` disagree (or drop the just-stored selection +// as stale) — Spec 105 PR D critique round 1. +// +// Every tier below that resolves a profile by name (pin, session selection) +// does it through profileIndexFor(cfg)'s precomputed serverPos/members data +// (EffectiveServersFor) instead of profileScopeForSlugIn, which rebuilds a +// fleet-sized "known servers" set on every single call. resolveActiveProfile +// runs on every admitted scoped READ (retrieve_tools, describe_tool, +// call_tool_*, code_execution — every consumer of resolveActiveProfile in +// mcp.go), so that rebuild previously happened once per pinned or +// session-profiled call, at the fleet's cost, not the pin/session profile's +// own: identical output, no timing promise broken, at O(profile size) +// instead (Spec 105 PR D review round 14 MUST-FIX; the wildcard grant here +// is deliberate — this tier renders the profile's OWN full membership, never +// intersected with the caller's AllowedServers, exactly as +// profileScopeForSlugIn always did; a caller-specific view is applied +// separately downstream, e.g. handleSetProfile's own EffectiveServersFor +// call and callerVisibleServers' pre-105 equivalent). +func (p *MCPProxyServer) resolveActiveProfileIn(ctx context.Context, cfg *config.Config) (string, *profile.ProfileScope) { + return p.resolveActiveProfileFromIndex(ctx, p.profileIndexFor(cfg)) +} + +// resolveActiveProfileFromIndex is resolveActiveProfileIn over an ALREADY +// resolved (index, snapshot) pair — the seam handleSetProfile uses to +// finish its own admission and payload rendering on the EXACT SAME pair +// profileIndexCurrent gave it, rather than resolving cfg's index a second, +// independent time. Two different lookups of "the index for this cfg" can +// legitimately disagree: profileIndexCurrent's bare-proxy fallback caches by +// cfg's pointer identity (TestHandleSetProfile_ScopedRefusalTouchesOnlySlug- +// AndPin relies on exactly that to reuse a manually warmed index), while a +// test config a caller mutates IN PLACE between calls (cfg.Profiles = nil to +// simulate a deleted profile, never replacing the *config.Config pointer — +// TestSetProfileClearReportsPinnedScope, +// TestReadCache_DeletedPinnedProfileRevokesCachedAccess) makes that cached +// index's positions describe a Profiles slice the pointer no longer holds. +// Resolving the name and rendering its servers from two independently +// (re-)resolved indices could therefore pair a stale position with the +// current (shorter) Profiles slice and index out of range; resolving both +// from the ONE pair the caller already has cannot. +func (p *MCPProxyServer) resolveActiveProfileFromIndex(ctx context.Context, idx *profileIndex) (string, *profile.ProfileScope) { // 1. Agent-token pin (T3). When present it is authoritative and bounds // everything below — including the case where the pinned profile has been // removed from config since the token was minted. @@ -130,7 +251,7 @@ func (p *MCPProxyServer) resolveActiveProfile(ctx context.Context) (string, *pro // pin against an empty server set for the same reason, so the session and // preflight paths cannot disagree about what a pinned token may see. if pin := profilePinFromContext(ctx); pin != "" { - if scope := p.profileScopeForSlug(pin); scope != nil { + if scope := profileScopeFromIndex(idx, pin); scope != nil { return pin, scope } if p.logger != nil { @@ -150,7 +271,7 @@ func (p *MCPProxyServer) resolveActiveProfile(ctx context.Context) (string, *pro if p.sessionStore != nil { if sid := sessionIDFromContext(ctx); sid != "" { if name := p.sessionStore.GetActiveProfile(sid); name != "" { - if scope := p.profileScopeForSlug(name); scope != nil { + if scope := profileScopeFromIndex(idx, name); scope != nil { return name, scope } // Stored profile vanished from config — drop the stale selection. @@ -162,3 +283,72 @@ func (p *MCPProxyServer) resolveActiveProfile(ctx context.Context) (string, *pro // 4. No profile in effect. return "", nil } + +// resolveEffectiveProfileForJustSetSlug is resolveActiveProfileFromIndex's +// precedence (pin > URL > session selection) for the ONE caller that must +// never re-read the session store's mutable selection to answer it: +// handleSetProfile, immediately after it has itself just written slug via +// SetActiveProfile. Tiers 1 (pin) and 2 (URL) are per-request context values +// and safe to re-resolve as-is; tier 3 uses slug DIRECTLY instead of calling +// SessionStore.GetActiveProfile — closing a race a concurrent set_profile +// call on the SAME session could otherwise open between this call's own +// write and its own response render: call A sets "research", call B +// (interleaved) sets "deploy", and A's subsequent GetActiveProfile would see +// B's "deploy" — so A's response would report active_profile: "research" +// (A's own requested slug) with "deploy"'s servers, an FR-003 stored- +// selection/effective-scope consistency violation (cross-model review, PR +// D). slug is already validated selectable against idx immediately before +// the write (handleSetProfile's own profiles.selectable(ctx, slug) check), +// so idx.position(slug) cannot miss here the way tier 3's general "stored +// profile vanished from config" fallback anticipates for a session's OLD +// selection read on some later, unrelated call. +// +// Returns only the profile NAME, never a *ProfileScope: its one caller +// renders through EffectiveServersFor(name, callerAllowed), which never +// touches this scope. The first version of this function called +// profileScopeFromIndex and discarded the *ProfileScope it built — but that +// always resolves the WILDCARD-derived (`[]string{"*"}`) full membership, +// an O(profile size) allocation paid for a value nothing used (cross-model +// review round 2). idx.position is the O(1) existence check the pin/slug +// tiers actually need. +func (p *MCPProxyServer) resolveEffectiveProfileForJustSetSlug(ctx context.Context, idx *profileIndex, slug string) string { + if pin := profilePinFromContext(ctx); pin != "" { + if idx != nil && idx.position(pin) >= 0 { + return pin + } + // Stale pin (profile deleted since the token was minted): still + // authoritative — the caller stays deny-all under its own pin, + // never falls through to slug — and still worth an operator's + // attention, exactly as resolveActiveProfileFromIndex's own stale- + // pin branch logs it. + if p.logger != nil { + p.logger.Warn("agent-token profile_pin no longer matches any configured profile; resolving to a deny-all scope", + zap.String("profile_pin", pin)) + } + return pin + } + if urlScope := profile.ProfileScopeFromContext(ctx); urlScope != nil { + return urlScope.Name + } + if slug != "" && idx != nil && idx.position(slug) >= 0 { + return slug + } + return "" +} + +// profileScopeFromIndex builds the ProfileScope for slug's FULL membership +// (declared servers ∩ configured servers, unintersected with any caller +// credential — see resolveActiveProfileIn's doc comment) from idx, or nil +// when idx's snapshot has no such profile. It is profileScopeForSlugIn's +// O(profile size) counterpart, resolving through the index's precomputed +// data instead of rebuilding a fleet-sized set on every call. +func profileScopeFromIndex(idx *profileIndex, slug string) *profile.ProfileScope { + if idx == nil || idx.cfg == nil { + return nil + } + candidate := idx.position(slug) + if candidate < 0 { + return nil + } + return profile.NewProfileScope(slug, idx.effectiveServersForCandidate(candidate, []string{"*"})) +} diff --git a/internal/server/profile_resolver_test.go b/internal/server/profile_resolver_test.go index ffe125768..7d8b40f23 100644 --- a/internal/server/profile_resolver_test.go +++ b/internal/server/profile_resolver_test.go @@ -205,3 +205,57 @@ func TestSessionStore_ActiveProfileLifecycle(t *testing.T) { store.SetActiveProfile("", "research") require.Equal(t, "", store.GetActiveProfile("")) } + +// TestResolveActiveProfile_UsesInjectedPairDirectly_NeverFallsBackToFor (Spec +// 105 PR D review round 15, MUST-FIX): resolveActiveProfile must decide +// straight from the (index, snapshot) PAIR profileMiddleware already injected +// on the context (profileRequestIndexFromContext) — never extract only its +// cfg and re-resolve the index a second, independent time through +// resolveActiveProfileIn/profileIndexFor(cfg), which does an O(1) +// Published(cfg) match that falls back to a fleet-sized For(cfg) build on a +// miss. Here the injected snapshot (A) is aged out of BOTH the warm and +// previous slots by two further publications before resolveActiveProfile +// ever runs — exactly the pair-acquisition bypass rounds 11/13 closed on the +// admission path (profileIndexCurrent/Acquire), reopened here on the +// downstream resolution path a paused request reaches next. Using the +// already-resolved pair outright cannot miss: there is no lookup left to +// fall back from. +func TestResolveActiveProfile_UsesInjectedPairDirectly_NeverFallsBackToFor(t *testing.T) { + cfgA := &config.Config{ + Servers: []*config.ServerConfig{{Name: "a-srv"}}, + Profiles: []config.ProfileConfig{{Name: "research", Servers: []string{"a-srv"}}}, + } + cfgB := &config.Config{ + Servers: []*config.ServerConfig{{Name: "b-srv"}}, + Profiles: []config.ProfileConfig{{Name: "research", Servers: []string{"b-srv"}}}, + } + cfgC := &config.Config{ + Servers: []*config.ServerConfig{{Name: "c-srv"}}, + Profiles: []config.ProfileConfig{{Name: "research", Servers: []string{"c-srv"}}}, + } + + p := &MCPProxyServer{logger: zap.NewNop(), sessionStore: NewSessionStore(zap.NewNop())} + srv := &Server{logger: zap.NewNop(), mcpProxy: p} + p.mainServer = srv + + idxA := srv.profileIndexes.warmPublishing(cfgA) + // Two further publications land, aging A out of BOTH the warm and the + // previous slots. + srv.profileIndexes.warmPublishing(cfgB) + srv.profileIndexes.warmPublishing(cfgC) + require.Nil(t, srv.profileIndexes.Published(cfgA), "premise: A must no longer be a Published cache hit") + + ctx := withProfileRequestIndex(context.Background(), idxA) + ctx = auth.WithAuthContext(ctx, &auth.AuthContext{ + Type: auth.AuthTypeAgent, ProfilePin: "research", AllowedServers: []string{"*"}, + }) + + name, scope := p.resolveActiveProfile(ctx) + require.Equal(t, "research", name) + require.NotNil(t, scope) + require.Equal(t, []string{"a-srv"}, scope.AllowedServerNames(), + "must decide from the injected pair (A), never a config/index resolved independently") + + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), + "resolveActiveProfile with an injected pair must never fall back to a fleet-sized For rebuild") +} diff --git a/internal/server/profile_tool.go b/internal/server/profile_tool.go index 18ce16af2..9c7015e0c 100644 --- a/internal/server/profile_tool.go +++ b/internal/server/profile_tool.go @@ -4,8 +4,10 @@ import ( "context" "encoding/json" "fmt" - "slices" + "sort" "strings" + "sync" + "sync/atomic" "github.com/mark3labs/mcp-go/mcp" mcpserver "github.com/mark3labs/mcp-go/server" @@ -48,8 +50,20 @@ func buildSetProfileTool() mcp.Tool { } // handleSetProfile implements the set_profile tool. It validates the requested -// slug against live config, records it on the session (mutex-guarded, cleared -// on session close), and returns the resolved {active_profile, servers}. +// slug against ONE live-config snapshot, records it on the session +// (mutex-guarded, cleared on session close), and returns {active_profile, +// servers} where `active_profile` is the STORED session selection and +// `servers` is what the session can reach after the update. +// +// For a scoped caller (agent token) `servers` is the EFFECTIVE scope — +// resolveActiveProfileIn (pin > URL > session) over the same snapshot, +// intersected with the credential (Spec 105 FR-003): on a URL-scoped endpoint +// the URL governs the reported servers, and clearing a pinned token's +// selection reports active_profile == "" while servers still reports the +// pin's reach. Administrators (API key, socket, anonymous back-compat) keep +// the pre-105 payload byte-for-byte — the selected profile's servers, or every +// configured server on clear — because SC-005 names no FR-003 exception for +// them (an administrator is never pinned, so only the URL tier could differ). func (p *MCPProxyServer) handleSetProfile(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { slug := strings.TrimSpace(request.GetString("profile", "")) @@ -58,54 +72,125 @@ func (p *MCPProxyServer) handleSetProfile(ctx context.Context, request mcp.CallT return mcp.NewToolResultError("set_profile requires an active MCP session; no session id is bound to this request"), nil } - cfg := p.currentConfig() - - // Profiles v2 T3: a profile-pinned agent token may not switch away from its - // pinned profile. - pin := profilePinFromContext(ctx) - if pin != "" && slug != "" && slug != pin { - return mcp.NewToolResultError(fmt.Sprintf("agent token is pinned to profile '%s' and cannot switch to '%s'", pin, slug)), nil + // One (index, snapshot) pair for the whole call: admission, the stored + // selection's server list and the effective scope all read cfg, the + // snapshot the index was built from — never the live config, which may + // move underneath the call (Spec 105 PR D critique round 1 / codex round 6). + // Over a URL-scoped request (/mcp/p/) this is the exact pair + // serveProfileURL already admitted the request against, injected on the + // context — never a fresh, independent read (round 9 MUST-FIX 1). + profiles := p.profileIndexCurrent(ctx) + if profiles == nil { + // Acquire's bounded retries could not pair a config snapshot with its + // index (a publication storm outran them); profileIndexCurrent only + // ever returns nil for a scoped caller — an administrator falls back + // to a fresh build instead. Fail closed with the exact uniform + // refusal every other non-selectable slug gets: no state change, no + // disclosure, no build (Spec 105 PR D review round 11, MUST-FIX). + return mcp.NewToolResultError(fmt.Sprintf("unknown profile '%s'", slug)), nil } + cfg := profiles.cfg - // Empty slug clears the session selection (back to all servers). - if slug == "" { - p.sessionStore.SetActiveProfile(sessionID, "") - // A pinned token keeps its pin: clearing only drops the session tier, - // which the pin outranks anyway. Report what the session can actually - // reach — the pin's servers, or NOTHING when the pinned profile has been - // deleted — instead of the full server list, which would advertise a - // reach the resolver denies. - if pin != "" { - pinnedName, pinnedScope := p.resolveActiveProfile(ctx) - return setProfileResult(pinnedName, callerVisibleServers(ctx, pinnedScope.AllowedServerNames())) - } - return setProfileResult("", callerVisibleServers(ctx, allServerNames(cfg))) - } - - // Validate the slug names a configured profile the caller may select. The - // selectable set is computed BEFORE any session mutation or success log, so - // a profile outside the caller's reach is indistinguishable from an unknown - // one (FR-016b): same error, same `available:` list, no state change. - selectable := selectableProfileNames(ctx, cfg) - var match *config.ProfileConfig - if cfg != nil && slices.Contains(selectable, slug) { - for i := range cfg.Profiles { - if cfg.Profiles[i].Name == slug { - match = &cfg.Profiles[i] - break + // A non-empty slug must name a configured profile the caller may select + // (an empty slug clears the selection and is always accepted). The check + // runs BEFORE any session mutation or success log, so a profile outside + // the caller's reach — including a pinned token's own pin once it has + // zero reach (research D1), and a pin MISMATCH (the caller's slug names a + // different profile than its pin) — is indistinguishable from an unknown + // one (FR-016b / FR-003): same error, no state change. A pin mismatch is + // simply a non-selectable profile like any other and must never be + // decided by an earlier, distinctly-worded branch — that let a pinned + // caller confirm from the wording alone that it IS pinned, and to what, + // from a refusal aimed at a different slug (Spec 105 PR D review round 9, + // MUST-FIX 2; contracts/refusals.md: `set_profile ` is + // one format string). + // + // It decides the REQUESTED slug alone, through the per-snapshot index + // (profileIndex.selectable: one lookup of the slug, one of the pin, one + // precomputed-reach test), never through the selectable list — that list + // is one reach computation per configured profile, so a refusal that + // built it cost 0.3 µs over a one-profile fleet and 63 µs over 4 097 with + // a byte-identical body: a fleet-population timing oracle (codex review, + // PR D round 3; spec Definitions: non-disclosing = status, body AND + // timing class). For the same reason a scoped caller's refusal carries no + // `available:` list at all; administrators keep the pre-105 discovery + // affordance (every configured profile — SC-005), the one path that may + // legitimately enumerate. + if slug != "" { + if !profiles.selectable(ctx, slug) { + if auth.IsScopedCaller(ctx) { + return mcp.NewToolResultError(fmt.Sprintf("unknown profile '%s'", slug)), nil } + return mcp.NewToolResultError(fmt.Sprintf("unknown profile '%s' (available: %s)", slug, strings.Join(profiles.selectableNames(ctx), ", "))), nil } } - if match == nil { - return mcp.NewToolResultError(fmt.Sprintf("unknown profile '%s' (available: %s)", slug, strings.Join(selectable, ", "))), nil - } p.sessionStore.SetActiveProfile(sessionID, slug) - p.logger.Info("set_profile: session profile updated", - zap.String("session_id", sessionID), - zap.String("profile", slug), - ) - return setProfileResult(slug, callerVisibleServers(ctx, match.EffectiveServers(cfg))) + if slug != "" { + p.logger.Info("set_profile: session profile updated", + zap.String("session_id", sessionID), + zap.String("profile", slug), + ) + } + + // SC-005: administrators report the stored selection's own servers. + if !auth.IsScopedCaller(ctx) { + return setProfileResult(slug, profileServersIn(cfg, slug)) + } + + // Scoped caller: report what the session can actually reach after the + // update — the resolver's effective profile (a pin outranks the URL, which + // outranks the stored selection; a deleted pin is deny-all) bounded by the + // credential — never the stored selection's own servers when something + // else governs. Same snapshot as the admission check above. + // + // Rendered directly through the index's EffectiveServersFor, never + // scopeServersIn/callerVisibleServers (removed): that pair rebuilt a + // fleet-sized "known servers" set (profileServersIn → EffectiveServers) + // for the effective profile AND, on a cleared/no-profile session, walked + // every configured server a second time to apply the credential + // (allServerNames + a CanEnumerateServer test per server) — fleet-sized + // work on a request already admitted for exactly this caller (Spec 105 + // PR D review round 14 MUST-FIX). effectiveProfileName == "" renders the + // no-profile/cleared-session set; a name no longer present in cfg (a + // stale pin) renders empty, matching the deny-all scope it resolved to + // — EffectiveServersFor needs only the resolved NAME, never the scope's + // own (possibly already caller-filtered, e.g. via a /mcp/p/ URL) + // server set, so intersecting it again here with the caller's real + // grant is idempotent, not a second, different filter. + // + // resolveEffectiveProfileForJustSetSlug, NOT resolveActiveProfileFromIndex: + // the latter's tier 3 re-reads SessionStore.GetActiveProfile, which a + // concurrent set_profile on the SAME session could have already + // overwritten between this call's own SetActiveProfile write above and + // this render — reporting THIS call's slug in active_profile with a + // DIFFERENT call's servers (cross-model review, PR D). + effectiveProfileName := p.resolveEffectiveProfileForJustSetSlug(ctx, profiles, slug) + var allowed []string + if ac := auth.AuthContextFromContext(ctx); ac != nil { + allowed = ac.AllowedServers + } + return setProfileResult(slug, profiles.EffectiveServersFor(effectiveProfileName, allowed)) +} + +// profileServersIn renders the pre-105 set_profile server list for a stored +// selection: the named profile's effective servers in profile-declared order +// (duplicates kept, exactly as EffectiveServers returns them), every +// configured server in config order for an empty slug, and nothing for a slug +// cfg does not know. +func profileServersIn(cfg *config.Config, slug string) []string { + if slug == "" { + return allServerNames(cfg) + } + if cfg == nil { + return nil + } + for i := range cfg.Profiles { + if cfg.Profiles[i].Name == slug { + return cfg.Profiles[i].EffectiveServers(cfg) + } + } + return nil } // setProfileResult renders the standard set_profile success payload. @@ -124,18 +209,6 @@ func setProfileResult(activeProfile string, servers []string) (*mcp.CallToolResu return mcp.NewToolResultText(string(body)), nil } -// profileNames returns all configured profile slugs (for error messages). -func profileNames(cfg *config.Config) []string { - if cfg == nil { - return nil - } - names := make([]string, 0, len(cfg.Profiles)) - for i := range cfg.Profiles { - names = append(names, cfg.Profiles[i].Name) - } - return names -} - // allServerNames returns the names of every configured server (the "all // servers" set returned when a profile selection is cleared). func allServerNames(cfg *config.Config) []string { @@ -151,65 +224,758 @@ func allServerNames(cfg *config.Config) []string { return names } -// callerVisibleServers filters a server list through the caller's credential. -// set_profile's payload advertises what the session can reach, so a token -// restricted to server A must never be told about server B — whether the list -// is "all servers" (cleared selection), a profile's full set, or a pinned -// profile's scope (Spec 104 FR-016b). +// selectableProfileNames returns the profile slugs the caller may select. An +// unrestricted caller may select any configured profile; a profile-pinned +// token only its pin, and only while the pin has reach (it exists and its +// server set intersects the token's allowed servers — a deleted pin, an empty +// or ghost profile and a disjoint grant all yield nothing, Spec 105 research +// D1); a scoped caller only the profiles that overlap the servers it can +// enumerate. // -// The predicate is auth.CanEnumerateServer — the same CanAccessServer rule -// serverInScope applies to retrieve_tools / describe_tool — so this surface -// cannot disagree with visibility: admin (API-key / socket / anonymous -// back-compat) and absent contexts pass everything through untouched; any -// non-admin context (agent token, server-edition user) keeps only the servers -// its AllowedServers names, "*" allows all, and an EMPTY list grants nothing. -func callerVisibleServers(ctx context.Context, servers []string) []string { - if !auth.IsScopedCaller(ctx) { - return servers +// This is the same rule profileIndex.selectable evaluates for ONE profile — +// the admission rule on set_profile AND on the /mcp/p/ URL +// (profileMiddleware): a profile entirely outside the caller's reach is +// treated exactly like a nonexistent one (FR-016b, FR-003/004). The LIST is +// fleet-sized work, so only the administrator's `available:` affordance +// renders it; no refusal a scoped caller receives is allowed to compute it. +// +// The result is accumulated by forEachProfileSelectable in configured order; +// see there for why it never returns early. Both build a throwaway index +// over cfg; production callers hold a per-snapshot one and use its methods. +func selectableProfileNames(ctx context.Context, cfg *config.Config) []string { + if cfg == nil { + return nil } - out := make([]string, 0, len(servers)) - for _, s := range servers { - if auth.CanEnumerateServer(ctx, s) { - out = append(out, s) - } + return newProfileIndex(cfg).selectableNames(ctx) +} + +// forEachProfileSelectable visits EVERY configured profile, in configured +// order, and reports to visit whether the caller may select it (the rule +// documented on selectableProfileNames). See profileIndex.forEachSelectable. +func forEachProfileSelectable(ctx context.Context, cfg *config.Config, visit func(name string, selectable bool)) { + if cfg == nil { + return } - return out + newProfileIndex(cfg).forEachSelectable(ctx, visit) } -// selectableProfileNames returns the profile slugs the caller may select. An -// unrestricted caller may select any configured profile; a profile-pinned -// token only its pin (and nothing when the pin no longer exists); a scoped -// caller only the profiles that overlap the servers it can enumerate. +// profileIndex is an immutable index over ONE config snapshot: slug → +// profile, plus each profile's precomputed reach set. It lets the +// /mcp/p/ gate and set_profile decide the requested profile (and a +// pinned caller's pin) directly instead of walking cfg.Profiles, so the work +// a scoped refusal costs does not grow with the number of OTHER profiles the +// operator has configured — the whole selectable list is fleet-sized, and a +// refusal that computed it did zero iterations over an empty fleet and one +// EffectiveServers per profile over a populated one: same status and body, +// fleet-population timing oracle (codex review, PR D round 2; FR-004). // -// This is both the `available:` list of the unknown-slug error and the -// admission rule for a selection: a profile entirely outside the caller's -// reach is treated exactly like a nonexistent one, so the error text cannot be -// used to confirm which profiles the operator has configured (FR-016b). -func selectableProfileNames(ctx context.Context, cfg *config.Config) []string { +// Reach is precomputed per profile as a SET of the configured servers it +// declares (a bitset over server positions, members, plus a non-empty flag), +// built once per snapshot: at request time the reach test walks the READER's +// own allowed_servers and tests each against that set — one O(1) membership +// test per granted name, or one "is the set non-empty" test for a wildcard +// or administrator reader — so its cost is the size of the caller's own +// grant and nothing else. A profile the snapshot lacks reads the all-zero +// placeholder (none) at the same cost. Computing reach from the candidate's +// declared list instead cost nothing for a missing profile and |servers| × +// |declared| for an existing one, so a pinned token asking for its own +// zero-reach pin could tell "deleted" from "exists" by timing (codex review, +// PR D round 3; research D1); walking every configured server and running +// the credential check on each cost one iteration over a fleet of one server +// and 4 096 over a fleet with 4 095 hidden ones — same 404, same zero +// allocations, 36 ns vs 25 µs — so the operator's server population was a +// timing oracle (codex review, PR D round 4). +// +// Duplicate slugs cannot load (ValidateProfiles), but a hand-built config may +// carry them: the first occurrence wins, exactly like every linear lookup in +// this package (profileServersIn, the middleware's own scan). +type profileIndex struct { + cfg *config.Config + byName map[string]int // slug → position in cfg.Profiles + + // serverPos maps a configured server's name to its position in + // cfg.Servers (first named, non-nil occurrence), so a granted name + // resolves to its membership bit in O(1). + serverPos map[string]int + + // words is the bitset length in uint64 words: ceil(len(cfg.Servers)/64). + // members holds len(cfg.Profiles) consecutive bitsets of that length — + // bit i of profile p's set is on when cfg.Servers[i] is one of p's + // declared servers (EffectiveServers as a set). nonEmpty[p] is on when + // that set has at least one bit — the whole reach test for a reader + // that may see every server. none is the all-zero placeholder read for + // a slug the snapshot has no profile for. + words int + members []uint64 + nonEmpty []bool + none []uint64 + + // declaredOccurrences[p] maps a server name to the ordered list of + // indices at which it appears in cfg.Profiles[p].Servers — that + // profile's OWN declared order, duplicates tracked individually. + // Precomputed once per snapshot (same amortization as members/ + // serverPos) so effectiveServersForCandidate's restricted-grant branch + // can render "profile-declared order, duplicates kept" by touching only + // the caller's own grant, never a walk of the profile's full declared + // list per request (cross-model review, PR D: the prior restricted-grant + // branch walked `declared` directly — O(profile size) — reopening the + // exact SC-005 timing-disclosure class rounds 14-17 closed for every + // other admitted-read path, at this one call site). + declaredOccurrences []map[string][]int + + // lookupHook, when set, observes every slug the index resolves. It is the + // seam the traversal-counter tests use to prove the gate and set_profile + // touch at most the requested slug and the pin; nil in production. + lookupHook func(slug string) + + // reachHook, when set, observes every unit of work reach performs — one + // call per membership test. It is the seam the fleet-parity tests use to + // prove a reach test costs the reader's grant, never the fleet; nil in + // production. + reachHook func() +} + +func newProfileIndex(cfg *config.Config) *profileIndex { + idx := &profileIndex{cfg: cfg, byName: map[string]int{}} if cfg == nil { + return idx + } + idx.byName = make(map[string]int, len(cfg.Profiles)) + for i := range cfg.Profiles { + if _, dup := idx.byName[cfg.Profiles[i].Name]; !dup { + idx.byName[cfg.Profiles[i].Name] = i + } + } + + // Reach sets: server name → position once, then one bit per declared + // server that exists in the snapshot (the EffectiveServers rule; nil + // entries and empty names excluded — CanAccessServer never grants an + // empty name, so such a server is reachable by nobody). + idx.serverPos = make(map[string]int, len(cfg.Servers)) + for i, s := range cfg.Servers { + if s == nil || s.Name == "" { + continue + } + if _, dup := idx.serverPos[s.Name]; !dup { + idx.serverPos[s.Name] = i + } + } + idx.words = (len(cfg.Servers) + 63) / 64 + idx.none = make([]uint64, idx.words) + idx.members = make([]uint64, len(cfg.Profiles)*idx.words) + idx.nonEmpty = make([]bool, len(cfg.Profiles)) + idx.declaredOccurrences = make([]map[string][]int, len(cfg.Profiles)) + for p := range cfg.Profiles { + set := idx.membersOf(p) + occ := make(map[string][]int, len(cfg.Profiles[p].Servers)) + for declaredPos, name := range cfg.Profiles[p].Servers { + if i, ok := idx.serverPos[name]; ok { + set[i/64] |= 1 << (uint(i) % 64) + idx.nonEmpty[p] = true + occ[name] = append(occ[name], declaredPos) + } + } + idx.declaredOccurrences[p] = occ + } + return idx +} + +// membersOf returns profile p's reach bitset, or the all-zero placeholder +// for p < 0 (no such profile). +func (idx *profileIndex) membersOf(p int) []uint64 { + if p < 0 { + return idx.none + } + return idx.members[p*idx.words : (p+1)*idx.words] +} + +// position resolves one slug to its position in cfg.Profiles in O(1), or -1 +// when the snapshot has no such profile. The position is also bounds-checked +// against idx.cfg.Profiles' CURRENT length, not merely built at "ok": every +// other profileIndex field (members, nonEmpty, serverPos) was snapshotted at +// construction from an assumed-immutable *config.Config and is safe to +// index by a byName position regardless, but idx.cfg itself is a pointer a +// raw test fixture may mutate IN PLACE after building this index (cfg. +// Profiles = nil to simulate a deleted profile, never replacing the +// pointer — TestSetProfileClearReportsPinnedScope, +// TestReadCache_DeletedPinnedProfileRevokesCachedAccess), which would +// otherwise let a stale byName entry index a Profiles slice that has since +// shrunk out from under it. For a real (never-mutated) snapshot this check +// is always true — the position and idx.cfg.Profiles' length were built +// together — so it costs nothing on the path that matters. +func (idx *profileIndex) position(slug string) int { + if idx.lookupHook != nil { + idx.lookupHook(slug) + } + if i, ok := idx.byName[slug]; ok && idx.cfg != nil && i < len(idx.cfg.Profiles) { + return i + } + return -1 +} + +// lookup resolves one slug in O(1), or nil when the snapshot has no such +// profile. +func (idx *profileIndex) lookup(slug string) *config.ProfileConfig { + if i := idx.position(slug); i >= 0 { + return &idx.cfg.Profiles[i] + } + return nil +} + +// profileAt returns the profile at an ALREADY resolved position (candidate < +// 0: no such profile), the int-keyed counterpart to lookup — the seam a +// caller that already paid for position(slug) elsewhere in the same request +// (serveProfileURL's admission gate) uses to fetch the *ProfileConfig +// without resolving the slug a second time through the lookup-hook seam. +func (idx *profileIndex) profileAt(candidate int) *config.ProfileConfig { + if candidate < 0 || idx.cfg == nil || candidate >= len(idx.cfg.Profiles) { + return nil + } + return &idx.cfg.Profiles[candidate] +} + +// EffectiveServersFor is the caller-facing counterpart to +// config.ProfileConfig.EffectiveServers: the servers within profileName's +// declared set — or, for profileName == "", every configured server — that +// allowed also grants (the same "*" / exact-name rule as +// AuthContext.CanAccessServer). It is what every admitted scoped READ +// (set_profile, /mcp/p/, and the pin/session tiers resolveActiveProfile +// consults on every call) renders its payload/scope from instead of +// re-deriving a fleet-sized "known servers" set on every call the way +// EffectiveServers does. +// +// Its cost is O(len(allowed)) (plus, only for a wildcard grant, one walk of +// the TARGET population — the profile's own declared servers, or every +// configured server for profileName == "", never a SECOND, nested walk per +// grant entry) — never O(len(cfg.Servers)): each granted name is tested +// against the index's precomputed serverPos/members data, built once per +// snapshot, rather than rebuilding a fleet-sized set on every call (Spec 105 +// PR D review round 14 MUST-FIX). allowed == nil or empty is deny-all (an +// agent token's empty AllowedServers grants nothing — CanAccessServer's own +// rule) and returns nil without touching the index at all. +// +// The order matches every other EffectiveServers consumer: profile-declared +// order (duplicates kept) for a named profile, config order for profileName +// == "" — reproduced from the reader's own grant via serverPos, so it never +// needs cfg.Servers itself to get there. +// +// (cross-model review, PR D: the named-profile restricted-grant branch had +// walked the profile's own declared list to get that order — O(profile +// size), not O(len(allowed)) as documented above, reopening the SC-005 +// timing class rounds 14-17 closed elsewhere. Fixed via +// declaredOccurrences, precomputed once per snapshot alongside members/ +// serverPos, so the order is reproduced from the CALLER's own grant.) +func (idx *profileIndex) EffectiveServersFor(profileName string, allowed []string) []string { + if profileName == "" { + return idx.effectiveServersForAllowed(allowed) + } + return idx.effectiveServersForCandidate(idx.position(profileName), allowed) +} + +// effectiveServersForCandidate is EffectiveServersFor for an ALREADY resolved +// profile position — see profileAt. +func (idx *profileIndex) effectiveServersForCandidate(candidate int, allowed []string) []string { + if idx.cfg == nil || candidate < 0 || candidate >= len(idx.cfg.Profiles) || len(allowed) == 0 { return nil } - if pin := profilePinFromContext(ctx); pin != "" { - for i := range cfg.Profiles { - if cfg.Profiles[i].Name == pin { - return []string{pin} + if hasWildcardGrant(allowed) { + declared := idx.cfg.Profiles[candidate].Servers + out := make([]string, 0, len(declared)) + for _, name := range declared { + if _, ok := idx.serverPos[name]; ok { + out = append(out, name) } } + return out + } + // Restricted grant: touch only the caller's own allowed entries — never + // declared (the profile's full, possibly hidden-from-this-caller, size). + // declaredOccurrences[candidate] was precomputed once per snapshot + // (newProfileIndex), so a hit costs one map lookup per grant entry (plus + // one append per occurrence, for the rare authored duplicate — bounded + // by the admin-authored declared list, never by anything the caller + // controls) instead of a walk of the profile's own declared list. + // + // allowed itself may repeat a name (an unvalidated AllowedServers list); + // dedupeSeen guards against that so a repeated grant entry doesn't + // re-expand the same occurrences again — the old grant-map-based walk + // deduped allowed implicitly (map keys), so declared's own duplicates + // alone drove the output count, and this must reproduce that exactly + // (cross-model review round 2: a naive per-`allowed`-entry expansion + // multiplied by BOTH allowed's and declared's duplicate counts). + occ := idx.declaredOccurrences[candidate] + type hit struct { + pos int + name string + } + hits := make([]hit, 0, len(allowed)) + dedupeSeen := make(map[string]struct{}, len(allowed)) + for _, name := range allowed { + if _, dup := dedupeSeen[name]; dup { + continue + } + dedupeSeen[name] = struct{}{} + for _, pos := range occ[name] { + hits = append(hits, hit{pos: pos, name: name}) + } + } + sort.Slice(hits, func(i, j int) bool { return hits[i].pos < hits[j].pos }) + out := make([]string, len(hits)) + for i, h := range hits { + out[i] = h.name + } + return out +} + +// effectiveServersForAllowed is EffectiveServersFor("", allowed): every +// configured server allowed grants, in config order — the "no profile in +// effect" / cleared-selection rendering. A wildcard grant is unrestricted by +// definition (SC-005 administrator parity: the same full list, at the same +// cost administrators already pay for it), so it returns allServerNames +// directly rather than resolving each configured server through allowed. A +// restricted grant resolves each of the reader's OWN entries to its config +// position via serverPos — O(1) each — and sorts the (small) result by that +// position, so it reproduces config order without ever walking cfg.Servers: +// its cost is O(len(allowed) log len(allowed)), never the fleet's. +func (idx *profileIndex) effectiveServersForAllowed(allowed []string) []string { + if idx.cfg == nil || len(allowed) == 0 { return nil } + if hasWildcardGrant(allowed) { + return allServerNames(idx.cfg) + } + type grantHit struct { + pos int + name string + } + hits := make([]grantHit, 0, len(allowed)) + seen := make(map[int]struct{}, len(allowed)) + for _, name := range allowed { + pos, ok := idx.serverPos[name] + if !ok { + continue + } + if _, dup := seen[pos]; dup { + continue + } + seen[pos] = struct{}{} + hits = append(hits, grantHit{pos: pos, name: name}) + } + sort.Slice(hits, func(i, j int) bool { return hits[i].pos < hits[j].pos }) + out := make([]string, len(hits)) + for i, h := range hits { + out[i] = h.name + } + return out +} + +// hasWildcardGrant reports whether allowed carries the "*" entry +// AuthContext.CanAccessServer treats as unrestricted. +func hasWildcardGrant(allowed []string) bool { + for _, name := range allowed { + if name == "*" { + return true + } + } + return false +} + +// hasMember reports whether profile candidate's reach set is non-empty — +// false for the placeholder (candidate < 0). +func (idx *profileIndex) hasMember(candidate int) bool { + return candidate >= 0 && idx.nonEmpty[candidate] +} + +// reach reports whether the caller can enumerate at least one server of +// profile candidate (-1: no such profile) — the rule behind +// selectableProfileNames (research D1), i.e. +// len(callerVisibleServers(ctx, p.EffectiveServers(cfg))) > 0, without +// either allocation. Its cost is O(|reader grant|), never O(|fleet|): a +// reader that may see every server (administrator, absent context, or a +// wildcard entry) needs one test — is the set non-empty — and a restricted +// reader needs one membership test per entry of its own AllowedServers +// (the same "*" / exact-name rule as AuthContext.CanAccessServer; an empty +// name never matches). It never returns early and does the same work for +// the placeholder set, an empty profile and one declaring every server, so +// neither the outcome, the candidate nor the number of configured servers +// can be told from the work — only the caller's own grant, which it knows. +func (idx *profileIndex) reach(ctx context.Context, candidate int) bool { + if idx.cfg == nil { + return false + } if !auth.IsScopedCaller(ctx) { - return profileNames(cfg) + idx.step() + return idx.hasMember(candidate) } - names := make([]string, 0, len(cfg.Profiles)) - for i := range cfg.Profiles { - if len(callerVisibleServers(ctx, cfg.Profiles[i].EffectiveServers(cfg))) > 0 { - names = append(names, cfg.Profiles[i].Name) + members := idx.membersOf(candidate) + reach := false + // IsScopedCaller guarantees a non-nil, non-admin AuthContext. + for _, name := range auth.AuthContextFromContext(ctx).AllowedServers { + idx.step() + hit := false + if name == "*" { + hit = idx.hasMember(candidate) + } else if i, ok := idx.serverPos[name]; ok { + hit = members[i/64]&(1<<(uint(i)%64)) != 0 } + if hit { + reach = true + } + } + return reach +} + +// step reports one unit of reach work to the test seam. +func (idx *profileIndex) step() { + if idx.reachHook != nil { + idx.reachHook() + } +} + +// selectable reports whether the caller may select the profile named slug — +// the rule forEachSelectable applies to every profile, evaluated for this +// ONE profile. It is the predicate of the URL gate and of set_profile's +// admission, and must never consult the selectable list: whatever the +// outcome (slug absent, profile out of reach, pin deleted, pin zero-reach, +// pin mismatch, empty fleet) it does the same work — one lookup of the slug, +// one lookup of the pin when the caller is pinned, and exactly one +// allocation-free reach test of the caller's own grant against the +// candidate's precomputed set or the all-zero placeholder when there is none +// — so no branch can be told from another by its cost, and none of it +// depends on how many other profiles exist, on how many servers the +// candidate declares or on how many servers are configured. +func (idx *profileIndex) selectable(ctx context.Context, slug string) bool { + candidate := idx.position(slug) + pin := profilePinFromContext(ctx) + if pin != "" { + // The pin is the only profile a pinned caller may select; resolve it + // whether or not the URL named it so a mismatch costs what a match does. + pinned := idx.position(pin) + candidate = -1 + if slug == pin { + candidate = pinned + } + } + reach := idx.reach(ctx, candidate) + // Administrators (and absent contexts) select any configured profile, + // including empty or ghost ones (SC-005); everyone else needs reach. + needsReach := pin != "" || auth.IsScopedCaller(ctx) + return candidate >= 0 && (!needsReach || reach) +} + +// forEachSelectable visits EVERY configured profile, in configured order, +// and reports to visit whether the caller may select it — the same rule as +// selectable, applied to each profile. +// +// It deliberately has no early return and does the same per-profile work +// whatever the outcome: a scoped caller's reach is computed for each profile +// even when a pin already rules it out, and a pinned caller keeps walking +// after its pin is found — a version that answered after one iteration for +// a live pin and after the whole slice for a deleted or zero-reach pin let a +// pinned caller tell those apart by the work its own refusal cost (codex +// review, PR D round 1). It is fleet-sized by nature, so no scoped refusal +// may run it (round 3): it feeds the administrator's `available:` list and +// the tests that pin selectable to it. +func (idx *profileIndex) forEachSelectable(ctx context.Context, visit func(name string, selectable bool)) { + if idx.cfg == nil { + return + } + pin := profilePinFromContext(ctx) + // Administrators (and absent contexts) select any configured profile, + // including empty or ghost ones (SC-005); everyone else needs reach. + needsReach := pin != "" || auth.IsScopedCaller(ctx) + for i := range idx.cfg.Profiles { + p := &idx.cfg.Profiles[i] + selectable := true + if needsReach { + selectable = idx.reach(ctx, i) + } + if pin != "" && p.Name != pin { + selectable = false + } + visit(p.Name, selectable) } +} + +// selectableNames accumulates forEachSelectable into a pre-sized slice, in +// configured order. +func (idx *profileIndex) selectableNames(ctx context.Context) []string { + if idx.cfg == nil { + return nil + } + names := make([]string, 0, len(idx.cfg.Profiles)) + idx.forEachSelectable(ctx, func(name string, selectable bool) { + if selectable { + names = append(names, name) + } + }) return names } +// profileIndexCache holds the profileIndex of the config snapshot a request +// decides over. Config snapshots are replaced, never mutated in place +// (configsvc copy-on-write; every reload publishes a new *Config), so an +// index is keyed on its snapshot's pointer identity and retains the snapshot +// it was built from (idx.cfg), so its address cannot be recycled under it. +// The zero value is ready to use. +// +// A request must decide over the snapshot that is actually PUBLISHED — +// runtime.Config() is the one atomic publication boundary every reader +// agrees on — never over a snapshot merely prepared ahead of it. Published +// returns the prepared pair whose cfg equals the caller's own read of +// runtime.Config(): the latest pair when it already covers it, or the +// previous one during the narrow window in which the pre-publish observer +// has warmed the NEXT snapshot but configsvc has not yet stored it (warm +// already points past runtime.Config(); previous still matches it). Round 7 +// (codex) found that taking the unconditional warm slot — always the latest +// prepared pair, whether or not it was stored yet — let a request be +// admitted against the next config while resolveActiveProfileIn's pin tier, +// reading runtime.Config() independently, still answered the previous one: +// admission and the effective scope could disagree within one request. +// Keeping the last TWO prepared pairs closes that window without ever +// building: the observer runs under updateMu, so at most one +// prepared-but-not-yet-stored pair can exist at any time, and "latest, else +// previous" is therefore always either the published pair or the one about +// to replace it — never a third, older one a request could still be reading +// runtime.Config() as. +// +// For is the fallback for bare test servers (no runtime, so no warm path): +// it never writes the warm slot and builds into the lazy slot, so even a +// caller that reached it with a runtime could not roll the warm index back. +type profileIndexCache struct { + warm atomic.Pointer[profileIndex] // latest prepared pair + previous atomic.Pointer[profileIndex] // second-latest prepared pair (see Published) + lazy atomic.Pointer[profileIndex] + + // warmMu serialises warm-slot writers; observed flips once the + // pre-publish observer has warmed a snapshot, after which the event-driven + // warm defers to it (a late event's build could otherwise land over a + // newer observer build). + warmMu sync.Mutex + observed bool + + // lazyBuilds counts For's fallback builds — the seam the tests use to + // prove no request over a live runtime pays for the index. + lazyBuilds atomic.Int64 +} + +// Current returns the latest warmed (index, snapshot) pair — the one the +// warm path most recently prepared, whether or not it has been published +// yet — or nil before any warm path has run. Cache-mechanics callers only +// (bare-cache tests, and callers with no runtime.Config() to match against); +// request paths that must stay consistent with a specific runtime.Config() +// read use Published instead, which is what production wires through +// profileMiddleware and profileIndexCurrent. +func (c *profileIndexCache) Current() *profileIndex { + return c.warm.Load() +} + +// Published returns the prepared pair whose snapshot is published — cfg == +// the caller's own runtime.Config() read — or nil when neither the latest +// nor the previous prepared pair covers it: a bare cache with no warm path +// (no runtime behind it), or, should the pre-publish observer ever not have +// run on a stored snapshot (it always does — every publication path funnels +// through updateLocked), a snapshot more than one publication further back +// than what warmPublishing has seen. Callers fall back to For(published) in +// that case, counted on lazyBuilds; over a live runtime with the observer +// wired the fallback must never fire. +func (c *profileIndexCache) Published(published *config.Config) *profileIndex { + if idx := c.warm.Load(); idx != nil && idx.cfg == published { + return idx + } + if idx := c.previous.Load(); idx != nil && idx.cfg == published { + return idx + } + return nil +} + +// warmPublishing indexes cfg as the configsvc pre-publish observer, on the +// exact snapshot about to be published, and returns the index. It runs under +// the config update mutex, so it does one insertion per profile and nothing +// else. The prior warm pair is demoted to previous (never dropped outright) +// so a request whose own runtime.Config() read still answers it — because +// this snapshot has not been Store'd yet — can still find it via Published. +// Requests that already hold either pair keep it (Current/Published hand out +// the pointer, not the slot). +func (c *profileIndexCache) warmPublishing(cfg *config.Config) *profileIndex { + c.warmMu.Lock() + defer c.warmMu.Unlock() + c.observed = true + old := c.warm.Load() + if old != nil && old.cfg == cfg { + return old + } + idx := newProfileIndex(cfg) + if old != nil { + c.previous.Store(old) + } + c.warm.Store(idx) + return idx +} + +// warmCurrent indexes the runtime's current snapshot cfg from outside the +// publication path (construction, config events). It is the whole warm path +// for the initial snapshot — NewService stores it without running any +// observer — and belt-and-braces afterwards: once the observer has warmed a +// snapshot it is a no-op, so a late event can never roll the slot back. +func (c *profileIndexCache) warmCurrent(cfg *config.Config) { + c.warmMu.Lock() + defer c.warmMu.Unlock() + if c.observed { + return + } + if idx := c.warm.Load(); idx != nil && idx.cfg == cfg { + return + } + c.warm.Store(newProfileIndex(cfg)) +} + +// For returns the index for cfg: the warm slot when it covers cfg, else the +// lazy slot, else a build of its own into the lazy slot. It is the fallback +// for bare test servers only (no runtime, so no warm path ever runs); a +// request over a runtime takes Current() and must never reach it +// (TestProfileRequests_NeverBuildTheIndexOverARuntime pins that through the +// lazyBuilds seam). It never writes the warm slot. Two goroutines racing on +// a lazy build may both build; either result is correct and the later Store +// wins. +func (c *profileIndexCache) For(cfg *config.Config) *profileIndex { + if idx := c.warm.Load(); idx != nil && idx.cfg == cfg { + return idx + } + if idx := c.lazy.Load(); idx != nil && idx.cfg == cfg { + return idx + } + c.lazyBuilds.Add(1) + idx := newProfileIndex(cfg) + c.lazy.Store(idx) + return idx +} + +// profileIndexAcquireRetries bounds Acquire's read-and-match loop. Each +// iteration is O(1) (Published never builds), so the bound caps the cost of +// even a publication storm at O(retries) — never a profile-index build. +const profileIndexAcquireRetries = 8 + +// Acquire returns the prepared pair whose snapshot equals readCfg()'s OWN +// result, read and matched atomically inside the cache — never a separate +// read-then-match at the call site, which a publication landing strictly +// between the two steps could invalidate before Published ever ran (Spec 105 +// PR D review round 11, MUST-FIX). profileMiddleware and profileIndexCurrent +// previously read runtime.Config() once, then called Published against that +// stale read; a request paused across two publications between those two +// steps found neither the latest nor the previous prepared pair, and fell +// back to For, which builds the whole fleet inline — fleet-sized work behind +// a status/body identical to the constant-cost refusal every other miss +// gets, exactly the timing-class disclosure D17 exists to close. +// +// Acquire closes it structurally: each iteration re-reads readCfg() (never +// reusing a prior read) and asks Published for THAT read's own snapshot, so +// a publication racing the previous iteration is simply observed on the +// next one — the loop chases a moving config instead of freezing a read +// that may already be stale by the time it is used. It returns nil only +// when readCfg keeps outrunning the bounded retry budget, which the +// pre-publish observer makes structurally near-impossible on any snapshot a +// reader actually reaches (Published matches on the very first iteration in +// the overwhelmingly common case — no race in flight). +// +// Callers decide what a miss means for their own caller shape: production +// call sites (profileMiddleware, profileIndexCurrent for the base /mcp +// endpoint) refuse a scoped caller uniformly on nil (fail closed, O(1), +// never a build) and fall back to For for an administrator-shaped caller, +// whose timing is not contract-bound (SC-005). For itself remains the +// fallback for bare test servers with no runtime to read atomically at all. +func (c *profileIndexCache) Acquire(readCfg func() *config.Config) *profileIndex { + for i := 0; i < profileIndexAcquireRetries; i++ { + cfg := readCfg() + if idx := c.Published(cfg); idx != nil { + return idx + } + } + return nil +} + // setProfileServerTool wraps buildSetProfileTool as a ServerTool for routing-mode registration. func (p *MCPProxyServer) setProfileServerTool() mcpserver.ServerTool { return mcpserver.ServerTool{Tool: buildSetProfileTool(), Handler: p.handleSetProfile} } + +// profileIndexCurrent returns the profile index a set_profile call decides +// with — and, as idx.cfg, the config snapshot it decides over. +// +// When ctx carries the pair serveProfileURL already admitted this request +// against (profileRequestIndexFromContext — a /mcp/p/ request), THAT +// pair is preferred outright: no build, no runtime.Config() read of any +// kind, because re-reading here is exactly the bug this seam exists to +// close — a reload landing between the gate's admission and this call could +// otherwise hand set_profile a different snapshot than the one the URL gate +// used for the very same request (Spec 105 PR D review round 9, MUST-FIX 1). +// +// Otherwise (the plain /mcp endpoint, which admits no snapshot of its own) +// the pair is taken from the main Server's cache via Acquire, which reads +// currentConfig() and matches it atomically inside the cache (round 7/8: +// taking the cache's unconditional latest pair here let set_profile admit +// and scope a profile that existed only in the config about to be +// published, one publication ahead of what resolveActiveProfileIn would +// independently read moments later; round 11: reading currentConfig() and +// matching it in two separate steps — as a bare Published(currentConfig()) +// call did — left a window in which a request paused between the two steps +// missed both prepared pairs and fell back to For, building the whole fleet +// inline for a scoped caller's refusal), so the call never builds an index +// on a scoped path and never pairs a snapshot with an index built from +// another one. On the rare exhaustion of Acquire's bounded retries (a +// publication storm outrunning the loop), a scoped caller gets nil here — +// handleSetProfile fails closed with the same uniform refusal any other +// non-selectable slug gets, O(1), never a build; an administrator-shaped +// caller is not timing-contract-bound (SC-005) and falls back to a fresh +// build over currentConfig(). A proxy with no warmed main Server (bare test +// servers) falls back to a lazily built index over its construction config, +// keyed by identity. +func (p *MCPProxyServer) profileIndexCurrent(ctx context.Context) *profileIndex { + if injected, ok := profileRequestIndexFromContext(ctx); ok { + return injected + } + if p.mainServer != nil { + if idx := p.mainServer.profileIndexes.Acquire(p.currentConfig); idx != nil { + return idx + } + if auth.IsScopedCaller(ctx) { + return nil + } + return p.mainServer.profileIndexes.For(p.currentConfig()) + } + return p.profileIndexes.For(p.currentConfig()) +} + +// profileIndexFor returns the profile index for an EXPLICIT snapshot cfg — +// the one resolveActiveProfileIn and its callers already hold, never a fresh +// runtime.Config() read (cfg may be older than the live config, e.g. the +// snapshot handleSetProfile admitted a selection against). Over a wired +// runtime it prefers the cache's Published pair — an O(1) pointer-identity +// match against a snapshot the warm path already indexed, never a build — +// and falls back to For only when cfg is not one of the two most recently +// prepared pairs (a snapshot captured further back than the cache retains): +// TestProfileRequests_NeverBuildTheIndexOverARuntime pins that a live +// runtime's own snapshot always hits Published, so this fallback never +// fires there. Unlike profileIndexCurrent it never reads currentConfig() +// itself and never returns nil — cfg is already fixed, so there is nothing +// to (re-)match atomically the way Acquire does. +// +// Without a runtime behind it (bare test construction: p.mainServer == nil) +// it builds fresh on every call rather than consulting p.profileIndexes' +// pointer-identity cache: a bare proxy's cfg is a raw fixture a test may +// mutate IN PLACE between calls (e.g. +// TestReadCache_DeletedPinnedProfileRevokesCachedAccess sets +// proxy.config.Profiles = nil to simulate a deleted pin, never replacing the +// *config.Config pointer) — caching by that same pointer's identity would +// silently keep serving the profile positions of whatever Profiles slice was +// in place the first time this cfg was seen. There is no hot-path cost to +// protect without a runtime behind it. +func (p *MCPProxyServer) profileIndexFor(cfg *config.Config) *profileIndex { + if p.mainServer != nil { + if idx := p.mainServer.profileIndexes.Published(cfg); idx != nil { + return idx + } + return p.mainServer.profileIndexes.For(cfg) + } + return newProfileIndex(cfg) +} diff --git a/internal/server/profile_tool_test.go b/internal/server/profile_tool_test.go index 60c204488..5baea9e9e 100644 --- a/internal/server/profile_tool_test.go +++ b/internal/server/profile_tool_test.go @@ -3,6 +3,8 @@ package server import ( "context" "encoding/json" + "fmt" + "math" "slices" "strings" "testing" @@ -14,6 +16,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" ) // setProfileCtx builds a request context carrying a stable session id and an @@ -72,13 +75,22 @@ func setProfileResultText(t *testing.T, res *mcp.CallToolResult) string { // TestHandleSetProfile_PinnedRejectsOtherSlug verifies a profile-pinned agent // token cannot switch away from its pinned profile via set_profile (Profiles v2 T3). +// The refusal is the uniform scoped "unknown profile" body (Spec 105 PR D +// review round 9, MUST-FIX 2): a pin mismatch is a non-selectable profile +// like any other, decided through the same profileIndex.selectable predicate +// as a deleted or zero-reach profile — never a distinct "pinned to..." +// message, which would let a caller confirm the pin's own name from a +// refusal aimed at a DIFFERENT slug (contracts/refusals.md: `set_profile +// ` is one format string). func TestHandleSetProfile_PinnedRejectsOtherSlug(t *testing.T) { p := newSetProfileTestServer() ctx := setProfileCtx("sess-pinned", "research") res := callSetProfileTool(t, p, ctx, "deploy") require.True(t, res.IsError, "switching a pinned token to another profile must error") - require.Contains(t, setProfileResultText(t, res), "pinned to profile 'research'") + require.Equal(t, "unknown profile 'deploy'", setProfileResultText(t, res)) + require.NotContains(t, setProfileResultText(t, res), "pinned", + "a pin-mismatch refusal must not confirm the caller is pinned or to what") // The session selection must NOT have been changed by the rejected call. require.Equal(t, "", p.sessionStore.GetActiveProfile("sess-pinned")) @@ -129,10 +141,12 @@ func TestHandleSetProfile_DeletedPinDoesNotEnumerateProfiles(t *testing.T) { require.Contains(t, text, "unknown profile 'research'") require.NotContains(t, text, "deploy", "a pinned token must not learn the other profiles' names: %s", text) - // Unpinned callers keep the discovery affordance — proven with an actual - // unpinned AGENT identity (ProfilePin ""), not merely the absence of an - // auth context, which is administrator-shaped and would leave the agent - // contract unproven (cross-review round 2). + // An unpinned AGENT identity (ProfilePin "") is a scoped caller too and + // gets the same list-free refusal (Spec 105 PR D codex round 3: the + // selectable list is fleet-sized work, so no scoped refusal carries it; + // inverted from the pre-105 "unpinned callers keep the discovery + // affordance" assertion). Proven with a real agent identity, not merely + // the absence of an auth context, which is administrator-shaped. unpinnedAgent := auth.WithAuthContext(setProfileCtx("sess-unpinned", ""), &auth.AuthContext{ Type: auth.AuthTypeAgent, AgentName: "unpinned-bot", @@ -141,9 +155,10 @@ func TestHandleSetProfile_DeletedPinDoesNotEnumerateProfiles(t *testing.T) { }) res = callSetProfileTool(t, p, unpinnedAgent, "research") require.True(t, res.IsError) - require.Contains(t, setProfileResultText(t, res), "available: deploy") + require.Equal(t, "unknown profile 'research'", setProfileResultText(t, res)) - // And an administrator-shaped caller (no auth context) likewise. + // An administrator-shaped caller (no auth context) keeps the discovery + // affordance (SC-005). res = callSetProfileTool(t, p, setProfileCtx("sess-admin", ""), "research") require.True(t, res.IsError) require.Contains(t, setProfileResultText(t, res), "available: deploy") @@ -300,8 +315,13 @@ func TestHandleSetProfile_ServerEditionUserScopedLikeVisibility(t *testing.T) { } // TestHandleSetProfile_ScopedTokenUnknownSlugDoesNotEnumerateAllProfiles: the -// invalid-selection error for an agent token names only the profiles the token -// may select — never profiles entirely outside its reach (Spec 104 FR-016b). +// invalid-selection error for an agent token never names a profile outside +// its reach (Spec 104 FR-016b) — and since Spec 105 PR D (codex round 3) it +// names no profile at all: the selectable list is one reach computation per +// configured profile, fleet-sized work a non-disclosing refusal may not do, +// so the pre-105 "profiles overlapping the token's scope stay listed" +// assertion is inverted here. Administrators keep the list (SC-005, +// TestHandleSetProfile_AdminUnknownSlugKeepsAvailableList). func TestHandleSetProfile_ScopedTokenUnknownSlugDoesNotEnumerateAllProfiles(t *testing.T) { p := newSetProfileTestServer() ctx := setProfileScopedCtx("sess-scoped-unknown", "research-srv") @@ -310,8 +330,7 @@ func TestHandleSetProfile_ScopedTokenUnknownSlugDoesNotEnumerateAllProfiles(t *t res := callSetProfileTool(t, p, ctx, "nope") require.True(t, res.IsError) text := setProfileResultText(t, res) - require.Contains(t, text, "unknown profile 'nope'") - require.Contains(t, text, "research", "profiles overlapping the token's scope stay selectable") + require.Equal(t, "unknown profile 'nope'", text, "a scoped refusal names no profile, selectable or not") require.NotContains(t, text, "deploy", "a profile fully outside the token's scope must not be disclosed") require.Equal(t, "mixed", p.sessionStore.GetActiveProfile("sess-scoped-unknown"), "a refused selection must leave the prior session selection untouched") @@ -330,8 +349,7 @@ func TestHandleSetProfile_StalePinUnknownSlugDisclosesNoProfiles(t *testing.T) { res := callSetProfileTool(t, p, ctx, "gone") require.True(t, res.IsError) text := setProfileResultText(t, res) - require.Contains(t, text, "unknown profile 'gone'") - require.NotContains(t, text, "available: gone", "a removed pin is not a selectable profile") + require.Equal(t, "unknown profile 'gone'", text, "a removed pin is refused with the list-free scoped body") for _, name := range []string{"research", "deploy", "mixed"} { require.NotContains(t, text, name) } @@ -340,7 +358,10 @@ func TestHandleSetProfile_StalePinUnknownSlugDisclosesNoProfiles(t *testing.T) { } // TestHandleSetProfile_PinnedTokenClearIntersectsAllowedServers: a pinned token -// whose AllowedServers is narrower than its pin sees the intersection. +// whose AllowedServers is narrower than its pin sees the intersection in +// `servers`, while `active_profile` reports the STORED selection — "" after a +// clear, not the pin (Spec 105 FR-003, FR003-G7; inverted from the pre-105 +// expectation that the pin name was echoed as the active profile). func TestHandleSetProfile_PinnedTokenClearIntersectsAllowedServers(t *testing.T) { p := newSetProfileTestServer() helper := mcpserver.NewMCPServer("test", "1.0.0") @@ -350,50 +371,139 @@ func TestHandleSetProfile_PinnedTokenClearIntersectsAllowedServers(t *testing.T) }) active, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "")) - require.Equal(t, "mixed", active) + require.Equal(t, "", active, "a cleared selection is reported as cleared even under a pin") require.ElementsMatch(t, []string{"deploy-srv"}, servers) } +// newSetProfileOrderingTestServer is the SC-005 byte-parity fixture: five +// servers declared in a non-alphabetical order and a profile that lists a +// subset in ITS OWN order (with one repeated name — a legal, unvalidated +// configuration). The pre-105 payload rendered `match.EffectiveServers(cfg)` +// (profile-declared order, duplicates kept) on select and `allServerNames` +// (config order) on clear; a set-backed rendering (Go map iteration) is +// nondeterministic on ≥3 names and dedupes, so `require.Equal` against these +// exact slices is what an ElementsMatch on a 2-server profile could not catch +// (PR D critique round 1, finding A1). +func newSetProfileOrderingTestServer() *MCPProxyServer { + cfg := &config.Config{ + Servers: []*config.ServerConfig{ + {Name: "zeta-srv"}, + {Name: "alpha-srv"}, + {Name: "mid-srv"}, + {Name: "beta-srv"}, + {Name: "omega-srv"}, + }, + Profiles: []config.ProfileConfig{ + {Name: "wide", Servers: []string{"omega-srv", "alpha-srv", "mid-srv", "alpha-srv", "zeta-srv"}}, + {Name: "narrow", Servers: []string{"mid-srv"}}, + }, + } + return &MCPProxyServer{ + config: cfg, + logger: zap.NewNop(), + sessionStore: NewSessionStore(zap.NewNop()), + } +} + +// Exact pre-105 renderings for the ordering fixture. +var ( + orderingAllServers = []string{"zeta-srv", "alpha-srv", "mid-srv", "beta-srv", "omega-srv"} + orderingWideServers = []string{"omega-srv", "alpha-srv", "mid-srv", "alpha-srv", "zeta-srv"} +) + // TestHandleSetProfile_AdminUnchanged pins the administrator (API-key / socket) -// behaviour: full server list on clear, the profile's complete set on select, -// and every configured profile in the unknown-slug error. +// behaviour byte-for-byte (SC-005): config-ordered full server list on clear, +// the profile's complete set in profile-declared order (duplicates kept) on +// select, and every configured profile in the unknown-slug error. func TestHandleSetProfile_AdminUnchanged(t *testing.T) { - p := newSetProfileTestServer() + p := newSetProfileOrderingTestServer() ctx := setProfileAdminCtx("sess-admin") _, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "")) - require.ElementsMatch(t, []string{"research-srv", "deploy-srv"}, servers) + require.Equal(t, orderingAllServers, servers, "clear must report every server in config order") - active, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "mixed")) - require.Equal(t, "mixed", active) - require.ElementsMatch(t, []string{"research-srv", "deploy-srv"}, servers) + active, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "wide")) + require.Equal(t, "wide", active) + require.Equal(t, orderingWideServers, servers, "select must report the profile's servers in profile-declared order") res := callSetProfileTool(t, p, ctx, "nope") require.True(t, res.IsError) text := setProfileResultText(t, res) - for _, name := range []string{"research", "deploy", "mixed"} { + for _, name := range []string{"wide", "narrow"} { require.Contains(t, text, name) } } +// TestHandleSetProfile_AdminURLScopeUnchanged (SC-005, PR D critique round 1 +// finding A2): FR-003's "URL still governs the reported servers" is an +// agent-token rule. An administrator (or anonymous back-compat caller) on a +// /mcp/p/ endpoint keeps the pre-105 payload — the SELECTED profile's +// servers on select and every server on clear — because SC-005 names no +// FR-003 exception for administrators. +func TestHandleSetProfile_AdminURLScopeUnchanged(t *testing.T) { + p := newSetProfileOrderingTestServer() + helper := mcpserver.NewMCPServer("test", "1.0.0") + ctx := helper.WithContext(context.Background(), &fakeClientSession{id: "sess-admin-url"}) + ctx = auth.WithAuthContext(ctx, auth.AdminContext()) + ctx = profile.WithProfileScope(ctx, p.profileScopeForSlug("narrow")) + + active, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "wide")) + require.Equal(t, "wide", active) + require.Equal(t, "wide", p.sessionStore.GetActiveProfile("sess-admin-url")) + require.Equal(t, orderingWideServers, servers, "an administrator keeps the selected profile's servers on a URL-scoped endpoint") + + active, servers = setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "")) + require.Equal(t, "", active) + require.Equal(t, orderingAllServers, servers, "an administrator keeps the full server list on clear, URL or not") +} + // TestHandleSetProfile_WildcardTokenUnchanged: an agent token with the "*" -// wildcard is unrestricted by servers and keeps the full listings. +// wildcard is unrestricted by servers and keeps the full listings — in the +// same deterministic order an administrator sees (SC-005 control). Its +// REFUSAL body is a scoped caller's (spec "Unrestricted agent tokens": their +// refusal bodies change where this spec changes error shapes): list-free +// since PR D codex round 3, inverted from the pre-105 `available:` form. func TestHandleSetProfile_WildcardTokenUnchanged(t *testing.T) { - p := newSetProfileTestServer() + p := newSetProfileOrderingTestServer() ctx := setProfileScopedCtx("sess-wild", "*") _, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "")) - require.ElementsMatch(t, []string{"research-srv", "deploy-srv"}, servers) + require.Equal(t, orderingAllServers, servers) + + _, servers = setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "wide")) + require.Equal(t, orderingWideServers, servers) res := callSetProfileTool(t, p, ctx, "nope") require.True(t, res.IsError) - require.Contains(t, setProfileResultText(t, res), "deploy") + require.Equal(t, "unknown profile 'nope'", setProfileResultText(t, res)) +} + +// TestHandleSetProfile_ScopedServersKeepProfileOrder: a restricted token's +// effective list is the profile's servers ∩ allowed_servers rendered in the +// profile-declared order every other consumer of EffectiveServers uses — not +// set iteration order — including on a URL-scoped endpoint where the URL +// profile governs. +func TestHandleSetProfile_ScopedServersKeepProfileOrder(t *testing.T) { + p := newSetProfileOrderingTestServer() + ctx := setProfileScopedCtx("sess-scoped-order", "zeta-srv", "mid-srv", "omega-srv", "beta-srv") + + _, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "wide")) + require.Equal(t, []string{"omega-srv", "mid-srv", "zeta-srv"}, servers) + + _, servers = setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "")) + require.Equal(t, []string{"zeta-srv", "mid-srv", "beta-srv", "omega-srv"}, servers, "clear reports allowed servers in config order") + + urlCtx := profile.WithProfileScope(ctx, p.profileScopeForSlug("wide")) + _, servers = setProfileScopedPayload(t, callSetProfileTool(t, p, urlCtx, "narrow")) + require.Equal(t, []string{"omega-srv", "mid-srv", "zeta-srv"}, servers, "the URL profile governs, in its declared order") } -// TestHandleSetProfile_PinnedTokenSelectsDisjointPin locks the admission -// decision: a configured pin is always selectable by its own token (the token -// already knows its pin exists), even when the pin's servers are disjoint from -// the token's AllowedServers — the reach is then correctly empty. +// TestHandleSetProfile_PinnedTokenSelectsDisjointPin is the INVERTED #1225 F2 +// admission decision (Spec 105 FR-003, research D1): a configured pin whose +// servers are disjoint from the token's AllowedServers has zero reach and is +// NOT selectable by its own token — admitting it (with an empty reach) told the +// token that its pin still exists, an existence oracle a deleted pin does not +// give. The refusal is the deleted-pin body and the session is not mutated. func TestHandleSetProfile_PinnedTokenSelectsDisjointPin(t *testing.T) { p := newSetProfileTestServer() helper := mcpserver.NewMCPServer("test", "1.0.0") @@ -402,10 +512,14 @@ func TestHandleSetProfile_PinnedTokenSelectsDisjointPin(t *testing.T) { Type: auth.AuthTypeAgent, ProfilePin: "deploy", AllowedServers: []string{"research-srv"}, }) - active, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "deploy")) - require.Equal(t, "deploy", active) - require.Empty(t, servers) - require.Equal(t, "deploy", p.sessionStore.GetActiveProfile("sess-pin-disjoint")) + res := callSetProfileTool(t, p, ctx, "deploy") + require.True(t, res.IsError, "a zero-reach pin must not be selectable: %s", setProfileResultText(t, res)) + text := setProfileResultText(t, res) + require.Contains(t, text, "unknown profile 'deploy'") + require.NotContains(t, text, "deploy-srv", "the refusal must not name servers outside the token's reach") + require.NotContains(t, text, "pinned", "the refusal must not confirm the pin") + require.Equal(t, "", p.sessionStore.GetActiveProfile("sess-pin-disjoint"), + "a refused selection must leave the session untouched") } // TestSetProfileFixtureIsLoadable guards the shared fixture against slugs that @@ -460,13 +574,10 @@ func TestHandleSetProfile_WildcardTokenRefusesEmptyProfileAdminSelectsIt(t *test strings.ReplaceAll(setProfileResultText(t, unknown), "'nope'", "'"+slug+"'"), setProfileResultText(t, refused), "an empty profile must be refused exactly like a nonexistent one") - // The error echoes the caller's own slug; non-disclosure is about - // the `available:` list, which must name neither empty profile. - _, available, found := strings.Cut(setProfileResultText(t, refused), "available:") - require.True(t, found) - for _, name := range []string{"empty", "ghost"} { - require.NotContains(t, available, name) - } + // The error echoes the caller's own slug and nothing else: a + // scoped refusal carries no `available:` list at all (Spec 105 + // PR D codex round 3; inverted from the list-carrying form). + require.Equal(t, "unknown profile '"+slug+"'", setProfileResultText(t, refused)) require.Equal(t, "research", p.sessionStore.GetActiveProfile("sess-wild-"+slug), "a refused selection must leave the prior session selection untouched") @@ -478,3 +589,638 @@ func TestHandleSetProfile_WildcardTokenRefusesEmptyProfileAdminSelectsIt(t *test }) } } + +// --------------------------------------------------------------------------- +// Spec 105 PR D (FR-003): URL precedence in the set_profile payload (G6) and +// the pinned zero-reach refusal (G8, research D1). +// --------------------------------------------------------------------------- + +// setProfileURLScopedCtx builds the context of a set_profile call arriving on +// /mcp/p/: an unpinned agent token allowed BOTH servers, with the URL +// profile scope the middleware injects for that request. +func setProfileURLScopedCtx(p *MCPProxyServer, sessionID, urlSlug string) context.Context { + ctx := setProfileScopedCtx(sessionID, "research-srv", "deploy-srv") + scope := p.profileScopeForSlug(urlSlug) + if scope == nil { + panic("setProfileURLScopedCtx: fixture profile " + urlSlug + " is not configured") + } + return profile.WithProfileScope(ctx, scope) +} + +// TestHandleSetProfile_URLScopeGovernsReportedServers (FR003-G6): on a +// URL-scoped endpoint the selection is stored, but the URL still governs the +// request (resolveActiveProfile: pin > URL > session). `active_profile` +// therefore reports the stored selection while `servers` reports the +// EFFECTIVE scope — URL profile ∩ token — not the selected profile's servers +// (spec.md "URL precedence"). Selecting the disjoint `deploy` profile on +// /mcp/p/research reports research ∩ token, and clearing the selection on the +// same endpoint reports the same URL scope, never every allowed server. +func TestHandleSetProfile_URLScopeGovernsReportedServers(t *testing.T) { + p := newSetProfileTestServer() + ctx := setProfileURLScopedCtx(p, "sess-url-research", "research") + + active, servers := setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "deploy")) + require.Equal(t, "deploy", active, "active_profile reports the STORED selection") + require.Equal(t, "deploy", p.sessionStore.GetActiveProfile("sess-url-research"), "the selection must still be stored") + require.ElementsMatch(t, []string{"research-srv"}, servers, + "servers must report the URL profile ∩ token, which governs this request — not the selected profile") + + active, servers = setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "")) + require.Equal(t, "", active) + require.Equal(t, "", p.sessionStore.GetActiveProfile("sess-url-research")) + require.ElementsMatch(t, []string{"research-srv"}, servers, + "clearing on a URL-scoped endpoint must still report the URL scope, not all allowed servers") + + // Selecting the URL's own profile is the degenerate case: both agree. + active, servers = setProfileScopedPayload(t, callSetProfileTool(t, p, ctx, "mixed")) + require.Equal(t, "mixed", active) + require.ElementsMatch(t, []string{"research-srv"}, servers, + "mixed ∩ URL research ∩ token = research-srv only") +} + +// TestHandleSetProfile_PinnedZeroReachRefusedLikeDeletedPin (FR003-G8, +// research D1): a pinned token whose pin exists but has zero reach — an empty +// profile, a ghost profile, or a disjoint grant — must not be able to select +// its pin: the refusal is byte-identical (slug-normalised) to the one a +// DELETED pin produces, so the token cannot learn whether its pin still +// exists, and the session is not mutated. Inverts the #1225 F2 admission +// (`TestHandleSetProfile_PinnedTokenSelectsDisjointPin`). +func TestHandleSetProfile_PinnedZeroReachRefusedLikeDeletedPin(t *testing.T) { + cases := []struct { + name string + pin string + allowed []string + }{ + {name: "empty-profile", pin: "empty", allowed: []string{"*"}}, + {name: "ghost-profile", pin: "ghost", allowed: []string{"*"}}, + {name: "disjoint-grant", pin: "deploy", allowed: []string{"research-srv"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := newSetProfileTestServerWithEmptyProfiles(t) + helper := mcpserver.NewMCPServer("test", "1.0.0") + sid := "sess-zero-reach-" + tc.name + ctx := helper.WithContext(context.Background(), &fakeClientSession{id: sid}) + ctx = auth.WithAuthContext(ctx, &auth.AuthContext{ + Type: auth.AuthTypeAgent, ProfilePin: tc.pin, AllowedServers: tc.allowed, + }) + // A prior selection no code path on this request would write, so + // "no mutation" cannot be satisfied by re-storing the same value. + p.sessionStore.SetActiveProfile(sid, "mixed") + + refused := callSetProfileTool(t, p, ctx, tc.pin) + require.True(t, refused.IsError, "a zero-reach pin must not be selectable: %s", setProfileResultText(t, refused)) + require.Equal(t, "mixed", p.sessionStore.GetActiveProfile(sid), + "a refused selection must leave the prior session selection untouched") + + // Oracle: the same token shape with a DELETED pin. + deletedCtx := helper.WithContext(context.Background(), &fakeClientSession{id: sid + "-deleted"}) + deletedCtx = auth.WithAuthContext(deletedCtx, &auth.AuthContext{ + Type: auth.AuthTypeAgent, ProfilePin: "gone", AllowedServers: tc.allowed, + }) + deleted := callSetProfileTool(t, p, deletedCtx, "gone") + require.True(t, deleted.IsError) + require.Equal(t, + strings.ReplaceAll(setProfileResultText(t, deleted), "'gone'", "''"), + strings.ReplaceAll(setProfileResultText(t, refused), "'"+tc.pin+"'", "''"), + "a zero-reach pin must be refused with the deleted-pin body") + }) + } +} + +// TestResolveActiveProfileIn_UsesGivenSnapshot (PR D critique round 1, +// findings S4/N7): handleSetProfile captures ONE config snapshot for the +// admission check and must render its payload from that same snapshot, so a +// hot reload between the two cannot report `active_profile: ""` beside +// a scope (or a stale-drop) computed from a different config. The resolver +// therefore takes the snapshot explicitly: a session selection present in the +// given snapshot resolves from it even when the live config no longer has it, +// and the selection is not dropped as stale. +func TestResolveActiveProfileIn_UsesGivenSnapshot(t *testing.T) { + p := newSetProfileTestServer() + snapshot := p.config + live := &config.Config{Servers: snapshot.Servers} // profiles gone from the live config + p.config = live + + ctx := setProfileScopedCtx("sess-snapshot", "*") + p.sessionStore.SetActiveProfile("sess-snapshot", "mixed") + + name, scope := p.resolveActiveProfileIn(ctx, snapshot) + require.Equal(t, "mixed", name) + require.NotNil(t, scope) + require.ElementsMatch(t, []string{"research-srv", "deploy-srv"}, scope.AllowedServerNames()) + require.Equal(t, "mixed", p.sessionStore.GetActiveProfile("sess-snapshot"), + "a selection the snapshot still knows must not be dropped as stale") + + // The live-config entry point keeps its behaviour: the profile is gone + // there, so the selection is stale and resolution falls through to none. + name, scope = p.resolveActiveProfile(ctx) + require.Equal(t, "", name) + require.Nil(t, scope) + require.Equal(t, "", p.sessionStore.GetActiveProfile("sess-snapshot")) + + // A pinned token resolves its pin from the snapshot too. + pinned := setProfileCtx("sess-snapshot-pin", "research") + name, scope = p.resolveActiveProfileIn(pinned, snapshot) + require.Equal(t, "research", name) + require.Equal(t, []string{"research-srv"}, scope.AllowedServerNames()) +} + +// selectableProbeConfig builds a fleet of n+1 profiles: "pin" (reaching +// "pin-srv") FIRST, followed by n profiles that reach only "other-srv". Placing +// the pin first is the adversarial layout for an early-returning predicate: +// a reachable pin would answer after one iteration while a deleted or +// zero-reach pin would walk the whole slice. +func selectableProbeConfig(n int) *config.Config { + cfg := &config.Config{Servers: []*config.ServerConfig{{Name: "pin-srv"}, {Name: "other-srv"}}} + cfg.Profiles = append(cfg.Profiles, config.ProfileConfig{Name: "pin", Servers: []string{"pin-srv"}}) + for i := 0; i < n; i++ { + cfg.Profiles = append(cfg.Profiles, config.ProfileConfig{Name: fmt.Sprintf("p%d", i), Servers: []string{"other-srv"}}) + } + return cfg +} + +func selectablePinnedCtx(pin string, allowed ...string) context.Context { + return auth.WithAuthContext(context.Background(), &auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: pin, AllowedServers: allowed}) +} + +// TestSelectableProfileNames_PinOutcomesDoSameWork (Spec 105 PR D codex round +// 1, finding 1): profileMiddleware answers every scoped refusal through one +// constructor so status and body cannot tell "pin exists but the URL names +// another slug" from "pin deleted" or "pin has zero reach" — but the predicate +// it consults must not tell them apart by the WORK it does either (spec +// Definitions: non-disclosing = status, body AND timing class). An early +// return on the first reachable pin made a pinned caller's refusal cost one +// iteration when the pin was alive and a full slice walk when it was gone. +// +// The oracle here is deterministic, not wall-clock: the allocation profile of +// one predicate call is identical for every pin outcome over the same fleet +// (the early-returning version allocated 2 / 0 / 1 times respectively). +func TestSelectableProfileNames_PinOutcomesDoSameWork(t *testing.T) { + const n = 64 + alive := selectableProbeConfig(n) + deleted := selectableProbeConfig(n) + deleted.Profiles[0].Name = "was-the-pin" // same fleet size, the pin is gone + + cases := map[string]struct { + ctx context.Context + cfg *config.Config + }{ + "reachable pin first": {selectablePinnedCtx("pin", "pin-srv"), alive}, + "zero-reach pin first": {selectablePinnedCtx("pin", "other-srv"), alive}, + "deleted pin": {selectablePinnedCtx("pin", "pin-srv"), deleted}, + } + // AllocsPerRun counts process-wide mallocs, so a goroutine still winding + // down from an earlier test (a runtime fixture's shutdown, an index + // observer) inflates whichever case it overlaps — CI once read 48 for + // one case and 12 for the others. Noise only ever ADDS, so the minimum + // over a few samples of the predicate alone (index built outside the + // window) is the deterministic figure this test is about. + allocs := map[string]float64{} + for name, c := range cases { + idx := newProfileIndex(c.cfg) + best := math.Inf(1) + for i := 0; i < 7; i++ { + best = math.Min(best, testing.AllocsPerRun(50, func() { idx.selectableNames(c.ctx) })) + } + allocs[name] = best + } + for name, got := range allocs { + require.Equal(t, allocs["reachable pin first"], got, "%s must allocate exactly like a reachable pin: %v", name, allocs) + } +} + +// TestForEachProfileSelectable_VisitsEveryProfileRegardlessOfOutcome pins the +// structural guarantee behind the allocation parity above: the predicate +// visits every configured profile, in order, for every caller kind and every +// pin outcome — never one iteration for a live pin and the whole slice for a +// dead one. A traversal counter, not a clock. +func TestForEachProfileSelectable_VisitsEveryProfileRegardlessOfOutcome(t *testing.T) { + const n = 8 + cfg := selectableProbeConfig(n) // "pin" first, then p0..p7 reaching other-srv + want := make([]string, 0, n+1) + for i := range cfg.Profiles { + want = append(want, cfg.Profiles[i].Name) + } + + cases := map[string]struct { + ctx context.Context + selectable []string + }{ + "admin": {auth.WithAuthContext(context.Background(), auth.AdminContext()), want}, + "absent context": {context.Background(), want}, + "scoped, pin-srv only": {setProfileScopedCtx("s", "pin-srv"), []string{"pin"}}, + "scoped, empty allowlist": {setProfileScopedCtx("s"), []string{}}, + "reachable pin first": {selectablePinnedCtx("pin", "pin-srv"), []string{"pin"}}, + "zero-reach pin first": {selectablePinnedCtx("pin", "other-srv"), []string{}}, + "deleted pin": {selectablePinnedCtx("gone", "pin-srv", "other-srv"), []string{}}, + "wildcard pin on last one": {selectablePinnedCtx("p7", "*"), []string{"p7"}}, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + visited := make([]string, 0, n+1) + picked := []string{} + forEachProfileSelectable(c.ctx, cfg, func(profileName string, selectable bool) { + visited = append(visited, profileName) + if selectable { + picked = append(picked, profileName) + } + }) + require.Equal(t, want, visited, "every profile must be visited exactly once, in configured order") + require.Equal(t, c.selectable, picked) + require.Equal(t, c.selectable, selectableProfileNames(c.ctx, cfg)) + }) + } + + // A nil config visits nothing (and selectableProfileNames stays nil). + forEachProfileSelectable(context.Background(), nil, func(string, bool) { t.Fatal("visited a profile of a nil config") }) + require.Nil(t, selectableProfileNames(context.Background(), nil)) +} + +// TestProfileIndex_SelectableAllocatesNothing (Spec 105 PR D codex round 2, +// finding 1): the URL gate's predicate does constant, allocation-free work — +// zero allocations for every refusal and admission branch, over a fleet of +// one profile and of 4 097 — so a scoped caller's refusal cannot reveal how +// many other profiles exist. Pure function, so exact and retry-free. +func TestProfileIndex_SelectableAllocatesNothing(t *testing.T) { + fleets := map[string]*profileIndex{ + "no profiles": newProfileIndex(&config.Config{Servers: []*config.ServerConfig{{Name: "pin-srv"}, {Name: "other-srv"}}}), + "pin only": newProfileIndex(selectableProbeConfig(0)), + "4096 others": newProfileIndex(selectableProbeConfig(4096)), + "nil config": newProfileIndex(nil), + } + cases := map[string]struct { + ctx context.Context + slug string + }{ + "pin mismatch, absent slug": {selectablePinnedCtx("pin", "pin-srv"), "nope"}, + "pin mismatch, existing slug": {selectablePinnedCtx("pin", "pin-srv"), "p0"}, + "deleted pin": {selectablePinnedCtx("gone", "pin-srv"), "gone"}, + "zero-reach pin": {selectablePinnedCtx("pin", "other-srv"), "pin"}, + "reachable pin": {selectablePinnedCtx("pin", "pin-srv"), "pin"}, + "scoped, absent slug": {setProfileScopedCtx("s", "pin-srv"), "nope"}, + "scoped, disjoint slug": {setProfileScopedCtx("s", "pin-srv"), "p0"}, + "scoped, reachable slug": {setProfileScopedCtx("s", "pin-srv"), "pin"}, + "scoped, empty allowlist": {setProfileScopedCtx("s"), "pin"}, + "admin": {auth.WithAuthContext(context.Background(), auth.AdminContext()), "p0"}, + "absent context": {context.Background(), "nope"}, + "empty slug": {selectablePinnedCtx("pin", "pin-srv"), ""}, + } + for fleet, idx := range fleets { + for name, c := range cases { + allocs := testing.AllocsPerRun(50, func() { idx.selectable(c.ctx, c.slug) }) + require.Zero(t, allocs, "%s over fleet %q must not allocate", name, fleet) + } + } +} + +// TestProfileIndex_SelectableMatchesSelectableProfileNames pins the O(1) +// predicate to the list predicate it replaces on the URL gate: for every +// caller kind and every slug (configured, absent, empty, the pin, an empty +// and a ghost profile), profileIndex.selectable answers exactly +// "slug ∈ selectableProfileNames" — the two are one rule, so the gate and +// set_profile can never disagree on admission. (Duplicate slugs are outside +// the contract: ValidateProfiles refuses to load them; the index resolves the +// first occurrence like every other lookup in this package.) +func TestProfileIndex_SelectableMatchesSelectableProfileNames(t *testing.T) { + cfg := selectableProbeConfig(4) // pin, p0..p3 + cfg.Profiles = append(cfg.Profiles, + config.ProfileConfig{Name: "empty"}, + config.ProfileConfig{Name: "ghost", Servers: []string{"missing-srv"}}, + config.ProfileConfig{Name: "both", Servers: []string{"pin-srv", "other-srv"}}, + ) + _, err := config.ValidateProfiles(cfg) + require.NoError(t, err, "fixture must be a loadable profile set") + idx := newProfileIndex(cfg) + + callers := map[string]context.Context{ + "admin": auth.WithAuthContext(context.Background(), auth.AdminContext()), + "absent context": context.Background(), + "scoped, pin-srv": setProfileScopedCtx("s", "pin-srv"), + "scoped, other-srv": setProfileScopedCtx("s", "other-srv"), + "scoped, wildcard": setProfileScopedCtx("s", "*"), + "scoped, empty allowlist": setProfileScopedCtx("s"), + "reachable pin": selectablePinnedCtx("pin", "pin-srv"), + "zero-reach pin": selectablePinnedCtx("pin", "other-srv"), + "deleted pin": selectablePinnedCtx("gone", "*"), + "pinned to empty": selectablePinnedCtx("empty", "*"), + "pinned to ghost": selectablePinnedCtx("ghost", "*"), + } + slugs := []string{"pin", "p0", "p1", "p3", "empty", "ghost", "both", "nope", "", "gone", "missing-srv"} + for caller, ctx := range callers { + list := selectableProfileNames(ctx, cfg) + for _, slug := range slugs { + require.Equal(t, slices.Contains(list, slug), idx.selectable(ctx, slug), "%s asking for %q (list: %v)", caller, slug, list) + } + } + + // A nil snapshot selects nothing for anyone. + nilIdx := newProfileIndex(nil) + for caller, ctx := range callers { + require.False(t, nilIdx.selectable(ctx, "pin"), "%s over a nil config", caller) + } + require.Nil(t, nilIdx.lookup("pin")) +} + +// TestProfileIndexCache_BuiltOncePerSnapshot: the cache keys on the snapshot +// pointer — the same *Config hands back the same index, a new snapshot (a +// reload always publishes a new pointer) rebuilds it, and the index reflects +// the snapshot it was built from. +func TestProfileIndexCache_BuiltOncePerSnapshot(t *testing.T) { + var cache profileIndexCache + first := selectableProbeConfig(2) + idx := cache.For(first) + require.Same(t, idx, cache.For(first)) + require.Equal(t, &first.Profiles[0], idx.lookup("pin")) + require.Equal(t, &first.Profiles[2], idx.lookup("p1")) + require.Nil(t, idx.lookup("p2")) + + second := selectableProbeConfig(3) + next := cache.For(second) + require.NotSame(t, idx, next, "a new snapshot must rebuild the index") + require.Equal(t, &second.Profiles[3], next.lookup("p2")) + require.Same(t, next, cache.For(second)) + + require.Nil(t, cache.For(nil).lookup("pin"), "a nil snapshot yields an empty index") +} + +// --------------------------------------------------------------------------- +// Spec 105 PR D codex round 3. +// --------------------------------------------------------------------------- + +// setProfilePinnedCtx builds a session-bearing request context for an agent +// token pinned to pin with the given AllowedServers. +func setProfilePinnedCtx(sessionID, pin string, allowed ...string) context.Context { + helper := mcpserver.NewMCPServer("test", "1.0.0") + ctx := helper.WithContext(context.Background(), &fakeClientSession{id: sessionID}) + return auth.WithAuthContext(ctx, &auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: pin, AllowedServers: allowed}) +} + +// TestProfileIndex_ReachIsPrecomputedAtBuild (Spec 105 PR D codex round 3, +// finding 1): the reach behind the selectable predicate must be fixed when +// the index is built, never derived from the candidate profile's declared +// list at request time. A reach that scanned the declared list for every +// configured server cost nothing for a profile the snapshot lacks (nil list) +// and |servers| × |declared| for one it has — so a pinned token asking for +// its own zero-reach pin could tell "pin deleted" from "pin exists" by the +// work its uniform refusal cost (9.3 ms vs 3.2 µs over 4 096 servers on the +// tree before this fix), which research D1 forbids. +// +// The witness is the mechanism, not a clock: once the index is built, the +// declared list is not consulted any more — emptying it changes nothing for +// the built index and everything for a fresh one. +func TestProfileIndex_ReachIsPrecomputedAtBuild(t *testing.T) { + const n = 4096 + cfg := &config.Config{} + declared := make([]string, 0, n) + for i := 0; i < n; i++ { + name := fmt.Sprintf("srv%d", i) + cfg.Servers = append(cfg.Servers, &config.ServerConfig{Name: name}) + declared = append(declared, name) + } + cfg.Profiles = []config.ProfileConfig{{Name: "pin", Servers: declared}} + idx := newProfileIndex(cfg) + + reachable := selectablePinnedCtx("pin", "srv4095") + zeroReach := selectablePinnedCtx("pin", "nowhere") + require.True(t, idx.selectable(reachable, "pin")) + require.False(t, idx.selectable(zeroReach, "pin")) + + cfg.Profiles[0].Servers = nil + require.True(t, idx.selectable(reachable, "pin"), + "reach must be read from the index built at snapshot time, not from the declared list walked per request") + require.False(t, newProfileIndex(cfg).selectable(reachable, "pin"), + "sanity: a fresh index over the emptied profile has no reach") +} + +// TestHandleSetProfile_ScopedRefusalTouchesOnlySlugAndPin (Spec 105 PR D +// codex round 3, finding 2): a scoped caller's set_profile refusal must not +// enumerate the fleet. Deciding the requested slug through the whole +// selectable list — one reach computation per configured profile — cost +// 0.3 µs over a one-profile fleet and 63 µs over 4 097 on the tree before +// this fix, with a byte-identical body: a fleet-population timing oracle +// (spec Definitions: non-disclosing = status, body AND timing class). The +// refusal now decides the requested slug alone through the per-snapshot +// index and carries no `available:` list for scoped callers (the list is +// fleet-sized work by definition; administrators keep it, SC-005). +// +// Traversal-counter seam: through the index's lookup hook, every scoped +// refusal over every fleet resolves at most the requested slug and the +// caller's pin — never a third profile — and the body is the one format +// string, so neither work nor bytes depend on the hidden fleet. +func TestHandleSetProfile_ScopedRefusalTouchesOnlySlugAndPin(t *testing.T) { + cases := map[string]struct { + ctx context.Context + slug string + }{ + "deleted pin": {setProfilePinnedCtx("s", "gone", "pin-srv"), "gone"}, + "zero-reach pin": {setProfilePinnedCtx("s", "pin", "other-srv"), "pin"}, + "scoped, absent slug": {setProfileScopedCtx("s", "pin-srv"), "nope"}, + "scoped, disjoint slug": {setProfileScopedCtx("s", "pin-srv"), "p0"}, + "scoped, empty allowlist": {setProfileScopedCtx("s"), "pin"}, + "scoped wildcard, absent slug": {setProfileScopedCtx("s", "*"), "nope"}, + // A pin mismatch is a non-selectable profile like any other (Spec + // 105 PR D review round 9, MUST-FIX 2): handleSetProfile no longer + // short-circuits it before the index, so it costs — and reads — the + // same as every other refusal in this table. + "pin mismatch": {setProfilePinnedCtx("s", "pin", "pin-srv"), "p0"}, + } + for fleet, n := range map[string]int{"pin only": 0, "4096 others": 4096} { + cfg := selectableProbeConfig(n) + p := &MCPProxyServer{config: cfg, logger: zap.NewNop(), sessionStore: NewSessionStore(zap.NewNop())} + var touched []string + idx := newProfileIndex(cfg) + idx.lookupHook = func(slug string) { touched = append(touched, slug) } + p.profileIndexes.warm.Store(idx) + + for name, c := range cases { + touched = nil + p.sessionStore.SetActiveProfile("s", "prior") + res := callSetProfileTool(t, p, c.ctx, c.slug) + require.True(t, res.IsError, "%s/%s must be refused", fleet, name) + require.Equal(t, "prior", p.sessionStore.GetActiveProfile("s"), "%s/%s: a refusal must not mutate the session", fleet, name) + + pin := profilePinFromContext(c.ctx) + allowed := map[string]bool{c.slug: true} + if pin != "" { + allowed[pin] = true + } + require.Equal(t, fmt.Sprintf("unknown profile '%s'", c.slug), setProfileResultText(t, res), + "%s/%s: a scoped refusal carries no available list, and a pin mismatch carries no distinct wording either", fleet, name) + require.NotEmpty(t, touched, "%s/%s: the refusal must decide through the index", fleet, name) + require.LessOrEqual(t, len(touched), 2, "%s/%s: at most the slug and the pin: %v", fleet, name, touched) + for _, got := range touched { + require.True(t, allowed[got], "%s/%s: touched profile %q outside {slug, pin}: %v", fleet, name, got, touched) + } + } + require.Same(t, idx, p.profileIndexCurrent(context.Background()), "%s: the cached index must be reused for the same snapshot", fleet) + } +} + +// TestHandleSetProfile_AdminUnknownSlugKeepsAvailableList is the SC-005 +// control for the refusal above: an administrator's unknown-slug error keeps +// the pre-105 discovery affordance byte-for-byte — every configured profile, +// in configured order, empty and ghost ones included. +func TestHandleSetProfile_AdminUnknownSlugKeepsAvailableList(t *testing.T) { + p := newSetProfileTestServerWithEmptyProfiles(t) + p.sessionStore.SetActiveProfile("sess-admin-unknown", "research") + + res := callSetProfileTool(t, p, setProfileAdminCtx("sess-admin-unknown"), "nope") + require.True(t, res.IsError) + require.Equal(t, "unknown profile 'nope' (available: research, deploy, mixed, empty, ghost)", setProfileResultText(t, res)) + require.Equal(t, "research", p.sessionStore.GetActiveProfile("sess-admin-unknown")) + + // Anonymous back-compat callers are administrator-shaped and keep it too. + res = callSetProfileTool(t, p, setProfileCtx("sess-anon-unknown", ""), "nope") + require.True(t, res.IsError) + require.Equal(t, "unknown profile 'nope' (available: research, deploy, mixed, empty, ghost)", setProfileResultText(t, res)) +} + +// --------------------------------------------------------------------------- +// Spec 105 PR D codex round 4. +// --------------------------------------------------------------------------- + +// selectableProbeConfigWithHiddenServers is selectableProbeConfig(n) over a +// fleet with hidden further configured servers that no profile declares and +// no test token is granted — the population a scoped caller must not be able +// to measure. +func selectableProbeConfigWithHiddenServers(n, hidden int) *config.Config { + cfg := selectableProbeConfig(n) + for i := 0; i < hidden; i++ { + cfg.Servers = append(cfg.Servers, &config.ServerConfig{Name: fmt.Sprintf("hidden%d", i)}) + } + return cfg +} + +// TestProfileIndex_ReachCostsTheGrantNotTheFleet (Spec 105 PR D codex round +// 4, finding 1): the reach test is O(|reader grant|), never O(|fleet|). A +// reach that walked every configured server and ran the credential check on +// each cost 36 ns over one server and 25 µs over 4 096 for the same refusal +// (zero allocations either way, so the allocation guards were blind): the +// number of servers the operator runs was a timing oracle (FR-004; spec +// Definitions: timing class). +// +// Now a restricted reader performs exactly one membership test per entry of +// its own allowed_servers against the candidate's precomputed set, and a +// wildcard (or administrator) reader performs exactly one — "is the set +// non-empty" — so the count depends on nothing but the reader's own grant: +// not on the fleet, not on the candidate (present, absent, empty), not on +// the outcome. The traversal counter is the witness, not a clock; the +// outcome column pins that the cheaper rule is still the same rule. +func TestProfileIndex_ReachCostsTheGrantNotTheFleet(t *testing.T) { + fleets := map[string]*profileIndex{ + "2 servers": newProfileIndex(selectableProbeConfigWithHiddenServers(0, 0)), + "4096 hidden servers": newProfileIndex(selectableProbeConfigWithHiddenServers(0, 4096)), + } + cases := map[string]struct { + ctx context.Context + slug string + steps int + reach bool + }{ + "restricted, reachable": {setProfileScopedCtx("s", "pin-srv"), "pin", 1, true}, + "restricted, disjoint": {setProfileScopedCtx("s", "other-srv"), "pin", 1, false}, + "restricted, absent slug": {setProfileScopedCtx("s", "pin-srv"), "nope", 1, false}, + "restricted, three-name grant": {setProfileScopedCtx("s", "nowhere", "pin-srv", "hidden7"), "pin", 3, true}, + "restricted, unknown names only": {setProfileScopedCtx("s", "nowhere", "hidden7"), "pin", 2, false}, + "restricted, empty allowlist": {setProfileScopedCtx("s"), "pin", 0, false}, + "wildcard, reachable": {setProfileScopedCtx("s", "*"), "pin", 1, true}, + "wildcard, absent slug": {setProfileScopedCtx("s", "*"), "nope", 1, false}, + "wildcard among names": {setProfileScopedCtx("s", "nowhere", "*"), "pin", 2, true}, + "pinned, reachable": {selectablePinnedCtx("pin", "pin-srv"), "pin", 1, true}, + "pinned, zero reach": {selectablePinnedCtx("pin", "other-srv"), "pin", 1, false}, + "pinned, deleted pin": {selectablePinnedCtx("gone", "pin-srv", "other-srv"), "gone", 2, false}, + "pinned, mismatch": {selectablePinnedCtx("pin", "pin-srv"), "p0", 1, false}, + "restricted, empty-name grant": {setProfileScopedCtx("s", ""), "pin", 1, false}, + "restricted, duplicate grant name": {setProfileScopedCtx("s", "pin-srv", "pin-srv"), "pin", 2, true}, + } + for fleet, idx := range fleets { + for name, c := range cases { + steps := 0 + idx.reachHook = func() { steps++ } + require.Equal(t, c.reach, idx.selectable(c.ctx, c.slug), "%s over %s: outcome", name, fleet) + require.Equal(t, c.steps, steps, "%s over %s: reach must cost one membership test per granted server", name, fleet) + } + } + // A profile that declares every hidden server is still reached only + // through the reader's own grant: one test per granted name. + wide := selectableProbeConfigWithHiddenServers(0, 4096) + declared := make([]string, 0, len(wide.Servers)) + for _, s := range wide.Servers { + declared = append(declared, s.Name) + } + wide.Profiles = append(wide.Profiles, config.ProfileConfig{Name: "wide", Servers: declared}) + idx := newProfileIndex(wide) + steps := 0 + idx.reachHook = func() { steps++ } + require.True(t, idx.selectable(setProfileScopedCtx("s", "hidden4095"), "wide")) + require.Equal(t, 1, steps, "a 4 098-server candidate costs one test for a one-server grant") +} + +// TestHandleSetProfile_ScopedRefusalReachCostsTheGrantNotTheFleet is the +// set_profile leg of the round-4 fix: the scoped unknown-profile refusal +// shares profileIndex.reach with the URL gate, so its cost is bounded by the +// token's own allowed_servers over a two-server and a 4 098-server fleet +// alike, for every refusal branch that reaches the index. +func TestHandleSetProfile_ScopedRefusalReachCostsTheGrantNotTheFleet(t *testing.T) { + cases := map[string]struct { + ctx context.Context + slug string + }{ + "deleted pin": {setProfilePinnedCtx("s", "gone", "pin-srv"), "gone"}, + "zero-reach pin": {setProfilePinnedCtx("s", "pin", "other-srv"), "pin"}, + "scoped, absent slug": {setProfileScopedCtx("s", "pin-srv"), "nope"}, + "scoped, disjoint slug": {setProfileScopedCtx("s", "pin-srv", "nowhere"), "p0"}, + "scoped, empty allowlist": {setProfileScopedCtx("s"), "pin"}, + "scoped wildcard, absent slug": {setProfileScopedCtx("s", "*"), "nope"}, + // Sibling of the round-9 fix: a pin mismatch now reaches the index + // too, so it must cost exactly what every other refusal here costs. + "pin mismatch": {setProfilePinnedCtx("s", "pin", "pin-srv"), "p0"}, + } + for name, c := range cases { + steps := map[string]int{} + for fleet, hidden := range map[string]int{"2 servers": 0, "4096 hidden servers": 4096} { + cfg := selectableProbeConfigWithHiddenServers(1, hidden) + p := &MCPProxyServer{config: cfg, logger: zap.NewNop(), sessionStore: NewSessionStore(zap.NewNop())} + idx := newProfileIndex(cfg) + idx.reachHook = func() { steps[fleet]++ } + p.profileIndexes.warm.Store(idx) + + res := callSetProfileTool(t, p, c.ctx, c.slug) + require.True(t, res.IsError, "%s/%s must be refused", fleet, name) + require.Equal(t, fmt.Sprintf("unknown profile '%s'", c.slug), setProfileResultText(t, res), "%s/%s", fleet, name) + require.Equal(t, len(auth.AuthContextFromContext(c.ctx).AllowedServers), steps[fleet], + "%s/%s: reach must cost one membership test per granted server: %v", fleet, name, steps) + } + require.Equal(t, steps["2 servers"], steps["4096 hidden servers"], "%s: %v", name, steps) + } +} + +// TestProfileIndex_EffectiveServersForRestrictedGrant_DuplicatesFollowDeclaredOnly +// (cross-model review round 2, PR D): declaredOccurrences lets a restricted +// grant reproduce "profile-declared order, duplicates kept" in O(len(allowed)) +// instead of walking the profile's full declared list — but round 2 caught +// that the first version of this multiplied a NAME's occurrences by both the +// number of times IT appears in the profile's declared list AND the number +// of times it appears in the caller's own (unvalidated, possibly repeating) +// AllowedServers. The pre-fix grant-map-based walk implicitly deduped +// `allowed` (a Go map's keys), so declared's own duplicate count alone drove +// the output; this pins that exact contract: a repeated grant entry must not +// re-expand the same occurrences again. +func TestProfileIndex_EffectiveServersForRestrictedGrant_DuplicatesFollowDeclaredOnly(t *testing.T) { + cfg := &config.Config{ + Servers: []*config.ServerConfig{{Name: "a-srv"}, {Name: "b-srv"}}, + Profiles: []config.ProfileConfig{ + {Name: "dup", Servers: []string{"a-srv", "b-srv", "a-srv"}}, + }, + } + idx := newProfileIndex(cfg) + + // declared has "a-srv" twice; a NON-repeating grant must still report it + // twice (duplicates kept, per the documented contract). + require.Equal(t, []string{"a-srv", "a-srv"}, idx.EffectiveServersFor("dup", []string{"a-srv"})) + + // A REPEATING grant for the same name must not multiply the output any + // further: still exactly declared's own two occurrences, not four. + require.Equal(t, []string{"a-srv", "a-srv"}, idx.EffectiveServersFor("dup", []string{"a-srv", "a-srv", "a-srv"})) +} diff --git a/internal/server/profile_url_gate_test.go b/internal/server/profile_url_gate_test.go new file mode 100644 index 000000000..54d20f550 --- /dev/null +++ b/internal/server/profile_url_gate_test.go @@ -0,0 +1,959 @@ +package server + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "path/filepath" + "slices" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" +) + +// newProfileGateTestServer builds a Server whose logger is observed, with two +// configured servers and two single-server profiles, so profileMiddleware +// can be driven directly with a hand-built AuthContext (auth already ran). +func newProfileGateTestServer(t *testing.T) (*Server, *observer.ObservedLogs) { + t.Helper() + + core, logs := observer.New(zap.DebugLevel) + cfg := config.DefaultConfig() + cfg.DataDir = t.TempDir() + cfg.Listen = "127.0.0.1:0" + cfg.Servers = []*config.ServerConfig{{Name: "research-srv"}, {Name: "deploy-srv"}} + cfg.Profiles = []config.ProfileConfig{ + {Name: "research", Servers: []string{"research-srv"}}, + {Name: "deploy", Servers: []string{"deploy-srv"}}, + } + + srv, err := NewServer(cfg, zap.New(core)) + require.NoError(t, err) + t.Cleanup(func() { _ = srv.Shutdown() }) + // Background initialization writes the search index under DataDir; let it + // finish before the test body runs, or its writes race the TempDir + // cleanup (a "directory not empty" / open-handle failure on CI). + require.Eventually(t, func() bool { + return srv.runtime.CurrentPhase() == runtime.PhaseReady + }, 10*time.Second, 10*time.Millisecond, "runtime never reached PhaseReady") + return srv, logs +} + +// TestProfileMiddleware_ScopedRefusalIsLoggedForOperator (Spec 105 PR D +// critique round 1, finding S1): the uniform FR-004 refusal is deliberately +// silent towards the AGENT, but it must not be silent towards the OPERATOR. +// The gate answers before the request reaches the logging handler mounted +// inside it, so without its own log line a scoped token walking the slug +// space of /mcp/p/ leaves no trace at all. One structured line per refusal, +// naming the agent, the slug it asked for and where it came from — and none +// on admission. +func TestProfileMiddleware_ScopedRefusalIsLoggedForOperator(t *testing.T) { + srv, logs := newProfileGateTestServer(t) + + reached := false + handler := srv.profileMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + reached = true + })) + agent := &auth.AuthContext{Type: auth.AuthTypeAgent, AgentName: "a-only", AllowedServers: []string{"research-srv"}} + + req := httptest.NewRequest(http.MethodPost, "/mcp/p/deploy", http.NoBody) + req.RemoteAddr = "203.0.113.7:4242" + req = req.WithContext(auth.WithAuthContext(req.Context(), agent)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.Equal(t, http.StatusNotFound, rec.Code) + require.False(t, reached, "a non-selectable slug must not reach the MCP handler") + + refusals := logs.FilterMessage("profile URL refused for scoped caller").All() + require.Len(t, refusals, 1, "exactly one operator-facing line per refusal") + fields := refusals[0].ContextMap() + require.Equal(t, "a-only", fields["agent_name"]) + require.Equal(t, "deploy", fields["profile"]) + require.Equal(t, "203.0.113.7:4242", fields["remote_addr"]) + + // Admission through a selectable profile is not a refusal and logs none. + logs.TakeAll() + req = httptest.NewRequest(http.MethodPost, "/mcp/p/research", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), agent)) + rec = httptest.NewRecorder() + handler.ServeHTTP(rec, req) + require.True(t, reached, "the selectable profile must be admitted") + require.Empty(t, logs.FilterMessage("profile URL refused for scoped caller").All()) +} + +// profileGateFleetConfig builds a config over a fleet of 1+n profiles: "pin" +// (reaching "pin-srv") followed by n profiles "p0".."p" that reach only +// "other-srv". With n == -1 the fleet has no profiles at all. hidden further +// servers "hidden0".."hidden" are configured but declared by no +// profile and granted to no test token — the server population an operator +// runs and a scoped token must not be able to measure (codex round 4). +func profileGateFleetConfig(n, hidden int) *config.Config { + cfg := &config.Config{Servers: []*config.ServerConfig{{Name: "pin-srv"}, {Name: "other-srv"}}} + for i := 0; i < hidden; i++ { + cfg.Servers = append(cfg.Servers, &config.ServerConfig{Name: fmt.Sprintf("hidden%d", i)}) + } + if n >= 0 { + cfg.Profiles = []config.ProfileConfig{{Name: "pin", Servers: []string{"pin-srv"}}} + for i := 0; i < n; i++ { + cfg.Profiles = append(cfg.Profiles, config.ProfileConfig{Name: fmt.Sprintf("p%d", i), Servers: []string{"other-srv"}}) + } + } + return cfg +} + +// profileGateFleet is one fleet shape driven through serveProfileURL — the +// whole gate after the (index, snapshot) pair is taken — on a bare Server. +// No runtime stands behind it on purpose: a live runtime over thousands of +// profiles spends the test building per-profile indexes in the background, +// which both inflates allocation readings and races the TempDir cleanup. The +// pair comes from the bare Server's cache (For: the warm slot when a test +// stored an instrumented index there, else the lazily built one). +type profileGateFleet struct { + srv *Server + cfg *config.Config +} + +func (f profileGateFleet) handler(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f.srv.serveProfileURL(w, r, f.srv.profileIndexes.For(f.cfg), next) + }) +} + +// profileGateFleets builds the fleet shapes the gate tests replay: the +// profile population (none / the pin alone / 4 096 hidden profiles) and the +// server population (two servers / 4 096 hidden servers behind the same two). +func profileGateFleets() map[string]profileGateFleet { + fleets := map[string]profileGateFleet{} + for name, shape := range map[string][2]int{ + "no profiles": {-1, 0}, + "pin only": {0, 0}, + "4096 others": {4096, 0}, + "4096 hidden servers": {0, 4096}, + } { + fleets[name] = profileGateFleet{srv: &Server{logger: zap.NewNop()}, cfg: profileGateFleetConfig(shape[0], shape[1])} + } + return fleets +} + +// profileGateRefusalCases enumerates every scoped refusal branch of the +// /mcp/p/ gate. Each one is a refusal in EVERY fleet shape the tests +// below build (p0 is absent in a one-profile fleet, present but disjoint or +// pin-mismatched in a larger one), so the same table can be replayed against +// fleets of different population and the results compared. +var profileGateRefusalCases = map[string]struct { + agent *auth.AuthContext + path string +}{ + "pin mismatch, absent slug": {&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "pin", AllowedServers: []string{"pin-srv"}}, "/mcp/p/nope"}, + "pin mismatch, existing slug": {&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "pin", AllowedServers: []string{"pin-srv"}}, "/mcp/p/p0"}, + "deleted pin": {&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "gone", AllowedServers: []string{"pin-srv"}}, "/mcp/p/gone"}, + "zero-reach pin": {&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "pin", AllowedServers: []string{"other-srv"}}, "/mcp/p/pin"}, + "scoped, absent slug": {&auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"pin-srv"}}, "/mcp/p/nope"}, + "scoped, disjoint slug": {&auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"pin-srv"}}, "/mcp/p/p0"}, + "scoped, empty allowlist": {&auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{}}, "/mcp/p/pin"}, + "pinned, slug-less /mcp/p": {&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "pin", AllowedServers: []string{"pin-srv"}}, "/mcp/p"}, + "scoped wildcard, absent slug": {&auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"*"}}, "/mcp/p/nope"}, +} + +// profileGateRefusal drives one request through the gate and returns the +// recorder, asserting the uniform refusal shape. +func profileGateRefusal(t *testing.T, handler http.Handler, agent *auth.AuthContext, path string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodPost, path, http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), agent)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + require.Equal(t, http.StatusNotFound, rec.Code, "%s must be refused", path) + return rec +} + +// TestProfileMiddleware_RefusalWorkIndependentOfFleet (Spec 105 PR D codex +// round 2, finding 1): the uniform refusal must not cost work proportional to +// the number of OTHER profiles the operator has configured. A gate that +// computed the whole selectable-profile list before refusing did zero +// iterations over an empty fleet and one EffectiveServers per profile over a +// populated one — same status and body, fleet-sized difference in work, so a +// scoped token could learn whether hidden profiles exist from how long its +// own refusal took (FR-004; spec Definitions: non-disclosing = status, body +// AND timing class). +// +// The witness is deterministic, not wall-clock: the allocation profile of one +// refusal is identical over a fleet with no profiles, one profile and 4 097 +// profiles, for every refusal branch. (HEAD before the fix: 31 allocations +// over the empty fleet, ~4 129 over the large one — 1.7 µs vs 0.9 ms at +// 10 000 profiles.) AllocsPerRun counts every goroutine's mallocs and the +// package's other tests may leave background work behind, so a reading is +// retried into a quiet window — noise only ever adds, and a fleet- +// proportional gate is off by thousands, so it can never pass. The pure +// predicate is pinned at zero allocations without any retry in +// TestProfileIndex_SelectableAllocatesNothing. +func TestProfileMiddleware_RefusalWorkIndependentOfFleet(t *testing.T) { + fleets := profileGateFleets() + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + t.Errorf("%s must not reach the MCP handler", r.URL.Path) + }) + // Build every index up front: on a bare Server the first request pays the + // one-off lazy build, which is not part of a refusal's cost (over a + // runtime the warm path pays it before publication). + for _, f := range fleets { + f.srv.profileIndexes.For(f.cfg) + } + + for name, c := range profileGateRefusalCases { + t.Run(name, func(t *testing.T) { + var allocs map[string]float64 + for attempt := 0; attempt < 10; attempt++ { + allocs = map[string]float64{} + for fleet, f := range fleets { + handler := f.handler(next) + allocs[fleet] = testing.AllocsPerRun(20, func() { profileGateRefusal(t, handler, c.agent, c.path) }) + } + same := true + for fleet := range fleets { + same = same && allocs[fleet] == allocs["no profiles"] + } + if same { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("%s must allocate exactly like the empty fleet on every fleet: %v", name, allocs) + }) + } +} + +// TestProfileMiddleware_GateTouchesOnlyRequestedSlugAndPin is the traversal- +// counter seam behind the allocation parity above: through the index's lookup +// hook, every scoped request to the gate — refused or admitted, over any fleet +// — resolves at most the slug it asked for and the caller's pin, never a +// third profile. (Admission resolves the slug twice: once to decide, once to +// build the scope.) +func TestProfileMiddleware_GateTouchesOnlyRequestedSlugAndPin(t *testing.T) { + fleets := profileGateFleets() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + cases := map[string]struct { + agent *auth.AuthContext + path string + }{} + for name, c := range profileGateRefusalCases { + cases[name] = c + } + cases["admitted pin"] = struct { + agent *auth.AuthContext + path string + }{&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "pin", AllowedServers: []string{"pin-srv"}}, "/mcp/p/pin"} + cases["admitted scoped"] = struct { + agent *auth.AuthContext + path string + }{&auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"*"}}, "/mcp/p/pin"} + + for fleet, f := range fleets { + var touched []string + idx := newProfileIndex(f.cfg) + idx.lookupHook = func(slug string) { touched = append(touched, slug) } + f.srv.profileIndexes.warm.Store(idx) + handler := f.handler(next) + + for name, c := range cases { + touched = nil + req := httptest.NewRequest(http.MethodPost, c.path, http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), c.agent)) + handler.ServeHTTP(httptest.NewRecorder(), req) + + slug := strings.Trim(strings.TrimPrefix(c.path, "/mcp/p"), "/") + allowed := map[string]bool{slug: true} + if c.agent.ProfilePin != "" { + allowed[c.agent.ProfilePin] = true + } + require.NotEmpty(t, touched, "%s/%s: the gate must resolve through the index", fleet, name) + require.LessOrEqual(t, len(touched), 3, "%s/%s: at most slug, pin and the admission re-lookup: %v", fleet, name, touched) + for _, got := range touched { + require.True(t, allowed[got], "%s/%s: the gate touched profile %q, outside {slug, pin}: %v", fleet, name, got, touched) + } + } + require.Same(t, idx, f.srv.profileIndexes.For(f.cfg), "%s: the cached index must be reused for the same snapshot", fleet) + } +} + +// TestProfileMiddleware_RefusalReachCostsTheGrantNotTheFleet (Spec 105 PR D +// codex round 4, finding 1): the reach test behind every scoped refusal must +// cost the READER's grant, never the fleet. A reach that walked every +// configured server and ran the credential check on each did one iteration +// over a fleet of one server and 4 096 over an otherwise identical fleet with +// 4 095 hidden servers behind it — same 404, same zero allocations (so the +// allocation-parity test above was blind to it), 36 ns vs 25 µs — a timing +// oracle on the number of servers the operator runs (FR-004; spec +// Definitions: non-disclosing = status, body AND timing class). +// +// Traversal-counter seam, not a clock: through the index's reach hook, every +// scoped refusal branch over every fleet shape performs exactly one +// membership test per entry of the token's own allowed_servers — a size the +// agent controls and already knows — and the count is identical across the +// two-server and the 4 098-server fleet. +func TestProfileMiddleware_RefusalReachCostsTheGrantNotTheFleet(t *testing.T) { + fleets := profileGateFleets() + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + t.Errorf("%s must not reach the MCP handler", r.URL.Path) + }) + + for name, c := range profileGateRefusalCases { + t.Run(name, func(t *testing.T) { + steps := map[string]int{} + for fleet, f := range fleets { + idx := newProfileIndex(f.cfg) + idx.reachHook = func() { steps[fleet]++ } + f.srv.profileIndexes.warm.Store(idx) + profileGateRefusal(t, f.handler(next), c.agent, c.path) + } + for fleet, got := range steps { + require.Equal(t, len(c.agent.AllowedServers), got, + "%s over fleet %q: reach must test exactly one membership per granted server, never per configured server: %v", name, fleet, steps) + } + require.Equal(t, steps["no profiles"], steps["4096 hidden servers"], + "%s: 4 096 hidden servers must cost exactly what an empty fleet costs: %v", name, steps) + }) + } +} + +// TestProfileMiddleware_RefusesThroughTheSnapshotSeam pins the production +// wiring the fleet tests bypass: profileMiddleware over a live runtime reaches +// the same gate (serveProfileURL) with the warm (index, snapshot) pair — a +// scoped refusal and an admission behave identically through either entry. +func TestProfileMiddleware_RefusesThroughTheSnapshotSeam(t *testing.T) { + srv, _ := newProfileGateTestServer(t) + require.Eventually(t, func() bool { + idx := srv.profileIndexes.warm.Load() + return idx != nil && idx.cfg == srv.runtime.Config() + }, 5*time.Second, 10*time.Millisecond) + agent := &auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"research-srv"}} + reached := 0 + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { reached++ }) + + for _, entry := range []struct { + name string + handler http.Handler + }{ + {"profileMiddleware", srv.profileMiddleware(next)}, + {"serveProfileURL", profileGateFleet{srv: srv, cfg: srv.runtime.Config()}.handler(next)}, + } { + rec := profileGateRefusal(t, entry.handler, agent, "/mcp/p/deploy") + require.JSONEq(t, `{"error":"unknown profile 'deploy'"}`, rec.Body.String(), entry.name) + + before := reached + req := httptest.NewRequest(http.MethodPost, "/mcp/p/research", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), agent)) + rec = httptest.NewRecorder() + entry.handler.ServeHTTP(rec, req) + require.Equal(t, before+1, reached, "%s must admit the selectable profile", entry.name) + } +} + +// TestProfileIndex_WarmedBeforeFirstRequest (Spec 105 PR D codex round 3, +// prior item): the per-snapshot index must not be built by the first request +// after startup or a hot reload — that request would pay one insertion per +// configured profile (4 096 over a hidden fleet, none over an empty one), +// the fleet-population cost the index exists to remove (FR-004). The Server +// builds it when it is constructed and again on every config event (and, +// since round 5, before every publication — TestProfileIndex_BuiltBefore- +// Publication), so the gate's lazy build never serves a live runtime. +func TestProfileIndex_WarmedBeforeFirstRequest(t *testing.T) { + srv, _ := newProfileGateTestServer(t) + + // Construction indexes the constructor's snapshot; background + // initialization then publishes its own (followed by its config event), + // so "covers the current snapshot" is reached, never requested. + require.NotNil(t, srv.profileIndexes.warm.Load(), "the index must be built at construction, not by the first request") + covered := func() bool { + idx := srv.profileIndexes.warm.Load() + return idx != nil && idx.cfg == srv.runtime.Config() + } + require.Eventually(t, covered, 5*time.Second, 10*time.Millisecond, "the startup snapshot must be indexed without a request") + first := srv.runtime.Config() + + // A hot reload publishes a new snapshot; its config event rebuilds the + // index before any request arrives. + next := *first + next.Profiles = append(slices.Clone(first.Profiles), config.ProfileConfig{Name: "extra", Servers: []string{"research-srv"}}) + _, err := srv.ApplyConfig(&next, filepath.Join(t.TempDir(), "mcp_config.json")) + require.NoError(t, err) + require.Eventually(t, func() bool { return srv.runtime.Config() != first && covered() }, + 5*time.Second, 10*time.Millisecond, "the reloaded snapshot must be indexed without a request") + require.NotNil(t, srv.profileIndexes.warm.Load().lookup("extra")) +} + +// TestProfileIndex_BuiltBeforePublication (Spec 105 PR D codex round 5, +// prior item P): warming from the config EVENT left a window — snapshot +// stored, event not yet delivered — in which a request built the fleet-sized +// index inline, and a token with server-write permission can open that +// window itself (upstream_servers add, then probe). The index is now built +// by a configsvc pre-publish observer, on the exact snapshot pointer, before +// it is stored: across N reloads over a live runtime, the request that +// follows each publication immediately — no event delivered, no wait — +// finds its index ready, and the request-path build seam never fires. +func TestProfileIndex_BuiltBeforePublication(t *testing.T) { + srv, _ := newProfileGateTestServer(t) + agent := &auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"research-srv"}} + reached := 0 + handler := srv.profileMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { reached++ })) + + // Let background initialization publish its startup snapshots first, so + // the loop below measures the reloads it drives, not startup. + require.Eventually(t, func() bool { + idx := srv.profileIndexes.warm.Load() + return idx != nil && idx.cfg == srv.runtime.Config() + }, 5*time.Second, 10*time.Millisecond) + + // Deterministic seam, since the event listener may win the race on a + // quiet machine: observers run in registration order, so this one sees + // the warm slot right after the Server's observer and before the snapshot + // is stored — the index for the config being published must already + // cover that exact pointer. + var unindexed atomic.Int32 + srv.runtime.ConfigService().AddPrePublishObserver(func(cfg *config.Config) { + if idx := srv.profileIndexes.warm.Load(); idx == nil || idx.cfg != cfg { + unindexed.Add(1) + } + }) + + cfgPath := filepath.Join(t.TempDir(), "mcp_config.json") + for i := 0; i < 5; i++ { + before := srv.runtime.Config() + slug := fmt.Sprintf("extra-%d", i) + next := *before + next.Profiles = append(slices.Clone(before.Profiles), config.ProfileConfig{Name: slug, Servers: []string{"research-srv"}}) + _, err := srv.ApplyConfig(&next, cfgPath) + require.NoError(t, err) + require.NotSame(t, before, srv.runtime.Config(), "ApplyConfig publishes synchronously") + + req := httptest.NewRequest(http.MethodPost, "/mcp/p/"+slug, http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), agent)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + require.Equal(t, i+1, reached, "reload %d: the new profile must be admitted through the freshly published snapshot: %s", i, rec.Body.String()) + } + require.Zero(t, unindexed.Load(), "every published snapshot must be indexed before it is stored") + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), "no request may build the index") +} + +// TestProfileIndexCache_StaleRequestCannotEvictTheWarmIndex (Spec 105 PR D +// codex round 5, finding 1): the single-slot cache could be rolled back by a +// request — R1 captures snapshot A and stalls; a reload publishes and warms +// B; R1 resumes, builds A and overwrites the cached B; the next request under +// B rebuilds the whole fleet inline. The warm slot is written only by the +// warm path; For's fallback build lands in the lazy slot and leaves the warm +// index where it is. Since round 6 no request over a runtime reaches For at +// all (it holds the pair Current handed it); this pins the defence in depth +// for a caller that would. +func TestProfileIndexCache_StaleRequestCannotEvictTheWarmIndex(t *testing.T) { + older := &config.Config{Profiles: []config.ProfileConfig{{Name: "a"}}} + current := &config.Config{Profiles: []config.ProfileConfig{{Name: "b"}}} + + var c profileIndexCache + warmed := c.warmPublishing(current) + require.Same(t, warmed, c.For(current), "the warmed index serves the current snapshot") + + stale := c.For(older) // an in-flight request that captured the previous snapshot + require.Same(t, older, stale.cfg) + require.Equal(t, int64(1), c.lazyBuilds.Load(), "the stale request builds for itself") + + require.Same(t, warmed, c.For(current), "the stale request must not have evicted the warm index") + require.Same(t, stale, c.For(older), "the stale request's own index is retained beside it") + require.Equal(t, int64(1), c.lazyBuilds.Load(), "and nothing was rebuilt") +} + +// TestProfileRequests_ServeThePublishedSnapshotDuringObserverWindow (Spec 105 +// PR D review round 8, MUST-FIX; supersedes round 6's +// TestProfileRequests_ServeTheIndexAboutToBePublished, which pinned the +// opposite — and wrong — behaviour): the request-visible (index, snapshot) +// pair must track the PUBLISHED snapshot exactly, with runtime.Config() as +// the one atomic publication boundary every reader agrees on — never a pair +// merely prepared ahead of it. Taking the cache's unconditional latest pair +// let a request be admitted (and, for a pinned caller, scoped) against the +// config about to be published while resolveActiveProfileIn's pin tier, +// reading runtime.Config() independently a moment later, still answered the +// previous one: admission and the effective scope could disagree within one +// request (codex round 7). +// +// During the observer-to-Store window a request must instead be served from +// the PUBLISHED pair — the old index, the old cfg — so a profile that exists +// only in the config about to be published is refused exactly as it would be +// a moment before or after the reload (not admitted early), and a profile +// being WIDENED reports its still-published, narrower server set through +// BOTH the URL gate's injected scope and a pinned caller's downstream +// resolveActiveProfile — proving admission and pin resolution decide over the +// same snapshot rather than splitting across it. After Store, the very next +// request gets the new pair — admitted, widened — with zero index builds +// throughout. +func TestProfileRequests_ServeThePublishedSnapshotDuringObserverWindow(t *testing.T) { + srv, _ := newProfileGateTestServer(t) + require.Eventually(t, func() bool { + idx := srv.profileIndexes.warm.Load() + return idx != nil && idx.cfg == srv.runtime.Config() + }, 5*time.Second, 10*time.Millisecond) + + pinnedAgent := &auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "deploy", AllowedServers: []string{"*"}} + scopedAgent := &auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"research-srv"}} + var scopedFromURL, scopedFromResolver []string + handler := srv.profileMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + scopedFromURL = profile.ProfileScopeFromContext(r.Context()).AllowedServerNames() + _, resolved := srv.mcpProxy.resolveActiveProfile(r.Context()) + scopedFromResolver = resolved.AllowedServerNames() + })) + + // Registered after the Server's own observer, so it runs on the same + // publication with the warm slot already covering the NEXT cfg and the + // snapshot not yet stored. + type observation struct { + stored bool + onlyInNextStatus int + onlyInNextResult *mcp.CallToolResult + deployStatus int + deployURLScope []string + deployResolved []string + lazyBuilds int64 + proxyBuilds int64 + } + observed := make(chan observation, 1) + srv.runtime.ConfigService().AddPrePublishObserver(func(cfg *config.Config) { + o := observation{stored: srv.runtime.Config() == cfg} + + // A slug that exists ONLY in the config about to be published must + // not be admitted before it actually is. + req := httptest.NewRequest(http.MethodPost, "/mcp/p/only-in-next", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), scopedAgent)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + o.onlyInNextStatus = rec.Code + o.onlyInNextResult = callSetProfileTool(t, srv.mcpProxy, setProfileScopedCtx("s", "research-srv"), "only-in-next") + + // "deploy" exists in both snapshots but is being WIDENED in the next + // one; during this window a pinned request must see the still- + // published (narrow) set through both the URL gate and the resolver. + scopedFromURL, scopedFromResolver = nil, nil + req2 := httptest.NewRequest(http.MethodPost, "/mcp/p/deploy", http.NoBody) + req2 = req2.WithContext(auth.WithAuthContext(req2.Context(), pinnedAgent)) + rec2 := httptest.NewRecorder() + handler.ServeHTTP(rec2, req2) + o.deployStatus = rec2.Code + o.deployURLScope = scopedFromURL + o.deployResolved = scopedFromResolver + + o.lazyBuilds = srv.profileIndexes.lazyBuilds.Load() + o.proxyBuilds = srv.mcpProxy.profileIndexes.lazyBuilds.Load() + observed <- o + }) + + before := srv.runtime.Config() + next := *before + next.Profiles = append(slices.Clone(before.Profiles), config.ProfileConfig{Name: "only-in-next", Servers: []string{"research-srv"}}) + for i := range next.Profiles { + if next.Profiles[i].Name == "deploy" { + next.Profiles[i].Servers = []string{"deploy-srv", "research-srv"} + } + } + _, err := srv.ApplyConfig(&next, filepath.Join(t.TempDir(), "mcp_config.json")) + require.NoError(t, err) + + o := <-observed + require.False(t, o.stored, "the observer runs before the snapshot is stored") + + require.Equal(t, http.StatusNotFound, o.onlyInNextStatus, + "a profile that exists only in the config about to be published must not be admitted before it is published") + require.True(t, o.onlyInNextResult.IsError, + "set_profile must not admit a profile that exists only in the config about to be published: %s", setProfileResultText(t, o.onlyInNextResult)) + + require.Equal(t, http.StatusOK, o.deployStatus, "the still-published 'deploy' profile stays admitted through the window") + require.Equal(t, []string{"deploy-srv"}, o.deployURLScope, + "the URL gate must scope by the still-published (narrow) snapshot, not the one about to replace it") + require.Equal(t, []string{"deploy-srv"}, o.deployResolved, + "downstream pin resolution must agree with the URL gate's own snapshot, not an independently-read one") + + require.Zero(t, o.lazyBuilds, "no request may build the index") + require.Zero(t, o.proxyBuilds, "set_profile over a runtime decides with the main Server's index") + + // After Store, the very next request gets the NEW pair: "only-in-next" is + // admitted and "deploy" reports its widened set, with zero further builds. + req := httptest.NewRequest(http.MethodPost, "/mcp/p/only-in-next", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), scopedAgent)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "after Store the next request must be admitted through the newly published snapshot: %s", rec.Body.String()) + + scopedFromURL, scopedFromResolver = nil, nil + req2 := httptest.NewRequest(http.MethodPost, "/mcp/p/deploy", http.NoBody) + req2 = req2.WithContext(auth.WithAuthContext(req2.Context(), pinnedAgent)) + rec2 := httptest.NewRecorder() + handler.ServeHTTP(rec2, req2) + require.Equal(t, http.StatusOK, rec2.Code) + require.ElementsMatch(t, []string{"deploy-srv", "research-srv"}, scopedFromURL, "the next request must see the newly published, widened snapshot") + require.ElementsMatch(t, []string{"deploy-srv", "research-srv"}, scopedFromResolver) + + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), "no request may build the index") + require.Zero(t, srv.mcpProxy.profileIndexes.lazyBuilds.Load()) +} + +// TestProfileIndexCache_PausedRequestKeepsTheIndexItWasHanded (Spec 105 PR D +// codex round 6, finding 1): a request holds the index it took from the warm +// slot, so however many publications pass while it is paused it decides with +// that index and builds nothing — the two-slot cache of round 5 still rebuilt +// snapshot A inline for a request that captured A and resumed after warm had +// moved to C (lazy B). Cache level: Current() hands out the warm index and +// later publications leave the handed one untouched; gate level: the gate +// decides with the index it is handed — A's profile is admitted from A's +// index while the warm slot already holds C — and the build seam stays 0. +func TestProfileIndexCache_PausedRequestKeepsTheIndexItWasHanded(t *testing.T) { + cfgA := &config.Config{Servers: []*config.ServerConfig{{Name: "srv"}}, Profiles: []config.ProfileConfig{{Name: "only-in-a", Servers: []string{"srv"}}}} + cfgB := &config.Config{Servers: []*config.ServerConfig{{Name: "srv"}}, Profiles: []config.ProfileConfig{{Name: "only-in-b", Servers: []string{"srv"}}}} + cfgC := &config.Config{Servers: []*config.ServerConfig{{Name: "srv"}}, Profiles: []config.ProfileConfig{{Name: "only-in-c", Servers: []string{"srv"}}}} + + srv := &Server{logger: zap.NewNop()} + require.Nil(t, srv.profileIndexes.Current(), "no warm index before the first publication") + idxA := srv.profileIndexes.warmPublishing(cfgA) + held := srv.profileIndexes.Current() // the request takes its (index, snapshot) pair here and pauses + require.Same(t, idxA, held) + require.Same(t, cfgA, held.cfg) + + srv.profileIndexes.warmPublishing(cfgB) + idxC := srv.profileIndexes.warmPublishing(cfgC) + require.Same(t, idxC, srv.profileIndexes.Current(), "later publications move the warm slot") + require.Same(t, idxA, held, "and leave the handed index where it is") + require.Same(t, cfgA, held.cfg) + + agent := &auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"srv"}} + var scoped []string + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + scoped = append(scoped, profile.ProfileScopeFromContext(r.Context()).Name) + }) + req := httptest.NewRequest(http.MethodPost, "/mcp/p/only-in-a", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), agent)) + rec := httptest.NewRecorder() + srv.serveProfileURL(rec, req, held, next) + require.Equal(t, http.StatusOK, rec.Code, "the resumed request decides with the index it was handed: %s", rec.Body.String()) + require.Equal(t, []string{"only-in-a"}, scoped) + + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), "the resumed request builds nothing") + require.Same(t, idxC, srv.profileIndexes.Current(), "and moves nothing") +} + +// TestProfileMiddleware_InjectsAdmittedSnapshotForDownstreamResolution (Spec +// 105 PR D review round 8, MUST-FIX): serveProfileURL must pin the request to +// the exact snapshot admission decided with, so a pinned caller's downstream +// resolveActiveProfile (tier 1, pin resolution) decides over that same +// snapshot rather than an independent runtime.Config() read — one a reload +// landing strictly BETWEEN admission and the handler running could otherwise +// have already moved past. The bare MCPProxyServer here has no runtime, so +// currentConfig() falls back to reading p.config directly; mutating it inside +// the downstream handler simulates exactly that landing. +func TestProfileMiddleware_InjectsAdmittedSnapshotForDownstreamResolution(t *testing.T) { + cfgOld := &config.Config{ + Servers: []*config.ServerConfig{{Name: "deploy-srv"}, {Name: "research-srv"}}, + Profiles: []config.ProfileConfig{{Name: "deploy", Servers: []string{"deploy-srv"}}}, + } + cfgNew := &config.Config{ + Servers: cfgOld.Servers, + Profiles: []config.ProfileConfig{{Name: "deploy", Servers: []string{"deploy-srv", "research-srv"}}}, + } + + p := &MCPProxyServer{logger: zap.NewNop(), sessionStore: NewSessionStore(zap.NewNop())} + srv := &Server{logger: zap.NewNop(), mcpProxy: p} + p.mainServer = srv + + idx := srv.profileIndexes.warmPublishing(cfgOld) + pin := &auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "deploy", AllowedServers: []string{"*"}} + + var resolvedServers []string + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + // A reload "lands" here, strictly after admission: without the + // context injection, resolveActiveProfile's fresh currentConfig() + // read would see the widened profile instead of the one the gate + // admitted this request against. + p.config = cfgNew + _, scope := p.resolveActiveProfile(r.Context()) + resolvedServers = scope.AllowedServerNames() + }) + + req := httptest.NewRequest(http.MethodPost, "/mcp/p/deploy", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), pin)) + rec := httptest.NewRecorder() + srv.serveProfileURL(rec, req, idx, next) + require.Equal(t, http.StatusOK, rec.Code, "%s", rec.Body.String()) + require.Equal(t, []string{"deploy-srv"}, resolvedServers, + "downstream pin resolution must decide over the snapshot the gate admitted against, not a config that changed after admission") +} + +// TestProfileMiddleware_SetProfileDecidesWithTheAdmittedIndexNotAFreshRead +// (Spec 105 PR D review round 9, MUST-FIX 1): handleSetProfile must decide +// with the SAME (index, snapshot) pair serveProfileURL already admitted this +// request against, not an independent profileIndexCurrent() build — which +// re-reads currentConfig() and can therefore land on a DIFFERENT snapshot +// than the one the URL gate used, even within the one request the gate +// admitted. Sibling of +// TestProfileMiddleware_InjectsAdmittedSnapshotForDownstreamResolution +// above, but for the set_profile TOOL call inside the request rather than +// resolveActiveProfile's pin tier. +// +// Both directions of the split are proven with one admin session (SC-005 +// lets an administrator select any configured profile, so the reach +// predicate is not in play — only which SNAPSHOT the decision reads): +// - "existing" is configured in the admitted snapshot (A) only; a reload +// that removes it must not retroactively refuse it mid-request. +// - "new-profile" is configured in the reloaded snapshot (B) only; it must +// not become selectable mid-request just because a reload happened to +// land before set_profile ran. +func TestProfileMiddleware_SetProfileDecidesWithTheAdmittedIndexNotAFreshRead(t *testing.T) { + cfgOld := &config.Config{ + Servers: []*config.ServerConfig{{Name: "deploy-srv"}, {Name: "research-srv"}}, + Profiles: []config.ProfileConfig{ + {Name: "deploy", Servers: []string{"deploy-srv"}}, + {Name: "existing", Servers: []string{"research-srv"}}, + }, + } + cfgNew := &config.Config{ + Servers: cfgOld.Servers, + Profiles: []config.ProfileConfig{ + {Name: "deploy", Servers: []string{"deploy-srv"}}, + {Name: "new-profile", Servers: []string{"research-srv"}}, + }, + } + + p := &MCPProxyServer{logger: zap.NewNop(), sessionStore: NewSessionStore(zap.NewNop()), config: cfgOld} + srv := &Server{logger: zap.NewNop(), mcpProxy: p} + p.mainServer = srv + + idx := srv.profileIndexes.warmPublishing(cfgOld) + + var removedRes, addedRes *mcp.CallToolResult + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + // A reload "lands" here, strictly after admission — before the fix + // profileIndexCurrent() would re-read currentConfig() (now cfgNew) + // instead of the pair the gate injected. + p.config = cfgNew + + removedRes = callSetProfileTool(t, p, r.Context(), "existing") + addedRes = callSetProfileTool(t, p, r.Context(), "new-profile") + }) + + req := httptest.NewRequest(http.MethodPost, "/mcp/p/deploy", http.NoBody) + req = req.WithContext(setProfileAdminCtx("sess-admitted")) + rec := httptest.NewRecorder() + srv.serveProfileURL(rec, req, idx, next) + require.Equal(t, http.StatusOK, rec.Code, "%s", rec.Body.String()) + + require.False(t, removedRes.IsError, + "a profile present in the admitted snapshot must stay selectable for the rest of the request, even after a reload removes it live: %s", setProfileResultText(t, removedRes)) + require.True(t, addedRes.IsError, + "a profile that exists only in a reload landing after admission must not become selectable mid-request: %s", setProfileResultText(t, addedRes)) +} + +// TestProfileRequests_NeverBuildTheIndexOverARuntime (Spec 105 PR D codex +// round 6, finding 1): over a live runtime, no request entry — every scoped +// refusal branch and an admission through the URL gate, scoped and +// administrator set_profile refusals and admissions — builds a profile index, +// before or after reloads it drives itself. The request path takes the +// (index, snapshot) pair the pre-publish observer prepared; the build seam on +// both caches (the Server's and the proxy's own) stays at zero. +func TestProfileRequests_NeverBuildTheIndexOverARuntime(t *testing.T) { + srv, _ := newProfileGateTestServer(t) + require.Eventually(t, func() bool { + idx := srv.profileIndexes.warm.Load() + return idx != nil && idx.cfg == srv.runtime.Config() + }, 5*time.Second, 10*time.Millisecond) + + reached := 0 + handler := srv.profileMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { reached++ })) + admin := auth.AdminContext() + drive := func(round int) { + t.Helper() + for _, c := range profileGateRefusalCases { + profileGateRefusal(t, handler, c.agent, c.path) + } + for _, c := range []struct { + agent *auth.AuthContext + path string + code int + }{ + {&auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"research-srv"}}, "/mcp/p/research", http.StatusOK}, + {&auth.AuthContext{Type: auth.AuthTypeAgent, ProfilePin: "deploy", AllowedServers: []string{"*"}}, "/mcp/p/deploy", http.StatusOK}, + {admin, "/mcp/p/research", http.StatusOK}, + {admin, "/mcp/p/nope", http.StatusNotFound}, + } { + req := httptest.NewRequest(http.MethodPost, c.path, http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), c.agent)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + require.Equal(t, c.code, rec.Code, "round %d: %s: %s", round, c.path, rec.Body.String()) + } + for _, c := range []struct { + ctx context.Context + slug string + isErr bool + }{ + {setProfileScopedCtx("s", "research-srv"), "research", false}, + {setProfileScopedCtx("s", "research-srv"), "deploy", true}, + {setProfileScopedCtx("s", "research-srv"), "nope", true}, + {setProfilePinnedCtx("s", "deploy", "deploy-srv"), "deploy", false}, + {setProfilePinnedCtx("s", "gone", "deploy-srv"), "gone", true}, + {setProfileAdminCtx("s"), "deploy", false}, + {setProfileAdminCtx("s"), "nope", true}, + {setProfileAdminCtx("s"), "", false}, + } { + res := callSetProfileTool(t, srv.mcpProxy, c.ctx, c.slug) + require.Equal(t, c.isErr, res.IsError, "round %d: set_profile %q: %s", round, c.slug, setProfileResultText(t, res)) + } + } + + cfgPath := filepath.Join(t.TempDir(), "mcp_config.json") + for round := 0; round < 3; round++ { + drive(round) + before := srv.runtime.Config() + next := *before + next.Profiles = append(slices.Clone(before.Profiles), config.ProfileConfig{Name: fmt.Sprintf("extra-%d", round), Servers: []string{"research-srv"}}) + _, err := srv.ApplyConfig(&next, cfgPath) + require.NoError(t, err) + drive(round) // immediately after the publication, before any config event + } + require.Positive(t, reached) + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), "no request over a runtime may build the index") + require.Zero(t, srv.mcpProxy.profileIndexes.lazyBuilds.Load(), "set_profile over a runtime decides with the main Server's index, never its own") +} + +// TestProfileIndexCache_Acquire_RecoversAfterConfigMovesBetweenReads (Spec 105 +// PR D review round 11, MUST-FIX): profileMiddleware and set_profile's +// base-endpoint path used to read runtime.Config() ONCE and only then call +// Published against that frozen read — a request paused across two +// publications between those two steps found neither the latest nor the +// previous prepared pair and fell back to For, which builds the whole fleet +// inline. Acquire closes the window structurally by re-reading readCfg() on +// EVERY retry instead of matching one frozen read: here the first call +// observes A, but by the time Acquire checks Published(A) two publications +// have already landed (B, then C) so the match misses; the retry re-reads +// readCfg(), which now answers the settled C, and Acquire returns C's own +// pair — without ever falling through to a build. +func TestProfileIndexCache_Acquire_RecoversAfterConfigMovesBetweenReads(t *testing.T) { + cfgA := &config.Config{Profiles: []config.ProfileConfig{{Name: "a"}}} + cfgB := &config.Config{Profiles: []config.ProfileConfig{{Name: "b"}}} + cfgC := &config.Config{Profiles: []config.ProfileConfig{{Name: "c"}}} + + var c profileIndexCache + c.warmPublishing(cfgA) + + calls := 0 + readCfg := func() *config.Config { + calls++ + if calls == 1 { + // Simulate two publications racing between this request's own + // runtime.Config() read (which returned A) and Acquire's match + // against it: by the time the caller's first read is used, warm + // and previous have already moved past A entirely. + c.warmPublishing(cfgB) + c.warmPublishing(cfgC) + return cfgA + } + return cfgC + } + + idx := c.Acquire(readCfg) + require.NotNil(t, idx, "Acquire must recover once readCfg answers a snapshot the cache still has a pair for") + require.Same(t, cfgC, idx.cfg) + require.Equal(t, 2, calls, "the first (stale) read misses; the second (fresh) read hits") + require.Zero(t, c.lazyBuilds.Load(), "a recovered match must never fall through to a build") +} + +// TestProfileIndexCache_Acquire_ExhaustsBoundedRetriesWithoutBuilding (Spec +// 105 PR D review round 11, MUST-FIX): when readCfg keeps answering a +// snapshot the cache never warmed a pair for — a publication storm that +// outruns the retry loop entirely — Acquire gives up after its bounded +// budget and returns nil. It must NEVER fall through to For: a miss is for +// the caller to fail closed (scoped) or rebuild explicitly (administrator), +// never something Acquire itself pays fleet-sized cost for. +func TestProfileIndexCache_Acquire_ExhaustsBoundedRetriesWithoutBuilding(t *testing.T) { + neverWarmed := &config.Config{Profiles: []config.ProfileConfig{{Name: "never-published"}}} + + var c profileIndexCache + c.warmPublishing(&config.Config{Profiles: []config.ProfileConfig{{Name: "other"}}}) + + calls := 0 + idx := c.Acquire(func() *config.Config { + calls++ + return neverWarmed + }) + + require.Nil(t, idx, "Acquire must give up, not build, once its retry budget is exhausted") + require.Equal(t, profileIndexAcquireRetries, calls, "Acquire must retry exactly its bounded budget, no more and no less") + require.Zero(t, c.lazyBuilds.Load(), "Acquire must never build — a miss is the caller's to handle") +} + +// TestProfileMiddleware_AcquireMissFailsClosedForScopedFallsBackForAdmin +// (Spec 105 PR D review round 11, MUST-FIX): when Acquire cannot pair a +// config with its index at all (profiles == nil reaching serveProfileURL — +// the same shape as a publication storm outrunning it), a scoped caller must +// be refused with the uniform, fleet-independent, zero-build refusal; only +// an administrator-shaped caller — not timing-contract-bound, SC-005 — +// falls back to a fresh build so its pre-105 behaviour (including the "no +// profiles configured" branch here, since the bare Server this test drives +// has no runtime config to build from) is unchanged. +func TestProfileMiddleware_AcquireMissFailsClosedForScopedFallsBackForAdmin(t *testing.T) { + srv := &Server{logger: zap.NewNop()} + reached := false + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { reached = true }) + + scoped := &auth.AuthContext{Type: auth.AuthTypeAgent, AllowedServers: []string{"srv"}} + req := httptest.NewRequest(http.MethodPost, "/mcp/p/anything", http.NoBody) + req = req.WithContext(auth.WithAuthContext(req.Context(), scoped)) + rec := httptest.NewRecorder() + srv.serveProfileURL(rec, req, nil, next) + require.Equal(t, http.StatusNotFound, rec.Code) + require.False(t, reached, "a scoped caller must fail closed when no (index, snapshot) pair could be acquired") + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), "the fail-closed refusal must never build an index") + + admin := auth.AdminContext() + req2 := httptest.NewRequest(http.MethodPost, "/mcp/p/anything", http.NoBody) + req2 = req2.WithContext(auth.WithAuthContext(req2.Context(), admin)) + rec2 := httptest.NewRecorder() + srv.serveProfileURL(rec2, req2, nil, next) + require.Equal(t, http.StatusNotFound, rec2.Code) + require.Contains(t, rec2.Body.String(), "no profiles configured", + "an administrator falls back to a fresh build (pre-105 behaviour), not the uniform scoped refusal") +} + +// TestHandleSetProfile_AcquireMissFailsClosedForScopedFallsBackForAdmin +// (Spec 105 PR D review round 11, MUST-FIX): the base /mcp endpoint's +// set_profile mirrors the URL gate's fail-closed/fall-back split when +// profileIndexCurrent's Acquire call cannot pair a config with its index — a +// scoped caller gets nil back and refuses uniformly (no build), while an +// administrator falls back to a fresh build and proceeds normally. +func TestHandleSetProfile_AcquireMissFailsClosedForScopedFallsBackForAdmin(t *testing.T) { + cfg := &config.Config{ + Servers: []*config.ServerConfig{{Name: "research-srv"}}, + Profiles: []config.ProfileConfig{{Name: "research", Servers: []string{"research-srv"}}}, + } + p := &MCPProxyServer{logger: zap.NewNop(), sessionStore: NewSessionStore(zap.NewNop()), config: cfg} + srv := &Server{logger: zap.NewNop()} // no runtime: currentConfig() falls back to p.config + p.mainServer = srv + // srv.profileIndexes is never warmed for cfg, so Acquire(p.currentConfig) + // misses on every one of its bounded retries — the exhaustion path. + + scopedRes := callSetProfileTool(t, p, setProfileScopedCtx("s-scoped", "research-srv"), "research") + require.True(t, scopedRes.IsError, "a scoped caller must fail closed when Acquire cannot pair a config with its index") + require.Contains(t, setProfileResultText(t, scopedRes), "unknown profile", + "the fail-closed refusal must be the same uniform wording any other non-selectable slug gets") + require.Zero(t, srv.profileIndexes.lazyBuilds.Load(), "the scoped fail-closed path must never build an index") + + adminRes := callSetProfileTool(t, p, setProfileAdminCtx("s-admin"), "research") + require.False(t, adminRes.IsError, "an administrator falls back to a fresh build (pre-105 behaviour), not the uniform scoped refusal") + require.Positive(t, srv.profileIndexes.lazyBuilds.Load(), "the administrator fallback builds explicitly, distinct from the scoped path above") +} diff --git a/internal/server/server.go b/internal/server/server.go index 4b51177f8..73b8ced96 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -103,9 +103,12 @@ func (s *Server) setSecurityScanner(svc securityScannerService) { // Server wraps the MCP proxy server with all its dependencies type Server struct { - logger *zap.Logger - runtime *runtime.Runtime - mcpProxy *MCPProxyServer + logger *zap.Logger + runtime *runtime.Runtime + // profileIndexes caches the slug → profile index of the current config + // snapshot for the /mcp/p/ gate (Spec 105 FR-004, O(1) in the fleet). + profileIndexes profileIndexCache + mcpProxy *MCPProxyServer // Server control httpServer *http.Server @@ -393,6 +396,15 @@ func NewServerWithConfigPath(cfg *config.Config, configPath string, logger *zap. routingEvents := server.runtime.SubscribeEvents() go server.listenForRoutingModeRefresh(routingEvents) + // Spec 105 FR-004: index every config snapshot before it is published — + // the observer runs on the exact *Config about to be stored — and the + // startup snapshot now, which NewService stored without running any + // observer (see warmProfileIndex). + if svc := server.runtime.ConfigService(); svc != nil { + svc.AddPrePublishObserver(func(cfg *config.Config) { server.profileIndexes.warmPublishing(cfg) }) + } + server.warmProfileIndex() + server.runtime.StartBackgroundInitialization() return server, nil @@ -466,7 +478,14 @@ func (s *Server) mcpAuthMiddleware(next http.Handler) http.Handler { if strings.HasPrefix(token, auth.TokenPrefixStr) { cfg := s.runtime.Config() if cfg == nil { - next.ServeHTTP(w, r) + // Fail closed. Forwarding here would hand the request on with NO + // AuthContext, and every scope predicate downstream reads an + // absent context as an administrator (auth.IsScopedCaller) — + // the one path where an unvalidated agent token could take the + // administrator branches (Spec 105 PR D critique round 1). + s.logger.Error("Agent token presented before any configuration was published; refusing", + zap.String("remote_addr", r.RemoteAddr)) + http.Error(w, `{"error":"Server not ready"}`, http.StatusServiceUnavailable) return } @@ -663,6 +682,14 @@ func (s *Server) listenForRoutingModeRefresh(eventCh chan runtime.Event) { defer s.runtime.UnsubscribeEvents(eventCh) for evt := range eventCh { + // Every config event follows a snapshot publication (config.saved, + // config.reloaded and servers.changed are all emitted after the new + // *Config is current), so index the new snapshot here rather than in + // the first request that reads it (Spec 105 FR-004, warmProfileIndex). + switch evt.Type { + case runtime.EventTypeServersChanged, runtime.EventTypeConfigReloaded, runtime.EventTypeConfigSaved: + s.warmProfileIndex() + } switch evt.Type { case runtime.EventTypeServersChanged: s.logger.Debug("servers changed, refreshing routing mode tools", @@ -2400,79 +2427,232 @@ func withHSTS(next http.Handler) http.Handler { // injects it into the request context, then delegates to the retrieve_tools-mode // MCP handler (next). Auth has already run at this point via mcpAuthMiddleware. // -// 404 responses: -// - No profiles configured at all → {"error":"no profiles configured"} -// - Slug not found → {"error":"unknown profile ''","available":[...]} +// Scoped callers (auth.IsScopedCaller — in practice agent tokens: the only +// non-admin identity mcpAuthMiddleware mints on /mcp*; the server edition's +// user contexts are minted on the REST router only) are admitted only through +// a profile the same selectable-profile rule set_profile applies (reach ∩ +// token, pin honoured, zero-reach pin refused — Spec 105 FR-004, research +// D1), evaluated for the requested slug alone (profileIndex.selectable) so +// the refusal's cost is independent of the fleet's population. Every other outcome +// — slug missing, profile deleted, configured but not selectable, pin +// mismatch, empty fleet, slug-less /mcp/p — is answered by ONE constructor +// (profileNotSelectable) so status, body and timing class cannot tell them +// apart; the "no profiles configured" branch deliberately runs AFTER this gate. // -// "available" is an administrator affordance. A profile-pinned agent token -// reaches this branch only when its pinned profile has been deleted, and the -// resolver treats that pin as deny-all (resolveActiveProfile) — so the error -// omits the list rather than enumerate profiles the token may never select -// (Spec 104 FR-016b). +// Administrator-shaped callers (API key, socket, anonymous back-compat) keep +// the pre-105 branches unchanged (SC-005): +// - No profiles configured at all → 404 {"error":"no profiles configured"} +// - Slug not found → 404 {"error":"unknown profile ''","available":[...]} func (s *Server) profileMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - cfg := s.runtime.Config() - - // FR-008: no profiles configured. - if cfg == nil || len(cfg.Profiles) == 0 { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusNotFound) - _ = json.NewEncoder(w).Encode(map[string]interface{}{ - "error": "no profiles configured", - }) - return + // Over a live runtime the (index, snapshot) pair must match this + // request's own runtime.Config() read exactly — Acquire, never a + // separate read-then-match. The cache's unconditional latest pair + // can sit one publication AHEAD of runtime.Config() for the + // microseconds between the pre-publish observer warming it and + // configsvc storing it (round 7/8: admitting against that ahead + // snapshot let a scoped caller's effective scope split from what + // resolveActiveProfileIn would independently resolve moments later + // against the still-published one). Reading runtime.Config() once + // and matching it afterward has its own window: a request paused + // between those two steps across two publications misses both + // prepared pairs and falls back to For, building the whole fleet + // inline for a scoped caller's refusal (round 11 MUST-FIX). Acquire + // closes it by re-reading runtime.Config() on every retry instead of + // freezing one read that may already be stale by the time it is + // matched. A bare Server with no runtime (tests) has no + // runtime.Config() to match, so it falls back to whatever the warm + // path last set. + var profiles *profileIndex + if s.runtime != nil { + profiles = s.profileIndexes.Acquire(s.runtime.Config) + } else { + profiles = s.profileIndexes.Current() + if profiles == nil { + profiles = s.profileIndexes.For(s.runtimeConfig()) + } } + s.serveProfileURL(w, r, profiles, next) + }) +} - // Strip the /mcp/p/ prefix to obtain the slug. - slug := strings.TrimPrefix(r.URL.Path, "/mcp/p/") - slug = strings.TrimPrefix(slug, "/mcp/p") // handle /mcp/p with no trailing slash - slug = strings.Trim(slug, "/") - - // Profiles v2 T3: a profile-pinned agent token may only operate within its - // pinned profile. A request to any other /mcp/p/ is forbidden (403), - // regardless of whether that slug is a real profile. Auth has already run - // (mcpAuthMiddleware wraps this handler), so the pin is on the context. - if pin := profilePinFromContext(r.Context()); pin != "" && pin != slug { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusForbidden) - _ = json.NewEncoder(w).Encode(map[string]interface{}{ - "error": fmt.Sprintf("agent token is pinned to profile '%s' and cannot access profile '%s'", pin, slug), - }) +// runtimeConfig returns the runtime's current config snapshot, or nil on a +// Server built without a runtime (bare test servers). +func (s *Server) runtimeConfig() *config.Config { + if s.runtime == nil { + return nil + } + return s.runtime.Config() +} + +// warmProfileIndex builds the profile index for the runtime's current config +// snapshot ahead of any request. The index is fleet-sized to build (one +// insertion per profile, one reach bitset per profile) and constant to use; +// leaving the build to the first /mcp/p/ request after startup or a +// reload made that one request's refusal cost 4 096 insertions over a hidden +// fleet and none over an empty one (codex review, PR D round 3). Called at +// construction — the initial snapshot is stored without running observers — +// and on every config event as belt-and-braces; the structural guarantee is +// the configsvc pre-publish observer wired in NewServer, which indexes every +// later snapshot before it is stored, so no request lands in a +// publication-to-event window (round 5, prior item P), and every request +// takes the index and its snapshot as one pair (round 6). +func (s *Server) warmProfileIndex() { + if s.runtime == nil { + return + } + s.profileIndexes.warmCurrent(s.runtime.Config()) +} + +// serveProfileURL is profileMiddleware over ONE (index, snapshot) pair — the +// whole gate after the pair is taken, so it can be exercised against any +// fleet shape without a runtime behind it (the fleet-parity tests build a +// bare Server; a live runtime over thousands of profiles spends the test +// building per-profile indexes in the background). The snapshot it decides +// over is the one the index was built from, profiles.cfg; it never reads +// the live config. Same split as resolveActiveProfile / resolveActiveProfileIn. +func (s *Server) serveProfileURL(w http.ResponseWriter, r *http.Request, profiles *profileIndex, next http.Handler) { + // Strip the /mcp/p/ prefix to obtain the slug. Computed before profiles + // is dereferenced: a nil profiles (Acquire's bounded retries exhausted, + // below) still needs it to log and refuse a scoped caller uniformly. + slug := strings.TrimPrefix(r.URL.Path, "/mcp/p/") + slug = strings.TrimPrefix(slug, "/mcp/p") // handle /mcp/p with no trailing slash + slug = strings.Trim(slug, "/") + + if profiles == nil { + // Acquire could not pair this request's own runtime.Config() read + // with a prepared index within its bounded retry budget — a + // publication storm outran the loop faster than it could catch up + // (structurally rare: the pre-publish observer means Acquire matches + // on its first iteration in the overwhelmingly common case). A + // scoped caller fails closed here, O(1): the uniform refusal never + // depends on a slug lookup or the fleet's population, so it stays + // non-disclosing even without a paired snapshot to evaluate against + // (Spec 105 PR D review round 11, MUST-FIX). An administrator-shaped + // caller is not timing-contract-bound (SC-005) and falls back to a + // fresh build over the live config, matching pre-105 behaviour. + if auth.IsScopedCaller(r.Context()) { + var agentName string + if ac := auth.AuthContextFromContext(r.Context()); ac != nil { + agentName = ac.AgentName + } + s.logger.Info("profile URL refused for scoped caller", + zap.String("agent_name", agentName), + zap.String("profile", slug), + zap.String("remote_addr", r.RemoteAddr)) + profileNotSelectable(w, slug) return } - - // Look up profile by slug (lock-free snapshot). - var found *config.ProfileConfig - for i := range cfg.Profiles { - if cfg.Profiles[i].Name == slug { - found = &cfg.Profiles[i] - break + profiles = s.profileIndexes.For(s.runtimeConfig()) + } + cfg := profiles.cfg + + // One slug → profile index per snapshot (built before the snapshot was + // published, see warmProfileIndex): the gate below and the lookup after + // it resolve the slug directly, so neither the refusal nor the admission + // walks cfg.Profiles. + + // Spec 105 FR-004: the selectable-profile gate for scoped callers. It + // evaluates the requested profile (and the pin) ONLY — never the + // selectable list, whose cost is fleet-sized (profileIndex.selectable). + if auth.IsScopedCaller(r.Context()) { + if !profiles.selectable(r.Context(), slug) { + // Silent towards the agent, not towards the operator: the gate + // answers before the logging handler mounted inside it, so this + // line is the only trace a token probing the slug space leaves. + var agentName string + if ac := auth.AuthContextFromContext(r.Context()); ac != nil { + agentName = ac.AgentName } + s.logger.Info("profile URL refused for scoped caller", + zap.String("agent_name", agentName), + zap.String("profile", slug), + zap.String("remote_addr", r.RemoteAddr)) + profileNotSelectable(w, slug) + return } + } else if cfg == nil || len(cfg.Profiles) == 0 { + // FR-008: no profiles configured (administrator affordance). + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "error": "no profiles configured", + }) + return + } - // FR-009: slug not found. - if found == nil { - body := map[string]interface{}{ - "error": fmt.Sprintf("unknown profile '%s'", slug), - } - if profilePinFromContext(r.Context()) == "" { - available := make([]string, 0, len(cfg.Profiles)) - for _, p := range cfg.Profiles { - available = append(available, p.Name) - } - body["available"] = available - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusNotFound) - _ = json.NewEncoder(w).Encode(body) - return + // Look up profile by slug (lock-free snapshot). A scoped caller that + // passed the gate always resolves here — the predicate only admits + // configured profiles. The position is kept, not just the *ProfileConfig, + // so the effective-server computation below can reuse this exact + // resolution instead of resolving the slug a third time through the + // lookup-hook seam (round 14 MUST-FIX; TestProfileMiddleware_Gate- + // TouchesOnlyRequestedSlugAndPin bounds admission to slug-twice-plus-pin). + candidate := profiles.position(slug) + found := profiles.profileAt(candidate) + + // FR-009: slug not found — administrator callers only, with the + // discovery affordance. + if found == nil { + available := make([]string, 0, len(cfg.Profiles)) + for _, p := range cfg.Profiles { + available = append(available, p.Name) } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "error": fmt.Sprintf("unknown profile '%s'", slug), + "available": available, + }) + return + } - // Build scope from the effective server set (unknown-server warn-skip applied). - effectiveServers := found.EffectiveServers(cfg) - scope := profile.NewProfileScope(found.Name, effectiveServers) - ctx := profile.WithProfileScope(r.Context(), scope) - next.ServeHTTP(w, r.WithContext(ctx)) + // Build scope from the effective server set (unknown-server warn-skip + // applied). A scoped caller already passed the reach gate above for + // exactly this profile: render ITS view through the index's + // O(len(allowed))-cost EffectiveServersFor rather than + // config.EffectiveServers, which rebuilds a fleet-sized set on every + // call — an admitted READ must never cost the hidden server population + // any more than the refusal above does (Spec 105 PR D review round 14 + // MUST-FIX). Administrators (and absent contexts) keep the unchanged, + // fleet-proportional EffectiveServers path: SC-005 makes no timing + // promise for them, and they already pay this cost on every other + // admin-shaped read. + var effectiveServers []string + if auth.IsScopedCaller(r.Context()) { + var allowed []string + if ac := auth.AuthContextFromContext(r.Context()); ac != nil { + allowed = ac.AllowedServers + } + effectiveServers = profiles.effectiveServersForCandidate(candidate, allowed) + } else { + effectiveServers = found.EffectiveServers(cfg) + } + scope := profile.NewProfileScope(found.Name, effectiveServers) + ctx := profile.WithProfileScope(r.Context(), scope) + // Pin the request to the exact (index, snapshot) PAIR admission decided + // with — profiles itself, not merely its cfg — so every downstream + // profile read on this request, resolveActiveProfile's pin tier and + // set_profile's own admission alike, decides over that same pair rather + // than an independent Published()/runtime.Config() read that a reload + // landing mid-request could have already moved past it (round 8; round 9 + // MUST-FIX 1 extended this to set_profile, which previously ignored the + // injected context entirely). + ctx = withProfileRequestIndex(ctx, profiles) + next.ServeHTTP(w, r.WithContext(ctx)) +} + +// profileNotSelectable writes the single refusal a scoped caller receives from +// the profile URL whenever the requested slug is not one it may select. The +// body echoes the caller's own slug (not a disclosure) and never carries an +// `available` list or the pin, so a missing, deleted, unreachable or +// pin-mismatched profile — and an empty fleet — are indistinguishable +// (Spec 105 FR-004). +func profileNotSelectable(w http.ResponseWriter, slug string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "error": fmt.Sprintf("unknown profile '%s'", slug), }) } diff --git a/specs/105-agent-scope-hardening/research.md b/specs/105-agent-scope-hardening/research.md index 68b37fb58..241c13d63 100644 --- a/specs/105-agent-scope-hardening/research.md +++ b/specs/105-agent-scope-hardening/research.md @@ -7,6 +7,7 @@ **Decision**: refuse. `set_profile ` and `/mcp/p/` answer with the same body a deleted pin produces; no session mutation. **Rationale**: the observable today is "does my configured pin still exist" (admitted if it exists with zero reach, refused if deleted) — a profile-existence oracle held by the one caller the pin was meant to confine. FR-004's non-disclosure goal covers it even though the predicate text is worded for unpinned tokens. #1225 F2 locked "configured pin always selectable" before this oracle was noticed; the test (`TestHandleSetProfile_PinnedTokenSelectsDisjointPin`) is inverted, not deleted. **Alternatives**: keep admitting (preserves #1225 behaviour, keeps the oracle); admit but answer with an empty server set (still discloses existence via status). Rejected. +**Round-9 extension (review round 9, MUST-FIX 2)**: the same non-disclosure rule covers a pin **mismatch** — the caller's requested slug names a different, otherwise-reachable profile than its pin — which `handleSetProfile` had left as a distinct early branch (`"agent token is pinned to profile '%s' and cannot switch to '%s'"`, checked before any index lookup). That branch let a pinned token learn from the wording alone, for a slug it merely guessed, that it IS pinned and to what — an oracle finer than "does my pin exist" (D1's original scope): "is THIS caller pinned at all, and to THIS name". The branch is removed; a pin mismatch now falls through to `profileIndex.selectable`, the same predicate and the same `unknown profile ''` body as a deleted pin, a zero-reach pin, a disjoint unpinned grant and a nonexistent slug — one refusal shape for every non-selectable outcome, matching `contracts/refusals.md`'s `set_profile ` row (already "one format string" before this fix; the code had drifted from it). ## D2 — Anonymous callers and legacy/internal cache entries (FR-002, gap FR001-G1/G2) @@ -101,6 +102,23 @@ Per [prior-diff-assessment.md](prior-diff-assessment.md): port hunks 1–6 and 1 **Rationale**: codex rounds 2–5 kept finding the same root: any refusal that must *load* the producer snapshot (round 4's content-addressed snapshot behind a 128-entry LRU was O(1) only when warm — a restart or 129 distinct snapshots made the first probe decode a fleet-sized snapshot) or *decode* the legacy value (round 4's one-shot `preFramePayloadSize`) does work proportional to hidden state, which the spec's non-disclosing refusal forbids in the timing class. Measured before the fix: a cold scope refusal against a 5,000-server producer allocated 1,657,864 B against a 58,104 B miss; a legacy 4 MB entry's refusal allocated 8,500,992 B. After: wide = small = miss = 25,112 B on every probe, cold included. Including the principal in the digest is what lets a user be gated on the header (identity is necessary for users) and makes "digest-equal" mean *the same credential* for agents; a same-scope sibling agent is therefore refused too (fail-closed, same rule). **Alternatives**: a larger LRU or pre-warming at startup (still O(snapshot) somewhere, and a warm cache is itself hidden state); a bounded snapshot in the header (round 3, rejected in round 4: refused legitimate fleets); keeping the one-shot legacy decode (round 4, rejected in round 5: it is the oracle). Rejected. +## D17 — Profile index: warmed before publication, taken with its snapshot as one pair, matched to the PUBLISHED snapshot (FR-004; PR D codex rounds 3–7, review rounds 8–9) + +**Round 17 (codex, MUST-FIX — fixed)**: round 16 traced a consumer round 15's finding-B refutation did not reach — `cache_authz.go`'s producer/reader stamp — and gave a concrete failing scenario: `cacheAuthorizationWith` (`internal/server/cache_authz.go`) stamped `cache.Authorization.ProfileServers` from `scope.AllowedServerNames()` — the resolver's WILDCARD-derived FULL profile membership (round 15 finding B's accepted tier), unintersected with the caller's own `AllowedServers`. `CouldHaveProduced`/`coversAll` (`internal/cache/authorization.go`) then compares that stamped set on cache redemption, so a profile member entirely OUTSIDE an agent token's own grant — a server the token never had, and never will have, access to — enters the comparison: token `AllowedServers={a}` pinned to profile `P={a,b}` produces a cache entry stamped `{a,b}`; an operator later removes the hidden, never-authorized `b` from `P` (an event this token has no authorization to observe); the SAME token's next resolved scope is `{a}`, and `coversAll({a},{a,b})` is false, so its own previously-cached, still-authorized access to `a` is wrongly revoked — worse than an availability quirk, this makes the token's cache hit/miss for its OWN server an observable side-channel for whether an unrelated, never-authorized server still exists in the profile (SC-005-class disclosure through the cache layer, not the resolver/dispatch surfaces rounds 14-16 already covered). Round 15 finding B's refutation is UNCHANGED by this: `.Allows()`/`.DeniesAll()` stay O(1) and the resolver's profile's-own-membership semantic stays correct for them — this is a genuinely distinct consumer (the cache redemption comparison) that finding B's trace did not follow through to. Fix, precisely scoped to that one consumer: a new `resolveActiveProfileWithIndex` seam (`internal/server/profile_resolver.go`) mirrors `resolveActiveProfile` exactly (same branches, same `profileIndexFor(p.currentConfig())` fallback) but also returns the `*profileIndex` the (name, scope) pair was resolved against — so a caller needing something else from that IDENTICAL pair never re-resolves a possibly different published snapshot's index (the same discipline rounds 9/11/14/15 established for this exact seam). `cacheAuthorizationWith` now takes that `idx` and, for an AGENT caller specifically, stamps `ProfileServers` via `idx.EffectiveServersFor(profileName, ac.AllowedServers)` — the SAME O(len(caller-grant)) helper `handleSetProfile`'s own scoped-visible path already uses (`profile_tool.go:161-166`), never a profile-declared-size walk (closing round 16's restated cost concern at this call site too). Non-agent scoped callers (admin/user through a profile URL, no `AllowedServers` of their own) keep `scope.AllowedServerNames()` unchanged — round 15's wildcard/profile's-own-membership semantic for the resolver tier itself is untouched, exactly as scoped; `resolveActiveProfileIn`/`resolveActiveProfileFromIndex`'s own `ProfileScope` construction is not modified. Both call sites that previously resolved `(name, scope)` and `cacheAuthorizationWith` separately (`mcp.go` retrieve_tools and the `server:tool` dispatch path) now thread the same `idx` through; `cacheAuthorization(ctx)` (shared by both the `handleCallTool` producer stamp and the `handleReadCache` reader stamp) updates the same way, so producer and reader compute `ProfileServers` identically. Red tests first: `TestReadCache_CacheAuthzIntersectsCallerGrant_HiddenProfileMemberRemovalDoesNotRevoke` (token grant `{github}`, profile `{github,weather}`, narrowed to `{github}` after caching — reproduces the scenario exactly, fails on the base with the entry wrongly revoked) and a companion, `TestReadCache_CacheAuthzIntersectsCallerGrant_OwnGrantMemberRemovalStillRevokes` (token grant `{github,weather}`, same narrowing — already passed on the base, guarding that the fix does not weaken the pre-existing narrowing-revokes-access guarantee for a server the token WAS itself authorized for). Both pass after the fix; the full `TestReadCache*`/`TestResolveActiveProfile*`/`TestHandleSetProfile*`/`TestProfileRequests*` sweep is unaffected. **Conclusion, recorded here so it is not re-litigated: the cache producer/reader stamp intersects an agent token's own grant with the profile, same as every other admitted caller-bounded view; the resolver's own profile-membership tier (`.Allows()`/`.DeniesAll()` and friends) stays wildcard/profile's-own, per round 15.** Full trace and evidence in `.review-tmp/critique-r1.md` under "## Review round 17". + +**Round 15, finding A (codex, MUST-FIX — fixed)**: `resolveActiveProfile` (`internal/server/profile_resolver.go`) extracted only `injected.cfg` from the context-carried (index, snapshot) pair and passed it to `resolveActiveProfileIn`, which called `profileIndexFor(cfg)` — an INDEPENDENT `Published(cfg)`/`For(cfg)` lookup, exactly the pair-acquisition bypass rounds 11/13 closed on the admission path (`profileIndexCurrent`/`Acquire`), reopened here on the downstream resolution path a paused request reaches next: a request admitted with pair A that then paused while two further publications (B, C) landed would find A evicted from both the `warm` and `previous` slots by the time `resolveActiveProfile` ran, so `Published(A)` would miss and `For(A)` would rebuild the whole fleet inline for what must stay an O(1) decision. Fix: `resolveActiveProfile` now calls `p.resolveActiveProfileFromIndex(ctx, injected)` directly with the already-resolved pair when `profileRequestIndexFromContext` succeeds, bypassing `resolveActiveProfileIn`/`profileIndexFor(cfg)` entirely on that path; the `profileIndexFor(cfg)` route is now reached only for the no-injected-pair case (plain `/mcp`, no URL admission), unchanged. Red test first (fails on the base with `lazyBuilds == 1`, confirmed by reverting only `profile_resolver.go` via a saved patch and rerunning, restored immediately): `TestResolveActiveProfile_UsesInjectedPairDirectly_NeverFallsBackToFor` (`internal/server/profile_resolver_test.go`) warms pair A, then publishes B and C so A is evicted from both slots, injects A on the context, resolves a pin against it, and asserts both the resolved scope (A's own servers, proving it decided from the injected pair and not a re-derived one) and `profileIndexes.lazyBuilds.Load() == 0` (proving `For` was never reached). `research.md` D17 gains this entry; `.review-tmp/critique-r1.md` gains "## Review round 15". + +**Round 15, finding B (codex, REFUTED — no code change)**: a review finding argued that `resolveActiveProfileIn`'s pin/session tiers pass a hardcoded wildcard grant (`[]string{"*"}`, `profile_resolver.go` ~265-273) into `effectiveServersForCandidate` instead of the caller's own `AllowedServers`, so a profile-scoped read materializes the profile's FULL declared membership (`NewProfileScope`'s underlying map, one entry per declared server) regardless of how small the caller's own grant is — e.g. a token allowed only `{a}` under a profile declaring 4 096 servers pays O(4096), not O(1), for a request that decides `{a}` either way — and that this timing gradient discloses the hidden profile population to a caller who cannot see most of it. Traced by tracing every downstream consumer of the ProfileScope this construction produces (`grep profile.ProfileScope` / `profileScope\.` across `mcp.go`, `mcp_routing.go`, `mcp_direct_scope.go`, `mcp_visibility.go`, `mcp_code_execution.go`, `cache_authz.go`, `preflight_glue.go`) and, separately, by tracing whether `resolveActiveProfileIn` can be changed to intersect with the caller's own grant without changing behaviour: **REFUTED, on both counts**. (1) Round 14 already implemented and reverted exactly this fix, and says so in its own D17 entry above ("`resolveActiveProfileIn`'s pin and session tiers render the profile's OWN full membership through `EffectiveServersFor(name, []string{\"*\"})` — a caller-agnostic perf substitute for `EffectiveServers`, not a caller-intersection change... changing that here broke `TestResolveActiveProfile_PinHighestPrecedence`'s fixture (a pin context with no `AllowedServers` set) until reverted to the wildcard-marker design"). Reproduced directly this round: swapping the wildcard for `auth.AuthContextFromContext(ctx).AllowedServers` inside `profileScopeFromIndex`'s two call sites breaks `TestResolveActiveProfile_PinHighestPrecedence` (`profile_resolver_test.go` — a pin context deliberately carrying no `AllowedServers`, i.e. an empty grant, asserts `scope.Allows("research-srv") == true`; with the caller's own grant substituted, `effectiveServersForCandidate`'s `len(allowed) == 0` guard returns nil and the assertion fails) — the tier's decoupling from the caller's `AllowedServers` is deliberate, tested, TWICE-established design (round 14's own history, reconfirmed here), not an oversight this round can silently re-litigate: the profile's own membership is the pin's/session-selection's authority, and the credential-specific view is applied SEPARATELY, downstream, on every dispatch path that actually executes something (agent-scope `CanAccessServer`/`WithToolFilter` enforcement, independent of and prior to any `profileScope.Allows()` check — `mcp.go:2300-2313`'s own comment: "profile filter — runs independently of agent-scope"). (2) Every actual downstream consumer of the returned scope performs O(1) work per item against the already-built set — `Allows()` (`mcp.go:2309`, `mcp_direct_scope.go:81/124/227`, `mcp_routing.go:425`, `mcp_visibility.go:81`) is one map lookup; `DeniesAll()` (`mcp.go:1746`, gating `index.ForProfile` selection — a shared, profile-NAME-keyed physical index, not caller-scoped) is `len(p.servers)==0`. The two consumers that DO enumerate the full set (`cache_authz.go:56` `scope.AllowedServerNames()`, stamping `cache.Authorization.ProfileServers`; `mcp_code_execution.go:1351` `applyProfileScopeToExecution`'s `AllowedServerNames()` before intersecting with the caller's own `options.AllowedServers`) render the PROFILE's declared membership as a value the design already treats as legitimately disclosable to an admitted holder of that pin/selection (it is what `set_profile`'s own admin-caller response and `cache_authz`'s producer stamp have always recorded — `handleSetProfile`, in the SAME function, separately computes a caller-intersected view via `profiles.EffectiveServersFor(effectiveProfileName, ac.AllowedServers)`, `profile_tool.go:161-166`, precisely where a caller-bounded answer IS the contract) — narrowing `cache_authz.go`'s `ProfileServers` stamp to the producer's own grant-intersected subset would also change `cache.Authorization.CouldHaveProduced`'s cross-caller/cross-profile comparison semantics (`internal/cache/authorization.go`), a SEPARATE, security-sensitive surface with its own dedicated tests (`mcp_read_cache_authz_test.go`) that this finding does not name and that a round-15 review has no mandate to re-derive. Conclusion: the O(profile-declared) cost is the round-14-ACCEPTED tier (explicitly traded down from O(fleet), never claimed to be O(caller-grant) — round 14's own text: "identical output, no timing promise broken, at O(profile size) instead [of O(fleet)]"), it is bound to the profile the caller is ADMITTED to (pin or session selection), not a foreign hidden one, and every consumer's own further work is O(1) or renders that same already-intended value — no consumer performs work proportional to a population the caller has no claim to be resolving against. Reopening it to O(caller-grant) requires reopening round 14's tested contract wholesale (decoupling `resolveActiveProfileIn` from `AllowedServers`, which two independent attempts — round 14's and this round's reproduction — both show breaks `TestResolveActiveProfile_PinHighestPrecedence`), which is out of scope for a round-15 finding review. Full trace and the reproduced-break evidence are in `.review-tmp/critique-r1.md` under "## Review round 15". + +**Round 14 (codex, MUST-FIX — fixed)**: after O(1) admission, a successful scoped READ still materialized its effective server set by walking every configured server: `/mcp/p/` (`serveProfileURL`, `internal/server/server.go`) called `config.ProfileConfig.EffectiveServers(cfg)`, which rebuilds a `known := make(map[string]struct{}, len(cfg.Servers))` set from scratch on EVERY call; `resolveActiveProfileIn`'s pin and session tiers (`internal/server/profile_resolver.go`) called the same function, once per admitted call to `resolveActiveProfile` — the seam every scoped `retrieve_tools` / `describe_tool` / `call_tool_*` / `code_execution` call runs through under a pinned or session-selected profile; and `handleSetProfile`'s success payload (`internal/server/profile_tool.go`) called it a second, independent time (via `profileServersIn`) even after the resolver had already paid for it once, plus walked `allServerNames(cfg)` — every configured server — for the "cleared session" case before filtering it through the caller's credential name by name. The bitset the index already carries (`profileIndex.members`, built once per snapshot by `newProfileIndex`) was reachability-only (`hasMember`/`reach`); nothing translated a caller's own grant into the actual SERVER NAMES admitted reads must return. Fix: `profileIndex.EffectiveServersFor(profileName string, allowed []string) []string` (and its int-keyed sibling `effectiveServersForCandidate`, for a caller that already paid for `position(profileName)` in the same request) intersects `allowed` — the reader's own `AllowedServers`, or the `"*"` marker for an unrestricted read — against the profile's servers using the index's precomputed `serverPos` map, never rebuilding a fleet-sized set: O(len(allowed)) for a restricted grant (plus, for a wildcard grant, one walk of the profile's own declared servers or, for the "no profile" case, of `cfg.Servers` — the same O(fleet) an administrator already pays for that answer, per SC-005), never O(len(cfg.Servers)) to reach it. `serveProfileURL` now renders a scoped caller's view through it instead of `EffectiveServers`, reusing the position `profiles.selectable`'s admission already resolved rather than looking the slug up a third time (the gate's own traversal-counter test, `TestProfileMiddleware_GateTouchesOnlyRequestedSlugAndPin`, bounds the admitted path to slug-twice-plus-pin, so the fix could not add a lookup, only replace what the existing third lookup renders). `resolveActiveProfileIn`'s pin and session tiers render the profile's OWN full membership through `EffectiveServersFor(name, []string{"*"})` — a caller-agnostic perf substitute for `EffectiveServers`, not a caller-intersection change, since a scope's own membership has always been intersected with the credential separately, downstream (existing tests pin a pin tier scope covering the full pinned profile regardless of the token's own, possibly empty-in-the-fixture, `AllowedServers`). `handleSetProfile`'s scoped success path now calls `EffectiveServersFor` directly with the caller's REAL grant, replacing `scopeServersIn`/`callerVisibleServers` (both deleted, now unreachable) outright, including the "cleared session" case that used to walk `allServerNames`. A structural hazard surfaced while fixing this: `profileIndex.position` previously trusted a `byName` position built at construction without re-checking it against `idx.cfg.Profiles`' CURRENT length, which a raw test fixture that mutates `*config.Config` in place after building an index (`cfg.Profiles = nil`, never replacing the pointer — `TestSetProfileClearReportsPinnedScope`, `TestReadCache_DeletedPinnedProfileRevokesCachedAccess`) could turn into an out-of-range index; `position` now bounds-checks against the CURRENT length too, a no-op for any snapshot that is never mutated in place (every production and warm-path snapshot) and a graceful "not found" for the ones that are. **Conclusion, recorded here verbatim so it is not re-litigated: admitted READS also decide from the index, never from a fleet walk.** + +**Round 13 (codex, REFUTED — no code change)**: a review finding argued that a write-capable agent's config mutation (`upstream_servers add`) now builds the profile index synchronously inside the pre-publish observer, so its latency scales with hidden profiles/servers, contrary to D17's "no request builds an index" rule. Traced by file:line and refuted: `Runtime.applyConfigLocked` (`internal/runtime/runtime.go`) already runs `DetectConfigChanges` (walking and JSON-diffing the whole `Servers` slice, `internal/runtime/config_hotreload.go:200-201`) at `runtime.go:1717` and again at `runtime.go:1798` (a third time, conditionally, at `runtime.go:1774`), `config.SaveConfig` (`json.MarshalIndent`s the entire config, `internal/config/loader.go:393`) at `runtime.go:1742`, and `upstreamManager.SetGlobalConfig` (walks every live client, `internal/upstream/manager.go:364-374`) at `runtime.go:1839` — all BEFORE `configSvc.Update` (`runtime.go:1884`) ever runs the pre-publish observers that build the index. Measured at 4096 profiles/servers: `newProfileIndex` = 437 µs/op vs `SaveConfig` = 19.42 ms/op and one `DetectConfigChanges` pass = 9.81 ms/op (called 2-3× per apply) — the observer is under 1.1% of the pre-existing O(fleet) work, not comparable to or larger than it, so the maintainer's move-the-bitset-out-of-the-observer branch does not fire. The `profiles×servers` bitset was also confirmed NOT to be a droppable dead artifact: `idx.reach` (`profile_tool.go:441-465`) reads it via an O(1) bit test on every scoped call, which is the entire mechanism that keeps profile selectability O(reader grant) rather than O(fleet) (D1). Conclusion, recorded here verbatim so it is not re-litigated: no request refusal or admitted read path builds or walks the index. The index is built once per publication, inside the publication, whose cost is already proportional to configuration size (marshal, save, change detection, reconcile). Full trace, the benchmark numbers and the bitset-consumer check are in `.review-tmp/critique-r1.md` under "## Review round 13". + +**Decision**: the per-snapshot profile index (`internal/server/profile_tool.go`, `profileIndexCache`) is built **before the snapshot it covers is published**, and **the request path takes the index and its snapshot as one pair — no request builds an index**. (1) `configsvc.Service` gains a read-only observer list, `AddPrePublishObserver(func(*config.Config))`, separate from the single `SetPrePublishHook` slot the #937 admission gate owns: observers run inside `updateLocked` after the gate hook has produced the final config and before `snapshot.Store`, i.e. on the exact `*Config` that will be published, while nothing else can observe it; they must not mutate it and, running under `updateMu`, must be cheap (the index is one insertion per profile) and must never publish (`Update` / `UpdateIfCurrent` / `ReloadFromFile` from an observer is a re-entrant publish and deadlocks by design, like the hook); the observer list is copied before observers run, so an observer may register another (it first runs on the next publication). `NewServer` registers `profileIndexes.warmPublishing` there; the construction-time warm stays for the initial snapshot (`NewService` stores it without running observers) and the config-event warm stays as belt-and-braces. (2) The **warm** slot holds the latest prepared pair, written only by the warm path (observer, construction, event — the event path defers to the observer once it has run, so a late event can never roll the slot back); `warmPublishing` now also demotes the prior warm pair into a **`previous`** slot instead of discarding it. (3) **The request-visible index tracks the PUBLISHED snapshot exactly — `runtime.Config()` is the one atomic publication boundary every reader agrees on.** `profileIndexCache.Published(published *config.Config)` returns whichever of `warm`/`previous` has `cfg == published` — O(1), no build — and is what `profileMiddleware` and `profileIndexCurrent` call with their own fresh `runtime.Config()` / `currentConfig()` read; `Current()` (the old, unconditional "latest pair" read) is kept only for cache-mechanics call sites with no runtime.Config() to match against (bare-cache tests). Because the observer runs under `updateMu`, at most one prepared-but-not-yet-stored pair can exist at any time, so "warm, else previous" is always either the published pair or the one about to replace it. When neither matches — a bare Server with no runtime — callers fall back to `For(published)`, counted on `lazyBuilds`, which must stay zero over a live runtime. (4) **The request is self-consistent end to end.** `serveProfileURL` injects the admitted **PAIR itself** into the request context (`withProfileRequestIndex`, alongside the existing `profile.WithProfileScope` — round 9 widened this from carrying only `cfg` to carrying the `*profileIndex`, so a downstream reader gets the index the gate already built, not merely its snapshot); `resolveActiveProfile` reads it (via `injected.cfg`) in preference to a fresh `currentConfig()` call, falling back to the live read only when no pair was injected (the base `/mcp` endpoint, or a bare test server) — so a reload landing strictly between admission and a downstream read (pin resolution) cannot split the two across snapshots. Round 8 believed `handleSetProfile` needed no context change because it threaded its own local `cfg` through admission and `resolveActiveProfileIn` — true, but that local `cfg` came from `profileIndexCurrent()` calling `Published(p.currentConfig())` **unconditionally**, an independent `runtime.Config()` read that ignored the very context `serveProfileURL` had just injected. Round 9 (MUST-FIX 1) closed that: `profileIndexCurrent(ctx)` now checks `profileRequestIndexFromContext` FIRST and returns the injected pair outright — no build, no `Published` lookup, no `runtime.Config()` read at all — falling through to the pre-round-9 `Published`/`For` chain only when the request carries no admitted pair (the base `/mcp` endpoint). `set_profile` on `/mcp/p/` therefore now decides admission, the reported server list and the effective scope from the SAME pair the URL gate admitted the request against, for the whole lifetime of that request, however many reloads land while it runs. (5) Round 11 (MUST-FIX) closed the last two production call sites that still read-then-matched instead of acquiring atomically: `profileMiddleware` (`s.runtime.Config()` then `Published`/`For`) and `profileIndexCurrent`'s base-`/mcp` branch (`p.currentConfig()` then `Published`/`For`) both replaced that two-step with `profileIndexCache.Acquire(readCfg func() *config.Config) *profileIndex`, which loops up to `profileIndexAcquireRetries` (8) times — each iteration RE-READS `readCfg()` and matches it via `Published` in one call, so a publication landing strictly between a caller's own config read and the match is simply observed on the next iteration instead of stranding the request on a stale snapshot neither `warm` nor `previous` covers any more. Every iteration is O(1) (`Published` never builds), so exhausting the whole budget costs O(retries), never a fleet-sized build. Acquire itself never builds and never falls back — a miss (nil) is for the CALLER to interpret: `serveProfileURL` and `handleSetProfile` fail a scoped caller closed on nil with the exact uniform, O(1) refusal every other non-selectable slug gets (no lookup, no disclosure, no build), while an administrator-shaped caller (not timing-contract-bound, SC-005) falls back to a fresh `For` build, matching pre-105 behaviour. `For` remains only the bare-test-server fallback (no runtime, so no atomic `readCfg` to loop against) and the two caches' own `Current()`/warm-path mechanics, which Acquire does not touch. +**Rationale**: round 3 warmed the index from the config event, which left the publication→event-delivery window in which a request built the fleet-sized index inline — reachable by a token with server-write permission (`upstream_servers` add, then probe). Round 5 found the single slot was also a rollback oracle: a request that captured old snapshot A, stalled through a reload to B (warmed) and then built A overwrote B's index, so the next request under B rebuilt 4 096 hidden profiles while the empty-fleet equivalent did nothing. Round 6 found the round-5 two-slot answer (previous warm demoted to the lazy slot) still let a request that captured A and paused across TWO publications (warm C, lazy B) rebuild A inline — any design in which the request captures its snapshot separately from the index leaves some such window; taking the pair closed the class. Round 7 (this round's MUST-FIX) found that taking the pair was not by itself enough: the pair was always the cache's unconditional **latest** one, which the pre-publish observer sets one publication AHEAD of `runtime.Config()` for the microseconds before `snapshot.Store` — so during that window a request could be *admitted* against the next config while `resolveActiveProfileIn`'s pin tier, reading `runtime.Config()` independently, still answered the previous one, splitting admission from the effective scope within one request (and, for an unpinned caller, admitting a profile the published config does not yet define). The two-slot `Published` match closes the admission-time half of that window (the request-visible pair is never ahead of what `runtime.Config()` answers at read time); the context-injected `cfg` closes the residual, purely-timing half (a reload completing strictly between the gate's own read and a downstream handler's independent one). Round 9 (MUST-FIX 1) found that round 8's context injection only reached `resolveActiveProfile` — `handleSetProfile`'s own admission still called `profileIndexCurrent()`, which did its OWN `Published(p.currentConfig())` read regardless of what the gate had injected, so a `set_profile` call inside a `/mcp/p/` request could decide with a *different* snapshot than the one the URL gate had just admitted that same request against (concretely: a profile present only at admission time, removed by a reload landing before `set_profile` ran, was wrongly refused; a profile present only in that later reload was wrongly admitted) — the very split D17 exists to close, reopened at one more call site. Widening the injected value from `cfg` to the whole pair, and making `profileIndexCurrent` check it FIRST, closes it: `set_profile` on a URL-scoped request no longer performs a `Published`/`runtime.Config()` read of any kind. Round 11 (codex, PR D review round 9's follow-up) found the round 7/8/9 fix still froze ONE `runtime.Config()`/`currentConfig()` read per request and matched it against the cache in a second, separate step (`profileMiddleware`, and `profileIndexCurrent`'s base-`/mcp` branch): a request paused between those two steps across two publications landed on a snapshot neither `warm` nor `previous` covered any more, and the existing `Published`-miss fallback to `For` built the whole fleet inline for what must be a constant-cost refusal — the very fleet-population timing oracle D17 exists to close, just relocated from the admission race (rounds 7-9) to the acquisition race between a request's own config read and the cache lookup. All are timing/consistency-class disclosures or splits across fleet-population and publication state (spec Definitions: non-disclosing = status, body AND timing class). +**Alternatives**: wrapping/chaining the admission-gate hook (a single `Store`-replaced slot with no getter; coupling the index to the gate's ownership) — rejected; a versioned single slot (needs the snapshot version in the observer signature) — rejected; keeping the demoted-previous slot with a deeper history (round 6) — rejected, since no history depth removes the paused-request window and the request no longer needs one (it holds the index it was handed); reading `runtime.Config()` once at the top of every downstream profile-resolution call instead of threading it through context (round 8) — rejected, since it does not close the gate-to-handler window a mid-request reload opens, only narrows it; having `handleSetProfile` re-derive its pair from a context-injected `cfg` via a second `Published(cfg)` call (round 9, rejected in favour of injecting the pair itself) — redundant work and still a lookup that could in principle miss (a `Published` slot evicted by two further publications between injection and the lookup) where handing over the pair outright cannot. +**Tests**: `TestProfileRequests_ServeThePublishedSnapshotDuringObserverWindow` (supersedes round 6's `TestProfileRequests_ServeTheIndexAboutToBePublished`, which pinned the pre-round-8 — wrong — "admit against the next snapshot" behaviour as expected: during the observer-to-Store window a profile that exists only in the next config is refused by both the URL gate and `set_profile`, a profile being widened reports its still-published narrow set through both the URL gate and the downstream resolver, and the very next request after `Store` gets the new pair — all with `lazyBuilds` at zero throughout); `TestProfileMiddleware_InjectsAdmittedSnapshotForDownstreamResolution` (a config change landing inside the downstream handler, after admission, must not change what a pinned caller's `resolveActiveProfile` resolves on that request); `TestProfileMiddleware_SetProfileDecidesWithTheAdmittedIndexNotAFreshRead` (round 9: same shape, for the `set_profile` TOOL call inside the request rather than `resolveActiveProfile`'s pin tier — a profile present only in the admitted snapshot stays selectable after a reload removes it, and a profile present only in the reload stays refused, both within the one admitted request); `TestProfileIndexCache_PausedRequestKeepsTheIndexItWasHanded`, `TestProfileRequests_NeverBuildTheIndexOverARuntime` unchanged. Round 11 adds `TestProfileIndexCache_Acquire_RecoversAfterConfigMovesBetweenReads` (a `readCfg` seam that answers stale A — after two publications have already landed B then C underneath it — misses on the first iteration and recovers C's own pair on the next, zero builds), `TestProfileIndexCache_Acquire_ExhaustsBoundedRetriesWithoutBuilding` (a `readCfg` seam that never answers a warmed snapshot exhausts exactly `profileIndexAcquireRetries` iterations and returns nil, never a build), `TestProfileMiddleware_AcquireMissFailsClosedForScopedFallsBackForAdmin` and `TestHandleSetProfile_AcquireMissFailsClosedForScopedFallsBackForAdmin` (an Acquire miss reaching `serveProfileURL` / `handleSetProfile` refuses a scoped caller uniformly with zero builds and falls back to a fresh administrator build). + ## H0 — accepted residual: unbounded per-rebuild index memory (codex r12 SHOULD, finding 2) **Decision**: not fixed. `internal/codescripts`'s directory-index rebuild (`dirfd_other.go`, `storednames_other.go`, `storednames_windows.go`) reads a full entry listing into a slice/map per rebuild with no per-directory size or byte cap; a scripts directory with an extreme entry count could make `Warm` or an async rebuild allocate proportionally. Left unbounded because rebuild concurrency is already capped process-wide at 2 (`maxConcurrentRebuilds`, `rebuildsemaphore.go`), the scripts directory is operator-controlled (not agent-writable), and bounding it is real scope beyond H0's SC-005 disclosure fixes. Revisit only if a real deployment reports memory pressure from this path. diff --git a/specs/105-agent-scope-hardening/tasks.md b/specs/105-agent-scope-hardening/tasks.md index 6dbd2d9e6..81cd57f03 100644 --- a/specs/105-agent-scope-hardening/tasks.md +++ b/specs/105-agent-scope-hardening/tasks.md @@ -93,26 +93,26 @@ ### Failing tests -- [ ] T035 [US1] Generalise `mintPinnedToken` to `mintAgentToken(t, env, name, allowed, perms, pin)` in `internal/server/profile_integration_test.go:627` (H1 reuses it) -- [ ] T036 [US1] FR003-G1/G2/G5: `a`-only unpinned token — `/mcp/p/deploy` (disjoint), `/mcp/p/nonexistent`, `/mcp/p`, `/mcp/p/`, deleted `deploy` → equal status+body, no `available`; `/mcp/p/research` positive control — `internal/server/profile_integration_test.go` -- [ ] T037 [P] [US1] FR003-G3/G4: pinned `research` — `/mcp/p/deploy`, `/mcp/p/nope`, deleted-pin `/mcp/p/research` → identical; fleet `[deploy]` vs `nil` → identical per slug — `internal/server/profile_integration_test.go` -- [ ] T038 [P] [US1] FR003-G6: ctx scoped `[research-srv, deploy-srv]` + `profile.WithProfileScope(research)`; `set_profile deploy` → `active_profile == "deploy"` (stored) but `servers == [research-srv]` (URL governs: URL profile ∩ token, `spec.md:112,126`); clear → `[research-srv]` — `internal/server/profile_tool_test.go` -- [ ] T039 [P] [US1] FR003-G7: pinned `research`, `set_profile ""` → `active_profile == ""`, `servers == [research-srv]` — `internal/server/profile_pin_enforcement_test.go` -- [ ] T040 [P] [US1] FR003-G8 (D1): pinned `empty`/ghost pin — `set_profile empty` → deleted-pin body, no mutation; `/mcp/p/empty` → uniform 404 — `internal/server/profile_tool_test.go` + `internal/server/profile_integration_test.go` +- [x] T035 [US1] Generalise `mintPinnedToken` to `mintAgentToken(t, env, name, allowed, perms, pin)` in `internal/server/profile_integration_test.go:627` (H1 reuses it) +- [x] T036 [US1] FR003-G1/G2/G5: `a`-only unpinned token — `/mcp/p/deploy` (disjoint), `/mcp/p/nonexistent`, `/mcp/p`, `/mcp/p/`, deleted `deploy` → equal status+body, no `available`; `/mcp/p/research` positive control — `internal/server/profile_integration_test.go` +- [x] T037 [P] [US1] FR003-G3/G4: pinned `research` — `/mcp/p/deploy`, `/mcp/p/nope`, deleted-pin `/mcp/p/research` → identical; fleet `[deploy]` vs `nil` → identical per slug — `internal/server/profile_integration_test.go` +- [x] T038 [P] [US1] FR003-G6: ctx scoped `[research-srv, deploy-srv]` + `profile.WithProfileScope(research)`; `set_profile deploy` → `active_profile == "deploy"` (stored) but `servers == [research-srv]` (URL governs: URL profile ∩ token, `spec.md:112,126`); clear → `[research-srv]` — `internal/server/profile_tool_test.go` +- [x] T039 [P] [US1] FR003-G7: pinned `research`, `set_profile ""` → `active_profile == ""`, `servers == [research-srv]` — `internal/server/profile_pin_enforcement_test.go` +- [x] T040 [P] [US1] FR003-G8 (D1): pinned `empty`/ghost pin — `set_profile empty` → deleted-pin body, no mutation; `/mcp/p/empty` → uniform 404 — `internal/server/profile_tool_test.go` + `internal/server/profile_integration_test.go` ### Implementation -- [ ] T041 [US1] `profileMiddleware` evaluates `selectableProfileNames` (keyed on `auth.IsScopedCaller`) for `/mcp/p/`, `/mcp/p`, `/mcp/p/`; ONE refusal constructor (`profileNotSelectable(w)`) for missing/deleted/not-selectable/pin-mismatch/no-profiles/zero-reach; no-profiles branch moved after the gate; admin/anonymous branches unchanged — `internal/server/server.go:2320-2384,2703-2705` -- [ ] T042 [US1] `handleSetProfile`: pin branch requires reach (D1); `servers` = effective scope after the update via `resolveActiveProfile` (pin > URL > session) ∩ token — on a URL-scoped endpoint that is the URL profile, not the stored selection; `active_profile` = stored selection; cleared pinned selection reports `active_profile == ""` — `internal/server/profile_tool.go:72-108,188-210` -- [ ] T043 [P] [US1] Update the cleared-selection line in `docs/features/profiles.md:70-72` +- [x] T041 [US1] `profileMiddleware` evaluates the selectable-profile rule for the requested slug ONLY (`profileIndex.selectable` over a per-snapshot slug index with precomputed per-profile reach bitsets, keyed on `auth.IsScopedCaller`; never `selectableProfileNames`, whose cost is fleet-sized — codex round 2; reach costs the same for a missing, deleted or 4 096-server candidate — codex round 3; reach is O(|token grant|), one membership test per `allowed_servers` entry against the candidate's precomputed set, never a walk of the configured servers — codex round 4; the index is warmed at construction and on every config event, not by the first request — codex round 3) for `/mcp/p/`, `/mcp/p`, `/mcp/p/`; ONE refusal constructor (`profileNotSelectable(w)`) for missing/deleted/not-selectable/pin-mismatch/no-profiles/zero-reach; no-profiles branch moved after the gate; admin/anonymous branches unchanged; every scoped refusal logs one operator-facing line (`profile URL refused for scoped caller`: agent_name, profile, remote_addr — critique round 1, S1) — `internal/server/server.go` +- [x] T042 [US1] `handleSetProfile`: admission decides the requested slug alone through the same per-snapshot index (`profileIndex.selectable`, never the selectable list); a scoped caller's refusal is the list-free `unknown profile ''`, administrators keep the `available:` list (codex round 3); pin branch requires reach (D1); for scoped callers `servers` = effective scope after the update via `resolveActiveProfileIn` (pin > URL > session, same config snapshot as the admission check) ∩ token, rendered in profile-declared order — on a URL-scoped endpoint that is the URL profile, not the stored selection; `active_profile` = stored selection; cleared pinned selection reports `active_profile == ""`. Administrators short-circuit to the pre-105 payload (selected profile's servers / all servers on clear) — SC-005 names no FR-003 exception, so the URL-precedence reporting is agent-only (critique round 1, A1/A2) — `internal/server/profile_tool.go` +- [x] T043 [P] [US1] Update the cleared-selection line in `docs/features/profiles.md:70-72` ### Inverted pinned tests -- [ ] T044 [US1] Invert `internal/server/profile_integration_test.go:201-233,647-703,775-808`, `internal/server/profile_tool_test.go:344,356,397-411` (`TestHandleSetProfile_PinnedTokenSelectsDisjointPin` → refuses), `internal/server/profile_pin_enforcement_test.go:150,160`; keep `TestHandleSetProfile_AdminUnchanged`, `TestProfile_404UnknownSlug/404NoProfiles` as admin controls +- [x] T044 [US1] Invert `internal/server/profile_integration_test.go:201-233,647-703,775-808`, `internal/server/profile_tool_test.go:344,356,397-411` (`TestHandleSetProfile_PinnedTokenSelectsDisjointPin` → refuses), `internal/server/profile_pin_enforcement_test.go:150,160`; keep `TestHandleSetProfile_AdminUnchanged`, `TestProfile_404UnknownSlug/404NoProfiles` as admin controls ### Verification -- [ ] T045 [US1] Common verification + `go test -tags server -race ./internal/serveredition/...` (AuthTypeUser is scoped) +- [x] T045 [US1] Common verification + `go test -tags server -race ./internal/serveredition/...` (AuthTypeUser is scoped) - [~] T046 [US1] Live check: daemon with two profiles; unpinned scoped token hits `/mcp/p/` and `/mcp/p/`; bodies diffed byte-equal - [~] T047 [US1] Astra rounds on FR-003/004 + FR003-G1…G8 + D1; quote final `VERDICT:`