diff --git a/docs/features/agent-tokens.md b/docs/features/agent-tokens.md index 9ecd3744..2da194b7 100644 --- a/docs/features/agent-tokens.md +++ b/docs/features/agent-tokens.md @@ -206,6 +206,7 @@ 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. - **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. Resolution precedence (highest wins): @@ -216,7 +217,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, requests are **warn-skipped** rather than hard-failed — the pin still blocks switching away, so the token can never silently widen its scope, but profile filtering falls through to the next precedence tier. 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. 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. 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 3d90a6cb..9b4ecfb0 100644 --- a/docs/features/profiles.md +++ b/docs/features/profiles.md @@ -60,8 +60,8 @@ 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_*`, and `code_execution` 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). +- 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. - An unknown slug is rejected: `unknown profile '' (available: research, deploy)`. - Session state is cleared automatically on session close. @@ -73,12 +73,12 @@ When more than one source could select a profile, the effective profile for a re | # | Source | Scope | |---|--------|-------| -| 1 | Agent-token `profile_pin` | Server-enforced, immutable for the connection. *(Hook reserved for Profiles v2 T3; inert until then.)* | +| 1 | Agent-token [`profile_pin`](./agent-tokens.md#profile-pinning) | Server-enforced, immutable for the connection. If the pinned profile has been deleted, the request resolves to a **deny-all** scope rather than falling to the tiers below. | | 2 | URL `/mcp/p/` | Explicit and authoritative **for that request** — overrides the session default. | | 3 | `set_profile` session selection | The default for the base `/mcp` endpoint for the session lifetime. | | 4 | None | No filtering (admin / all servers). | -So a request that arrives via `/mcp/p/` is scoped to `` even if the session previously ran `set_profile`; a session selection that no longer matches any configured profile is treated as stale and dropped. +So a request that arrives via `/mcp/p/` is scoped to `` even if the session previously ran `set_profile`; a session selection that no longer matches any configured profile is treated as stale and dropped. A stale **token pin** is not dropped the same way — a pin is a restriction an operator applied to a credential, so it fails closed (deny-all) instead of widening back to the token's own scope. ## REST API diff --git a/docs/features/tools-preflight.md b/docs/features/tools-preflight.md index 2de62cae..675c1442 100644 --- a/docs/features/tools-preflight.md +++ b/docs/features/tools-preflight.md @@ -184,7 +184,7 @@ Preflight answers with different candor depending on who is asking: The agent-token behavior is deliberate **scope-silence**: an out-of-scope probe learns nothing — not even that the server exists. `did_you_mean` suggestions (nearest-name, up to 3) are computed over the caller-visible index only and never name a quarantined server's tools. See [Agent Tokens](./agent-tokens.md) and [Profiles](./profiles.md). -A token's evaluation scope is the intersection of its `allowed_servers`, its `profile_pin`, and any `profile` in the request — so naming another profile can only narrow it. If the pinned profile has since been **deleted**, the scope becomes deny-all and every id answers `not_found`: the pin is a restriction the operator applied, and losing the profile it names must never hand the token a wider view than it had before. Re-mint the token (or re-create the profile) to restore it. +A token's evaluation scope is the intersection of its `allowed_servers`, its `profile_pin`, and any `profile` in the request — so naming another profile can only narrow it. If the pinned profile has since been **deleted**, the scope becomes deny-all and every id answers `not_found`: the pin is a restriction the operator applied, and losing the profile it names must never hand the token a wider view than it had before. The live MCP session path resolves the same way — a preflight's `not_found` for a stale pin is never a false alarm the session would contradict. Re-mint the token (or re-create the profile) to restore it. ## Transparency: every preflight is on the record diff --git a/internal/profile/context.go b/internal/profile/context.go index 74903566..32eb380a 100644 --- a/internal/profile/context.go +++ b/internal/profile/context.go @@ -1,19 +1,21 @@ // Package profile carries request-scoped in-proxy profile filtering (Spec 057). // // A profile is a named, stateless view over a subset of the configured upstream -// servers, addressable at /mcp/p/. A ProfileScope is resolved once by the -// profile middleware from the request URL and injected into the request context; -// the scope filters which servers a request may see/call. It is an independent, -// auth-type-agnostic primitive that composes with (but does not depend on) the -// Spec 028 agent-token scope — an unauthenticated /mcp/p/ connection runs -// as an admin AuthContext yet must still be profile-filtered. +// servers, addressable at /mcp/p/. A ProfileScope is resolved once per +// request and filters which servers that request may see/call. The URL tier is +// only one of its sources: the server package's resolver (Profiles v2) also +// builds a scope from an agent token's profile_pin and from a session's +// set_profile selection, in that precedence order. The scope stays an +// independent, auth-type-agnostic primitive that composes with (but does not +// depend on) the Spec 028 agent-token scope — an unauthenticated /mcp/p/ +// connection runs as an admin AuthContext yet must still be profile-filtered. package profile import "context" -// ProfileScope is the immutable, request-scoped set of servers a profile exposes. -// It is resolved by profileMiddleware from the /mcp/p/ URL and never mutated -// for the lifetime of a request. +// ProfileScope is the immutable, request-scoped set of servers a profile +// exposes. It is resolved once — from a token pin, a /mcp/p/ URL, or a +// session selection — and never mutated for the lifetime of a request. type ProfileScope struct { // Name is the resolved profile slug, used in rejection messages and activity // metadata (FR-012). @@ -36,10 +38,10 @@ func NewProfileScope(name string, servers []string) *ProfileScope { // Allows reports whether the named server is visible under this scope. // -// A nil receiver means the request did not enter via /mcp/p/ (it used /mcp, -// /mcp/code, or /mcp/call) and therefore is not profile-filtered — every server -// is allowed (FR-010). A non-nil scope allows only servers in its set; the empty -// server name is never allowed for a real scope. +// A nil receiver means no profile is in effect for this request — no URL slug, +// no session selection, no token pin — so it is not profile-filtered and every +// server is allowed (FR-010). A non-nil scope allows only servers in its set; +// the empty server name is never allowed for a real scope. func (p *ProfileScope) Allows(serverName string) bool { if p == nil { return true @@ -51,6 +53,18 @@ func (p *ProfileScope) Allows(serverName string) bool { return ok } +// DeniesAll reports whether this scope allows nothing at all — an empty +// profile, or the deny-all scope a stale agent-token pin resolves to. A nil +// receiver is "no profile filtering" and therefore never denies all. +// +// It exists so callers can distinguish "filtered to nothing" from "not +// filtered" without allocating an AllowedServerNames slice, notably to avoid +// lazily creating a per-profile Bleve index for a scope that can return no +// results anyway. +func (p *ProfileScope) DeniesAll() bool { + return p != nil && len(p.servers) == 0 +} + // AllowedServerNames returns the list of server names in this profile scope. // Returns nil for a nil receiver (allow-all — no restriction list). // Returns an empty slice for a non-nil scope with no servers (deny-all). @@ -73,8 +87,14 @@ func WithProfileScope(ctx context.Context, p *ProfileScope) context.Context { return context.WithValue(ctx, profileScopeKey{}, p) } -// ProfileScopeFromContext extracts the ProfileScope, or nil when the request did -// not enter via a profile URL (no filtering). +// ProfileScopeFromContext extracts the URL-injected ProfileScope, or nil when +// the request did not enter via a profile URL. +// +// This is the URL TIER ONLY. Enforcement code must not call it directly: a +// request can carry a higher-precedence token pin or a session selection that +// never touches the context. Resolve the effective scope through the server +// package's resolveActiveProfile instead — reading this alone is how the +// code_execution sandbox once ran outside a pinned token's profile. func ProfileScopeFromContext(ctx context.Context) *ProfileScope { p, _ := ctx.Value(profileScopeKey{}).(*ProfileScope) return p diff --git a/internal/server/mcp.go b/internal/server/mcp.go index e8550fdd..26d3383f 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -1526,9 +1526,16 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques // servers' tools, so switching costs no re-index. The shared index remains // the allow-all fallback; profileScope still post-filters as defense in depth // (and covers the fallback path below). + // + // A deny-all scope (an empty profile, or the scope a stale agent-token pin + // resolves to) deliberately skips ForProfile: that call lazily CREATES and + // caches an on-disk per-profile index, so honoring it would let a request + // 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) searchIndex := p.index - if profileName != "" { + if profileName != "" && !profileScope.DeniesAll() { if pIdx, perr := p.index.ForProfile(profileName); perr == nil && pIdx != nil { searchIndex = pIdx } else if perr != nil { diff --git a/internal/server/mcp_code_execution.go b/internal/server/mcp_code_execution.go index 9cf3d61d..a68e81d6 100644 --- a/internal/server/mcp_code_execution.go +++ b/internal/server/mcp_code_execution.go @@ -13,7 +13,6 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" "github.com/smart-mcp-proxy/mcpproxy-go/internal/jsruntime" - "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream" @@ -247,35 +246,7 @@ func (p *MCPProxyServer) handleCodeExecution(ctx context.Context, request mcp.Ca } // Spec 057 (Codex #621 finding 2): Intersect profile scope into code_execution. - // The jsruntime treats an empty AllowedServers as "allow all"; at a profile URL - // we must restrict to profile servers regardless of what the caller supplied. - if profileScope := profile.ProfileScopeFromContext(ctx); profileScope != nil { - // A profile is active: enforce its effective server set even when empty. - // A deny-all profile (servers: []) or a non-overlapping token∩profile - // yields an EMPTY allow-list, which the jsruntime would otherwise treat - // as "allow all" — leaking every server. RestrictToAllowed closes that. - options.RestrictToAllowed = true - // Build the effective allowed-servers list: profile servers only. - // If the caller also supplied allowed_servers, intersect the two sets. - profileServers := profileScope.AllowedServerNames() - if len(options.AllowedServers) == 0 { - // No caller-supplied restriction: use profile servers as the restriction. - options.AllowedServers = profileServers - } else { - // Intersect caller-supplied list with profile servers. - profileSet := make(map[string]struct{}, len(profileServers)) - for _, s := range profileServers { - profileSet[s] = struct{}{} - } - var intersected []string - for _, s := range options.AllowedServers { - if _, ok := profileSet[s]; ok { - intersected = append(intersected, s) - } - } - options.AllowedServers = intersected - } - } + p.applyProfileScopeToExecution(ctx, &options) // Execute code p.logger.Info("executing code", @@ -978,6 +949,53 @@ func (u *upstreamToolCaller) storeToolCallInHistory(serverName, toolName string, } } +// applyProfileScopeToExecution intersects the request's ACTIVE profile into the +// sandbox's allow-list (Spec 057, Codex #621 finding 2). +// +// It resolves through resolveActiveProfile — token pin > /mcp/p/ URL > +// session set_profile — rather than reading the URL-injected scope alone. The +// URL-only read was a scope hole: a profile-pinned agent token connected to the +// base /mcp endpoint carried no URL scope, so the sandbox ran under the token's +// full server scope and could call straight past its pin, including a stale pin +// that every other session path now answers deny-all. +// +// The jsruntime treats an empty AllowedServers as "allow all", so an active +// profile ALWAYS sets RestrictToAllowed: a deny-all profile, a stale pin, or a +// non-overlapping token∩profile must yield an empty allow-list that denies +// everything rather than leaking every server. +func (p *MCPProxyServer) applyProfileScopeToExecution(ctx context.Context, options *jsruntime.ExecutionOptions) { + if options == nil { + return + } + _, profileScope := p.resolveActiveProfile(ctx) + if profileScope == nil { + return + } + + options.RestrictToAllowed = true + profileServers := profileScope.AllowedServerNames() + if len(options.AllowedServers) == 0 { + // No caller-supplied restriction: the profile is the restriction. + // AllowedServerNames returns a non-nil empty slice for a deny-all + // scope, which RestrictToAllowed then enforces as "nothing". + options.AllowedServers = profileServers + return + } + + // Intersect the caller-supplied list with the profile's servers. + profileSet := make(map[string]struct{}, len(profileServers)) + for _, s := range profileServers { + profileSet[s] = struct{}{} + } + intersected := make([]string, 0, len(options.AllowedServers)) + for _, s := range options.AllowedServers { + if _, ok := profileSet[s]; ok { + intersected = append(intersected, s) + } + } + options.AllowedServers = intersected +} + // lookupToolPermission returns the required permission tier for a tool based on its annotations. // This is used by the JS runtime to enforce auth context permissions during code_execution. func (p *MCPProxyServer) lookupToolPermission(serverName, toolName string) string { diff --git a/internal/server/mcp_direct_scope.go b/internal/server/mcp_direct_scope.go index e5d87a9a..114dd888 100644 --- a/internal/server/mcp_direct_scope.go +++ b/internal/server/mcp_direct_scope.go @@ -43,20 +43,30 @@ func (p *MCPProxyServer) lookupDirectToolPermission(directName string) (string, return perm, ok } -// filterDirectModeToolsForAuth filters tools/list for scoped agent tokens. +// filterDirectModeToolsForAuth filters tools/list for scoped agent tokens and +// for any request with an active profile. // // Direct mode registers upstream tools globally as server__tool. Without this // filter, scoped agent tokens prevent execution but still disclose tool names, // descriptions, and schemas for servers outside their scope. Call-time auth is // still authoritative; this filter only removes tools that the current token // could not call from discovery responses. +// +// The profile filter (Spec 057) is applied to EVERY auth type, not just agent +// tokens: an unauthenticated /mcp/p/ connection runs as an admin context +// yet must still be profile-filtered, exactly as it is on the retrieve_tools +// path (see indexedToolVisible). Direct mode previously honored no profile at +// all — a profile-pinned token saw and could call every server in its token +// scope — so the pin was enforced on one routing mode and not the other. func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools []mcp.Tool) []mcp.Tool { if len(tools) == 0 { return tools } authCtx := auth.AuthContextFromContext(ctx) - if authCtx == nil || authCtx.Type != auth.AuthTypeAgent { + _, profileScope := p.resolveActiveProfile(ctx) + isScopedAgent := authCtx != nil && authCtx.Type == auth.AuthTypeAgent + if !isScopedAgent && profileScope == nil { return tools } @@ -68,6 +78,15 @@ func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools continue } + if !profileScope.Allows(serverName) { + continue + } + + if !isScopedAgent { + filtered = append(filtered, tool) + continue + } + if !authCtx.CanAccessServer(serverName) { continue } diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index e186eef2..c52b05ff 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -175,6 +175,18 @@ func (p *MCPProxyServer) makeDirectModeHandler(serverName, toolName string, anno requestID = mintActivityRequestID(serverName, toolName) } + // Spec 057 / Profiles v2: the active profile (token pin > URL > session + // set_profile) gates direct-mode dispatch exactly as it gates + // call_tool_* (mcp.go handleCallToolVariant). It runs independently of + // the agent-token gates below so an unauthenticated /mcp/p/ + // connection is filtered too, and it runs FIRST so a profile-pinned + // token cannot reach a server outside its pin through this routing mode. + if _, profileScope := p.resolveActiveProfile(ctx); profileScope != nil && !profileScope.Allows(serverName) { + errMsg := fmt.Sprintf("server '%s' is not in profile '%s'", serverName, profileScope.Name) + p.emitActivityPolicyDecision(serverName, toolName, sessionID, requestID, "blocked", errMsg, telemetry.BlockReasonProfileScope) + return mcp.NewToolResultError(errMsg), nil + } + // Check auth context for server access and permissions authCtx := auth.AuthContextFromContext(ctx) if authCtx != nil { diff --git a/internal/server/preflight_dispatch_parity_test.go b/internal/server/preflight_dispatch_parity_test.go index 3d6f16c7..0e94387f 100644 --- a/internal/server/preflight_dispatch_parity_test.go +++ b/internal/server/preflight_dispatch_parity_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "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/preflight" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" @@ -386,3 +387,65 @@ func TestDispatchKeepsLockMessagePreferenceForDisabledPendingTool(t *testing.T) assert.Equal(t, storage.ToolApprovalStatusPending, gate.lockStatus) assert.Equal(t, preflight.ToolClassBlockedByUser, gate.class) } + +// Scope parity: the session (MCP) path and the preflight path must answer a +// STALE agent-token profile_pin the same way — deny-all, not "fall back to the +// token's own scope". Preflight has intersected to deny-all since spec 098 +// (resolvePreflightScope); this asserts the live session path agrees, so an +// agent cannot reach through /mcp what a preflight tells it it cannot have. +func TestStaleTokenPinDeniesOnBothSessionAndPreflightPaths(t *testing.T) { + fixture := newPreflightFixture(t, func(cfg *config.Config) { + cfg.Profiles = []config.ProfileConfig{{Name: "ops", Servers: []string{"gh"}}} + }) + fixture.addServer(t, &config.ServerConfig{Name: "gh", Enabled: true, Protocol: "http"}) + fixture.addServer(t, &config.ServerConfig{Name: "secret", Enabled: true, Protocol: "http"}) + fixture.indexTool(t, "gh", "create_issue") + fixture.indexTool(t, "secret", "exfiltrate") + + // A token scoped to BOTH servers but pinned to "ops" (gh only): the pin is + // the narrowing under test. + authCtx := &auth.AuthContext{ + Type: auth.AuthTypeAgent, + AgentName: "agent-1", + ProfilePin: "ops", + AllowedServers: []string{"gh", "secret"}, + } + ctx := auth.WithAuthContext(context.Background(), authCtx) + params := preflight.Params{ + Tools: []preflight.ToolRef{{ID: "gh:create_issue"}, {ID: "secret:exfiltrate"}}, + Tier: preflight.TierAgentToken, + TokenProfilePin: "ops", + TokenServers: []string{"gh", "secret"}, + } + + // Baseline: while "ops" exists both paths allow gh and deny secret. + visible, _ := fixture.proxy.toolVisibleToSession(ctx, "gh", "create_issue") + assert.True(t, visible, "session path: the pinned profile's server is visible") + visible, reason := fixture.proxy.toolVisibleToSession(ctx, "secret", "exfiltrate") + assert.False(t, visible) + assert.Equal(t, visReasonServerNotInScope, reason) + + out, err := fixture.proxy.RunPreflight(ctx, params) + require.NoError(t, err) + assert.Equal(t, preflight.StatusReady, resultByID(t, out, "gh:create_issue").Status) + assert.Equal(t, preflight.ReasonNotFound, resultByID(t, out, "secret:exfiltrate").Reason) + + // The operator deletes the pinned profile. Neither path may widen. + fixture.cfg.Profiles = nil + + for _, id := range []struct{ server, tool string }{{"gh", "create_issue"}, {"secret", "exfiltrate"}} { + visible, reason := fixture.proxy.toolVisibleToSession(ctx, id.server, id.tool) + assert.False(t, visible, "session path: %s:%s must be denied under a stale pin", id.server, id.tool) + assert.Equal(t, visReasonServerNotInScope, reason) + + _, scope := fixture.proxy.resolveActiveProfile(ctx) + require.NotNil(t, scope) + vis, _ := fixture.proxy.indexedToolVisible(authCtx, scope, id.server, id.tool) + assert.False(t, vis, "search path: %s:%s must be denied under a stale pin", id.server, id.tool) + } + + out, err = fixture.proxy.RunPreflight(ctx, params) + require.NoError(t, err) + assert.Equal(t, preflight.ReasonNotFound, resultByID(t, out, "gh:create_issue").Reason) + assert.Equal(t, preflight.ReasonNotFound, resultByID(t, out, "secret:exfiltrate").Reason) +} diff --git a/internal/server/profile_pin_enforcement_test.go b/internal/server/profile_pin_enforcement_test.go new file mode 100644 index 00000000..baeecadd --- /dev/null +++ b/internal/server/profile_pin_enforcement_test.go @@ -0,0 +1,169 @@ +package server + +import ( + "context" + "encoding/json" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "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/jsruntime" +) + +// A profile pin is only a security boundary if EVERY dispatch surface resolves +// it. These tests cover the three surfaces that read the resolver (or used to +// skip it) besides retrieve_tools/call_tool: the code_execution sandbox, +// direct-routing mode, and the set_profile response. +// +// Each case is exercised twice — with the pinned profile present (it narrows) +// and after it has been deleted (it denies) — because a surface that ignores +// profiles entirely passes the second assertion for the wrong reason. + +func pinnedProxy(t *testing.T, profiles []config.ProfileConfig) (*MCPProxyServer, *config.Config) { + t.Helper() + cfg := &config.Config{ + Servers: []*config.ServerConfig{ + {Name: "research-srv", Enabled: true}, + {Name: "deploy-srv", Enabled: true}, + }, + Profiles: profiles, + } + return &MCPProxyServer{config: cfg, sessionStore: NewSessionStore(zap.NewNop()), logger: zap.NewNop()}, cfg +} + +// pinnedAgentContext is a token scoped to BOTH servers but pinned to one +// profile — the pin, not the token scope, is what must bound it. +func pinnedAgentContext(pin string) context.Context { + return auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, + AgentName: "pinned-agent", + ProfilePin: pin, + AllowedServers: []string{"research-srv", "deploy-srv"}, + Permissions: []string{auth.PermRead, auth.PermWrite}, + }) +} + +// The sandbox used to read the URL-injected scope only, so a token pinned on +// the BASE /mcp endpoint (no URL scope) ran with its full token scope. +func TestCodeExecutionHonorsTokenProfilePin(t *testing.T) { + proxy, cfg := pinnedProxy(t, []config.ProfileConfig{ + {Name: "research", Servers: []string{"research-srv"}}, + }) + ctx := pinnedAgentContext("research") + + opts := jsruntime.ExecutionOptions{} + proxy.applyProfileScopeToExecution(ctx, &opts) + assert.True(t, opts.RestrictToAllowed, "an active pin must restrict the sandbox") + assert.Equal(t, []string{"research-srv"}, opts.AllowedServers, + "the sandbox must see the pinned profile's servers, not the token's wider scope") + + // A caller-supplied allow-list can only narrow further, never widen. + opts = jsruntime.ExecutionOptions{AllowedServers: []string{"research-srv", "deploy-srv"}} + proxy.applyProfileScopeToExecution(ctx, &opts) + assert.Equal(t, []string{"research-srv"}, opts.AllowedServers) + + // The operator deletes the pinned profile: deny-all, and RestrictToAllowed + // must stay set — the jsruntime reads an empty allow-list as "allow all". + cfg.Profiles = nil + opts = jsruntime.ExecutionOptions{} + proxy.applyProfileScopeToExecution(ctx, &opts) + assert.True(t, opts.RestrictToAllowed) + assert.Empty(t, opts.AllowedServers, "a stale pin must leave the sandbox with no reachable server") + + // An unpinned, unprofiled request is untouched (allow-all stays allow-all). + opts = jsruntime.ExecutionOptions{} + proxy.applyProfileScopeToExecution(context.Background(), &opts) + assert.False(t, opts.RestrictToAllowed) + assert.Nil(t, opts.AllowedServers) + + // nil options is a no-op, not a panic. + assert.NotPanics(t, func() { proxy.applyProfileScopeToExecution(ctx, nil) }) +} + +// Direct-routing mode registers upstream tools as server__tool and enforced only +// the token's allowed_servers — no profile at all. A pinned token therefore saw +// and could call every server in its token scope. +func TestDirectModeHonorsTokenProfilePin(t *testing.T) { + proxy, cfg := pinnedProxy(t, []config.ProfileConfig{ + {Name: "research", Servers: []string{"research-srv"}}, + }) + proxy.setDirectToolPermissions(map[string]string{ + "research-srv__search": auth.PermRead, + "deploy-srv__ship": auth.PermRead, + }) + tools := []mcp.Tool{{Name: "research-srv__search"}, {Name: "deploy-srv__ship"}} + ctx := pinnedAgentContext("research") + + names := func(filtered []mcp.Tool) []string { + out := make([]string, 0, len(filtered)) + for _, tool := range filtered { + out = append(out, tool.Name) + } + return out + } + + assert.Equal(t, []string{"research-srv__search"}, names(proxy.filterDirectModeToolsForAuth(ctx, tools)), + "direct-mode discovery must drop tools outside the pinned profile") + + handler := proxy.makeDirectModeHandler("deploy-srv", "ship", nil) + result, err := handler(ctx, mcp.CallToolRequest{}) + require.NoError(t, err) + require.True(t, result.IsError, "a call outside the pinned profile must be refused") + assert.Contains(t, resultText(t, result), "is not in profile 'research'") + + // Profile deleted → deny-all on both discovery and dispatch. + cfg.Profiles = nil + assert.Empty(t, proxy.filterDirectModeToolsForAuth(ctx, tools), + "a stale pin must hide every direct-mode tool") + + handler = proxy.makeDirectModeHandler("research-srv", "search", nil) + result, err = handler(ctx, mcp.CallToolRequest{}) + require.NoError(t, err) + require.True(t, result.IsError, "a stale pin must refuse even the formerly pinned server") + + // An admin (no auth context, no profile) is unaffected. + assert.Len(t, proxy.filterDirectModeToolsForAuth(context.Background(), tools), 2) +} + +// set_profile("") clears the session tier, which a pin outranks anyway. The +// response must describe the scope that remains, not the full server list. +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"}) + + request := mcp.CallToolRequest{} + request.Params.Arguments = map[string]interface{}{"profile": ""} + + result, err := proxy.handleSetProfile(ctx, request) + require.NoError(t, err) + require.False(t, result.IsError) + payload := decodeSetProfilePayload(t, result) + assert.Equal(t, "research", payload["active_profile"]) + assert.Equal(t, []interface{}{"research-srv"}, payload["servers"], + "clearing must not advertise servers the pin still denies") + + // With the pinned profile deleted, the honest answer is an empty list. + cfg.Profiles = nil + result, err = proxy.handleSetProfile(ctx, request) + require.NoError(t, err) + require.False(t, result.IsError) + payload = decodeSetProfilePayload(t, result) + assert.Equal(t, "research", payload["active_profile"]) + assert.Empty(t, payload["servers"]) +} + +func decodeSetProfilePayload(t *testing.T, result *mcp.CallToolResult) map[string]interface{} { + t.Helper() + var payload map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(resultText(t, result)), &payload)) + return payload +} diff --git a/internal/server/profile_resolver.go b/internal/server/profile_resolver.go index e4e7fe06..8b3d2b3f 100644 --- a/internal/server/profile_resolver.go +++ b/internal/server/profile_resolver.go @@ -96,19 +96,27 @@ func (p *MCPProxyServer) profileScopeForSlug(slug string) *profile.ProfileScope // through to "none". func (p *MCPProxyServer) resolveActiveProfile(ctx context.Context) (string, *profile.ProfileScope) { // 1. Agent-token pin (T3). When present it is authoritative and bounds - // everything below. If the pinned profile was removed from config after - // the token was minted, we warn-skip rather than hard-fail (parity with - // the unknown-server warn-skip): resolution degrades to URL/session/none, - // while the set_profile and /mcp/p/ guards still pin the token by - // its stored slug so it cannot silently widen scope by switching. + // everything below — including the case where the pinned profile has been + // removed from config since the token was minted. + // + // A stale pin resolves to a DENY-ALL scope that keeps the removed + // profile's name, never to the next resolver tier. Falling through would + // hand the session the token's own (wider) server scope — precisely the + // privilege widening the pin exists to prevent — and it would do so + // silently, from an operator action (deleting a profile) that reads as a + // restriction. This mirrors resolvePreflightScope + // (internal/server/preflight_glue.go), which intersects an unresolvable + // 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 { return pin, scope } if p.logger != nil { - p.logger.Warn("agent-token profile_pin no longer matches any configured profile; falling through", + 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, profile.NewProfileScope(pin, nil) } // 2. Explicit URL profile (Spec 057). Authoritative for this request, so it diff --git a/internal/server/profile_resolver_test.go b/internal/server/profile_resolver_test.go index 973f7160..ffe12576 100644 --- a/internal/server/profile_resolver_test.go +++ b/internal/server/profile_resolver_test.go @@ -134,6 +134,54 @@ func TestResolveActiveProfile_PinHighestPrecedence(t *testing.T) { require.False(t, scope.Allows("deploy-srv")) } +// TestResolveActiveProfile_StalePinDeniesAll is the session-path half of the +// stale-pin contract (the preflight half is +// TestRunPreflightStaleTokenPinDeniesRatherThanWidens). A pin naming a profile +// the operator has since deleted must NOT degrade to the next resolver tier — +// doing so hands the session the token's own, wider scope, i.e. exactly the +// privilege widening the pin existed to prevent. It resolves to a deny-all +// scope that still carries the removed profile's name, so rejections and +// activity records name it. +func TestResolveActiveProfile_StalePinDeniesAll(t *testing.T) { + cfg := &config.Config{ + Servers: []*config.ServerConfig{ + {Name: "research-srv"}, + {Name: "deploy-srv"}, + }, + Profiles: []config.ProfileConfig{ + {Name: "deploy", Servers: []string{"deploy-srv"}}, + }, + } + p := &MCPProxyServer{config: cfg, sessionStore: NewSessionStore(zap.NewNop()), logger: zap.NewNop()} + + helper := mcpserver.NewMCPServer("test", "1.0.0") + base := helper.WithContext(context.Background(), &fakeClientSession{id: "sess-stale"}) + + // The token is pinned to "research", which no longer exists, and its own + // scope covers BOTH servers — the widening the old warn-skip enabled. + pinned := auth.WithAuthContext(base, &auth.AuthContext{ + Type: auth.AuthTypeAgent, + ProfilePin: "research", + AllowedServers: []string{"research-srv", "deploy-srv"}, + }) + + name, scope := p.resolveActiveProfile(pinned) + require.Equal(t, "research", name, "the removed profile's name must survive for logs/rejections") + require.NotNil(t, scope, "a stale pin must produce a scope, not fall through to nil (allow-all)") + require.True(t, scope.DeniesAll()) + require.False(t, scope.Allows("research-srv")) + require.False(t, scope.Allows("deploy-srv")) + + // The lower resolver tiers must not rescue the pin: neither a session + // selection nor an explicit URL scope may re-widen it. + p.sessionStore.SetActiveProfile("sess-stale", "deploy") + withURL := profile.WithProfileScope(pinned, profile.NewProfileScope("deploy", []string{"deploy-srv"})) + name, scope = p.resolveActiveProfile(withURL) + require.Equal(t, "research", name) + require.NotNil(t, scope) + require.False(t, scope.Allows("deploy-srv"), "a stale pin must not be widened by URL or session state") +} + // TestSessionStore_ActiveProfileLifecycle verifies the per-session profile map // is set, read and cleared on session close. func TestSessionStore_ActiveProfileLifecycle(t *testing.T) { diff --git a/internal/server/profile_tool.go b/internal/server/profile_tool.go index 1ab04a8c..4383c34f 100644 --- a/internal/server/profile_tool.go +++ b/internal/server/profile_tool.go @@ -16,6 +16,15 @@ import ( // buildSetProfileTool constructs the set_profile MCP tool definition (Profiles // v2 T2). Factored out so it can be registered on the default server and every // routing-mode server (call-tool / code-exec) from one source of truth. +// +// The wording below is snapshotted byte-for-byte by the spec-098 FR-015 +// tools/list goldens (testdata/toolslist_goldens/), which were captured from +// the pre-098 merge base — editing this text is an intentional MCP-surface +// change that must regenerate them, and is deliberately NOT bundled with +// unrelated fixes. One nuance it therefore still understates: "back to all +// servers" is bounded by the caller's credential, so a profile-pinned token +// that clears its session selection stays inside its pin (handleSetProfile +// reports the pinned scope, not every server). func buildSetProfileTool() mcp.Tool { return mcp.NewTool("set_profile", mcp.WithDescription("Switch the active profile for THIS session. A profile scopes tool discovery "+ @@ -49,15 +58,25 @@ func (p *MCPProxyServer) handleSetProfile(ctx context.Context, request mcp.CallT cfg := p.currentConfig() - // Profiles v2 T3 hook: a profile-pinned agent token may not switch away from - // its pinned profile. Inert until T3 populates the pin (always "" for now). - if pin := profilePinFromContext(ctx); pin != "" && slug != "" && slug != pin { + // 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 } // 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, pinnedScope.AllowedServerNames()) + } return setProfileResult("", allServerNames(cfg)) } diff --git a/specs/028-agent-tokens/spec.md b/specs/028-agent-tokens/spec.md index 20d830f0..682a526c 100644 --- a/specs/028-agent-tokens/spec.md +++ b/specs/028-agent-tokens/spec.md @@ -118,7 +118,7 @@ Users manage agent tokens through the MCPProxy web dashboard. The UI provides a - What happens when a token with `allowed_servers: ["*"]` is used after new servers are added? The wildcard includes all non-quarantined servers dynamically — new servers are automatically accessible. - What happens when the token's expiry is set beyond the maximum (365 days)? The system rejects the request and informs the user of the maximum allowed expiry. - What happens when a token is pinned to a profile (`profile_pin`) and the agent tries to switch profiles? The pin is server-enforced: `set_profile` to any other slug is rejected, and a `/mcp/p/` URL returns 403. The pinned profile is the highest-precedence resolution source (above URL scope and session `set_profile`). -- What happens when a pinned profile is later removed from the configuration? Creation validates the slug exists, but a later config change is **warn-skipped** at request time (logged, not hard-failed): profile resolution degrades to URL/session/none while the pin still blocks switching away — the token cannot silently widen its scope. +- What happens when a pinned profile is later removed from the configuration? Creation validates the slug exists; a later config change is logged (not hard-failed at the transport) and the pin resolves to a **deny-all scope** — the token sees no servers and no tools until the profile returns or the token is re-minted. *(Amended after the spec 098 review: the original answer degraded resolution to the URL/session/none tiers, which silently handed the session the token's own wider scope — the exact widening the pin exists to prevent. Preflight has resolved this to deny-all since spec 098; the session path now matches.)* ## Requirements *(mandatory)* @@ -144,7 +144,7 @@ Users manage agent tokens through the MCPProxy web dashboard. The UI provides a - **FR-018**: System MUST support agent token authentication via both `Authorization: Bearer` and `X-API-Key` headers. - **FR-019**: System MUST provide a web UI for token management including creation with server selection, permission picker, expiry setting, and revocation. - **FR-020**: System MUST support activity log filtering by agent name and authentication type. -- **FR-021** (Profiles v2 T3): System MUST support an optional per-token `profile_pin`. When set, the token operates ONLY within its pinned profile: `set_profile` to a different slug MUST be rejected, a `/mcp/p/` URL MUST return 403, and the pin MUST be the highest-precedence profile-resolution source (above URL scope and session `set_profile`). The pinned slug MUST be validated against configured profiles at creation time; a later config change MUST warn-skip (not hard-fail). Tokens without a pin retain unchanged behavior. +- **FR-021** (Profiles v2 T3): System MUST support an optional per-token `profile_pin`. When set, the token operates ONLY within its pinned profile: `set_profile` to a different slug MUST be rejected, a `/mcp/p/` URL MUST return 403, and the pin MUST be the highest-precedence profile-resolution source (above URL scope and session `set_profile`). The pinned slug MUST be validated against configured profiles at creation time; if the profile is later removed, the pin MUST resolve to a deny-all scope (logged with a warning naming the removed profile, not hard-failed at the transport) on every path that resolves it — MCP session and preflight alike — so a pinned token can never be widened by deleting the profile it names. Tokens without a pin retain unchanged behavior. ### Key Entities