From 4c750fd1ae7d81310860533aa04f579b802e47c7 Mon Sep 17 00:00:00 2001 From: niukanen1 <57656076+niukanen1@users.noreply.github.com> Date: Wed, 7 Oct 2026 09:11:45 +0400 Subject: [PATCH 1/4] fix(db): refuse an inline connection with no id before the cache key getOrCreateProvider computed its cache key ahead of the provider, and providerCacheKey length-frames connection.id, so an inline connection without an id crashed there with a TypeError that the routes answered as a 500 INTERNAL_ERROR. The provider's own validate() already refuses this record with DatabaseConfigError, which test-connection answered as a 400; getOrCreateProvider and acquireExecutionProfileProvider now raise the same refusal before their cache key, so no fallback key is invented for a missing id (#1539) --- src/lib/db/factory.ts | 26 ++++- tests/api/db/inline-connection-id.test.ts | 109 ++++++++++++++++++ tests/isolated/factory.test.ts | 22 ++++ .../lib/db/connection-fingerprint.test.ts | 4 +- 4 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 tests/api/db/inline-connection-id.test.ts diff --git a/src/lib/db/factory.ts b/src/lib/db/factory.ts index ff47db8b9..51857112d 100644 --- a/src/lib/db/factory.ts +++ b/src/lib/db/factory.ts @@ -109,6 +109,25 @@ const sanitize = (v: string) => v.replace(/[\r\n]/g, " ").replace(/[\x00-\x08\x0 * provider without refusing it. Exported for the tests; the published factory's three entry points * raise it (`src/exports/providers.ts`). */ +/** + * 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. + */ +export function assertConnectionIdPresent(connection: DatabaseConnection): void { + if (!connection.id) { + throw new DatabaseConfigError("Connection ID is required", connection.type); + } +} + export function assertReadOnlyHonoured(connection: DatabaseConnection): void { const readOnly: unknown = connection.readOnly; if (readOnly === undefined) return; @@ -736,7 +755,10 @@ export async function getOrCreateProvider( options: ProviderOptions = {}, execution: EditorExecutionContext = {}, ): Promise { - // 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 { // 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"); diff --git a/tests/api/db/inline-connection-id.test.ts b/tests/api/db/inline-connection-id.test.ts new file mode 100644 index 000000000..6982e5822 --- /dev/null +++ b/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/config-loader"; + +/** + * 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 = {}): Record { + 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; + +const routes: Record }> = { + 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, +): Promise<{ status: number; body: Record }> { + const response = await handler(createMockRequest("/api/db", { method: "POST", body }) as never); + return { status: response.status, body: await parseResponseJSON>(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"); + }); +}); diff --git a/tests/isolated/factory.test.ts b/tests/isolated/factory.test.ts index abcbc7955..b077c66de 100644 --- a/tests/isolated/factory.test.ts +++ b/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) ── diff --git a/tests/unit/lib/db/connection-fingerprint.test.ts b/tests/unit/lib/db/connection-fingerprint.test.ts index 780f4e080..e646f4e60 100644 --- a/tests/unit/lib/db/connection-fingerprint.test.ts +++ b/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 From ccbfc205580c56ff028a4d5f59f7fad9a2a21334 Mon Sep 17 00:00:00 2001 From: niukanen1 <57656076+niukanen1@users.noreply.github.com> Date: Wed, 7 Oct 2026 09:18:03 +0400 Subject: [PATCH 2/4] docs: re-measure the D85 auth-mock count for the new route test --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index d8704d995..c316b41b2 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -1160,7 +1160,7 @@ 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 48 hits, re-measured 2026-10-05. +`grep -rl 'mock.module("@/lib/auth"' tests/` returns exactly 49 hits, re-measured 2026-10-07. Sixteen 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 two write a shorter stub of their own, one with two keys and one with a single `getSession`. From cdb7340244fc8487faaa1e471f8d8b29ebfa06cf Mon Sep 17 00:00:00 2001 From: niukanen1 <57656076+niukanen1@users.noreply.github.com> Date: Wed, 7 Oct 2026 09:24:03 +0400 Subject: [PATCH 3/4] fix(db): keep the id refusal private to the factory module --- src/lib/db/factory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/db/factory.ts b/src/lib/db/factory.ts index 51857112d..fc5347e0a 100644 --- a/src/lib/db/factory.ts +++ b/src/lib/db/factory.ts @@ -122,7 +122,7 @@ const sanitize = (v: string) => v.replace(/[\r\n]/g, " ").replace(/[\x00-\x08\x0 * `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. */ -export function assertConnectionIdPresent(connection: DatabaseConnection): void { +function assertConnectionIdPresent(connection: DatabaseConnection): void { if (!connection.id) { throw new DatabaseConfigError("Connection ID is required", connection.type); } From 6d538dea9e88f7a2bad167e897c4df29450fd7a9 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 14:29:52 +0300 Subject: [PATCH 4/4] fix(db): keep assertReadOnlyHonoured's docblock on it, and import the seed reset from its new home The id guard was inserted between assertReadOnlyHonoured and its docblock, so the docblock attached to the new function. The route test imported @/lib/seed/config-loader, which #1570 removed on main; it now imports resetCache from @/lib/seed, as the other route tests do. --- src/lib/db/factory.ts | 38 +++++++++++------------ tests/api/db/inline-connection-id.test.ts | 2 +- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/lib/db/factory.ts b/src/lib/db/factory.ts index fc5347e0a..4c673f583 100644 --- a/src/lib/db/factory.ts +++ b/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). * @@ -109,25 +128,6 @@ const sanitize = (v: string) => v.replace(/[\r\n]/g, " ").replace(/[\x00-\x08\x0 * provider without refusing it. Exported for the tests; the published factory's three entry points * raise it (`src/exports/providers.ts`). */ -/** - * 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); - } -} - export function assertReadOnlyHonoured(connection: DatabaseConnection): void { const readOnly: unknown = connection.readOnly; if (readOnly === undefined) return; diff --git a/tests/api/db/inline-connection-id.test.ts b/tests/api/db/inline-connection-id.test.ts index 6982e5822..a1299502d 100644 --- a/tests/api/db/inline-connection-id.test.ts +++ b/tests/api/db/inline-connection-id.test.ts @@ -4,7 +4,7 @@ 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/config-loader"; +import { resetCache as resetSeedCache } from "@/lib/seed"; /** * An inline connection without an id is answered 400 CONFIG_ERROR, "Connection ID is required", by