diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e75389560..0d542ce63 100644 --- a/docs/BACKLOG.md +++ b/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`. diff --git a/src/lib/db/factory.ts b/src/lib/db/factory.ts index ff47db8b9..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). * @@ -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..a1299502d --- /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"; + +/** + * 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