fix(db): refuse an inline connection with no id before the cache key

docs/BACKLOG.md

@@ -1160,8 +1160,8 @@ test pins the behaviour that was chosen.
 
 ### D85. The `@/lib/auth` mock is hand-copied across a layer, untyped, and already misses four exports
 
-`grep -rl 'mock.module("@/lib/auth"' tests/` returns exactly 50 hits, re-measured 2026-10-07.
-Seventeen of them spread the real module and replace one function (`{ ...realAuth, getSession: mockGetSession }`, the agent routes' pattern).
+`grep -rl 'mock.module("@/lib/auth"' tests/` returns exactly 51 hits, re-measured 2026-10-07.
+Eighteen of them spread the real module and replace one function (`{ ...realAuth, getSession: mockGetSession }`, the agent routes' pattern).
 Thirty write out the same five-key object - `getSession`, `signJWT`, `verifyJWT`, `login`, `logout` - down to the same `mock(async () => "mock-token")` for a token nothing reads, and one of those thirty is `tests/helpers/object-edit-route-harness.ts`, a shared harness that could have been the factory and copied the stub instead.
 The remaining three write a shorter stub of their own, two with two keys and one with a single `getSession`.
 

src/lib/db/factory.ts

@@ -87,6 +87,25 @@ import * as path from "path";
  */
 const sanitize = (v: string) => v.replace(/[\r\n]/g, " ").replace(/[\x00-\x08\x0b\x0c\x0e-\x1f]/g, "");
 
+/**
+ * Refuse a connection with no id, before the cache key is computed (#1539).
+ *
+ * The provider's own `validate()` raises `DatabaseConfigError("Connection ID is required")` for this
+ * record, but `getOrCreateProvider` and `acquireExecutionProfileProvider` compute their cache key
+ * first, and `providerCacheKey` length-frames `connection.id`: a missing id crashed there with a
+ * `TypeError`, which `createErrorResponse` answered as a 500 `INTERNAL_ERROR` on every route that
+ * opens a cached provider (query, health and multi-query were measured). The same record reaching the
+ * provider through `createDatabaseProvider`, as `POST /api/db/test-connection` builds it, was already
+ * refused with a 400 `CONFIG_ERROR`. The refusal is raised here instead, as the same
+ * `DatabaseConfigError` the provider would have raised, so the key never sees a record the provider
+ * would refuse, and no fallback key is invented for a missing id.
+ */
+function assertConnectionIdPresent(connection: DatabaseConnection): void {
+  if (!connection.id) {
+    throw new DatabaseConfigError("Connection ID is required", connection.type);
+  }
+}
+
 /**
  * Refuse a `readOnly` this connection's engine cannot keep, before anything is built or dialled (#1089).
  *
@@ -736,7 +755,10 @@ export async function getOrCreateProvider(
   options: ProviderOptions = {},
   execution: EditorExecutionContext = {},
 ): Promise<DatabaseProvider> {
-  // First, ahead of the cache lookup and of any tunnel (#1089): see assertReadOnlyHonoured.
+  // First, ahead of the cache lookup and of any tunnel (#1089): see assertReadOnlyHonoured. The id
+  // refusal shares the place: the cache key length-frames connection.id, so a missing id must be
+  // refused before the key is computed, as the DatabaseConfigError the provider would have raised (#1539).
+  assertConnectionIdPresent(connection);
   assertReadOnlyHonoured(connection);
   // The writable cache never holds a read-only handle, and its key frames no such mode for an
   // execution context: a readOnly reaching the provider from here would be served to every later
@@ -985,6 +1007,8 @@ export async function acquireExecutionProfileProvider(
   requester: EditorExecutionContext = {},
 ): Promise<DatabaseProvider> {
   // First, ahead of the profiled cache lookup and of any tunnel (#1089): see assertReadOnlyHonoured.
+  // The id refusal shares the place for the same reason: the profiled key frames connection.id (#1539).
+  assertConnectionIdPresent(connection);
   assertReadOnlyHonoured(connection);
   if (!EXECUTION_PROFILES.has(profile)) {
     throw new ExecutionProfileError(`Unknown execution profile: ${String(profile)}`, "UNSUPPORTED_PROFILE");

tests/api/db/inline-connection-id.test.ts

@@ -0,0 +1,109 @@
+import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test";
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { createMockRequest, parseResponseJSON } from "../../helpers/mock-next";
+import { clearRateLimitState } from "@/lib/api/rate-limit";
+import { resetCache as resetSeedCache } from "@/lib/seed";
+
+/**
+ * An inline connection without an id is answered 400 CONFIG_ERROR, "Connection ID is required", by
+ * every route that opens a cached provider (#1539).
+ *
+ * The UI always sends an id, so only a direct API caller reaches this. Before the change the routes
+ * that open their provider through `getOrCreateProvider` crashed inside `providerCacheKey`, which
+ * length-frames `connection.id`: a missing id threw a `TypeError` there, which `createErrorResponse`
+ * answered as a 500 `INTERNAL_ERROR`. `POST /api/db/test-connection`, which builds its provider
+ * through `createDatabaseProvider`, was already refused by the provider's own `validate()` with the
+ * 400 this change brings to the rest.
+ *
+ * Unlike the other api/db tests, this file does NOT mock `@/lib/db`: the refusal is raised by the
+ * real factory ahead of its cache key, and only `getSession` is mocked, to answer as an admin. The
+ * session, `resolveConnection` and the factory are the real ones, per the acceptance criteria; the
+ * connection is an inline PostgreSQL record whose refusal happens before any socket is opened.
+ */
+
+const realAuth = await import("@/lib/auth");
+mock.module("@/lib/auth", () => ({
+  ...realAuth,
+  getSession: mock(async () => ({ role: "admin", username: "admin" })),
+}));
+
+const { POST: queryPost } = await import("@/app/api/db/query/route");
+const { POST: healthPost } = await import("@/app/api/db/health/route");
+const { POST: multiQueryPost } = await import("@/app/api/db/multi-query/route");
+const { POST: testConnectionPost } = await import("@/app/api/db/test-connection/route");
+const { clearProviderCache, getProviderCacheStats } = await import("@/lib/db/factory");
+
+const workDir = mkdtempSync(join(tmpdir(), "libredb-inline-id-"));
+const previousSeedConfigPath = process.env.SEED_CONFIG_PATH;
+// No seed file is read on these paths, but a stray SEED_CONFIG_PATH from another test file must not
+// leak into this one either; the loader is reset below per test.
+if (previousSeedConfigPath === undefined) delete process.env.SEED_CONFIG_PATH;
+
+/** An inline PostgreSQL connection with no id: the record every route here is refused for. */
+function bodyWithoutId(extra: Record<string, unknown> = {}): Record<string, unknown> {
+  return {
+    connection: {
+      name: "probe",
+      type: "postgres",
+      host: "127.0.0.1",
+      port: 1,
+      user: "u",
+      password: "p",
+      database: "d",
+    },
+    ...extra,
+  };
+}
+
+type Handler = (request: never) => Promise<Response>;
+
+const routes: Record<string, { handler: Handler; extra: Record<string, unknown> }> = {
+  query: { handler: queryPost as unknown as Handler, extra: { sql: "SELECT 1" } },
+  health: { handler: healthPost as unknown as Handler, extra: {} },
+  "multi-query": { handler: multiQueryPost as unknown as Handler, extra: { sql: "SELECT 1" } },
+};
+
+async function post(
+  handler: Handler,
+  body: Record<string, unknown>,
+): Promise<{ status: number; body: Record<string, unknown> }> {
+  const response = await handler(createMockRequest("/api/db", { method: "POST", body }) as never);
+  return { status: response.status, body: await parseResponseJSON<Record<string, unknown>>(response) };
+}
+
+beforeEach(() => {
+  clearRateLimitState();
+  resetSeedCache();
+});
+
+afterAll(() => {
+  if (previousSeedConfigPath !== undefined) process.env.SEED_CONFIG_PATH = previousSeedConfigPath;
+  resetSeedCache();
+  rmSync(workDir, { recursive: true, force: true });
+});
+
+describe("an inline connection without an id is refused with 400 CONFIG_ERROR (#1539)", () => {
+  for (const [name, { handler, extra }] of Object.entries(routes)) {
+    test(`${name}: answers 400 CONFIG_ERROR, "Connection ID is required"`, async () => {
+      const { status, body } = await post(handler, bodyWithoutId(extra));
+      expect(status).toBe(400);
+      expect(body.error).toBe("Connection ID is required");
+      expect(body.code).toBe("CONFIG_ERROR");
+    });
+
+    test(`${name}: opens no provider and caches nothing`, async () => {
+      await clearProviderCache();
+      await post(handler, bodyWithoutId(extra));
+      expect(getProviderCacheStats().size).toBe(0);
+    });
+  }
+
+  test("test-connection keeps the same 400 it already answered", async () => {
+    const { status, body } = await post(testConnectionPost as unknown as Handler, bodyWithoutId());
+    expect(status).toBe(400);
+    expect(body.error).toBe("Connection ID is required");
+    expect(body.code).toBe("CONFIG_ERROR");
+  });
+});

tests/isolated/factory.test.ts

@@ -1046,6 +1046,28 @@ describe("getOrCreateProvider", () => {
     expect(tunnel?.close).not.toHaveBeenCalled();
     expect(getProviderCacheStats().size).toBe(0);
   });
+
+  test("an inline connection without an id is refused before the cache key is computed (#1539)", async () => {
+    // The provider's own validate() refuses this record with DatabaseConfigError("Connection ID is
+    // required"), but the cache key is computed ahead of the provider: providerCacheKey length-frames
+    // connection.id, and a missing id crashed there with a TypeError, which createErrorResponse turned
+    // into a 500 INTERNAL_ERROR on query, health and multi-query. The refusal now happens first, as the
+    // same DatabaseConfigError the provider would have raised, so no cache key and no fallback key for a
+    // missing id.
+    const conn = makeConnection("sqlite", { id: undefined as unknown as string, database: ":memory:" });
+    await expect(getOrCreateProvider(conn)).rejects.toThrow("Connection ID is required");
+    await expect(getOrCreateProvider(conn)).rejects.toBeInstanceOf(DatabaseConfigError);
+    expect(getProviderCacheStats().size).toBe(0);
+  });
+
+  test("an execution-profile acquisition without an id is refused the same way (#1539)", async () => {
+    const conn = makeConnection("sqlite", { id: undefined as unknown as string, database: ":memory:" });
+    await expect(acquireExecutionProfileProvider(conn, "agent-operations")).rejects.toThrow(
+      "Connection ID is required",
+    );
+    await expect(acquireExecutionProfileProvider(conn, "agent-operations")).rejects.toBeInstanceOf(DatabaseConfigError);
+    expect(getExecutionProfileCacheStats().size).toBe(0);
+  });
 });
 
 // ─── The cache key may not be a string the caller typed (GHSA-3wh2-8x78-jfw4) ──

tests/unit/lib/db/connection-fingerprint.test.ts

@@ -69,7 +69,7 @@ describe("connectionFingerprint", () => {
     expect(await connectionFingerprint(vary({ serviceName: "XEPDB1" }))).not.toBe(base);
     expect(await connectionFingerprint(vary({ instanceName: "SQLEXPRESS" }))).not.toBe(base);
     // The tenth, which the four above were audited without and which a review of THAT audit found
-    // one field away: the bastion is the ROUTE, and `factory.ts:823-830` rewrites `host` and `port`
+    // one field away: the bastion is the ROUTE, and `factory.ts:845-852` rewrites `host` and `port`
     // to the tunnel's local endpoint before the provider is constructed, so the tunnel and not the
     // record decides which machine the sealed statement reaches.
     expect(await connectionFingerprint(vary({ sshTunnel: BASTION }))).not.toBe(base);
@@ -95,7 +95,7 @@ describe("connectionFingerprint", () => {
     expect(ours).not.toBe(await connectionFingerprint(vary({ sshTunnel: { ...BASTION, port: 2222 } })));
     expect(ours).not.toBe(await connectionFingerprint(vary({ sshTunnel: { ...BASTION, username: "mallory" } })));
     // A DISABLED tunnel is not the same route as an enabled one to the same bastion, because
-    // `factory.ts:823` branches on exactly that flag and only the enabled arm rewrites the endpoint.
+    // `factory.ts:845` branches on exactly that flag and only the enabled arm rewrites the endpoint.
     expect(ours).not.toBe(await connectionFingerprint(vary({ sshTunnel: { ...BASTION, enabled: false } })));
     // And the tunnel's SECRETS are out, on the rule the database password already follows: rotating
     // a key changes who may reach the bastion, never which machine it is. `hostKeyFingerprint` is