diff --git a/changelog.d/fixes/7234-bulk-add-keys-no-overwrite.md b/changelog.d/fixes/7234-bulk-add-keys-no-overwrite.md new file mode 100644 index 0000000000..853a1103d2 --- /dev/null +++ b/changelog.d/fixes/7234-bulk-add-keys-no-overwrite.md @@ -0,0 +1 @@ +- **api:** bulk-add API keys no longer overwrite existing provider connections — a colliding auto- or custom-generated name now gap-fills a free suffix instead of silently replacing a saved connection's key/state. (thanks @asynx6) diff --git a/src/app/api/providers/bulk/route.ts b/src/app/api/providers/bulk/route.ts index 2929a9ec1d..e2fef1b258 100644 --- a/src/app/api/providers/bulk/route.ts +++ b/src/app/api/providers/bulk/route.ts @@ -4,13 +4,19 @@ import { getProviderAuditTarget, summarizeProviderConnectionForAudit, } from "@/lib/compliance/providerAudit"; -import { createProviderConnection, getProviderNodeById, isCloudEnabled } from "@/models"; +import { + createProviderConnection, + getProviderConnections, + getProviderNodeById, + isCloudEnabled, +} from "@/models"; import { isAnthropicCompatibleProvider, isOpenAICompatibleProvider, supportsBulkApiKey, } from "@/shared/constants/providers"; import { getConsistentMachineId } from "@/shared/utils/machineId"; +import { resolveBulkNameCollisions } from "@/shared/utils/bulkApiKeyParser"; import { syncToCloud } from "@/lib/cloudSync"; import { bulkCreateProviderSchema } from "@/shared/validation/schemas"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; @@ -103,11 +109,23 @@ export async function POST(request: Request) { null : null; + // #2587 — createProviderConnection upserts apikey connections BY NAME, so a + // bulk-add name that collides with an already-saved connection (or with + // another entry in the same batch) would silently REPLACE that connection's + // apiKey/priority/testStatus instead of inserting a new one. Resolve every + // collision up front by gap-filling a free " " suffix so each entry + // reaches createProviderConnection as a genuine insert. + const existingConnections = await getProviderConnections({ provider, authType: "apikey" }); + const existingNames = existingConnections + .map((c) => (typeof c.name === "string" ? c.name : null)) + .filter((n): n is string => !!n); + const resolvedEntries = resolveBulkNameCollisions(entries, existingNames); + const created: Array> = []; const errors: Array<{ index: number; name: string; message: string }> = []; - for (let i = 0; i < entries.length; i++) { - const entry = entries[i]; + for (let i = 0; i < resolvedEntries.length; i++) { + const entry = resolvedEntries[i]; try { // Per-entry copy so each connection gets its own providerSpecificData. Cloudflare // Workers AI carries a per-key accountId (name|accountId|apiKey) that must NOT bleed diff --git a/src/shared/utils/bulkApiKeyParser.ts b/src/shared/utils/bulkApiKeyParser.ts index a7ca1d0ce9..f6b1441f21 100644 --- a/src/shared/utils/bulkApiKeyParser.ts +++ b/src/shared/utils/bulkApiKeyParser.ts @@ -111,3 +111,57 @@ export function parseBulkApiKeys( } export const BULK_API_KEY_MAX_LINES = MAX_BULK_LINES; + +// Strips a trailing " " suffix so a colliding name's numeric index can be +// regenerated instead of stacking (e.g. "Key 1" -> "Key", not "Key 1 2"). +function stripTrailingIndex(name: string): string { + const stripped = name.replace(/\s+\d+$/, ""); + return stripped.length > 0 ? stripped : name; +} + +/** + * Resolves name collisions across a batch of bulk-add entries. + * + * Background: `createProviderConnection` upserts apikey connections BY NAME + * (see `src/lib/db/providers.ts` — same provider + auth_type "apikey" + same + * `name` updates the existing row instead of inserting a new one, replacing + * its `apiKey`/`priority`/`testStatus`). `parseBulkApiKeys` auto-names + * unnamed lines "Key 1", "Key 2", ... per request, blind to names already + * saved for the provider — and a batch can also contain the same custom + * `name|apiKey` name twice. Either case previously reached the backend upsert + * path and silently overwrote (or self-collapsed) an existing connection + * instead of inserting a new one. + * + * This resolves every collision — against `existingNames` AND against names + * already assigned earlier in the same batch — by gap-filling the smallest + * free " " suffix, so a name is never reused and every entry reaches + * the backend as a genuine insert. + */ +export function resolveBulkNameCollisions( + entries: T[], + existingNames: readonly string[] | null | undefined +): T[] { + const used = new Set( + (Array.isArray(existingNames) ? existingNames : []) + .filter((n): n is string => typeof n === "string" && n.length > 0) + .map((n) => n.toLowerCase()) + ); + + return entries.map((entry) => { + const lowerName = entry.name.toLowerCase(); + if (!used.has(lowerName)) { + used.add(lowerName); + return entry; + } + + const base = stripTrailingIndex(entry.name); + let idx = 1; + let candidate = `${base} ${idx}`; + while (used.has(candidate.toLowerCase())) { + idx += 1; + candidate = `${base} ${idx}`; + } + used.add(candidate.toLowerCase()); + return { ...entry, name: candidate }; + }); +} diff --git a/tests/unit/bulk-add-keys-no-overwrite-2587.test.ts b/tests/unit/bulk-add-keys-no-overwrite-2587.test.ts new file mode 100644 index 0000000000..27927316e1 --- /dev/null +++ b/tests/unit/bulk-add-keys-no-overwrite-2587.test.ts @@ -0,0 +1,174 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +type ConnectionRow = Record & { id: string; name?: string | null }; + +// #2587 — bulk-add API keys must APPEND connections, never silently overwrite +// an existing one. `createProviderConnection` upserts apikey connections BY +// NAME (src/lib/db/providers.ts): same provider + auth_type "apikey" + same +// `name` updates the existing row (replacing its apiKey/priority/testStatus) +// instead of inserting a new one. Bulk-add auto-names unnamed lines +// "Key 1", "Key 2", ... starting fresh on every request, blind to names +// already saved for the provider — so re-running a bulk paste against a +// provider that already has "Key 1" silently replaced it instead of adding a +// new connection alongside it. +// +// The fix (POST /api/providers/bulk, src/app/api/providers/bulk/route.ts) +// fetches existing connection names for the provider and runs +// `resolveBulkNameCollisions` (src/shared/utils/bulkApiKeyParser.ts) before +// calling createProviderConnection, gap-filling a free " " suffix so +// every entry reaches createProviderConnection as a genuine insert. This test +// exercises that exact production sequence — getProviderConnections -> +// resolveBulkNameCollisions -> createProviderConnection — against a real +// SQLite-backed db module. + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-bulk-add-2587-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const { resolveBulkNameCollisions } = await import("../../src/shared/utils/bulkApiKeyParser.ts"); + +async function resetStorage() { + core.resetDbInstance(); + for (let attempt = 0; attempt < 10; attempt++) { + try { + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + } + break; + } catch (error) { + const code = (error as { code?: string } | undefined)?.code; + if ((code === "EBUSY" || code === "EPERM") && attempt < 9) { + await new Promise((resolve) => setTimeout(resolve, 50 * (attempt + 1))); + } else { + throw error; + } + } + } + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("bulk-add appends N+M connections and preserves the existing connection's state (the #2587 fix)", async () => { + // Existing connection carries live resilience state: an active rate-limit + // cooldown, a recorded error, and a non-default backoff level/priority. + const future = new Date(Date.now() + 60_000).toISOString(); + const created = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "Key 1", + apiKey: "sk-existing", + priority: 3, + testStatus: "unavailable", + lastError: "429 rate limited", + lastErrorType: "rate_limit", + rateLimitedUntil: future, + }); + assert.ok(created, "existing connection must be created"); + // `backoffLevel` is set by the resilience/cooldown path (not at creation + // time) — apply it the same way a real cooldown escalation would. + const existing = await providersDb.updateProviderConnection(created!.id as string, { + backoffLevel: 2, + }); + assert.ok(existing, "existing connection must be updatable"); + + const before = await providersDb.getProviderConnections({ provider: "openai" }); + assert.equal(before.length, 1); + + // Simulates a fresh bulk-add paste (parseBulkApiKeys restarts auto-naming at + // "Key 1" every request) — this is exactly what collides with the existing + // connection above. + const rawEntries = [ + { name: "Key 1", apiKey: "sk-new-1" }, + { name: "Key 2", apiKey: "sk-new-2" }, + ]; + + // Reproduces the route's fix: fetch existing apikey names for the provider, + // then resolve collisions before ever calling createProviderConnection. + const existingApikeyConnections = (await providersDb.getProviderConnections({ + provider: "openai", + authType: "apikey", + })) as ConnectionRow[]; + const existingNames = existingApikeyConnections + .map((c) => (typeof c.name === "string" ? c.name : null)) + .filter((n): n is string => !!n); + const resolvedEntries = resolveBulkNameCollisions(rawEntries, existingNames); + + // Neither new entry may reuse the existing connection's name. + assert.ok(!resolvedEntries.some((e) => e.name === "Key 1")); + + for (const entry of resolvedEntries) { + const created = await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: entry.name, + apiKey: entry.apiKey, + priority: 1, + testStatus: "unknown", + }); + assert.ok(created); + } + + const after = (await providersDb.getProviderConnections({ + provider: "openai", + })) as ConnectionRow[]; + // N (1 existing) + M (2 new) = 3 connections — never 2 (an insert masquerading + // as an overwrite) or 1 (both new entries collapsing into the existing row). + assert.equal(after.length, before.length + rawEntries.length); + + const survivor = after.find((c) => c.id === (existing as ConnectionRow).id); + assert.ok(survivor, "the pre-existing connection must still exist, unreplaced"); + assert.equal(survivor!.name, "Key 1"); + assert.equal(survivor!.apiKey, "sk-existing", "existing apiKey must not be overwritten"); + assert.equal(survivor!.priority, 3, "existing priority must survive the bulk-add"); + assert.equal(survivor!.testStatus, "unavailable", "existing testStatus must survive"); + assert.equal(survivor!.lastError, "429 rate limited", "existing lastError must survive"); + assert.equal(survivor!.rateLimitedUntil, future, "existing cooldown must survive"); + assert.equal(survivor!.backoffLevel, 2, "existing backoffLevel must survive"); + + const newNames = after + .filter((c) => c.id !== (existing as ConnectionRow).id) + .map((c) => c.name); + assert.equal(new Set(newNames).size, newNames.length, "no duplicate names among new entries"); + assert.ok(!newNames.includes("Key 1")); +}); + +test("without collision resolution, a colliding bulk entry silently replaces the existing connection (documents the pre-fix bug)", async () => { + // This is the exact bug reported upstream: createProviderConnection's + // name-based upsert is intentionally unchanged (other single-add/import + // flows depend on it) — the guard lives one layer up, in the bulk route. + // Skipping that guard reproduces the original data-loss behavior. + const existing = await providersDb.createProviderConnection({ + provider: "anthropic", + authType: "apikey", + name: "Key 1", + apiKey: "sk-existing", + }); + + const collided = await providersDb.createProviderConnection({ + provider: "anthropic", + authType: "apikey", + name: "Key 1", // same name, no collision resolution applied + apiKey: "sk-overwritten", + }); + + assert.equal(collided!.id, existing!.id, "same name upserts into the same row"); + + const rows = (await providersDb.getProviderConnections({ + provider: "anthropic", + })) as ConnectionRow[]; + assert.equal(rows.length, 1, "no new connection was inserted — this is the bug"); + assert.equal(rows[0].apiKey, "sk-overwritten", "the original key was overwritten"); +}); diff --git a/tests/unit/bulkApiKeyParser.test.ts b/tests/unit/bulkApiKeyParser.test.ts index 85f6990811..11158d7dbb 100644 --- a/tests/unit/bulkApiKeyParser.test.ts +++ b/tests/unit/bulkApiKeyParser.test.ts @@ -2,6 +2,7 @@ import test from "node:test"; import assert from "node:assert/strict"; import { parseBulkApiKeys, + resolveBulkNameCollisions, BULK_API_KEY_MAX_LINES, } from "../../src/shared/utils/bulkApiKeyParser.ts"; @@ -87,3 +88,66 @@ test("input exceeding cap is truncated with warning", () => { assert.equal(warnings.length, 1); assert.match(warnings[0], /only the first/); }); + +// #2587 — resolveBulkNameCollisions: guards against createProviderConnection's +// name-based upsert silently replacing an existing connection when bulk-add +// auto-naming ("Key 1", "Key 2", ...) restarts from 1 and collides with a +// name already saved for the provider. +test("resolveBulkNameCollisions: no-op when nothing collides", () => { + const entries = [{ name: "Key 1" }, { name: "Key 2" }]; + const out = resolveBulkNameCollisions(entries, ["Prod"]); + assert.deepEqual( + out.map((e) => e.name), + ["Key 1", "Key 2"] + ); +}); + +test("resolveBulkNameCollisions: renames an auto-named entry colliding with an existing connection", () => { + // Provider already has "Key 1" saved; a fresh bulk parse restarts auto-naming + // at "Key 1" too. Without renaming, createProviderConnection would upsert + // into the existing row instead of inserting. + const entries = [{ name: "Key 1" }, { name: "Key 2" }]; + const out = resolveBulkNameCollisions(entries, ["Key 1"]); + assert.deepEqual( + out.map((e) => e.name), + ["Key 2", "Key 3"] + ); + // Never reuses an existing name. + assert.ok(!out.some((e) => e.name === "Key 1")); +}); + +test("resolveBulkNameCollisions: renames a custom name colliding with an existing connection", () => { + const entries = [{ name: "Prod" }]; + const out = resolveBulkNameCollisions(entries, ["Prod"]); + assert.deepEqual(out.map((e) => e.name), ["Prod 1"]); +}); + +test("resolveBulkNameCollisions: dedupes two identical custom names within the same batch", () => { + // Two "Prod|apiKey" lines pasted in the same batch previously collided with + // EACH OTHER (custom names get no auto-index), so the second entry would + // upsert into the first instead of inserting a second connection. + const entries = [{ name: "Prod" }, { name: "Prod" }]; + const out = resolveBulkNameCollisions(entries, []); + const names = out.map((e) => e.name); + assert.equal(new Set(names).size, names.length); + assert.deepEqual(names, ["Prod", "Prod 1"]); +}); + +test("resolveBulkNameCollisions: preserves non-name fields on renamed entries", () => { + const entries = [{ name: "Key 1", apiKey: "sk-new" }]; + const out = resolveBulkNameCollisions(entries, ["Key 1"]); + assert.equal(out[0].apiKey, "sk-new"); + assert.equal(out[0].name, "Key 2"); +}); + +test("resolveBulkNameCollisions: name comparison is case-insensitive", () => { + const entries = [{ name: "key 1" }]; + const out = resolveBulkNameCollisions(entries, ["KEY 1"]); + assert.equal(out[0].name, "key 2"); +}); + +test("resolveBulkNameCollisions: tolerates non-array existingNames", () => { + const entries = [{ name: "Key 1" }]; + const out = resolveBulkNameCollisions(entries, null); + assert.equal(out[0].name, "Key 1"); +});