From 6894fee7c008bf61d8c702be55e9da062157cfb1 Mon Sep 17 00:00:00 2001 From: Minxi Hou Date: Wed, 5 Aug 2026 12:40:49 -0400 Subject: [PATCH 1/2] fix(providers): refuse to store the dashboard password as a connection API key A browser autofilled the management password into a connection's API-key field. The resulting credential authenticates against nothing, so every request routed through that connection came back 401, and because the field looks like any other password input the same autofill fired again while the connection was being repaired by hand. The refusal belongs on the write path rather than in the form. Twenty routes create or update connections and all of them funnel through createProviderConnection and updateProviderConnection, so one check there covers every entry point including a future one. The two other places that write api_key are left alone on purpose: one re-encrypts rows that already exist and the other is the one-time db.json import, and neither takes a value an operator just typed. Update checks the incoming value, never the merged one. A connection that already holds the password has to stay editable or the operator cannot repair the exact state this prevents, and re-checking the merged value would spend a bcrypt round on every unrelated field edit. Only a real match blocks the write. An unreadable settings row or a throwing bcrypt call logs and allows, because a guard against one specific mistake must not turn into a way to lock out every connection write. Signed-off-by: Minxi Hou --- src/lib/db/providers.ts | 63 +++++- ...ject-management-password-as-apikey.test.ts | 181 ++++++++++++++++++ 2 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 tests/unit/reject-management-password-as-apikey.test.ts diff --git a/src/lib/db/providers.ts b/src/lib/db/providers.ts index 02a39f74c510..fae797e35728 100644 --- a/src/lib/db/providers.ts +++ b/src/lib/db/providers.ts @@ -19,7 +19,12 @@ import { } from "@omniroute/open-sse/services/apiKeyRotator.ts"; import { invalidateReasoningRoutingRuleCache } from "./reasoningRoutingRules"; import { normalizeProviderSpecificData } from "@/lib/providers/requestDefaults"; -import { bumpProxyConfigGeneration } from "./settings"; +import { bumpProxyConfigGeneration, getSettings } from "./settings"; +import { + getStoredManagementPassword, + isBcryptHash, + verifyManagementPassword, +} from "@/lib/auth/managementPassword"; import { webSessionCredentialKey, parseProviderSpecificData } from "./webSessionDedup"; import { pickCodexConnectionForUser } from "@/lib/oauth/utils/codexConnectionSelection"; import { reconcileCodexUsageHistory } from "./providers/usageIdentityReconciliation"; @@ -40,6 +45,55 @@ type JsonRecord = Record; const CONNECTION_CREDENTIAL_FIELDS = ["apiKey", "accessToken", "refreshToken", "idToken"] as const; +/** Thrown when a write would store the dashboard login password as a provider credential. */ +export class ManagementPasswordAsCredentialError extends Error { + readonly code = "MANAGEMENT_PASSWORD_AS_CREDENTIAL" as const; + + constructor() { + super( + "That value is the dashboard login password, not a provider API key. Storing it would " + + "send it upstream on every request routed through this connection." + ); + this.name = "ManagementPasswordAsCredentialError"; + } +} + +/** + * Refuse to store the dashboard login password as a connection API key. + * + * A browser that autofills the management password into the API-key field + * produces a connection whose credential authenticates against nothing, and + * every request routed through it comes back 401. Rejecting it in the form + * would not be enough: the same autofill fires again while an operator is + * repairing the connection by hand, so the refusal has to sit on the write + * path that all of those forms funnel into. + * + * 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. + */ +async function assertApiKeyIsNotManagementPassword(apiKey: unknown): Promise { + const candidate = typeof apiKey === "string" ? apiKey.trim() : ""; + if (!candidate) return; + + try { + const settings = (await getSettings()) as JsonRecord; + const stored = getStoredManagementPassword(settings); + // Only a stored bcrypt hash is comparable. A fresh install that has never + // bootstrapped a password has nothing to collide with. + if (!isBcryptHash(stored)) return; + if (!(await verifyManagementPassword(candidate, stored))) return; + } catch (err) { + console.warn( + "[Providers] could not check the credential against the dashboard password:", + err instanceof Error ? err.message : String(err) + ); + return; + } + + throw new ManagementPasswordAsCredentialError(); +} + interface StatementLike { all: (...params: unknown[]) => TRow[]; get: (...params: unknown[]) => TRow | undefined; @@ -282,6 +336,7 @@ function findExistingCookieConnection( } export async function createProviderConnection(data: JsonRecord) { + await assertApiKeyIsNotManagementPassword(data.apiKey); const db = getDbInstance() as unknown as DbLike; const now = new Date().toISOString(); const normalizedProviderSpecificData = normalizeProviderSpecificData( @@ -730,6 +785,12 @@ export async function updateProviderConnection(id: string, data: JsonRecord) { const existing = db.prepare("SELECT * FROM provider_connections WHERE id = ?").get(id); if (!existing) return null; + // The incoming value only. A connection that already holds the password has + // to stay editable, or an operator cannot repair the one this guard exists + // to prevent -- and re-checking the merged value would spend a bcrypt round + // on every unrelated field edit. + await assertApiKeyIsNotManagementPassword(data.apiKey); + const merged: JsonRecord = { ...toRecord(rowToCamel(existing)), ...data, diff --git a/tests/unit/reject-management-password-as-apikey.test.ts b/tests/unit/reject-management-password-as-apikey.test.ts new file mode 100644 index 000000000000..2d6c241be09b --- /dev/null +++ b/tests/unit/reject-management-password-as-apikey.test.ts @@ -0,0 +1,181 @@ +/** + * The dashboard login password must never be storable as a provider API key. + * + * A browser autofilling the management password into the API-key field created + * connections that 401 every request routed through them, and the same autofill + * fired again while the connection was being repaired by hand. These assert the + * refusal sits on the write path rather than in any one form. + */ + +import { after, beforeEach, describe, it } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-apikey-guard-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.DISABLE_SQLITE_AUTO_BACKUP = "true"; + +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); +const mgmt = await import("../../src/lib/auth/managementPassword.ts"); + +const DASHBOARD_PASSWORD = "correct-horse-battery-staple"; + +/** getStoredManagementPassword reads `settings.password`, holding a bcrypt hash. */ +async function storeDashboardPassword(plaintext: string) { + await settingsDb.updateSettings({ password: await mgmt.hashManagementPassword(plaintext) }); +} + +beforeEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +}); + +after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +describe("management password as a provider credential", () => { + it("create is refused when the apiKey is the dashboard password", async () => { + await storeDashboardPassword(DASHBOARD_PASSWORD); + + await assert.rejects( + () => + providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "autofilled", + apiKey: DASHBOARD_PASSWORD, + isActive: true, + }), + (err: Error) => { + assert.equal(err.name, "ManagementPasswordAsCredentialError"); + assert.ok( + !err.message.includes(DASHBOARD_PASSWORD), + "the refusal must not echo the password back" + ); + return true; + } + ); + + const rows = await providersDb.getProviderConnections({ provider: "openai" }); + assert.equal(rows.length, 0, "nothing may be persisted when the guard fires"); + }); + + it("create is refused on surrounding whitespace, which a paste carries", async () => { + await storeDashboardPassword(DASHBOARD_PASSWORD); + + await assert.rejects( + () => + providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "pasted", + apiKey: ` ${DASHBOARD_PASSWORD} `, + isActive: true, + }), + /dashboard login password/ + ); + }); + + it("a real API key is stored normally", async () => { + await storeDashboardPassword(DASHBOARD_PASSWORD); + + const conn = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "genuine", + apiKey: "sk-a-real-provider-key", + isActive: true, + }); + assert.ok(conn?.id); + }); + + it("update is refused too, which is where the repair attempt gets re-infected", async () => { + await storeDashboardPassword(DASHBOARD_PASSWORD); + + const conn = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "genuine", + apiKey: "sk-a-real-provider-key", + isActive: true, + }); + assert.ok(conn?.id); + + await assert.rejects( + () => providersDb.updateProviderConnection(conn.id as string, { apiKey: DASHBOARD_PASSWORD }), + /dashboard login password/ + ); + + const stored = await providersDb.getProviderConnectionById(conn.id as string); + assert.equal(stored?.apiKey, "sk-a-real-provider-key", "the good key must survive the refusal"); + }); + + it("an update that does not carry an apiKey is untouched by the guard", async () => { + await storeDashboardPassword(DASHBOARD_PASSWORD); + + const conn = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "before", + apiKey: "sk-a-real-provider-key", + isActive: true, + }); + assert.ok(conn?.id); + + const updated = await providersDb.updateProviderConnection(conn.id as string, { + name: "after", + }); + assert.equal(updated?.name, "after"); + }); + + it("a connection already holding the password stays editable, so it can be repaired", async () => { + // Seed the bad state the way the incident produced it: the password was + // stored before the guard existed. Write it with no dashboard password + // configured, then configure one. + const conn = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "poisoned", + apiKey: DASHBOARD_PASSWORD, + isActive: true, + }); + assert.ok(conn?.id, "no dashboard password configured yet, so the write goes through"); + await storeDashboardPassword(DASHBOARD_PASSWORD); + + const repaired = await providersDb.updateProviderConnection(conn.id as string, { + apiKey: "sk-the-actual-key", + }); + assert.equal(repaired?.apiKey, "sk-the-actual-key"); + }); + + it("no dashboard password configured means nothing to collide with", async () => { + const conn = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "fresh install", + apiKey: DASHBOARD_PASSWORD, + isActive: true, + }); + assert.ok(conn?.id); + }); + + it("an OAuth connection carrying no apiKey never reaches the bcrypt round", async () => { + await storeDashboardPassword(DASHBOARD_PASSWORD); + + 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"); + }); +}); From f950b063f36aebd2db782ac265234309b21f04f8 Mon Sep 17 00:00:00 2001 From: Minxi Hou Date: Wed, 5 Aug 2026 14:04:22 -0400 Subject: [PATCH 2/2] 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 fae797e35728..f6853c355295 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" + ); }); });