From 8822b18f7052a83a697b502c77f8d762fe672cf8 Mon Sep 17 00:00:00 2001 From: Minxi Hou Date: Wed, 5 Aug 2026 14:04:22 -0400 Subject: [PATCH] fix(providers): compare the untrimmed credential, and cover the guard's branches The guard trimmed the incoming value before comparing it, which catches a paste carrying whitespace the password does not have. It missed the mirror case: neither the login route nor the set-password route trims, so a dashboard password may itself begin or end with a space, and an autofill reproducing it exactly was trimmed into a value that no longer matched the stored hash. The write then went through, which is the state this guard exists to prevent. Both forms are compared now, the second only when the first fails on a string that differs, so an ordinary key still costs a single bcrypt round. Two branches carried no coverage and both are load-bearing. The catch that logs and allows is the only path that lets a write through; a stored hash bcrypt cannot parse reaches it without needing a mock, since the shape check accepts an impossible cost factor that the comparison then rejects. The early return is what keeps a token renewal -- a write carrying tokens but no apiKey -- from paying for a settings read and a bcrypt round every time it fires, and the same unparseable hash makes that path observable, so an absent warning is proof the return happened. The narrower scope is deliberate and now says so in the code: the OAuth tokens arrive from a provider's token endpoint rather than from a form, so extending the comparison to them would charge every renewal for a field no autofill can reach. Signed-off-by: Minxi Hou --- src/lib/db/providers.ts | 20 +++- ...ject-management-password-as-apikey.test.ts | 97 +++++++++++++++++-- 2 files changed, 105 insertions(+), 12 deletions(-) diff --git a/src/lib/db/providers.ts b/src/lib/db/providers.ts index fae797e357..f6853c3552 100644 --- a/src/lib/db/providers.ts +++ b/src/lib/db/providers.ts @@ -71,10 +71,17 @@ export class ManagementPasswordAsCredentialError extends Error { * Only an actual match blocks the write. A settings row that cannot be read, * or a bcrypt call that throws, logs and allows -- a guard against one specific * operator mistake must not become a way to lock out every connection write. + * + * Deliberately narrower than CONNECTION_CREDENTIAL_FIELDS. The OAuth tokens + * arrive from a provider's token endpoint, and the refresh path writes them + * back through updateProviderConnection on every renewal, so checking them + * would put a bcrypt round on a renewal path to defend a field no autofill + * reaches. apiKey is the only credential an operator types into a form. */ async function assertApiKeyIsNotManagementPassword(apiKey: unknown): Promise { - const candidate = typeof apiKey === "string" ? apiKey.trim() : ""; - if (!candidate) return; + if (typeof apiKey !== "string") return; + const trimmed = apiKey.trim(); + if (!trimmed) return; try { const settings = (await getSettings()) as JsonRecord; @@ -82,7 +89,14 @@ async function assertApiKeyIsNotManagementPassword(apiKey: unknown): Promise { ); }); + it("a password that carries its own whitespace is caught too", async () => { + // Neither the login route nor the set-password route trims, so this is a + // password an operator can really have. Comparing only the trimmed value + // would miss it and store the password the guard exists to reject. + const padded = ` ${DASHBOARD_PASSWORD} `; + await storeDashboardPassword(padded); + + await assert.rejects( + () => + providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "autofilled", + apiKey: padded, + isActive: true, + }), + /dashboard login password/ + ); + }); + it("a real API key is stored normally", async () => { await storeDashboardPassword(DASHBOARD_PASSWORD); @@ -167,15 +187,74 @@ describe("management password as a provider credential", () => { }); it("an OAuth connection carrying no apiKey never reaches the bcrypt round", async () => { - await storeDashboardPassword(DASHBOARD_PASSWORD); + // Storing a hash bcrypt cannot parse turns the expensive path into an + // observable one: reaching it throws and the catch logs. Silence is then + // proof the guard returned before touching settings at all, which is what + // keeps a token renewal -- a write that carries tokens but no apiKey -- + // from paying for a database read and a bcrypt round on every renewal. + await settingsDb.updateSettings({ password: `$2a$99$${"a".repeat(53)}` }); - const conn = await providersDb.createProviderConnection({ - provider: "claude", - authType: "oauth", - name: "oauth", - accessToken: DASHBOARD_PASSWORD, - isActive: true, - }); - assert.ok(conn?.id, "the guard covers apiKey only; OAuth tokens are not operator-typed"); + const warnings: string[] = []; + const realWarn = console.warn; + console.warn = (...args: unknown[]) => { + warnings.push(args.map((a) => String(a)).join(" ")); + }; + + try { + const conn = await providersDb.createProviderConnection({ + provider: "claude", + authType: "oauth", + name: "oauth", + accessToken: DASHBOARD_PASSWORD, + isActive: true, + }); + assert.ok(conn?.id, "the guard covers apiKey only"); + } finally { + console.warn = realWarn; + } + + assert.equal( + warnings.filter((w) => w.includes("could not check the credential")).length, + 0, + "a write carrying no apiKey must not reach settings or bcrypt" + ); + }); + + it("a check that cannot complete allows the write instead of blocking it", async () => { + // isBcryptHash validates shape, not semantics, so a stored hash carrying an + // impossible cost factor passes it and then makes bcrypt.compare throw. That + // reaches the same branch as an unreadable settings row, without a mock. + const unparseable = `$2a$99$${"a".repeat(53)}`; + + // Asserted rather than assumed. Should bcrypt ever resolve instead of + // throwing here, the guard would take its no-match return and this test + // would quietly stop covering the catch branch, so name the reason first. + await assert.rejects(() => mgmt.verifyManagementPassword("anything", unparseable)); + + await settingsDb.updateSettings({ password: unparseable }); + + const warnings: string[] = []; + const realWarn = console.warn; + console.warn = (...args: unknown[]) => { + warnings.push(args.map((a) => String(a)).join(" ")); + }; + + try { + const conn = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "unverifiable", + apiKey: DASHBOARD_PASSWORD, + isActive: true, + }); + assert.ok(conn?.id, "one broken settings row must not block every connection write"); + } finally { + console.warn = realWarn; + } + + assert.ok( + warnings.some((w) => w.includes("could not check the credential")), + "failing open silently would hide that the guard stopped guarding" + ); }); });