From 05ae697ff49e51521417d2ee8eed32a57727b383 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 30 Aug 2026 05:32:19 -0700 Subject: [PATCH 1/2] Add test for verified oauth_token reconnect rotation Reproduces CL-7236: an interactive MCP OAuth reconnect passes credentialVerified: true after completing a fresh exchange, but a name conflict against a still-active existing row currently skips rotation entirely -- zero PATCH calls, and the stale credential id is returned as if the reconnect succeeded. --- packages/hub-client/test/seed.test.ts | 105 ++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/packages/hub-client/test/seed.test.ts b/packages/hub-client/test/seed.test.ts index 37a773b93..260b15e3a 100644 --- a/packages/hub-client/test/seed.test.ts +++ b/packages/hub-client/test/seed.test.ts @@ -1900,6 +1900,111 @@ describe("seedCatalog", () => { expect(patchCalls).toBe(0); }); + // CL-7236: an interactive MCP OAuth reconnect (packages/connections/src/ + // mcp-oauth-routes.ts) passes `credentialVerified: true` after a + // completed OAuth exchange — a user re-authorizing an integration whose + // stored row hasn't technically expired yet (refreshed provider-side + // scopes, or a proactive reconnect). Before the fix, the oauth_token + // branch of `shouldRotate` looked only at `existing.status`, ignoring + // `verified` entirely: zero PATCH calls, and the stale row's id returned + // as though the reconnect had worked. + test("a name conflict on a verified oauth_token reconnect rotates the still-active row", async () => { + const { lines, log } = collector(); + let patchCalls = 0; + let patchBody: unknown; + + const activeCredentialRow = () => ({ + id: "cre_active", + tenantId: TENANT_ID, + providerId: "prv_1", + name: "huggingface-default", + type: "oauth_token", + status: "active", + metadata: { expiresAt: "2026-09-01T00:00:00.000Z" }, + createdAt: TIMESTAMP, + updatedAt: TIMESTAMP, + }); + + const handler: FakeHandler = (method, path, body) => { + if (method === "POST" && path === `/api/tenants/${TENANT_ID}/providers`) + return { status: 201, data: providerRow("prv_1", "huggingface") }; + if (method === "POST" && path === `/api/tenants/${TENANT_ID}/credentials`) + return { status: 409, data: { error: "name taken" } }; + if (method === "GET" && path === `/api/tenants/${TENANT_ID}/credentials`) + return { + status: 200, + data: { data: [activeCredentialRow()], nextCursor: null }, + }; + if ( + method === "PATCH" && + path === `/api/tenants/${TENANT_ID}/credentials/cre_active` + ) { + patchCalls += 1; + patchBody = body; + return { + status: 200, + data: { + ...activeCredentialRow(), + metadata: { expiresAt: "2026-10-01T00:00:00.000Z" }, + }, + }; + } + if ( + method === "POST" && + path === `/api/tenants/${TENANT_ID}/catalog/models` + ) + return { + status: 201, + data: catalogModelRow("mdl_1", "deepseek-ai/DeepSeek-V4-Flash"), + }; + if ( + method === "POST" && + path === `/api/tenants/${TENANT_ID}/catalog/providers` + ) + return { + status: 201, + data: catalogProviderRow( + "cpv_1", + "huggingface", + "cre_active", + "openai-compatible", + "https://router.huggingface.co/v1", + ), + }; + if ( + method === "POST" && + path === `/api/tenants/${TENANT_ID}/catalog/offerings` + ) + return { + status: 201, + data: catalogOfferingRow("off_1", "mdl_1", "cpv_1"), + }; + return undefined; + }; + + await seedCatalog({ + api: fakeAPI(handler), + cookies: [], + tenantId: TENANT_ID, + provider: "huggingface", + apiKey: "hf_freshly_reauthorized_token", + credentialType: "oauth_token", + credentialVerified: true, + credentialMetadata: { expiresAt: "2026-10-01T00:00:00.000Z" }, + log, + }); + + expect(patchCalls).toBe(1); + expect(patchBody).toEqual({ + secret: "hf_freshly_reauthorized_token", + status: "active", + metadata: { expiresAt: "2026-10-01T00:00:00.000Z" }, + }); + expect(lines.some((line) => line.includes("rotated credential"))).toBe( + true, + ); + }); + // A regenerated OpenRouter key, or a retry after a bad paste, reconnects // under the same stable credential name — the caller has already proven // the fresh key against the provider's own probe (`credentialVerified: From 52ab8ae60cf844d61d39f80a4b57dc893c7bbd39 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 30 Aug 2026 05:34:30 -0700 Subject: [PATCH 2/2] ensureCredential: rotate a still-active oauth_token on verified reconnect The oauth_token rotate-on-conflict check only looked at the existing row's status, so a caller passing verified: true (an interactive MCP OAuth reconnect that just completed a fresh, probe-confirmed exchange) had that signal silently ignored whenever the stored row hadn't yet gone stale. The freshly minted token was dropped and the stale credential id returned as if the reconnect had worked. Rotate whenever the row is stale OR the caller marks the token verified, mirroring how the api_key branch already uses verified. --- packages/hub-client/src/seed.ts | 49 +++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/packages/hub-client/src/seed.ts b/packages/hub-client/src/seed.ts index 6893a7d2e..f3ddc5cc9 100644 --- a/packages/hub-client/src/seed.ts +++ b/packages/hub-client/src/seed.ts @@ -1121,35 +1121,44 @@ export async function ensureCredential( ); } - // An `oauth_token` credential (Hugging Face today) can reconnect under - // its same stable name with a fresh secret and a fresh `expiresAt` - // once the stored one has gone stale — reusing the stale row instead - // of rotating it would silently strand the reconnect on the old, - // already-expired secret, and since the row's `status` is already - // non-`active`, the expiry sweep would never see it again to re-notify. - // Scoped to exactly that case: an `active` row (the common idempotent - // re-seed) is left untouched, so a routine re-seed with an unchanged - // token never turns into a rotation. + // An `oauth_token` credential (Hugging Face, or an MCP server connected + // through OAuth) rotates on a name conflict in two distinct cases: + // + // 1. The stored row has gone stale (`status !== "active"`) — a plain + // re-seed or a reconnect after the expiry sweep already flipped it. + // Reusing the stale row instead of rotating it would silently strand + // the reconnect on the old, already-expired secret, and since the + // row's `status` is already non-`active`, the expiry sweep would + // never see it again to re-notify. + // 2. The caller sets `args.verified` — an interactive OAuth reconnect + // (`connections`' `mcp-oauth-routes.ts`) completed a fresh exchange + // and is handing `ensureCredential` a genuinely new token, even + // though the existing row hasn't technically expired yet (the user + // re-authorized proactively, or the provider-side scopes changed). + // Gating on `status` alone silently dropped this token (CL-7236): + // zero PATCH call, and the stale row's id returned as if the + // reconnect had worked. + // + // A plain `workbench seed` never sets `verified` on an `oauth_token` + // credential — its token comes straight from env with no OAuth exchange + // of its own — so an idempotent re-seed of a still-active row still + // just skips, exactly as before. // // An `api_key` credential (OpenRouter, an onboarding-picked provider) - // has no such staleness signal — its row stays `active` whether or not - // the person reconnecting regenerated the key or is retrying after a - // bad paste — so `status` can't gate it the way it gates `oauth_token`. - // It rotates on a name conflict only when `args.verified` is set, - // which a caller sets only for an explicit user submission through a - // connect UI: `testAndPersistCredential` + // has no staleness signal at all — its row stays `active` whether or + // not the person reconnecting regenerated the key or is retrying after + // a bad paste — so it rotates on a name conflict only when + // `args.verified` is set, which a caller sets only for an explicit user + // submission through a connect UI: `testAndPersistCredential` // (`@workbench/onboarding`'s `complete-credential.ts`) sets it // unconditionally for a pasted key or a completed OAuth exchange // (CL-6123 dropped the probe that used to gate this), and // `connections`' `POST /:connectorId/complete` (`routes.ts`) still // sets it only after `descriptor.probe` passes, since that surface - // (Settings > Connections) is allowed to block on a real check. A - // plain `workbench seed` never sets `verified` — its key comes - // straight from env with no probe of its own — so that idempotent - // re-seed still just skips, exactly as before. + // (Settings > Connections) is allowed to block on a real check. const shouldRotate = args.type === "oauth_token" - ? existing.status !== "active" + ? existing.status !== "active" || args.verified === true : args.verified === true; if (shouldRotate) { const rotated = await api(