fix(api): import route guards (provider,name) collisions instead of silent overwrite (#6836, #2587)

The /api/providers/import route called createProviderConnection directly, which
upserts-by-name for apikey auth — so re-imported or same-batch duplicate rows
silently overwrote existing credentials while still reporting 'created'. Mirror
the sibling /bulk route's resolveBulkNameCollisions guard.
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-07-17 19:42:25 -03:00
parent 07cde25829
commit 3e64b22e56
2 changed files with 119 additions and 3 deletions

View File

@@ -4,9 +4,15 @@ import {
getProviderAuditTarget,
summarizeProviderConnectionForAudit,
} from "@/lib/compliance/providerAudit";
import { createProviderConnection, getProviderNodeById, isCloudEnabled } from "@/models";
import {
createProviderConnection,
getProviderConnections,
getProviderNodeById,
isCloudEnabled,
} from "@/models";
import { isAnthropicCompatibleProvider, isOpenAICompatibleProvider } from "@/shared/constants/providers";
import { getConsistentMachineId } from "@/shared/utils/machineId";
import { resolveBulkNameCollisions } from "@/shared/utils/bulkApiKeyParser";
import { syncToCloud } from "@/lib/cloudSync";
import { bulkImportProviderSchema } from "@/shared/validation/schemas";
import { isValidationFailure, validateBody } from "@/shared/validation/helpers";
@@ -109,6 +115,44 @@ async function importOneEntry(
return { created: safe };
}
/**
* #2587 / #6836 — mirrors the guard in POST /api/providers/bulk: createProviderConnection
* upserts apikey connections BY NAME, so an imported row whose (provider, name) collides
* with an already-saved connection — or with an earlier row in the SAME import batch —
* would silently REPLACE that connection's apiKey/priority instead of inserting a new
* one, while the response still reported it as a fresh "created" success. Unlike /bulk
* (single provider per request), one import batch can span many DIFFERENT providers, so
* collisions are resolved per-provider: existing connection names are fetched once per
* distinct provider in the batch, then `resolveBulkNameCollisions` gap-fills a free
* "<name> <n>" suffix for every entry so each one reaches createProviderConnection as a
* genuine insert.
*/
async function resolveImportNameCollisions(entries: ImportEntry[]): Promise<ImportEntry[]> {
const indicesByProvider = new Map<string, number[]>();
entries.forEach((entry, index) => {
const indices = indicesByProvider.get(entry.provider) || [];
indices.push(index);
indicesByProvider.set(entry.provider, indices);
});
const resolved: ImportEntry[] = [...entries];
for (const [provider, indices] of indicesByProvider) {
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 providerEntries = indices.map((i) => ({ name: entries[i].name }));
const resolvedProviderEntries = resolveBulkNameCollisions(providerEntries, existingNames);
indices.forEach((originalIndex, i) => {
resolved[originalIndex] = { ...entries[originalIndex], name: resolvedProviderEntries[i].name };
});
}
return resolved;
}
async function syncToCloudIfEnabled() {
try {
const cloudEnabled = await isCloudEnabled();
@@ -143,12 +187,13 @@ export async function POST(request: Request) {
}
const { entries, validateKeys } = validation.data;
const resolvedEntries = await resolveImportNameCollisions(entries);
const created: Array<Record<string, unknown>> = [];
const errors: Array<{ index: number; name: string; provider: 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 {
const result = await importOneEntry(entry, !!validateKeys);
if ("error" in result) {

View File

@@ -119,6 +119,77 @@ test("providers import route: partial-failure — unresolvable compatible node f
assert.ok(!JSON.stringify(body).includes(" at /"));
});
test("providers import route: same-batch (provider,name) collision does not overwrite the first row (#2587-class data loss)", async () => {
await resetStorage();
const response = await postImport({
entries: [
{ provider: "openai", name: "Prod OpenAI", apiKey: "sk-openai-first" },
{ provider: "openai", name: "Prod OpenAI", apiKey: "sk-openai-second" },
],
});
assert.equal(response.status, 200);
const body = (await response.json()) as ImportRouteResponse;
assert.equal(body.total, 2);
assert.equal(body.success, 2, "both rows must be created — the second must not silently upsert into the first");
assert.equal(body.failed, 0);
assert.equal(body.created.length, 2);
const providersDb = await import("../../../src/lib/db/providers.ts");
const connections = (await providersDb.getProviderConnections({
provider: "openai",
})) as Array<{ id: string; name?: string | null; apiKey?: string }>;
assert.equal(connections.length, 2, "the collision must produce TWO distinct connections, never one");
const first = connections.find((c) => c.apiKey === "sk-openai-first");
const second = connections.find((c) => c.apiKey === "sk-openai-second");
assert.ok(first, "the first row's apiKey must survive unmodified");
assert.ok(second, "the second row must be a genuine insert, not a silent overwrite");
assert.notEqual(first!.id, second!.id, "the two rows must be distinct connections");
assert.notEqual(
first!.name,
second!.name,
"the colliding row must be disambiguated with a distinct name, not silently share the first row's name"
);
});
test("providers import route: re-importing an existing (provider,name) does not overwrite the saved connection", async () => {
await resetStorage();
const providersDb = await import("../../../src/lib/db/providers.ts");
const existing = await providersDb.createProviderConnection({
provider: "openai",
authType: "apikey",
name: "Prod OpenAI",
apiKey: "sk-existing",
priority: 3,
testStatus: "unavailable",
lastError: "429 rate limited",
});
assert.ok(existing);
const response = await postImport({
entries: [{ provider: "openai", name: "Prod OpenAI", apiKey: "sk-reimported" }],
});
assert.equal(response.status, 200);
const body = (await response.json()) as ImportRouteResponse;
assert.equal(body.success, 1);
const connections = (await providersDb.getProviderConnections({
provider: "openai",
})) as Array<{ id: string; name?: string | null; apiKey?: string; testStatus?: string; lastError?: string }>;
assert.equal(connections.length, 2, "re-import must APPEND a new connection, not replace the existing one");
const survivor = connections.find((c) => c.id === existing!.id);
assert.ok(survivor, "the pre-existing connection must still exist, unreplaced");
assert.equal(survivor!.apiKey, "sk-existing", "existing apiKey must not be overwritten by the re-import");
assert.equal(survivor!.testStatus, "unavailable", "existing testStatus must survive the re-import");
assert.equal(survivor!.lastError, "429 rate limited", "existing lastError must survive the re-import");
const imported = connections.find((c) => c.id !== existing!.id);
assert.ok(imported, "the newly imported row must exist as a distinct connection");
assert.equal(imported!.apiKey, "sk-reimported");
assert.notEqual(imported!.name, "Prod OpenAI", "the re-imported row must be disambiguated, not collide on name");
});
test("providers import route applies a per-entry baseUrl override for compatible providers", async () => {
await resetStorage();
// openai-compatible providers require a registered node; without one the row fails