Skip to content

Fix ensureCredential dropping a verified oauth_token reconnect #489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TheGreatAxios merged 2 commits into from
Aug 30, 2026
Merged

Fix ensureCredential dropping a verified oauth_token reconnect #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05ae697
Add test for verified oauth_token reconnect rotation
TheGreatAxios Aug 30, 2026
52ab8ae
ensureCredential: rotate a still-active oauth_token on verified recon…
TheGreatAxios Aug 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions packages/hub-client/src/seed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
105 changes: 105 additions & 0 deletions packages/hub-client/test/seed.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading