From ded204b277bb4bef50af91430a601989b38e771e Mon Sep 17 00:00:00 2001 From: Minxi Hou Date: Wed, 5 Aug 2026 12:40:49 -0400 Subject: [PATCH] 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 02a39f74c5..fae797e357 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 0000000000..2d6c241be0 --- /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"); + }); +});