Skip to content

Commit 33174dd

Browse files
committed
docs(057): cover upstream_servers + code_execution profile boundaries (Codex #621 review)
Related #55 Address Codex review of PR #621 (verdict request_changes), two findings: 1. upstream_servers introspection must honour the active profile (FR-004). The list path (mcp.go:2763) applied only the agent-token filter, so a profile URL could enumerate out-of-profile servers. Added T011a + test. 2. code_execution is a profile-boundary escape: the /mcp/p/ route reuses the retrieve-tools server which registers code_execution; the JS runtime treats an empty allowed_servers as ALL servers, so call_tool() inside code-exec could reach servers outside the profile. Intersect ProfileScope into the runtime (or reject out-of-profile calls). Added T011b + test. Updated plan.md (filter surfaces + source tree + phase-2 tasks), tasks.md (T011a/T011b under US1, dependency notes), and the endpoint contract to spell out both boundaries.
1 parent d64b197 commit 33174dd

3 files changed

Lines changed: 10 additions & 4 deletions

File tree

specs/057-in-proxy-profiles/contracts/mcp-profile-endpoint.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ Stateless, pinned selector. Same MCP protocol surface as `/mcp`; the only differ
1717

1818
- `retrieve_tools` returns only tools from servers in the **effective set** (profile ∩ token ∩ enabled ∩ not-quarantined ∩ user-visible). Tools the profile excludes MUST NOT appear (FR-004).
1919
- `call_tool_read` / `call_tool_write` / `call_tool_destructive` into an excluded server are rejected (FR-004).
20+
- `upstream_servers` introspection (e.g. `list`/`get`) MUST exclude servers outside the profile from its result — a profile URL cannot enumerate out-of-profile servers (FR-004). *(Codex #621 finding 1.)*
21+
- `code_execution` (when enabled on the reused retrieve-tools server) MUST run with the profile-intersected effective server set: `call_tool()` invoked from inside a code-execution sandbox at a profile URL is rejected for any server outside the active profile. An empty caller-supplied `allowed_servers` MUST NOT be interpreted as "all servers" at a profile URL. *(Codex #621 finding 2.)*
2022
- Per-server `enabled_tools`/`disabled_tools` continue to apply inside the profile (FR-006); no profile-level tool list.
2123

2224
## Scope composition & error attribution

specs/057-in-proxy-profiles/plan.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ Add an optional top-level `profiles` array to the config. Each profile `{name, s
99

1010
**Technical approach** (verified against current code, 2026-06-07):
1111
- A single `/mcp/p/` prefix handler + `profileMiddleware` (registered after `mcpAuthMiddleware`) strips the slug, resolves the profile from the **current** config snapshot (`runtime.Config()`, lock-free atomic read → hot-reload for free), and injects a `ProfileScope` into the request context. The existing retrieve_tools-mode MCP server instance (`p.server`) is reused — **no per-profile server instances** (http.ServeMux can't deregister routes at runtime).
12-
- Profile filtering runs as a **parallel, auth-type-independent** check at the two existing scope sites (`mcp.go:1113` retrieve_tools, `mcp.go:1529` call_tool_*). It MUST NOT ride the `enforceAgentScope` gate, because an unauthenticated `/mcp/p/...` connection gets `AdminContext()` (where `enforceAgentScope == false`). Two independent checks yield the FR-005 intersection and FR-012 distinct errors for free.
12+
- Profile filtering runs as a **parallel, auth-type-independent** check at **every server-boundary surface** the profile must cover (FR-004), not just two: `mcp.go:1113` retrieve_tools, `mcp.go:1529` call_tool_*, **`upstream_servers` introspection (`mcp.go:2763` list path)**, and **`code_execution`** (the `/mcp/p/` route reuses the retrieve-tools server, which registers `code_execution` when enabled — the JS runtime must receive the profile-intersected server set, not be allowed to treat an empty `allowed_servers` as "all"). It MUST NOT ride the `enforceAgentScope` gate, because an unauthenticated `/mcp/p/...` connection gets `AdminContext()` (where `enforceAgentScope == false`). Independent checks yield the FR-005 intersection and FR-012 distinct errors for free.
13+
> Both extra surfaces were flagged by Codex review of PR #621 (verdict `request_changes`): `upstream_servers list` currently applies only the agent-token filter, and `handleCodeExecution``jsruntime` treats an empty caller `allowed_servers` as all servers — each is a profile-boundary escape if not closed.
1314
- `metadata["profile"]` is attached to tool-call activity records originating from a profile URL via the existing `Metadata map[string]interface{}` field (no schema change).
1415

1516
## Technical Context
@@ -69,7 +70,8 @@ internal/
6970
│ └── context.go # NEW (~30 LOC) — ProfileScope{Allows(server) bool}, WithProfileScope/FromContext (mirrors auth/context.go)
7071
└── server/
7172
├── server.go # Register `/mcp/p/` prefix handler + profileMiddleware after mcpAuthMiddleware (near L1690)
72-
└── mcp.go # Two parallel filter conditions: retrieve_tools (~L1113) + call_tool_* (~L1529); profile metadata write at emitActivity* call sites
73+
├── mcp.go # Parallel filter conditions: retrieve_tools (~L1113), call_tool_* (~L1529), upstream_servers list (~L2763); profile metadata write at emitActivity* call sites
74+
└── mcp_code_execution.go # Pass profile-intersected server set into the JS runtime (~L181/L190) so call_tool() inside code_execution cannot reach out-of-profile servers
7375
```
7476

7577
**Structure Decision**: Single Go project, existing `internal/` layout. One new sub-package `internal/profile` (request-scoped scope type, peer of `internal/auth`) and one new file `internal/config/profiles.go`. No new top-level dirs, no frontend changes in the MVP (web-UI profile affordances are out of scope), no storage/index packages touched.
@@ -92,7 +94,7 @@ The spec's *Resolved Design Decisions*, *Assumptions*, and *Implementation Desig
9294
1. Config: `ProfileConfig` + validation (slug/reserved/dup/unknown-server) — unit tests first.
9395
2. `internal/profile` context + `ProfileScope.Allows`.
9496
3. Routing: `/mcp/p/` handler + `profileMiddleware` (auth→profile order).
95-
4. Filter wiring: parallel checks at both `mcp.go` sites + the mandated unauth-at-profile-URL regression test.
97+
4. Filter wiring: parallel checks at **all** profile-boundary surfaces — `retrieve_tools`, `call_tool_*`, `upstream_servers` introspection, and `code_execution` (intersect `ProfileScope` into the JS runtime's server set) + the mandated unauth-at-profile-URL regression test.
9698
5. Activity `metadata["profile"]`.
9799
6. Integration + E2E (two-profile isolation, intersection, 404 paths, reserved slug) + backward-compat E2E (SC-002).
98100
7. Docs (CLAUDE.md, README, docs/).

specs/057-in-proxy-profiles/tasks.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ description: "Task list for Spec 057 — In-Proxy Profiles + Permanent URLs"
5252
- [ ] T009 [US1] Register the `/mcp/p/` route near the existing mode routes (`internal/server/server.go` ~L1690), reusing `p.server` (retrieve_tools mode) wrapped as `mcpAuthMiddleware(profileMiddleware(loggingHandler(streamable)))`**auth before profile** so token scope composes downstream. Handle both `/mcp/p/` and `/mcp/p` forms.
5353
- [ ] T010 [US1] Add the parallel profile filter at the retrieve_tools site in `internal/server/mcp.go` (~L1113): before/independent of the `enforceAgentScope` gate, read `profile.ProfileScopeFromContext(ctx)`; if non-nil and `!scope.Allows(serverName)`, skip the result. MUST NOT depend on `enforceAgentScope`.
5454
- [ ] T011 [US1] Add the parallel profile filter at the call_tool_* site in `internal/server/mcp.go` `handleCallToolVariant` (~L1529): if `profileScope != nil && !profileScope.Allows(serverName)`, reject with `"server '<s>' is not in profile '<name>'"` and emit a policy-decision activity record (mirror the existing token-scope rejection just below).
55+
- [ ] T011a [US1] **(Codex #621 finding 1 — FR-004)** Add the parallel profile filter to the `upstream_servers` introspection path in `internal/server/mcp.go` (list path ~L2763, which today applies only the agent-token filter ~L2769/L2777 before returning server names/config ~L2800/L2810): exclude servers not in `ProfileScope` from the listing/get result so a profile URL cannot enumerate out-of-profile servers. Add an integration test asserting `upstream_servers(list)` at `/mcp/p/<slug>` returns only the profile's servers.
56+
- [ ] T011b [US1] **(Codex #621 finding 2 — profile-boundary escape)** Intersect `ProfileScope` into `code_execution`. The `/mcp/p/` route reuses the retrieve-tools server, which registers `code_execution` when `enable_code_execution` is true (`mcp.go:588`/`611`). `handleCodeExecution` (`internal/server/mcp_code_execution.go:181`/`190`) passes only auth scope into the JS runtime, and the runtime treats an empty `allowed_servers` as ALL servers (`internal/jsruntime/runtime.go:18`/`141`/`303`), with the nested caller invoking the named upstream directly (`mcp_code_execution.go:410`/`421`). Pass the profile-intersected effective server set into the runtime (or reject `call_tool()` for servers outside the active profile). Add a test: `call_tool()` inside `code_execution` at `/mcp/p/<slug>` is rejected for an out-of-profile server (and allowed for an in-profile one).
5557
- [ ] T012 [US1] **Mandatory regression test** (spec §Implementation Design): integration test in `internal/server/` asserting an **unauthenticated** connection at `/mcp/p/<slug>` is still filtered to the profile's servers (proves profile filtering does not ride `enforceAgentScope`/AdminContext).
5658
- [ ] T013 [P] [US1] Integration tests in `internal/server/` (httptest server, two profiles): `retrieve_tools` at `/mcp/p/research` returns only research tools; `/mcp` returns the full union (SC-002 spot-check); `/mcp/p/unknown` → 404 with available list; `/mcp/p/all` (reserved, even if a profile tried to define it) never routes; no-profiles config → `/mcp/p/x` 404.
5759
- [ ] T014 [P] [US1] E2E test (`internal/server/e2e_test.go` style): real proxy + two stub upstreams + two profiles + two MCP clients; verify bidirectional isolation via `retrieve_tools` and `call_tool_*` (SC-001).
@@ -97,7 +99,7 @@ description: "Task list for Spec 057 — In-Proxy Profiles + Permanent URLs"
9799
## Dependencies & Execution Order
98100

99101
- **Phase 1 (T001)****Phase 2 (T002–T007)** → user stories.
100-
- **US1 (T008–T014)** is the MVP and must land first — it builds the routing + filter seam.
102+
- **US1 (T008–T014 + T011a/T011b)** is the MVP and must land first — it builds the routing + filter seam across ALL profile-boundary surfaces (retrieve_tools, call_tool_*, upstream_servers, code_execution). T011a/T011b are correctness-critical (Codex #621 findings) — a profile that filters retrieve_tools but leaks via upstream_servers or code_execution is incomplete.
101103
- **US2 (T015–T017)** depends on US1's call_tool_* filter site (T011) and retrieve_tools test (T013).
102104
- **US3 (T018)** depends on US1 routing (T008–T009); otherwise independent.
103105
- **Phase 6** depends on US1; T019/T020/T021/T022 are mutually parallel.

0 commit comments

Comments
 (0)