From 320c200934e749a1a0ee085e492e42adf88a5d7b Mon Sep 17 00:00:00 2001 From: ditadi Date: Sat, 26 Sep 2026 15:29:40 +0100 Subject: [PATCH 1/4] fix(appkit): log Postgres text for schema and connection failures A driver failure reached the server log as a bare SQLSTATE, so a missing column logged only "SQLSTATE 42703" with no object named. Log the driver's own message, detail, and hint for SQLSTATE classes whose text names connections, credentials, or schema objects (08, 28, 3D, 3F, 42, 53, 57), and the text of Node system errors such as ENOTFOUND. Value-bearing classes (22 data, 23 constraints, P0 PL/pgSQL) still log only their code, the Drizzle wrapper's SQL and params are never read, and the thrown error and client message are unchanged. Co-authored-by: Isaac Signed-off-by: ditadi --- .../runtime/engine/drizzle-data-path.ts | 71 ++++++++-- .../runtime/tests/drizzle-data-path.test.ts | 125 ++++++++++++++++++ 2 files changed, 188 insertions(+), 8 deletions(-) diff --git a/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts b/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts index b89bcc208..b1cc16a50 100644 --- a/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts +++ b/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts @@ -159,27 +159,81 @@ function upsertUpdateValues( // nested `cause` rather than the thrown error. Walk a bounded chain to find it. const MAX_CAUSE_DEPTH = 5; -function sqlStateOf(error: unknown): string | undefined { +// These SQLSTATE classes describe the connection, the credentials, or schema +// objects, so the server's text names identifiers rather than row values: +// 08 connection, 28 authorization, 3D catalog, 3F schema, 42 undefined objects +// and privileges, 53 resources, 57 operator intervention. Data (22), +// constraint (23), and PL/pgSQL (P0) text can echo values and is never logged. +const DESCRIBED_SQLSTATE_CLASSES = new Set([ + "08", + "28", + "3D", + "3F", + "42", + "53", + "57", +]); +const MAX_DIAGNOSTIC_LENGTH = 500; + +interface DriverFailure { + readonly sqlState?: string; + /** Server-log-only driver text; it never reaches the thrown error. */ + readonly diagnostic?: string; +} + +/** Read the driver's own message, detail, and hint, never a wrapper's. */ +function driverText(carrier: object): string | undefined { + try { + const parts = ["message", "detail", "hint"] + .map((key) => Reflect.get(carrier, key)) + .filter( + (part): part is string => typeof part === "string" && part !== "", + ); + return parts.length > 0 + ? parts.join(" ").slice(0, MAX_DIAGNOSTIC_LENGTH) + : undefined; + } catch { + return undefined; + } +} + +function driverFailureOf(error: unknown): DriverFailure { let current = error; for (let depth = 0; depth < MAX_CAUSE_DEPTH; depth++) { - if (!current || typeof current !== "object") return undefined; + if (!current || typeof current !== "object") return {}; try { const candidate = Reflect.get(current, "code"); + // Node system errors (ECONNREFUSED, ENOTFOUND) name the host, not data. + if ( + typeof candidate === "string" && + /^E[A-Z]+$/.test(candidate) && + typeof Reflect.get(current, "syscall") === "string" + ) { + return { diagnostic: driverText(current) }; + } // SQLSTATE is always a five-character alphanumeric class code. if (typeof candidate === "string" && /^[0-9A-Z]{5}$/.test(candidate)) { - return candidate; + return { + sqlState: candidate, + diagnostic: DESCRIBED_SQLSTATE_CLASSES.has(candidate.slice(0, 2)) + ? driverText(current) + : undefined, + }; } current = Reflect.get(current, "cause"); } catch { - return undefined; + return {}; } } - return undefined; + return {}; } -/** Classify SQLSTATE without retaining the driver error or its properties. */ +/** + * Classify SQLSTATE without retaining the driver error or its properties. + * Described classes add the driver's text to the server log only. + */ function classifyDriverError(error: unknown): DatabasePluginError { - const code = sqlStateOf(error); + const { sqlState: code, diagnostic } = driverFailureOf(error); const category: DatabaseErrorCategory = code === "40001" || code === "40P01" || code === "57014" ? "TRANSIENT" @@ -189,9 +243,10 @@ function classifyDriverError(error: unknown): DatabasePluginError { ? "CONFLICT" : "INTERNAL"; logger.error( - "Database driver error classified as %s (SQLSTATE %s)", + "Database driver error classified as %s (SQLSTATE %s)%s", category, code ?? "unknown", + diagnostic ? `: ${diagnostic}` : "", ); return new DatabasePluginError(category, "runtime"); } diff --git a/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts b/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts index fbf223733..040aa8fc2 100644 --- a/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts +++ b/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts @@ -815,6 +815,131 @@ describe("database failures", () => { } }); + // Schema drift used to surface as a bare SQLSTATE, with no object named. + it.each([ + [ + "42703", + { + message: "column notes.board_id does not exist", + hint: 'Perhaps you meant to reference the column "notes.body".', + }, + ["column notes.board_id does not exist", '"notes.body"'], + ], + [ + "42P01", + { message: 'relation "public.boards" does not exist' }, + ['relation "public.boards" does not exist'], + ], + [ + "28000", + { + message: "External authorization failed.", + detail: "This could be due to paused instances.", + }, + ["External authorization failed.", "paused instances"], + ], + ] as const)( + "logs the driver's own text for described SQLSTATE %s, never the wrapper's", + async (code, fields, expected) => { + const fake = makeFakeDb(); + const query = fake.db.query as unknown as Record< + string, + { findMany: () => Promise } + >; + query.users.findMany = async () => { + const driver = Object.assign(new Error(fields.message), { + code, + ...fields, + }); + throw Object.assign( + new Error("Failed query: select * from users\nparams: alice@x.com"), + { cause: driver }, + ); + }; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + const error = await createDrizzleDataPath(fake.db, schema) + .select(users, {}) + .catch((caught) => caught); + + const output = errorLog.mock.calls.flat().map(String).join(" "); + for (const text of expected) expect(output).toContain(text); + expect(output).not.toContain("Failed query"); + expect(output).not.toContain("alice@x.com"); + // The diagnostic stays in the log; callers still get the stable error. + expect(error.message).toBe("Database operation failed"); + expect(JSON.stringify(error)).not.toContain(fields.message); + } finally { + errorLog.mockRestore(); + } + }, + ); + + it.each([ + ["22P02", 'invalid input syntax for type integer: "alice@x.com"'], + ["23502", "null value in column alice@x.com"], + ["P0001", "raised for alice@x.com"], + ])( + "never logs driver text for value-bearing SQLSTATE %s", + async (code, message) => { + const fake = makeFakeDb(); + const query = fake.db.query as unknown as Record< + string, + { findMany: () => Promise } + >; + query.users.findMany = async () => { + throw { code, message, detail: "Key (email)=(alice@x.com)" }; + }; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + await createDrizzleDataPath(fake.db, schema) + .select(users, {}) + .catch(() => undefined); + + const output = errorLog.mock.calls.flat().map(String).join(" "); + expect(output).toContain(code); + expect(output).not.toContain("alice@x.com"); + } finally { + errorLog.mockRestore(); + } + }, + ); + + it("logs a connection failure's system error text", async () => { + const fake = makeFakeDb(); + const query = fake.db.query as unknown as Record< + string, + { findMany: () => Promise } + >; + query.users.findMany = async () => { + throw Object.assign( + new Error("getaddrinfo ENOTFOUND ep-stale.database.example.com"), + { code: "ENOTFOUND", syscall: "getaddrinfo" }, + ); + }; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + const error = await createDrizzleDataPath(fake.db, schema) + .select(users, {}) + .catch((caught) => caught); + + const output = errorLog.mock.calls.flat().map(String).join(" "); + expect(output).toContain("ENOTFOUND ep-stale.database.example.com"); + expect(error).toMatchObject({ category: "INTERNAL" }); + } finally { + errorLog.mockRestore(); + } + }); + it("stops walking an error cause cycle", async () => { const fake = makeFakeDb(); const query = fake.db.query as unknown as Record< From d313b64e32d90fb7dedf8990e6075a5938915261 Mon Sep 17 00:00:00 2001 From: ditadi Date: Sat, 26 Sep 2026 15:29:54 +0100 Subject: [PATCH 2/4] feat(appkit): verify declared tables and columns at DatabasePlugin setup The plugin never migrates, so a database that did not match the declared schema passed setup and then failed every request with an opaque "Database operation failed". Setup now reads pg_catalog after the connectivity check and fails with the missing names, for example "table public.notes is missing columns board_id, author_email, body". pg_catalog is used because information_schema hides objects the role cannot access, which would report a privilege gap as a missing column. A setup failure now keeps its own reason when it propagates; other setup errors are still replaced so connector details stay out of it. The mvp integration test stubs the catalog check. Co-authored-by: Isaac Signed-off-by: ditadi --- .../client/src/routes/database.route.tsx | 2 +- docs/docs/plugins/database.md | 11 +- .../appkit/src/plugins/database/lifecycle.ts | 15 ++- .../src/plugins/database/schema-check.ts | 65 +++++++++++ .../database/tests/crud.integration.test.ts | 4 + .../plugins/database/tests/lifecycle.test.ts | 28 +++++ .../database/tests/mvp.integration.test.ts | 4 + .../database/tests/schema-check.test.ts | 106 ++++++++++++++++++ 8 files changed, 231 insertions(+), 4 deletions(-) create mode 100644 packages/appkit/src/plugins/database/schema-check.ts create mode 100644 packages/appkit/src/plugins/database/tests/schema-check.test.ts diff --git a/apps/dev-playground/client/src/routes/database.route.tsx b/apps/dev-playground/client/src/routes/database.route.tsx index 9b041cef6..daa3e1251 100644 --- a/apps/dev-playground/client/src/routes/database.route.tsx +++ b/apps/dev-playground/client/src/routes/database.route.tsx @@ -293,7 +293,7 @@ function DatabaseRoute() {
diff --git a/docs/docs/plugins/database.md b/docs/docs/plugins/database.md index dd0b9db74..0d88e83b4 100644 --- a/docs/docs/plugins/database.md +++ b/docs/docs/plugins/database.md @@ -32,7 +32,16 @@ server routes. App admission alone does not provide row-level isolation. Configure a Lakebase `postgres` resource and its connection environment variables as described in [Lakebase configuration](./lakebase.md#environment-variables). The database tables must already exist and match the declared schema. This plugin -checks connectivity during setup; it does not create or migrate tables. +does not create or migrate tables. During setup it checks connectivity and that +every declared table and column exists, and it fails with the missing names +(for example `table public.notes is missing columns board_id, body`) instead of +publishing routes that would fail on every request. + +When a query fails at runtime, the client receives only a stable message such as +`Database operation failed`. The server log adds the Postgres text for errors +that name connections, credentials, or schema objects (for example +`column notes.board_id does not exist`), and never for errors that can echo row +values. Apps scaffolded with the Database plugin selected include an empty `config/database/schema.ts`, so `database()` can start without requiring sample diff --git a/packages/appkit/src/plugins/database/lifecycle.ts b/packages/appkit/src/plugins/database/lifecycle.ts index 60fa80066..38347c54a 100644 --- a/packages/appkit/src/plugins/database/lifecycle.ts +++ b/packages/appkit/src/plugins/database/lifecycle.ts @@ -23,6 +23,7 @@ import type { SqlTag, TransactionClient, } from "./entity-types"; +import { assertSchemaMatchesDatabase } from "./schema-check"; import { createMutationScope, type MutationScope } from "./scope"; import type { DatabaseHooks } from "./types"; @@ -161,7 +162,10 @@ function buildDatabaseExports(context: ExportContext): DatabaseExports { return result; } -/** Validate the schema, create one pool-backed API, and verify connectivity. */ +/** + * Validate the schema, create one pool-backed API, and verify connectivity and + * that every declared table and column exists. + */ export async function createDatabaseState( schema: TSchema, execute: EntityExecute, @@ -200,6 +204,7 @@ export async function createDatabaseState( }); // Do not publish exports until an authenticated statement succeeds. await dataPath.raw`select 1`; + await assertSchemaMatchesDatabase(dataPath, schema); return { pool, exports, @@ -208,9 +213,15 @@ export async function createDatabaseState( }, }; } catch (error) { - logger.error("Database setup failed: %O", error); active = false; await pool?.end().catch(() => undefined); + // A setup failure names its own reason from schema metadata; anything + // else may carry connector or driver details and is replaced. + if (error instanceof DatabasePluginError && error.phase === "setup") { + logger.error("%s", error.message); + throw error; + } + logger.error("Database setup failed: %O", error); throw new DatabasePluginError("SETUP_FAILED", "setup"); } } diff --git a/packages/appkit/src/plugins/database/schema-check.ts b/packages/appkit/src/plugins/database/schema-check.ts new file mode 100644 index 000000000..9f012a4f0 --- /dev/null +++ b/packages/appkit/src/plugins/database/schema-check.ts @@ -0,0 +1,65 @@ +import { databaseSetupFailed } from "../../database/errors"; +import type { DataPath } from "../../database/runtime"; +import type { Schema } from "../../database/schema-builder"; + +interface CatalogColumn { + readonly table_name: string; + readonly column_name: string | null; +} + +/** + * Confirm every declared table and column exists before routes are published. + * The plugin never migrates, so drift would otherwise surface on each request + * as an undefined-table or undefined-column failure. pg_catalog is read rather + * than information_schema, which hides objects the role cannot access and + * would report a privilege gap as a missing column. + */ +export async function assertSchemaMatchesDatabase( + dataPath: DataPath, + schema: Schema, +): Promise { + const tables = Object.values(schema.$tables); + if (tables.length === 0) return; + const schemaName = schema.$schemaName; + const tableNames = tables.map((table) => table.$name); + // Tables, partitioned tables, views, materialized views, and foreign tables. + const rows = await dataPath.raw` + select c.relname::text as table_name, a.attname::text as column_name + from pg_catalog.pg_class c + join pg_catalog.pg_namespace n on n.oid = c.relnamespace + left join pg_catalog.pg_attribute a + on a.attrelid = c.oid and a.attnum > 0 and not a.attisdropped + where n.nspname::text = ${schemaName} + and c.relname::text = any(${tableNames}::text[]) + and c.relkind in ('r', 'p', 'v', 'm', 'f')`; + + const found = new Map>(); + for (const row of rows) { + const columns = found.get(row.table_name) ?? new Set(); + if (row.column_name) columns.add(row.column_name); + found.set(row.table_name, columns); + } + + const problems: string[] = []; + for (const table of tables) { + const qualified = `${schemaName}.${table.$name}`; + const columns = found.get(table.$name); + if (!columns) { + problems.push(`table ${qualified} does not exist`); + continue; + } + const missing = Object.values(table.$columns) + .map((meta) => meta.columnName) + .filter((name) => !columns.has(name)); + if (missing.length > 0) { + problems.push( + `table ${qualified} is missing ${missing.length === 1 ? "column" : "columns"} ${missing.join(", ")}`, + ); + } + } + if (problems.length > 0) { + throw databaseSetupFailed( + `the declared schema does not match the database: ${problems.join("; ")}. Create or migrate these before starting the app; the plugin does not.`, + ); + } +} diff --git a/packages/appkit/src/plugins/database/tests/crud.integration.test.ts b/packages/appkit/src/plugins/database/tests/crud.integration.test.ts index 7a2b6fe6f..6088a4315 100644 --- a/packages/appkit/src/plugins/database/tests/crud.integration.test.ts +++ b/packages/appkit/src/plugins/database/tests/crud.integration.test.ts @@ -20,6 +20,10 @@ vi.mock("../../../database/runtime/engine/drizzle-data-path", () => ({ createDrizzleDb: mocks.createDrizzleDb, createDrizzleDataPath: mocks.createDrizzleDataPath, })); +// The catalog comparison has its own suite; this fake answers no catalog. +vi.mock("../schema-check", () => ({ + assertSchemaMatchesDatabase: async () => undefined, +})); import { DatabasePlugin } from "../database"; diff --git a/packages/appkit/src/plugins/database/tests/lifecycle.test.ts b/packages/appkit/src/plugins/database/tests/lifecycle.test.ts index 20bbb04de..e70d822b5 100644 --- a/packages/appkit/src/plugins/database/tests/lifecycle.test.ts +++ b/packages/appkit/src/plugins/database/tests/lifecycle.test.ts @@ -8,6 +8,7 @@ const mocks = vi.hoisted(() => ({ initializeLakebasePool: vi.fn(), createDrizzleDb: vi.fn(), createDrizzleDataPath: vi.fn(), + assertSchemaMatchesDatabase: vi.fn(), })); vi.mock("../../../connectors/lakebase", () => ({ initializeLakebasePool: mocks.initializeLakebasePool, @@ -16,6 +17,10 @@ vi.mock("../../../database/runtime/engine/drizzle-data-path", () => ({ createDrizzleDb: mocks.createDrizzleDb, createDrizzleDataPath: mocks.createDrizzleDataPath, })); +// The catalog comparison has its own suite; these fakes answer no catalog. +vi.mock("../schema-check", () => ({ + assertSchemaMatchesDatabase: mocks.assertSchemaMatchesDatabase, +})); import { IDLE_IN_TRANSACTION_TIMEOUT_MS, @@ -182,6 +187,29 @@ describe("createDatabaseState", () => { }, ); + test("checks the catalog after readiness and keeps a drift failure's reason", async () => { + const { pool, path, execute } = arrange(); + const drift = new DatabasePluginError( + "SETUP_FAILED", + "setup", + "Database setup failed: table public.tags does not exist", + ); + mocks.assertSchemaMatchesDatabase.mockRejectedValueOnce(drift); + + const error = await createDatabaseState(schema, execute).catch( + (caught) => caught, + ); + + expect(path.raw).toHaveBeenCalledTimes(1); + expect(mocks.assertSchemaMatchesDatabase).toHaveBeenCalledWith( + path, + schema, + ); + expect(error).toBe(drift); + expect(error.message).toContain("table public.tags does not exist"); + expect(pool.end).toHaveBeenCalledTimes(1); + }); + test("sanitizes connector initialization failures", async () => { const { execute } = arrange(); mocks.initializeLakebasePool.mockRejectedValueOnce( diff --git a/packages/appkit/src/plugins/database/tests/mvp.integration.test.ts b/packages/appkit/src/plugins/database/tests/mvp.integration.test.ts index 7aab7a0ed..514f60bc0 100644 --- a/packages/appkit/src/plugins/database/tests/mvp.integration.test.ts +++ b/packages/appkit/src/plugins/database/tests/mvp.integration.test.ts @@ -22,6 +22,10 @@ vi.mock("../../../database/runtime/engine/drizzle-data-path", () => ({ createDrizzleDb: mocks.createDrizzleDb, createDrizzleDataPath: mocks.createDrizzleDataPath, })); +// The catalog comparison has its own suite; this fake answers no catalog. +vi.mock("../schema-check", () => ({ + assertSchemaMatchesDatabase: async () => undefined, +})); import { DatabasePlugin } from "../database"; diff --git a/packages/appkit/src/plugins/database/tests/schema-check.test.ts b/packages/appkit/src/plugins/database/tests/schema-check.test.ts new file mode 100644 index 000000000..df5a80c87 --- /dev/null +++ b/packages/appkit/src/plugins/database/tests/schema-check.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, test, vi } from "vitest"; + +import type { DataPath } from "../../../database/runtime"; +import { defineSchema, fk, id, text } from "../../../database/schema-builder"; +import { assertSchemaMatchesDatabase } from "../schema-check"; + +const schema = defineSchema(({ table }) => { + const boards = table("boards", { id: id(), title: text().notNull() }); + const notes = table("notes", { + id: id(), + board_id: fk(() => boards.id).notNull(), + author_email: text().private(), + body: text().notNull(), + }); + return { boards, notes }; +}); + +type CatalogRow = { table_name: string; column_name: string | null }; + +/** Answer the catalog query with the given tables and columns. */ +function catalog(tables: Record) { + const rows: CatalogRow[] = Object.entries(tables).flatMap( + ([name, columns]): CatalogRow[] => + columns.length === 0 + ? [{ table_name: name, column_name: null }] + : columns.map((column) => ({ table_name: name, column_name: column })), + ); + const raw = vi.fn(async () => rows); + return { raw, path: { raw } as unknown as DataPath }; +} + +describe("assertSchemaMatchesDatabase", () => { + test("accepts a database with every declared column, and extra ones", async () => { + const { path } = catalog({ + boards: ["id", "title", "archived_at"], + notes: ["id", "board_id", "author_email", "body"], + }); + await expect( + assertSchemaMatchesDatabase(path, schema), + ).resolves.toBeUndefined(); + }); + + test("names every missing table and column in one setup failure", async () => { + // The shape a same-named table from another app leaves behind. + const { path } = catalog({ notes: ["id", "case_id", "author", "content"] }); + + const error = await assertSchemaMatchesDatabase(path, schema).catch( + (caught) => caught, + ); + + expect(error).toMatchObject({ category: "SETUP_FAILED", phase: "setup" }); + expect(error.message).toContain("table public.boards does not exist"); + expect(error.message).toContain( + "table public.notes is missing columns board_id, author_email, body", + ); + // Only the stable message crosses a request boundary. + expect(error.clientMessage).toBe("Database setup failed"); + }); + + test("uses the singular for one missing column", async () => { + const { path } = catalog({ + boards: ["id", "title"], + notes: ["id", "board_id", "body"], + }); + await expect(assertSchemaMatchesDatabase(path, schema)).rejects.toThrow( + "table public.notes is missing column author_email", + ); + }); + + test("reports a table with no readable columns as missing them all", async () => { + const { path } = catalog({ + boards: [], + notes: ["id", "board_id", "author_email", "body"], + }); + await expect(assertSchemaMatchesDatabase(path, schema)).rejects.toThrow( + "table public.boards is missing columns id, title", + ); + }); + + test("passes the schema and table names as parameter values", async () => { + const scoped = defineSchema( + ({ table }) => ({ tags: table("tags", { id: id() }) }), + { schemaName: "playground" }, + ); + const { raw, path } = catalog({ tags: ["id"] }); + + await assertSchemaMatchesDatabase(path, scoped); + + expect(raw).toHaveBeenCalledTimes(1); + const [strings, ...values] = raw.mock.calls[0] as unknown as [ + TemplateStringsArray, + ...unknown[], + ]; + expect(strings.join("?")).toContain("pg_catalog.pg_attribute"); + expect(values).toEqual(["playground", ["tags"]]); + }); + + test("skips the catalog query for an empty schema", async () => { + const { raw, path } = catalog({}); + await assertSchemaMatchesDatabase( + path, + defineSchema(() => ({})), + ); + expect(raw).not.toHaveBeenCalled(); + }); +}); From dea43495dab792ef1c19be59bc51f9ae4cc2f825 Mon Sep 17 00:00:00 2001 From: ditadi Date: Sat, 26 Sep 2026 15:30:07 +0100 Subject: [PATCH 3/4] feat(appkit): warn when PGHOST is not a host of LAKEBASE_ENDPOINT Credentials are issued for LAKEBASE_ENDPOINT but the pool connects to PGHOST, so a host left over from another branch silently served that branch's tables. At startup, initializeLakebasePool and the lakebase plugin look the endpoint up and warn with both hosts when PGHOST is not among them, or when the endpoint no longer exists. The lookup runs alongside the identity lookup, is shared per endpoint and host, gives up after 3 seconds, and never fails startup. Co-authored-by: Isaac Signed-off-by: ditadi --- docs/docs/plugins/lakebase.md | 2 + .../src/connectors/lakebase/endpoint-host.ts | 111 +++++++++++ .../appkit/src/connectors/lakebase/index.ts | 7 +- .../lakebase/tests/endpoint-host.test.ts | 187 ++++++++++++++++++ .../lakebase/tests/initialize-pool.test.ts | 27 ++- .../appkit/src/plugins/lakebase/lakebase.ts | 6 +- 6 files changed, 337 insertions(+), 3 deletions(-) create mode 100644 packages/appkit/src/connectors/lakebase/endpoint-host.ts create mode 100644 packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts diff --git a/docs/docs/plugins/lakebase.md b/docs/docs/plugins/lakebase.md index cf1fa9cb2..32d1c2737 100644 --- a/docs/docs/plugins/lakebase.md +++ b/docs/docs/plugins/lakebase.md @@ -93,6 +93,8 @@ env: For local development, the `.env` file is automatically generated by `databricks apps init` with the correct values for your Lakebase project. +`PGHOST` must be a host of `LAKEBASE_ENDPOINT`. Credentials are issued for the endpoint, but the pool connects to `PGHOST`, so a host left over from another branch silently serves that branch's tables. AppKit looks the endpoint up at startup and logs a warning that names both hosts when they differ, or that the endpoint was not found. When you switch branches, update both variables; `databricks postgres list-endpoints projects/{project}/branches/{branch}` shows the endpoint name and its host. + For the full configuration reference (SSL, pool size, timeouts, logging, ORM examples), see the [`@databricks/lakebase` README](https://github.com/databricks/appkit/blob/main/packages/lakebase/README.md). ### Pool configuration diff --git a/packages/appkit/src/connectors/lakebase/endpoint-host.ts b/packages/appkit/src/connectors/lakebase/endpoint-host.ts new file mode 100644 index 000000000..f69a02532 --- /dev/null +++ b/packages/appkit/src/connectors/lakebase/endpoint-host.ts @@ -0,0 +1,111 @@ +import type { LakebasePoolConfig } from "@databricks/lakebase"; + +import { createLogger } from "../../logging/logger"; + +const logger = createLogger("connectors:lakebase"); + +/** The lookup is advisory, so it never holds startup longer than this. */ +const HOST_CHECK_TIMEOUT_MS = 3_000; +const MAX_REASON_LENGTH = 240; +const ENDPOINT_NAME = /^projects\/[^/]+\/branches\/[^/]+\/endpoints\/[^/]+$/; + +/** One lookup per endpoint and host, shared by every pool that asks. */ +const checks = new Map>(); + +type HostCheckConfig = Pick< + Partial, + "endpoint" | "host" | "workspaceClient" +>; + +/** + * Warn when PGHOST is not a host of LAKEBASE_ENDPOINT. Tokens are issued for + * the endpoint but the pool connects to the host, so a stale PGHOST quietly + * serves another branch's database and its tables. The check never fails + * startup: an endpoint that cannot be read only skips it. + */ +export function warnOnEndpointHostMismatch( + config: HostCheckConfig, +): Promise { + const endpoint = config.endpoint ?? process.env.LAKEBASE_ENDPOINT; + const host = config.host ?? process.env.PGHOST; + const client = config.workspaceClient; + if (!endpoint || !host || !client || !ENDPOINT_NAME.test(endpoint)) { + return Promise.resolve(); + } + const key = `${endpoint}\n${host.toLowerCase()}`; + let check = checks.get(key); + if (!check) { + check = compareHosts(client, endpoint, host); + checks.set(key, check); + } + return check; +} + +async function compareHosts( + client: NonNullable, + endpoint: string, + host: string, +): Promise { + let timer: ReturnType | undefined; + try { + const response = await Promise.race([ + client.apiClient.request({ + path: `/api/2.0/postgres/${endpoint}`, + method: "GET", + headers: new Headers({ Accept: "application/json" }), + raw: false, + }), + new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), HOST_CHECK_TIMEOUT_MS); + }), + ]); + const hosts = endpointHosts(response); + if (hosts.length === 0) { + logger.debug("Skipped the PGHOST check: %s listed no hosts", endpoint); + return; + } + if (hosts.includes(host.toLowerCase())) return; + logger.warn( + "PGHOST %s is not a host of LAKEBASE_ENDPOINT %s (expected %s). Credentials are issued for the endpoint, but queries run against whichever database PGHOST serves. Set PGHOST to the endpoint's host.", + host, + endpoint, + hosts.join(" or "), + ); + } catch (error) { + // SDK errors embed the whole response body; its message comes first. + const reason = ( + error instanceof Error ? error.message : String(error) + ).slice(0, MAX_REASON_LENGTH); + // An endpoint that no longer exists is itself the misconfiguration. + if ( + typeof error === "object" && + error !== null && + "statusCode" in error && + error.statusCode === 404 + ) { + logger.warn( + "LAKEBASE_ENDPOINT %s was not found (%s). Check the project, branch, and endpoint names; `databricks postgres list-endpoints projects/{project}/branches/{branch}` lists them with their hosts.", + endpoint, + reason, + ); + return; + } + logger.debug("Skipped the PGHOST check for %s: %s", endpoint, reason); + } finally { + clearTimeout(timer); + } +} + +/** Read every host the endpoint serves, read-write and read-only. */ +function endpointHosts(response: unknown): string[] { + if (!response || typeof response !== "object") return []; + const status = Reflect.get(response, "status"); + if (!status || typeof status !== "object") return []; + const hosts = Reflect.get(status, "hosts"); + if (!hosts || typeof hosts !== "object") return []; + return Object.values(hosts) + .filter( + (value): value is string => typeof value === "string" && value !== "", + ) + .map((value) => value.toLowerCase()); +} diff --git a/packages/appkit/src/connectors/lakebase/index.ts b/packages/appkit/src/connectors/lakebase/index.ts index b7bf2e1d4..c9fb2eee9 100644 --- a/packages/appkit/src/connectors/lakebase/index.ts +++ b/packages/appkit/src/connectors/lakebase/index.ts @@ -10,6 +10,7 @@ import { ServiceContext } from "../../context/service-context"; import { ConfigurationError } from "../../errors"; import { createLogger } from "../../logging/logger"; import { createWorkspaceClient } from "../../workspace-client"; +import { warnOnEndpointHostMismatch } from "./endpoint-host"; /** * Create a Lakebase pool with appkit's logger integration. @@ -47,7 +48,10 @@ export async function initializeLakebasePool( : createWorkspaceClient({ clientOptions: getClientOptions() }); resolved.workspaceClient = client.toLegacyWorkspaceClient(); } - const user = await getUsernameWithApiLookup(resolved); + const [user] = await Promise.all([ + getUsernameWithApiLookup(resolved), + warnOnEndpointHostMismatch(resolved), + ]); if (!user) { throw ConfigurationError.invalidConnection( "Lakebase", @@ -72,6 +76,7 @@ export { type RequestedResource, } from "@databricks/lakebase"; +export { warnOnEndpointHostMismatch } from "./endpoint-host"; export { createLakebasePoolManager, type LakebasePoolManager, diff --git a/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts b/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts new file mode 100644 index 000000000..5ca4062b4 --- /dev/null +++ b/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts @@ -0,0 +1,187 @@ +import type { LakebasePoolConfig } from "@databricks/lakebase"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +import { warnOnEndpointHostMismatch } from "../endpoint-host"; + +type Client = NonNullable; + +// Checks are shared per endpoint and host for the process, so every test +// names its own pair. +let sequence = 0; +function names() { + sequence += 1; + return { + endpoint: `projects/p${sequence}/branches/b/endpoints/primary`, + host: `ep-configured-${sequence}.database.example.com`, + }; +} + +function clientAnswering(response: unknown | Promise) { + const request = vi.fn(async () => response); + return { request, client: { apiClient: { request } } as unknown as Client }; +} + +const endpointWith = (hosts: Record) => ({ + name: "ignored", + status: { hosts }, +}); + +let warn: ReturnType; +const warnings = () => warn.mock.calls.flat().map(String).join(" "); + +beforeEach(() => { + vi.stubEnv("LAKEBASE_ENDPOINT", ""); + vi.stubEnv("PGHOST", ""); + warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); +}); +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + vi.unstubAllEnvs(); +}); + +describe("warnOnEndpointHostMismatch", () => { + test("warns with both hosts when PGHOST serves another endpoint", async () => { + const { endpoint, host } = names(); + const { request, client } = clientAnswering( + endpointWith({ host: "ep-expected.database.example.com" }), + ); + + await warnOnEndpointHostMismatch({ + endpoint, + host, + workspaceClient: client, + }); + + expect(request).toHaveBeenCalledWith( + expect.objectContaining({ + path: `/api/2.0/postgres/${endpoint}`, + method: "GET", + }), + ); + expect(warnings()).toContain(host); + expect(warnings()).toContain(endpoint); + expect(warnings()).toContain("ep-expected.database.example.com"); + }); + + test("reads the endpoint and host from the environment", async () => { + const { endpoint, host } = names(); + vi.stubEnv("LAKEBASE_ENDPOINT", endpoint); + vi.stubEnv("PGHOST", host); + const { request, client } = clientAnswering( + endpointWith({ host: "ep-other.database.example.com" }), + ); + + await warnOnEndpointHostMismatch({ workspaceClient: client }); + + expect(request).toHaveBeenCalledOnce(); + expect(warnings()).toContain(host); + }); + + test.each([ + ["the read-write host", (host: string) => ({ host: host.toUpperCase() })], + [ + "a read-only host", + (host: string) => ({ + host: "ep-primary.database.example.com", + read_only_host: host, + }), + ], + ])("stays quiet when PGHOST is %s", async (_label, hostsFor) => { + const { endpoint, host } = names(); + const { client } = clientAnswering(endpointWith(hostsFor(host))); + + await warnOnEndpointHostMismatch({ + endpoint, + host, + workspaceClient: client, + }); + + expect(warn).not.toHaveBeenCalled(); + }); + + test.each([ + ["no endpoint", { endpoint: undefined }], + ["no host", { host: undefined }], + ["no workspace client", { workspaceClient: undefined }], + [ + "an endpoint that is not a resource name", + { endpoint: "../../jobs/list" }, + ], + ])("sends no request with %s", async (_label, override) => { + const { request, client } = clientAnswering(endpointWith({})); + + await warnOnEndpointHostMismatch({ + ...names(), + workspaceClient: client, + ...override, + }); + + expect(request).not.toHaveBeenCalled(); + }); + + test.each([ + ["the lookup fails", () => Promise.reject(new Error("403 Forbidden"))], + ["the endpoint lists no hosts", () => endpointWith({})], + ["the response has no status", () => ({ name: "x" })], + ])("never throws or warns when %s", async (_label, answer) => { + const { endpoint, host } = names(); + const request = vi.fn(async () => answer()); + const client = { apiClient: { request } } as unknown as Client; + + await expect( + warnOnEndpointHostMismatch({ endpoint, host, workspaceClient: client }), + ).resolves.toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + }); + + test("warns when the endpoint no longer exists", async () => { + const { endpoint, host } = names(); + const request = vi.fn(async () => { + throw Object.assign(new Error("branch id not found"), { + statusCode: 404, + }); + }); + const client = { apiClient: { request } } as unknown as Client; + + await warnOnEndpointHostMismatch({ + endpoint, + host, + workspaceClient: client, + }); + + expect(warnings()).toContain(`${endpoint} was not found`); + expect(warnings()).toContain("branch id not found"); + }); + + test("gives up on a slow lookup without holding startup", async () => { + vi.useFakeTimers(); + const { endpoint, host } = names(); + const { client } = clientAnswering(new Promise(() => undefined)); + + const check = warnOnEndpointHostMismatch({ + endpoint, + host, + workspaceClient: client, + }); + await vi.advanceTimersByTimeAsync(3_000); + + await expect(check).resolves.toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + }); + + test("shares one lookup between pools for the same endpoint and host", async () => { + const { endpoint, host } = names(); + const { request, client } = clientAnswering( + endpointWith({ host: "ep-expected.database.example.com" }), + ); + + await Promise.all([ + warnOnEndpointHostMismatch({ endpoint, host, workspaceClient: client }), + warnOnEndpointHostMismatch({ endpoint, host, workspaceClient: client }), + ]); + + expect(request).toHaveBeenCalledOnce(); + expect(warn).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts b/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts index 4f16b9af9..22f3aef92 100644 --- a/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts @@ -8,9 +8,11 @@ import type { UserContext } from "../../../context/user-context"; const mocks = vi.hoisted(() => { const me = vi.fn(); - const client = { currentUser: { me } }; + const request = vi.fn(); + const client = { currentUser: { me }, apiClient: { request } }; return { me, + request, client, createPool: vi.fn(), createWorkspaceClient: vi.fn(() => ({ @@ -200,4 +202,27 @@ describe("AppKit Lakebase connector initialization", () => { }); expect(mocks.createPool).not.toHaveBeenCalled(); }); + + test("warns at startup when PGHOST is not a host of the endpoint", async () => { + vi.stubEnv( + "LAKEBASE_ENDPOINT", + "projects/p/branches/fresh/endpoints/primary", + ); + vi.stubEnv("PGHOST", "ep-stale.database.example.test"); + mocks.request.mockResolvedValue({ + status: { hosts: { host: "ep-fresh.database.example.test" } }, + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + + expect(await initializeLakebasePool()).toBe(pool); + + expect(mocks.request).toHaveBeenCalledWith( + expect.objectContaining({ + path: "/api/2.0/postgres/projects/p/branches/fresh/endpoints/primary", + }), + ); + const output = warn.mock.calls.flat().map(String).join(" "); + expect(output).toContain("ep-stale.database.example.test"); + expect(output).toContain("ep-fresh.database.example.test"); + }); }); diff --git a/packages/appkit/src/plugins/lakebase/lakebase.ts b/packages/appkit/src/plugins/lakebase/lakebase.ts index 5f351d827..4a9ba2c72 100644 --- a/packages/appkit/src/plugins/lakebase/lakebase.ts +++ b/packages/appkit/src/plugins/lakebase/lakebase.ts @@ -11,6 +11,7 @@ import { type LakebasePool, type LakebasePoolManager, RoutingPool, + warnOnEndpointHostMismatch, } from "../../connectors/lakebase"; import { getClientOptions } from "../../context/client-options"; import { getCallerContext } from "../../context/execution-context"; @@ -85,7 +86,10 @@ export class LakebasePlugin extends Plugin implements ToolProvider { clientOptions: getClientOptions(), }).toLegacyWorkspaceClient(), }; - const user = await getUsernameWithApiLookup(poolConfig); + const [user] = await Promise.all([ + getUsernameWithApiLookup(poolConfig), + warnOnEndpointHostMismatch(poolConfig), + ]); const spPool = createLakebasePool({ ...poolConfig, user }); logger.info("Lakebase SP pool initialized"); From ca2a2d0937beaa239de09fda936a91e253464c56 Mon Sep 17 00:00:00 2001 From: ditadi Date: Mon, 28 Sep 2026 10:20:53 +0100 Subject: [PATCH 4/4] fix(appkit): prevent wrong-database writes and sensitive log output Signed-off-by: ditadi --- docs/docs/plugins/database.md | 13 +- docs/docs/plugins/lakebase.md | 2 +- .../src/connectors/lakebase/endpoint-host.ts | 122 ++++++++---- .../appkit/src/connectors/lakebase/index.ts | 6 +- .../lakebase/tests/endpoint-host.test.ts | 187 +++++++++++++++--- .../lakebase/tests/initialize-pool.test.ts | 31 ++- .../runtime/engine/drizzle-data-path.ts | 66 ++++--- .../runtime/tests/drizzle-data-path.test.ts | 98 ++++++++- .../appkit/src/plugins/database/lifecycle.ts | 3 +- .../src/plugins/database/schema-check.ts | 35 +++- .../plugins/database/tests/lifecycle.test.ts | 12 ++ .../database/tests/schema-check.test.ts | 47 ++++- .../appkit/src/plugins/lakebase/lakebase.ts | 4 +- .../tests/lakebase-agent-tool.test.ts | 50 +++++ 14 files changed, 528 insertions(+), 148 deletions(-) diff --git a/docs/docs/plugins/database.md b/docs/docs/plugins/database.md index 0d88e83b4..6468b9e25 100644 --- a/docs/docs/plugins/database.md +++ b/docs/docs/plugins/database.md @@ -31,17 +31,12 @@ server routes. App admission alone does not provide row-level isolation. Configure a Lakebase `postgres` resource and its connection environment variables as described in [Lakebase configuration](./lakebase.md#environment-variables). -The database tables must already exist and match the declared schema. This plugin -does not create or migrate tables. During setup it checks connectivity and that -every declared table and column exists, and it fails with the missing names -(for example `table public.notes is missing columns board_id, body`) instead of -publishing routes that would fail on every request. When a query fails at runtime, the client receives only a stable message such as -`Database operation failed`. The server log adds the Postgres text for errors -that name connections, credentials, or schema objects (for example -`column notes.board_id does not exist`), and never for errors that can echo row -values. +`Database operation failed`. The server log records the SQLSTATE and only +identifier-only messages for known missing-column or missing-table errors +(for example `column notes.board_id does not exist`). Other driver messages, +details, and hints are omitted because they may contain row values. Apps scaffolded with the Database plugin selected include an empty `config/database/schema.ts`, so `database()` can start without requiring sample diff --git a/docs/docs/plugins/lakebase.md b/docs/docs/plugins/lakebase.md index 32d1c2737..c887073da 100644 --- a/docs/docs/plugins/lakebase.md +++ b/docs/docs/plugins/lakebase.md @@ -93,7 +93,7 @@ env: For local development, the `.env` file is automatically generated by `databricks apps init` with the correct values for your Lakebase project. -`PGHOST` must be a host of `LAKEBASE_ENDPOINT`. Credentials are issued for the endpoint, but the pool connects to `PGHOST`, so a host left over from another branch silently serves that branch's tables. AppKit looks the endpoint up at startup and logs a warning that names both hosts when they differ, or that the endpoint was not found. When you switch branches, update both variables; `databricks postgres list-endpoints projects/{project}/branches/{branch}` shows the endpoint name and its host. +For OAuth pools, `PGHOST` must be a host of `LAKEBASE_ENDPOINT`. Credentials are issued for the endpoint, but the pool connects to `PGHOST`, so a host left over from another branch can serve that branch's tables. At startup, `lakebase()` and `database()` look the endpoint up; a confirmed mismatch fails setup and names both hosts. Native password pools skip this check. If the lookup fails or takes more than three seconds, AppKit warns that it could not verify the host but does not block startup. A missing endpoint produces a warning. When you switch branches, update both variables; `databricks postgres list-endpoints projects/{project}/branches/{branch}` shows the endpoint name and its host. Direct `createLakebasePool()` calls do not perform this asynchronous check. For the full configuration reference (SSL, pool size, timeouts, logging, ORM examples), see the [`@databricks/lakebase` README](https://github.com/databricks/appkit/blob/main/packages/lakebase/README.md). diff --git a/packages/appkit/src/connectors/lakebase/endpoint-host.ts b/packages/appkit/src/connectors/lakebase/endpoint-host.ts index f69a02532..a3020901d 100644 --- a/packages/appkit/src/connectors/lakebase/endpoint-host.ts +++ b/packages/appkit/src/connectors/lakebase/endpoint-host.ts @@ -1,42 +1,59 @@ import type { LakebasePoolConfig } from "@databricks/lakebase"; +import { ConfigurationError } from "../../errors"; import { createLogger } from "../../logging/logger"; +import { contextFromAbortSignal } from "../context"; const logger = createLogger("connectors:lakebase"); -/** The lookup is advisory, so it never holds startup longer than this. */ +/** An unavailable lookup must not delay startup indefinitely. */ const HOST_CHECK_TIMEOUT_MS = 3_000; -const MAX_REASON_LENGTH = 240; -const ENDPOINT_NAME = /^projects\/[^/]+\/branches\/[^/]+\/endpoints\/[^/]+$/; +const ENDPOINT_NAME = + /^projects\/[A-Za-z0-9._-]+\/branches\/[A-Za-z0-9._-]+\/endpoints\/[A-Za-z0-9._-]+$/; +const HOST_NAME = /^[A-Za-z0-9](?:[A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$/; -/** One lookup per endpoint and host, shared by every pool that asks. */ -const checks = new Map>(); +/** Share in-flight lookups only between pools using the same workspace identity. */ +type Client = NonNullable; +const checks = new WeakMap>>(); type HostCheckConfig = Pick< Partial, - "endpoint" | "host" | "workspaceClient" + "endpoint" | "host" | "workspaceClient" | "password" >; /** - * Warn when PGHOST is not a host of LAKEBASE_ENDPOINT. Tokens are issued for - * the endpoint but the pool connects to the host, so a stale PGHOST quietly - * serves another branch's database and its tables. The check never fails - * startup: an endpoint that cannot be read only skips it. + * Refuse a confirmed OAuth host mismatch before the pool can access another branch. + * An endpoint that cannot be read is not proof of a mismatch, so it skips the + * check without blocking startup. */ -export function warnOnEndpointHostMismatch( +export function assertEndpointHostMatches( config: HostCheckConfig, ): Promise { const endpoint = config.endpoint ?? process.env.LAKEBASE_ENDPOINT; const host = config.host ?? process.env.PGHOST; const client = config.workspaceClient; - if (!endpoint || !host || !client || !ENDPOINT_NAME.test(endpoint)) { + if ( + !endpoint || + !host || + !client || + config.password !== undefined || + !ENDPOINT_NAME.test(endpoint) || + !HOST_NAME.test(host) + ) { return Promise.resolve(); } const key = `${endpoint}\n${host.toLowerCase()}`; - let check = checks.get(key); + let clientChecks = checks.get(client); + if (!clientChecks) { + clientChecks = new Map(); + checks.set(client, clientChecks); + } + let check = clientChecks.get(key); if (!check) { - check = compareHosts(client, endpoint, host); - checks.set(key, check); + check = compareHosts(client, endpoint, host).finally(() => { + clientChecks.delete(key); + }); + clientChecks.set(key, check); } return check; } @@ -47,35 +64,35 @@ async function compareHosts( host: string, ): Promise { let timer: ReturnType | undefined; + const controller = new AbortController(); + let hosts: string[]; + const warnUnverified = (reason: string) => + logger.warn( + "Could not verify PGHOST %s against LAKEBASE_ENDPOINT %s (%s). Check the endpoint host manually before using this pool.", + host, + endpoint, + reason, + ); try { const response = await Promise.race([ - client.apiClient.request({ - path: `/api/2.0/postgres/${endpoint}`, - method: "GET", - headers: new Headers({ Accept: "application/json" }), - raw: false, - }), + client.apiClient.request( + { + path: `/api/2.0/postgres/${endpoint}`, + method: "GET", + headers: new Headers({ Accept: "application/json" }), + raw: false, + }, + contextFromAbortSignal(controller.signal), + ), new Promise((resolve) => { - timer = setTimeout(() => resolve(undefined), HOST_CHECK_TIMEOUT_MS); + timer = setTimeout(() => { + controller.abort(); + resolve(undefined); + }, HOST_CHECK_TIMEOUT_MS); }), ]); - const hosts = endpointHosts(response); - if (hosts.length === 0) { - logger.debug("Skipped the PGHOST check: %s listed no hosts", endpoint); - return; - } - if (hosts.includes(host.toLowerCase())) return; - logger.warn( - "PGHOST %s is not a host of LAKEBASE_ENDPOINT %s (expected %s). Credentials are issued for the endpoint, but queries run against whichever database PGHOST serves. Set PGHOST to the endpoint's host.", - host, - endpoint, - hosts.join(" or "), - ); + hosts = endpointHosts(response); } catch (error) { - // SDK errors embed the whole response body; its message comes first. - const reason = ( - error instanceof Error ? error.message : String(error) - ).slice(0, MAX_REASON_LENGTH); // An endpoint that no longer exists is itself the misconfiguration. if ( typeof error === "object" && @@ -84,16 +101,36 @@ async function compareHosts( error.statusCode === 404 ) { logger.warn( - "LAKEBASE_ENDPOINT %s was not found (%s). Check the project, branch, and endpoint names; `databricks postgres list-endpoints projects/{project}/branches/{branch}` lists them with their hosts.", + "LAKEBASE_ENDPOINT %s was not found. Check the project, branch, and endpoint names; `databricks postgres list-endpoints projects/{project}/branches/{branch}` lists them with their hosts.", endpoint, - reason, ); return; } - logger.debug("Skipped the PGHOST check for %s: %s", endpoint, reason); + warnUnverified( + controller.signal.aborted ? "lookup timed out" : "lookup failed", + ); + return; } finally { clearTimeout(timer); } + if (hosts.length === 0) { + warnUnverified( + controller.signal.aborted + ? "lookup timed out" + : "endpoint returned no hosts", + ); + return; + } + if (hosts.includes(host.toLowerCase())) return; + logger.error( + "PGHOST %s is not a host of LAKEBASE_ENDPOINT %s (expected %s). Refusing to connect to the wrong branch.", + host, + endpoint, + hosts.join(" or "), + ); + throw new ConfigurationError( + `PGHOST ${host} is not a host of LAKEBASE_ENDPOINT ${endpoint} (expected ${hosts.join(" or ")}). Set PGHOST to a host of the configured endpoint before starting the app.`, + ); } /** Read every host the endpoint serves, read-write and read-only. */ @@ -105,7 +142,8 @@ function endpointHosts(response: unknown): string[] { if (!hosts || typeof hosts !== "object") return []; return Object.values(hosts) .filter( - (value): value is string => typeof value === "string" && value !== "", + (value): value is string => + typeof value === "string" && HOST_NAME.test(value), ) .map((value) => value.toLowerCase()); } diff --git a/packages/appkit/src/connectors/lakebase/index.ts b/packages/appkit/src/connectors/lakebase/index.ts index c9fb2eee9..08e169d14 100644 --- a/packages/appkit/src/connectors/lakebase/index.ts +++ b/packages/appkit/src/connectors/lakebase/index.ts @@ -10,7 +10,7 @@ import { ServiceContext } from "../../context/service-context"; import { ConfigurationError } from "../../errors"; import { createLogger } from "../../logging/logger"; import { createWorkspaceClient } from "../../workspace-client"; -import { warnOnEndpointHostMismatch } from "./endpoint-host"; +import { assertEndpointHostMatches } from "./endpoint-host"; /** * Create a Lakebase pool with appkit's logger integration. @@ -50,7 +50,7 @@ export async function initializeLakebasePool( } const [user] = await Promise.all([ getUsernameWithApiLookup(resolved), - warnOnEndpointHostMismatch(resolved), + assertEndpointHostMatches(resolved), ]); if (!user) { throw ConfigurationError.invalidConnection( @@ -76,7 +76,7 @@ export { type RequestedResource, } from "@databricks/lakebase"; -export { warnOnEndpointHostMismatch } from "./endpoint-host"; +export { assertEndpointHostMatches } from "./endpoint-host"; export { createLakebasePoolManager, type LakebasePoolManager, diff --git a/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts b/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts index 5ca4062b4..b66c0567d 100644 --- a/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/endpoint-host.test.ts @@ -1,12 +1,11 @@ import type { LakebasePoolConfig } from "@databricks/lakebase"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { warnOnEndpointHostMismatch } from "../endpoint-host"; +import { assertEndpointHostMatches } from "../endpoint-host"; type Client = NonNullable; -// Checks are shared per endpoint and host for the process, so every test -// names its own pair. +// Unique pairs keep concurrent checks from different tests independent. let sequence = 0; function names() { sequence += 1; @@ -40,28 +39,32 @@ afterEach(() => { vi.unstubAllEnvs(); }); -describe("warnOnEndpointHostMismatch", () => { - test("warns with both hosts when PGHOST serves another endpoint", async () => { +describe("assertEndpointHostMatches", () => { + test("rejects a confirmed mismatch with both hosts before a pool is allocated", async () => { const { endpoint, host } = names(); const { request, client } = clientAnswering( endpointWith({ host: "ep-expected.database.example.com" }), ); + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); - await warnOnEndpointHostMismatch({ - endpoint, - host, - workspaceClient: client, - }); + await expect( + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), + ).rejects.toThrow(host); expect(request).toHaveBeenCalledWith( expect.objectContaining({ path: `/api/2.0/postgres/${endpoint}`, method: "GET", }), + expect.anything(), ); - expect(warnings()).toContain(host); - expect(warnings()).toContain(endpoint); - expect(warnings()).toContain("ep-expected.database.example.com"); + expect(warn).not.toHaveBeenCalled(); + const output = errorLog.mock.calls.flat().map(String).join(" "); + expect(output).toContain(host); + expect(output).toContain(endpoint); + expect(output).toContain("ep-expected.database.example.com"); }); test("reads the endpoint and host from the environment", async () => { @@ -72,10 +75,11 @@ describe("warnOnEndpointHostMismatch", () => { endpointWith({ host: "ep-other.database.example.com" }), ); - await warnOnEndpointHostMismatch({ workspaceClient: client }); + await expect( + assertEndpointHostMatches({ workspaceClient: client }), + ).rejects.toThrow("ep-other.database.example.com"); expect(request).toHaveBeenCalledOnce(); - expect(warnings()).toContain(host); }); test.each([ @@ -91,7 +95,7 @@ describe("warnOnEndpointHostMismatch", () => { const { endpoint, host } = names(); const { client } = clientAnswering(endpointWith(hostsFor(host))); - await warnOnEndpointHostMismatch({ + await assertEndpointHostMatches({ endpoint, host, workspaceClient: client, @@ -108,10 +112,20 @@ describe("warnOnEndpointHostMismatch", () => { "an endpoint that is not a resource name", { endpoint: "../../jobs/list" }, ], + [ + "an endpoint with query characters", + { endpoint: "projects/p/branches/b/endpoints/primary?token=private" }, + ], + [ + "an endpoint with a trailing newline", + { endpoint: "projects/p/branches/b/endpoints/primary\n" }, + ], + ["a host with a newline", { host: "ep.example.com\nsecret" }], + ["a host with a trailing newline", { host: "ep.example.com\n" }], ])("sends no request with %s", async (_label, override) => { const { request, client } = clientAnswering(endpointWith({})); - await warnOnEndpointHostMismatch({ + await assertEndpointHostMatches({ ...names(), workspaceClient: client, ...override, @@ -124,15 +138,32 @@ describe("warnOnEndpointHostMismatch", () => { ["the lookup fails", () => Promise.reject(new Error("403 Forbidden"))], ["the endpoint lists no hosts", () => endpointWith({})], ["the response has no status", () => ({ name: "x" })], - ])("never throws or warns when %s", async (_label, answer) => { + ])("continues startup but warns when %s", async (_label, answer) => { const { endpoint, host } = names(); const request = vi.fn(async () => answer()); const client = { apiClient: { request } } as unknown as Client; await expect( - warnOnEndpointHostMismatch({ endpoint, host, workspaceClient: client }), + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), ).resolves.toBeUndefined(); - expect(warn).not.toHaveBeenCalled(); + expect(warnings()).toContain("Could not verify PGHOST"); + }); + + test("does not log or trust a malformed host in the endpoint response", async () => { + const { endpoint, host } = names(); + const { client } = clientAnswering( + endpointWith({ host: "ep-expected.database.example.com\nprivate row" }), + ); + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + await expect( + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), + ).resolves.toBeUndefined(); + expect(errorLog).not.toHaveBeenCalled(); + expect(warnings()).toContain("Could not verify PGHOST"); + expect(warnings()).not.toContain("private row"); }); test("warns when the endpoint no longer exists", async () => { @@ -144,14 +175,14 @@ describe("warnOnEndpointHostMismatch", () => { }); const client = { apiClient: { request } } as unknown as Client; - await warnOnEndpointHostMismatch({ + await assertEndpointHostMatches({ endpoint, host, workspaceClient: client, }); expect(warnings()).toContain(`${endpoint} was not found`); - expect(warnings()).toContain("branch id not found"); + expect(warnings()).not.toContain("branch id not found"); }); test("gives up on a slow lookup without holding startup", async () => { @@ -159,7 +190,7 @@ describe("warnOnEndpointHostMismatch", () => { const { endpoint, host } = names(); const { client } = clientAnswering(new Promise(() => undefined)); - const check = warnOnEndpointHostMismatch({ + const check = assertEndpointHostMatches({ endpoint, host, workspaceClient: client, @@ -167,7 +198,104 @@ describe("warnOnEndpointHostMismatch", () => { await vi.advanceTimersByTimeAsync(3_000); await expect(check).resolves.toBeUndefined(); - expect(warn).not.toHaveBeenCalled(); + expect(warnings()).toContain("Could not verify PGHOST"); + }); + + test("aborts the outbound SDK request when the three-second deadline expires", async () => { + vi.useFakeTimers(); + const { endpoint, host } = names(); + const request = vi.fn( + async (_request: unknown, _context?: unknown) => + new Promise(() => undefined), + ); + const client = { apiClient: { request } } as unknown as Client; + + const check = assertEndpointHostMatches({ + endpoint, + host, + workspaceClient: client, + }); + await vi.advanceTimersByTimeAsync(3_000); + await check; + + const context = request.mock.calls[0]?.[1] as + | { cancellationToken?: { isCancellationRequested: boolean } } + | undefined; + expect(context?.cancellationToken?.isCancellationRequested).toBe(true); + }); + + test("retries after an API failure instead of caching an unverified host", async () => { + const { endpoint, host } = names(); + const request = vi + .fn() + .mockRejectedValueOnce(new Error("403 Forbidden")) + .mockResolvedValueOnce( + endpointWith({ host: "ep-expected.database.example.com" }), + ); + const client = { apiClient: { request } } as unknown as Client; + + await assertEndpointHostMatches({ + endpoint, + host, + workspaceClient: client, + }); + await expect( + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), + ).rejects.toThrow("ep-expected.database.example.com"); + expect(request).toHaveBeenCalledTimes(2); + }); + + test("rechecks after a successful lookup in case the endpoint host changes", async () => { + const { endpoint, host } = names(); + const request = vi + .fn() + .mockResolvedValueOnce(endpointWith({ host })) + .mockResolvedValueOnce( + endpointWith({ host: "ep-new.database.example.com" }), + ); + const client = { apiClient: { request } } as unknown as Client; + + await assertEndpointHostMatches({ + endpoint, + host, + workspaceClient: client, + }); + await expect( + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), + ).rejects.toThrow("ep-new.database.example.com"); + expect(request).toHaveBeenCalledTimes(2); + }); + + test("does not reuse an inaccessible client's check for another identity", async () => { + const { endpoint, host } = names(); + const denied = vi.fn(async () => { + throw new Error("403 Forbidden"); + }); + const { request: allowed, client: allowedClient } = clientAnswering( + endpointWith({ host: "ep-expected.database.example.com" }), + ); + const deniedClient = { + apiClient: { request: denied }, + } as unknown as Client; + + const results = await Promise.allSettled([ + assertEndpointHostMatches({ + endpoint, + host, + workspaceClient: deniedClient, + }), + assertEndpointHostMatches({ + endpoint, + host, + workspaceClient: allowedClient, + }), + ]); + + expect(results.map(({ status }) => status)).toEqual([ + "fulfilled", + "rejected", + ]); + expect(allowed).toHaveBeenCalledOnce(); }); test("shares one lookup between pools for the same endpoint and host", async () => { @@ -176,12 +304,15 @@ describe("warnOnEndpointHostMismatch", () => { endpointWith({ host: "ep-expected.database.example.com" }), ); - await Promise.all([ - warnOnEndpointHostMismatch({ endpoint, host, workspaceClient: client }), - warnOnEndpointHostMismatch({ endpoint, host, workspaceClient: client }), + const results = await Promise.allSettled([ + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), + assertEndpointHostMatches({ endpoint, host, workspaceClient: client }), ]); expect(request).toHaveBeenCalledOnce(); - expect(warn).toHaveBeenCalledOnce(); + expect(results.map(({ status }) => status)).toEqual([ + "rejected", + "rejected", + ]); }); }); diff --git a/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts b/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts index 22f3aef92..2c4f5989b 100644 --- a/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/initialize-pool.test.ts @@ -192,6 +192,23 @@ describe("AppKit Lakebase connector initialization", () => { expect(mocks.me).not.toHaveBeenCalled(); }); + test("does not validate a Lakebase endpoint for native password authentication", async () => { + mocks.request.mockResolvedValue({ + status: { hosts: { host: "ep-other.database.example.com" } }, + }); + + expect( + await initializeLakebasePool({ + user: "native-user", + password: "test-only-password", + endpoint: "projects/native/branches/test/endpoints/primary", + host: "ep-native.database.example.com", + workspaceClient: mocks.client as unknown as Client, + }), + ).toBe(pool); + expect(mocks.request).not.toHaveBeenCalled(); + }); + test("does not allocate a pool when identity cannot be resolved", async () => { mocks.me.mockRejectedValueOnce(new Error("Private auth detail")); await expect(initializeLakebasePool()).rejects.toMatchObject({ @@ -203,7 +220,7 @@ describe("AppKit Lakebase connector initialization", () => { expect(mocks.createPool).not.toHaveBeenCalled(); }); - test("warns at startup when PGHOST is not a host of the endpoint", async () => { + test("refuses a pool when PGHOST is not a host of the endpoint", async () => { vi.stubEnv( "LAKEBASE_ENDPOINT", "projects/p/branches/fresh/endpoints/primary", @@ -212,17 +229,15 @@ describe("AppKit Lakebase connector initialization", () => { mocks.request.mockResolvedValue({ status: { hosts: { host: "ep-fresh.database.example.test" } }, }); - const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); - - expect(await initializeLakebasePool()).toBe(pool); + await expect(initializeLakebasePool()).rejects.toThrow( + "ep-fresh.database.example.test", + ); - expect(mocks.request).toHaveBeenCalledWith( + expect(mocks.request.mock.calls[0]?.[0]).toEqual( expect.objectContaining({ path: "/api/2.0/postgres/projects/p/branches/fresh/endpoints/primary", }), ); - const output = warn.mock.calls.flat().map(String).join(" "); - expect(output).toContain("ep-stale.database.example.test"); - expect(output).toContain("ep-fresh.database.example.test"); + expect(mocks.createPool).not.toHaveBeenCalled(); }); }); diff --git a/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts b/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts index b1cc16a50..5695a7140 100644 --- a/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts +++ b/packages/appkit/src/database/runtime/engine/drizzle-data-path.ts @@ -159,21 +159,23 @@ function upsertUpdateValues( // nested `cause` rather than the thrown error. Walk a bounded chain to find it. const MAX_CAUSE_DEPTH = 5; -// These SQLSTATE classes describe the connection, the credentials, or schema -// objects, so the server's text names identifiers rather than row values: -// 08 connection, 28 authorization, 3D catalog, 3F schema, 42 undefined objects -// and privileges, 53 resources, 57 operator intervention. Data (22), -// constraint (23), and PL/pgSQL (P0) text can echo values and is never logged. -const DESCRIBED_SQLSTATE_CLASSES = new Set([ - "08", - "28", - "3D", - "3F", - "42", - "53", - "57", +// Even schema and authorization errors can carry values in message/detail/hint. +// Admit only the exact identifier-only forms needed to diagnose schema drift. +const SAFE_COLUMN_ERROR = + /^column (?:[A-Za-z_][A-Za-z0-9_]*\.)?[A-Za-z_][A-Za-z0-9_]* does not exist$/; +const SAFE_RELATION_ERROR = + /^relation "[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)?" does not exist$/; +const SAFE_DNS_ERROR = /^getaddrinfo ENOTFOUND [A-Za-z0-9.-]+$/; +const MAX_DIAGNOSTIC_LENGTH = 200; +const SYSTEM_ERROR_CODES = new Set([ + "ENOTFOUND", + "EAI_AGAIN", + "ECONNREFUSED", + "ETIMEDOUT", + "EHOSTUNREACH", + "ENETUNREACH", + "EPIPE", ]); -const MAX_DIAGNOSTIC_LENGTH = 500; interface DriverFailure { readonly sqlState?: string; @@ -181,17 +183,21 @@ interface DriverFailure { readonly diagnostic?: string; } -/** Read the driver's own message, detail, and hint, never a wrapper's. */ -function driverText(carrier: object): string | undefined { +/** Never log unvalidated driver text, even for an otherwise known SQLSTATE. */ +function safeDriverText(carrier: object, code: string): string | undefined { try { - const parts = ["message", "detail", "hint"] - .map((key) => Reflect.get(carrier, key)) - .filter( - (part): part is string => typeof part === "string" && part !== "", - ); - return parts.length > 0 - ? parts.join(" ").slice(0, MAX_DIAGNOSTIC_LENGTH) - : undefined; + const message = Reflect.get(carrier, "message"); + if (typeof message !== "string" || message.length > MAX_DIAGNOSTIC_LENGTH) + return undefined; + if (code === "42703" && SAFE_COLUMN_ERROR.test(message)) return message; + if (code === "42P01" && SAFE_RELATION_ERROR.test(message)) return message; + if ( + code === "ENOTFOUND" && + Reflect.get(carrier, "syscall") === "getaddrinfo" && + SAFE_DNS_ERROR.test(message) + ) + return message; + return undefined; } catch { return undefined; } @@ -203,21 +209,19 @@ function driverFailureOf(error: unknown): DriverFailure { if (!current || typeof current !== "object") return {}; try { const candidate = Reflect.get(current, "code"); - // Node system errors (ECONNREFUSED, ENOTFOUND) name the host, not data. + // Node system errors are untrusted too; only a well-formed DNS host is safe. if ( typeof candidate === "string" && - /^E[A-Z]+$/.test(candidate) && + SYSTEM_ERROR_CODES.has(candidate) && typeof Reflect.get(current, "syscall") === "string" ) { - return { diagnostic: driverText(current) }; + return { diagnostic: safeDriverText(current, candidate) ?? candidate }; } // SQLSTATE is always a five-character alphanumeric class code. if (typeof candidate === "string" && /^[0-9A-Z]{5}$/.test(candidate)) { return { sqlState: candidate, - diagnostic: DESCRIBED_SQLSTATE_CLASSES.has(candidate.slice(0, 2)) - ? driverText(current) - : undefined, + diagnostic: safeDriverText(current, candidate), }; } current = Reflect.get(current, "cause"); @@ -230,7 +234,7 @@ function driverFailureOf(error: unknown): DriverFailure { /** * Classify SQLSTATE without retaining the driver error or its properties. - * Described classes add the driver's text to the server log only. + * Only allowlisted identifier-only messages enter the server log. */ function classifyDriverError(error: unknown): DatabasePluginError { const { sqlState: code, diagnostic } = driverFailureOf(error); diff --git a/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts b/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts index 040aa8fc2..b9d35409b 100644 --- a/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts +++ b/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts @@ -815,6 +815,91 @@ describe("database failures", () => { } }); + it.each(["42501", "57014", "42601", "42703", "42P01", "28000"])( + "does not log arbitrary driver text for SQLSTATE %s", + async (code) => { + const fake = makeFakeDb(); + const query = fake.db.query as unknown as Record< + string, + { findMany: () => Promise } + >; + query.users.findMany = async () => { + throw { + code, + message: "patient@example.test must not reach the log", + detail: "private row value patient@example.test", + hint: "try patient@example.test", + }; + }; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + await createDrizzleDataPath(fake.db, schema) + .select(users, {}) + .catch(() => undefined); + + const output = errorLog.mock.calls.flat().map(String).join(" "); + expect(output).toContain(code); + expect(output).not.toContain("patient@example.test"); + } finally { + errorLog.mockRestore(); + } + }, + ); + + it("does not admit trailing newlines in otherwise allowed diagnostics", async () => { + const fake = makeFakeDb(); + const query = fake.db.query as unknown as Record< + string, + { findMany: () => Promise } + >; + query.users.findMany = async () => { + throw { code: "42703", message: "column notes.body does not exist\n" }; + }; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + await createDrizzleDataPath(fake.db, schema) + .select(users, {}) + .catch(() => undefined); + expect(errorLog.mock.calls.flat().map(String).join(" ")).not.toContain( + "\n", + ); + } finally { + errorLog.mockRestore(); + } + }); + + it("bounds untrusted system error codes in server logs", async () => { + const fake = makeFakeDb(); + const query = fake.db.query as unknown as Record< + string, + { findMany: () => Promise } + >; + const oversizedCode = `E${"A".repeat(500)}`; + query.users.findMany = async () => { + throw { code: oversizedCode, syscall: "connect", message: "sensitive" }; + }; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + await createDrizzleDataPath(fake.db, schema) + .select(users, {}) + .catch(() => undefined); + expect(errorLog.mock.calls.flat().map(String).join(" ")).not.toContain( + oversizedCode, + ); + } finally { + errorLog.mockRestore(); + } + }); + // Schema drift used to surface as a bare SQLSTATE, with no object named. it.each([ [ @@ -823,23 +908,15 @@ describe("database failures", () => { message: "column notes.board_id does not exist", hint: 'Perhaps you meant to reference the column "notes.body".', }, - ["column notes.board_id does not exist", '"notes.body"'], + ["column notes.board_id does not exist"], ], [ "42P01", { message: 'relation "public.boards" does not exist' }, ['relation "public.boards" does not exist'], ], - [ - "28000", - { - message: "External authorization failed.", - detail: "This could be due to paused instances.", - }, - ["External authorization failed.", "paused instances"], - ], ] as const)( - "logs the driver's own text for described SQLSTATE %s, never the wrapper's", + "logs safe schema identifiers for SQLSTATE %s, never free-form text", async (code, fields, expected) => { const fake = makeFakeDb(); const query = fake.db.query as unknown as Record< @@ -867,6 +944,7 @@ describe("database failures", () => { const output = errorLog.mock.calls.flat().map(String).join(" "); for (const text of expected) expect(output).toContain(text); + expect(output).not.toContain("Perhaps you meant"); expect(output).not.toContain("Failed query"); expect(output).not.toContain("alice@x.com"); // The diagnostic stays in the log; callers still get the stable error. diff --git a/packages/appkit/src/plugins/database/lifecycle.ts b/packages/appkit/src/plugins/database/lifecycle.ts index 38347c54a..c50badf6f 100644 --- a/packages/appkit/src/plugins/database/lifecycle.ts +++ b/packages/appkit/src/plugins/database/lifecycle.ts @@ -221,7 +221,8 @@ export async function createDatabaseState( logger.error("%s", error.message); throw error; } - logger.error("Database setup failed: %O", error); + // Unknown driver and SDK errors may carry SQL, parameters, and credentials. + logger.error("Database setup failed (connector or driver error)"); throw new DatabasePluginError("SETUP_FAILED", "setup"); } } diff --git a/packages/appkit/src/plugins/database/schema-check.ts b/packages/appkit/src/plugins/database/schema-check.ts index 9f012a4f0..f1334ecda 100644 --- a/packages/appkit/src/plugins/database/schema-check.ts +++ b/packages/appkit/src/plugins/database/schema-check.ts @@ -5,6 +5,7 @@ import type { Schema } from "../../database/schema-builder"; interface CatalogColumn { readonly table_name: string; readonly column_name: string | null; + readonly relation_kind: string; } /** @@ -22,9 +23,10 @@ export async function assertSchemaMatchesDatabase( if (tables.length === 0) return; const schemaName = schema.$schemaName; const tableNames = tables.map((table) => table.$name); - // Tables, partitioned tables, views, materialized views, and foreign tables. + // Include non-tables so setup can report their kind instead of claiming they are absent. const rows = await dataPath.raw` - select c.relname::text as table_name, a.attname::text as column_name + select c.relname::text as table_name, a.attname::text as column_name, + c.relkind::text as relation_kind from pg_catalog.pg_class c join pg_catalog.pg_namespace n on n.oid = c.relnamespace left join pg_catalog.pg_attribute a @@ -33,24 +35,39 @@ export async function assertSchemaMatchesDatabase( and c.relname::text = any(${tableNames}::text[]) and c.relkind in ('r', 'p', 'v', 'm', 'f')`; - const found = new Map>(); + const found = new Map }>(); for (const row of rows) { - const columns = found.get(row.table_name) ?? new Set(); - if (row.column_name) columns.add(row.column_name); - found.set(row.table_name, columns); + const relation = found.get(row.table_name) ?? { + kind: row.relation_kind, + columns: new Set(), + }; + if (row.column_name) relation.columns.add(row.column_name); + found.set(row.table_name, relation); } const problems: string[] = []; for (const table of tables) { const qualified = `${schemaName}.${table.$name}`; - const columns = found.get(table.$name); - if (!columns) { + const relation = found.get(table.$name); + if (!relation) { problems.push(`table ${qualified} does not exist`); continue; } + if (relation.kind !== "r" && relation.kind !== "p") { + const kind = + relation.kind === "v" + ? "view" + : relation.kind === "m" + ? "materialized view" + : relation.kind === "f" + ? "foreign table" + : "non-table relation"; + problems.push(`table ${qualified} is a ${kind}, not a table`); + continue; + } const missing = Object.values(table.$columns) .map((meta) => meta.columnName) - .filter((name) => !columns.has(name)); + .filter((name) => !relation.columns.has(name)); if (missing.length > 0) { problems.push( `table ${qualified} is missing ${missing.length === 1 ? "column" : "columns"} ${missing.join(", ")}`, diff --git a/packages/appkit/src/plugins/database/tests/lifecycle.test.ts b/packages/appkit/src/plugins/database/tests/lifecycle.test.ts index e70d822b5..aae7c04d2 100644 --- a/packages/appkit/src/plugins/database/tests/lifecycle.test.ts +++ b/packages/appkit/src/plugins/database/tests/lifecycle.test.ts @@ -161,6 +161,9 @@ describe("createDatabaseState", () => { async (stage) => { const { pool, execute } = arrange(); const raw = new Error("secret constraint detail"); + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); if (stage === "drizzle") mocks.createDrizzleDb.mockImplementationOnce(() => { throw raw; @@ -184,6 +187,9 @@ describe("createDatabaseState", () => { expect(error.message).toBe("Database setup failed"); expect(error.cause).toBeUndefined(); expect(pool.end).toHaveBeenCalledTimes(1); + expect(errorLog.mock.calls.flat().map(String).join(" ")).not.toContain( + raw.message, + ); }, ); @@ -212,6 +218,9 @@ describe("createDatabaseState", () => { test("sanitizes connector initialization failures", async () => { const { execute } = arrange(); + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); mocks.initializeLakebasePool.mockRejectedValueOnce( new Error("secret host and credential details"), ); @@ -223,6 +232,9 @@ describe("createDatabaseState", () => { expect(error).toMatchObject({ category: "SETUP_FAILED", phase: "setup" }); expect(error.message).toBe("Database setup failed"); expect(error.cause).toBeUndefined(); + expect(errorLog.mock.calls.flat().map(String).join(" ")).not.toContain( + "secret host and credential details", + ); }); test("runs root SQL directly, maps failures safely, and rejects after deactivation", async () => { diff --git a/packages/appkit/src/plugins/database/tests/schema-check.test.ts b/packages/appkit/src/plugins/database/tests/schema-check.test.ts index df5a80c87..6f10836a8 100644 --- a/packages/appkit/src/plugins/database/tests/schema-check.test.ts +++ b/packages/appkit/src/plugins/database/tests/schema-check.test.ts @@ -15,15 +15,32 @@ const schema = defineSchema(({ table }) => { return { boards, notes }; }); -type CatalogRow = { table_name: string; column_name: string | null }; +type CatalogRow = { + table_name: string; + column_name: string | null; + relation_kind: string; +}; /** Answer the catalog query with the given tables and columns. */ -function catalog(tables: Record) { +function catalog( + tables: Record, + kinds: Record = {}, +) { const rows: CatalogRow[] = Object.entries(tables).flatMap( ([name, columns]): CatalogRow[] => columns.length === 0 - ? [{ table_name: name, column_name: null }] - : columns.map((column) => ({ table_name: name, column_name: column })), + ? [ + { + table_name: name, + column_name: null, + relation_kind: kinds[name] ?? "r", + }, + ] + : columns.map((column) => ({ + table_name: name, + column_name: column, + relation_kind: kinds[name] ?? "r", + })), ); const raw = vi.fn(async () => rows); return { raw, path: { raw } as unknown as DataPath }; @@ -77,6 +94,27 @@ describe("assertSchemaMatchesDatabase", () => { ); }); + test.each([ + ["v", "view"], + ["m", "materialized view"], + ["f", "foreign table"], + ])( + "rejects %s as a CRUD table even when all columns exist", + async (kind, description) => { + const { path } = catalog( + { + boards: ["id", "title"], + notes: ["id", "board_id", "author_email", "body"], + }, + { boards: kind }, + ); + + await expect(assertSchemaMatchesDatabase(path, schema)).rejects.toThrow( + `public.boards is a ${description}, not a table`, + ); + }, + ); + test("passes the schema and table names as parameter values", async () => { const scoped = defineSchema( ({ table }) => ({ tags: table("tags", { id: id() }) }), @@ -92,6 +130,7 @@ describe("assertSchemaMatchesDatabase", () => { ...unknown[], ]; expect(strings.join("?")).toContain("pg_catalog.pg_attribute"); + expect(strings.join("?")).toContain("c.relkind::text as relation_kind"); expect(values).toEqual(["playground", ["tags"]]); }); diff --git a/packages/appkit/src/plugins/lakebase/lakebase.ts b/packages/appkit/src/plugins/lakebase/lakebase.ts index 4a9ba2c72..dd4fe5a92 100644 --- a/packages/appkit/src/plugins/lakebase/lakebase.ts +++ b/packages/appkit/src/plugins/lakebase/lakebase.ts @@ -11,7 +11,7 @@ import { type LakebasePool, type LakebasePoolManager, RoutingPool, - warnOnEndpointHostMismatch, + assertEndpointHostMatches, } from "../../connectors/lakebase"; import { getClientOptions } from "../../context/client-options"; import { getCallerContext } from "../../context/execution-context"; @@ -88,7 +88,7 @@ export class LakebasePlugin extends Plugin implements ToolProvider { }; const [user] = await Promise.all([ getUsernameWithApiLookup(poolConfig), - warnOnEndpointHostMismatch(poolConfig), + assertEndpointHostMatches(poolConfig), ]); const spPool = createLakebasePool({ ...poolConfig, user }); diff --git a/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts b/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts index b33eaef5c..214d236ee 100644 --- a/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts +++ b/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts @@ -1,3 +1,4 @@ +import type { LakebasePoolConfig } from "@databricks/lakebase"; import { beforeEach, describe, expect, test, vi } from "vitest"; /** * Tests the agent-tool surface of the Lakebase plugin. @@ -106,6 +107,55 @@ describe("LakebasePlugin - agent tool opt-in", () => { }); }); +test("LakebasePlugin rejects a confirmed host mismatch before creating either pool", async () => { + const { createLakebasePool, createLakebasePoolManager } = + await import("../../../connectors/lakebase"); + vi.mocked(createLakebasePool).mockClear(); + vi.mocked(createLakebasePoolManager).mockClear(); + const request = vi.fn(async () => ({ + status: { hosts: { host: "ep-expected.database.example.com" } }, + })); + const workspaceClient = { apiClient: { request } } as unknown as NonNullable< + LakebasePoolConfig["workspaceClient"] + >; + const plugin = makePlugin({ + pool: { + endpoint: "projects/plugin/branches/test/endpoints/primary", + host: "ep-other.database.example.com", + workspaceClient, + }, + }); + + await expect(plugin.setup()).rejects.toThrow( + "ep-expected.database.example.com", + ); + expect(createLakebasePool).not.toHaveBeenCalled(); + expect(createLakebasePoolManager).not.toHaveBeenCalled(); +}); + +test("LakebasePlugin permits a separate host when using native password authentication", async () => { + const { createLakebasePool } = await import("../../../connectors/lakebase"); + const request = vi.fn(async () => ({ + status: { hosts: { host: "ep-expected.database.example.com" } }, + })); + const workspaceClient = { apiClient: { request } } as unknown as NonNullable< + LakebasePoolConfig["workspaceClient"] + >; + const plugin = makePlugin({ + pool: { + user: "native-user", + password: "test-only-password", + endpoint: "projects/native/branches/test/endpoints/primary", + host: "ep-native.database.example.com", + workspaceClient, + }, + }); + + await expect(plugin.setup()).resolves.toBeUndefined(); + expect(request).not.toHaveBeenCalled(); + expect(createLakebasePool).toHaveBeenCalled(); +}); + describe("LakebasePlugin - readOnly enforcement", () => { let plugin: LakebasePlugin;