diff --git a/CLAUDE.md b/CLAUDE.md index cb90d14..e517db4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,8 +20,8 @@ Self-hosted MCP manager/gateway: one streamable-HTTP `/mcp` endpoint federating - `domain/policy.ts` — `PolicyService`: toolEnabled ∧ (override(allow) ∨ (tier ≤ maxTier ∧ ¬deny)); maxTier = per-upstream grant ?? role default. Same function gates tools/list AND tools/call. `allowsFor(principal, entry)` = envelope ∧ personal prefs (deny rows in `user_prefs`; "enable" deletes the row — narrowing can never widen). Spec `userDefault: "off"` inverts the personal layer for that upstream: nothing is live until an explicit opt-in row exists (per tool or server-wide `''`), still capped by the envelope — for servers with hundreds of tools. `/api/me/access` derives its `enabled` flags from `allowsFor` so the page can't disagree with the boundary - `auth/` — `static-tokens.ts` (timing-safe bearer match), `oidc.ts` (jose JWKS resource-server verifier for inbound *access* tokens), `login.ts` (interactive login: openid-client cookie+PKCE confidential-client flow consuming an *id-token*; signed identity-only session cookie, HMAC + freshness; `safeReturnTo`), `authz-server.ts` (OAuth AS facade: RFC 8414 metadata, RFC 7591 DCR for public clients, single-use hashed 60s codes + PKCE S256, HS256 gateway JWTs keyed by `GATEWAY_JWT_SECRET` (default derived from `SESSION_SECRET`), rotating refresh tokens — 30d sliding, family-revoked on replay, client-bound consume that can't burn a live token — register rate limit; clients managed via `/api/oauth-clients` + Users tab), `prm.ts` (RFC 9728 doc + WWW-Authenticate; lists the gateway itself as AS when login is configured, else the raw IdP), `directory.ts` (app-only Graph search of Entra users/groups via the login app's own creds — powers the admin UI group-mapping typeahead at `/api/directory/search`; null for non-Entra issuers → UI degrades to paste-an-id), `principal.ts` (session binding key; a Principal carries EVERY role it holds and the envelope is their union — `resolveOidcRoles` returns all mapped groups' roles, an explicit `users.role_id` override replaces them, and `user_login_roles` remembers the group-derived set for the cookie/JWT paths that see no group claims). Four inbound auth paths in `createAuthResolver`: static token, gateway-issued JWT (routed by unverified `iss == PUBLIC_URL`, then fully verified), OIDC bearer, and the cookie session — the cookie/JWT carry only identity and the roles are re-resolved every request (group-derived set persisted at callback via `setLoginRoles`), so a session id never carries privilege. `loginUpsert()` is shared by the bearer + callback paths so they can't drift. `/oauth/authorize` brokers user auth to Entra by piggybacking the interactive login: the pending request rides in the signed transient cookie and `/auth/callback` mints the code. - `secrets/` — `SecretStore` interface (scheme-tagged: `bao` | `kv`), `openbao.ts` (KV v2, AppRole or token, 5-min cache), `keyvault.ts` (Azure Key Vault, `DefaultAzureCredential`, lazy SDK import, same 5-min cache; `put(path, field)` writes `path-field`), `memory.ts` (tests). Refs: `bao:path#field` / `kv:secret-name`; env refs: `${VAR}` — all resolved only at upstream connect time. One store at a time (`BAO_ADDR` xor `KEY_VAULT_URI`) -- `upstream/connection.ts` — one pooled SDK `Client` per upstream; header/env injection; backoff reconnect (1s→60s) + `onRecovered`; retry-once on dropped transport AND on server-side session expiry (upstream 404 "unknown session" → transparent re-initialize + retry, per MCP spec). Optional spec `auth` block (`oauth2-client-credentials`): the gateway mints the upstream's bearer itself (secret via `${VAR}`/`bao:`/`kv:` ref), caches it, and rebuilds the connection when it nears expiry (60s skew) — for third-party servers that want a finished token, e.g. CIPP behind Easy Auth. Neither secret nor token is ever logged -- `upstream/manager.ts` also pools **per-principal links** for `sessionMode:"per-user"` upstreams: spec clone with the caller's credential REFS layered over headers/env (still resolved via the secret store at connect — anti-passthrough intact); catalog discovery stays on the shared link; personal pool flushed on upstream upsert/remove. `requirePersonalCredentials` refuses the shared fallback +- `upstream/connection.ts` — one pooled SDK `Client` per upstream; header/env injection; backoff reconnect (1s→60s) + `onRecovered`; retry-once on dropped transport AND on server-side session expiry (upstream 404 "unknown session" → transparent re-initialize + retry, per MCP spec) — on `callTool` **and** `listTools` (issue #42: discovery had no recovery, so one upstream restart emptied its half of the catalog until a human bounced it). Optional spec `auth` block (`oauth2-client-credentials`): the gateway mints the upstream's bearer itself (secret via `${VAR}`/`bao:`/`kv:` ref), caches it, and rebuilds the connection when it nears expiry (60s skew) — for third-party servers that want a finished token, e.g. CIPP behind Easy Auth. Neither secret nor token is ever logged +- `upstream/manager.ts` also pools **per-principal links** for `sessionMode:"per-user"` upstreams: spec clone with the caller's credential REFS layered over headers/env (still resolved via the secret store at connect — anti-passthrough intact); catalog discovery stays on the shared link; personal pool flushed on upstream upsert/remove AND on a `/me` credential write or delete (issue #44 — the link outlived the credential it was built from). Pool keys go through the single `PERSONAL_KEY_SEP` constant: writer and flusher used different separators, so the flush had never matched anything. A failed discovery is recorded per upstream and surfaced as `lastError`, because `connected: true / toolCount: 0` is indistinguishable from an upstream that genuinely has no tools. `requirePersonalCredentials` refuses the shared fallback - `upstream/manager.ts` — policy-free catalog owner; hot `upsertUpstream`/`removeUpstream`; `summaries()` for the UI - `mcp/gateway-server.ts` — low-level SDK `Server` per session, closes over the Principal; unknown and forbidden tools get the same error (no existence oracle). The ONE exception is a denial the caller owns — `PolicyService.denialReason` returns `personal`/`optIn` only when the admin envelope allows the tool and the user's own layer closes it, and such a tool is already listed on their /me page, so the message names it and points at /me; role denials, kill switches and unknown names keep the vague wording verbatim - `http/app.ts` — `/mcp` (origin check → resolveAuth → principal-bound sessions), PRM endpoints, per-session fingerprint-diffed `list_changed` (fingerprint is per PRINCIPAL — `visibleEntriesFor`, so a user's own `/me` switch notifies too; role-level diffing missed it), mounts `/api` + `/admin`. `http/event-store.ts` gives each session a bounded replay window (`ReplayEventStore`): a notification emitted while the client's GET stream is closed is buffered and replayed on `Last-Event-ID` resume instead of silently dropped by the SDK, and a missing stream is logged since it is otherwise unobservable diff --git a/packages/gateway/src/http/me-api.ts b/packages/gateway/src/http/me-api.ts index 1bce23f..fe884f9 100644 --- a/packages/gateway/src/http/me-api.ts +++ b/packages/gateway/src/http/me-api.ts @@ -257,22 +257,27 @@ export function createMeRouter(deps: AppDeps, me: MeDeps): Router { await secretStore.put(path, body.field, body.value); const ref = secretStore.refFor(path, body.field); repo.upsertUserCredential(prefsIdentity(principal), upstreamId, body.field, ref); + // The caller's pooled per-user link was built with the OLD refs and is + // memoized until the upstream is bounced — drop it here or the credential + // they just rotated keeps being ignored (#44). + const dropped = await manager.closePersonalLink(upstreamId, prefsIdentity(principal)); // Never echo the value. - res.json({ ok: true, ref }); + res.json({ ok: true, ref, reconnected: dropped }); }) ); router.delete( "/credentials/:upstreamId/:field", - h((req, res) => { - const removed = repo.deleteUserCredential( - prefsIdentity(req.principal!), - String(req.params.upstreamId ?? ""), - String(req.params.field ?? "") - ); + h(async (req, res) => { + const who = prefsIdentity(req.principal!); + const upstreamId = String(req.params.upstreamId ?? ""); + const removed = repo.deleteUserCredential(who, upstreamId, String(req.params.field ?? "")); + // Same reason as the write path: the pooled link still holds the ref we + // just deleted, so it has to go too (#44). + const dropped = await manager.closePersonalLink(upstreamId, who); // The secret store copy is left for rotation-history; re-registering // overwrites it. (Store-side cleanup can come with sessionMode.) - res.json({ ok: removed }); + res.json({ ok: removed, reconnected: dropped }); }) ); diff --git a/packages/gateway/src/upstream/connection.test.ts b/packages/gateway/src/upstream/connection.test.ts index 0186cbb..b1ece9d 100644 --- a/packages/gateway/src/upstream/connection.test.ts +++ b/packages/gateway/src/upstream/connection.test.ts @@ -114,9 +114,32 @@ describe("UpstreamConnection — stale streamable-HTTP sessions", () => { expect(connection.connected).toBe(true); }); + it("lists tools over the pooled session", async () => { + expect((await connection.listTools()).map((t) => t.name)).toEqual(["ping"]); + }); + + /** + * Regression for #42: discovery had no stale-session recovery, so after an + * upstream restart listTools() threw, the manager omitted the upstream, and + * every one of its tools disappeared from the catalog until a human bounced it. + */ + it("re-initializes and retries once when DISCOVERY hits an expired session", async () => { + await connection.listTools(); + const initializesBefore = upstream.initializeCount; + + upstream.expireAllSessions(); + const tools = await connection.listTools(); + + expect(tools.map((t) => t.name)).toEqual(["ping"]); + expect(upstream.initializeCount).toBe(initializesBefore + 1); + }); + it("does not re-initialize for genuine tool errors", async () => { + const initializesBefore = upstream.initializeCount; const result = await connection.callTool("does-not-exist", {}); expect(result.isError).toBe(true); // the upstream's error passes through - expect(upstream.initializeCount).toBe(2); // no needless session churn + // No needless session churn — counted relative to this test, so adding + // cases above cannot shift an absolute number. + expect(upstream.initializeCount).toBe(initializesBefore); }); }); diff --git a/packages/gateway/src/upstream/connection.ts b/packages/gateway/src/upstream/connection.ts index bd64b08..565ff69 100644 --- a/packages/gateway/src/upstream/connection.ts +++ b/packages/gateway/src/upstream/connection.ts @@ -256,9 +256,33 @@ export class UpstreamConnection { this.reconnectTimer.unref?.(); } - /** List all tools, following pagination. */ + /** + * List all tools, following pagination. Same stale-session recovery as + * `callTool`: an upstream that restarted answers 404 for our forgotten + * session while the local transport still looks healthy, and without a retry + * discovery would throw, the manager would omit the upstream, and every one of + * its tools would drop out of the catalog until someone bounced it by hand. + */ async listTools(): Promise { const client = await this.requireClient(); + try { + return await this.collectTools(client); + } catch (err) { + if (this.connected && !isStaleSession(err)) { + throw err; // upstream answered with a real error — not a session problem + } + console.error( + this.connected + ? `[upstream:${this.spec.id}] tool discovery hit an expired upstream session — re-initializing` + : `[upstream:${this.spec.id}] tool discovery hit a dropped connection — retrying once` + ); + if (this.connected) await this.resetClient(); + const fresh = await this.requireClient(); + return await this.collectTools(fresh); + } + } + + private async collectTools(client: Client): Promise { const tools: Tool[] = []; let cursor: string | undefined; do { diff --git a/packages/gateway/src/upstream/manager.test.ts b/packages/gateway/src/upstream/manager.test.ts index 028a7db..0e3824a 100644 --- a/packages/gateway/src/upstream/manager.test.ts +++ b/packages/gateway/src/upstream/manager.test.ts @@ -19,6 +19,7 @@ class FakeLink implements UpstreamLink { onRecovered: (() => void) | null = null; connectCalls = 0; failConnect = false; + failDiscovery = false; lastCall: { name: string; args: Record } | null = null; constructor( @@ -32,6 +33,7 @@ class FakeLink implements UpstreamLink { } async listTools(): Promise { + if (this.failDiscovery) throw new Error("Unknown or expired MCP session"); return this.tools; } @@ -99,6 +101,30 @@ describe("UpstreamManager", () => { expect(exposedNames(manager)).toEqual(["demo_echo"]); }); + /** + * #42: discovery can fail while the transport still reports connected, and + * `connected: true, lastError: null, toolCount: 0` reads exactly like "this + * server has no tools" — which is how an emptied catalog went unnoticed. + */ + it("reports WHY an upstream contributed no tools, instead of looking healthy and empty", async () => { + const link = new FakeLink(spec("everything", "demo"), [tool("echo")]); + const { manager } = setup([link]); + await manager.start(); + expect(manager.summaries()[0]!.toolCount).toBe(1); + + link.failDiscovery = true; + await manager.refreshCatalog(); + + const summary = manager.summaries()[0]!; + expect(summary.toolCount).toBe(0); + expect(summary.lastError).toMatch(/tool discovery failed/i); + + // …and the error clears once discovery works again. + link.failDiscovery = false; + await manager.refreshCatalog(); + expect(manager.summaries()[0]!).toMatchObject({ toolCount: 1, lastError: null }); + }); + it("skips disabled upstreams but still reports them in summaries", async () => { const { manager } = setup( [new FakeLink(spec("everything", "demo"), [tool("echo")])], diff --git a/packages/gateway/src/upstream/manager.ts b/packages/gateway/src/upstream/manager.ts index 3058893..6cc166f 100644 Binary files a/packages/gateway/src/upstream/manager.ts and b/packages/gateway/src/upstream/manager.ts differ diff --git a/packages/gateway/src/upstream/personal-sessions.test.ts b/packages/gateway/src/upstream/personal-sessions.test.ts index ecb83ed..898f030 100644 --- a/packages/gateway/src/upstream/personal-sessions.test.ts +++ b/packages/gateway/src/upstream/personal-sessions.test.ts @@ -197,6 +197,63 @@ describe("per-user upstream sessions", () => { expect(after).toMatch(/^who via bao:gw-user-/); }); + /** + * Regression for #44: the personal link was memoized per principal and only + * ever flushed by the upstream lifecycle, so a credential the user rotated on + * /me kept being ignored — silently — until someone bounced the upstream. + */ + it("a credential write drops that caller's pooled link so the new value is picked up", async () => { + await putCredential("tok-alice", "peruser", "first-secret"); + await callTool("tok-alice", "peruser_who"); // builds + pools the link + const before = linksCreated.length; + + const rotated = await fetch(`${base}/api/me/credentials/peruser`, { + method: "PUT", + headers: { "Content-Type": "application/json", Authorization: "Bearer tok-alice" }, + body: JSON.stringify({ field: "Authorization", value: "rotated-secret" }), + }); + expect((await rotated.json()) as { reconnected: boolean }).toMatchObject({ reconnected: true }); + + await callTool("tok-alice", "peruser_who"); + expect(linksCreated.length).toBeGreaterThan(before); // rebuilt, not reused + }); + + it("only the writer's link is dropped — other principals keep theirs", async () => { + await putCredential("tok-bob", "peruser", "bob-secret"); + await callTool("tok-bob", "peruser_who"); + await callTool("tok-alice", "peruser_who"); + const before = linksCreated.length; + + await putCredential("tok-alice", "peruser", "alice-again"); + await callTool("tok-bob", "peruser_who"); // bob's link untouched + + expect(linksCreated.length).toBe(before); + }); + + it("a credential delete drops the link too, so the stale ref cannot linger", async () => { + await putCredential("tok-alice", "peruser", "to-be-deleted"); + await callTool("tok-alice", "peruser_who"); + const before = linksCreated.length; + + const removed = await fetch(`${base}/api/me/credentials/peruser/Authorization`, { + method: "DELETE", + headers: { Authorization: "Bearer tok-alice" }, + }); + expect((await removed.json()) as { ok: boolean; reconnected: boolean }).toMatchObject({ + ok: true, + reconnected: true, + }); + + // No creds left → the call falls back to the SHARED link (this upstream + // allows it), so the caller now travels on the gateway's own credential + // rather than on a personal ref that no longer exists. + const text = await callTool("tok-alice", "peruser_who"); + expect(text).toContain("bao:upstreams/peruser"); + expect(text).not.toContain("gw-user-"); + // The shared link already existed, so nothing new had to be built. + expect(linksCreated.length).toBe(before); + }); + it("upstream removal closes and forgets personal links", async () => { const before = linksCreated.length; await manager.upsertUpstream(perUserSpec); // upsert = remove + re-register