From 63ef8d8505dd467d351ed6f15d95af08e61a7971 Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 6 Apr 2026 00:16:21 -0700 Subject: [PATCH 1/4] fix(core): parameterize sampler SQL to prevent injection via source data The referential-integrity sampler built WHERE clauses by string-concatenating primary key values taken from sampled source rows. Two failure modes: 1. Benign: a text PK containing a single quote (e.g. "O'Brien") produced invalid SQL, which was then caught by an empty catch{} and silently dropped. Users got a branch with dangling foreign keys and never knew. 2. Security: sow's entire pitch is "safe for your production database". A crafted PK value like "'; DROP TABLE x; --" would execute against the source DB under whatever permissions the connection string had. Read-only in intent, not in SQL effect. Changes: - adapters/postgres.ts: the query() method accepted a `params` argument in the interface but silently dropped it (`_params?: unknown[]`) and called sql.unsafe(sqlStr) with no binding. Now it passes params through when present. The interface finally matches the runtime behavior. - sampler/referential.ts: all three dynamic-SQL call sites now use $1,$2,... placeholders and pass values via the params array. Added a quoteIdent() helper that escapes double quotes in table/column names per the SQL standard as defense in depth for the catalog names. - sampler/referential.test.ts: 9 new tests covering quoteIdent, a regression test for the O'Brien single-quote crash, composite FK placeholders, and the hostile-payload case. Uses a spying adapter to assert that values flow through params and never into SQL text. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/adapters/postgres.ts | 10 +- packages/core/src/sampler/referential.test.ts | 261 ++++++++++++++++++ packages/core/src/sampler/referential.ts | 45 ++- 3 files changed, 305 insertions(+), 11 deletions(-) create mode 100644 packages/core/src/sampler/referential.test.ts diff --git a/packages/core/src/adapters/postgres.ts b/packages/core/src/adapters/postgres.ts index 144ef87..3136f76 100644 --- a/packages/core/src/adapters/postgres.ts +++ b/packages/core/src/adapters/postgres.ts @@ -413,10 +413,16 @@ export class PostgresAdapter implements DatabaseAdapter { async query>( sqlStr: string, - _params?: unknown[], + params?: unknown[], ): Promise { const sql = this.db(); - const rows = await sql.unsafe(sqlStr); + // postgres@3 `sql.unsafe(query, parameters)` binds values via real + // placeholders ($1, $2, ...) when `parameters` is provided. Passing + // undefined runs the query literally. This is the only safe path for + // SQL built with dynamic structure (see sampler/referential.ts). + const rows = params && params.length > 0 + ? await sql.unsafe(sqlStr, params as unknown as Parameters[1]) + : await sql.unsafe(sqlStr); return rows as unknown as T[]; } } diff --git a/packages/core/src/sampler/referential.test.ts b/packages/core/src/sampler/referential.test.ts new file mode 100644 index 0000000..b5121ea --- /dev/null +++ b/packages/core/src/sampler/referential.test.ts @@ -0,0 +1,261 @@ +import { + ensureReferentialIntegrity, + quoteIdent, +} from "./referential.js"; +import type { + DatabaseAdapter, + Relationship, + SchemaInfo, + TableStats, + ColumnStats, + ConnectionInfo, +} from "../types.js"; + +// ----------------------------------------------------------------------- +// quoteIdent — SQL identifier safety +// ----------------------------------------------------------------------- + +describe("quoteIdent", () => { + it("wraps a simple identifier in double quotes", () => { + expect(quoteIdent("users")).toBe('"users"'); + }); + + it("wraps a column name with underscores", () => { + expect(quoteIdent("created_at")).toBe('"created_at"'); + }); + + it("doubles embedded double quotes to escape them", () => { + // An identifier containing " must become "" + expect(quoteIdent('weird"name')).toBe('"weird""name"'); + }); + + it("handles multiple embedded quotes", () => { + expect(quoteIdent('a"b"c')).toBe('"a""b""c"'); + }); + + it("handles an identifier that is itself quoted (defense in depth)", () => { + // A catalog row that accidentally includes a semicolon or SQL keywords + // cannot break out of the quoted identifier. + expect(quoteIdent('users"; DROP TABLE users; --')).toBe( + '"users""; DROP TABLE users; --"', + ); + }); + + it("handles empty string", () => { + expect(quoteIdent("")).toBe('""'); + }); +}); + +// ----------------------------------------------------------------------- +// SpyAdapter — records every query call so tests can assert SQL shape +// ----------------------------------------------------------------------- + +interface QueryCall { + sql: string; + params?: unknown[]; +} + +class SpyAdapter implements DatabaseAdapter { + calls: QueryCall[] = []; + // Rows keyed by targetTable; returned from query() when the table is hit. + responses: Map[]> = new Map(); + + async connect(): Promise {} + async disconnect(): Promise {} + + async getSchema(): Promise { + return { tables: [], constraints: [], relationships: [], indexes: [], enumTypes: [] }; + } + async getTableStats(): Promise { + return { table: "", rowCount: 0, sizeBytes: 0, columns: [] }; + } + async getColumnStats(): Promise { + return { + column: "", + dataType: "", + nullable: false, + nullCount: 0, + distinctCount: 0, + sampleValues: [], + }; + } + async getSampleRows(): Promise[]> { + return []; + } + async getAllRows(): Promise[]> { + return []; + } + async getRandomSample(): Promise[]> { + return []; + } + async getRowCount(): Promise { + return 0; + } + + async query>( + sql: string, + params?: unknown[], + ): Promise { + this.calls.push({ sql, params: params ? [...params] : undefined }); + // Route by the FROM clause — find any response whose key appears in the SQL. + for (const [key, rows] of this.responses) { + if (sql.includes(`"${key}"`)) { + return rows as T[]; + } + } + return []; + } + + getConnectionInfo(): ConnectionInfo { + return { + host: "localhost", + port: 5432, + database: "test", + user: "test", + }; + } +} + +// ----------------------------------------------------------------------- +// Regression tests — SQL injection safety in ensureReferentialIntegrity +// ----------------------------------------------------------------------- + +describe("ensureReferentialIntegrity — SQL parameterization", () => { + it("binds text primary keys with embedded single quotes via $1 placeholders (regression: O'Brien crash)", async () => { + // The child table `orders` references a user whose PK contains a single + // quote. Pre-fix, the SQL was built with raw interpolation: + // SELECT * FROM "users" WHERE "id" = 'O'Brien' <- parse error + // The catch{} swallowed the error silently and the parent row was dropped. + // Post-fix, the value flows through $1 and the query is valid. + const adapter = new SpyAdapter(); + adapter.responses.set("users", [ + { id: "O'Brien", name: "Sean" }, + ]); + + const sampledTables = new Map[]>([ + // Seed users with an unrelated row so the loop body actually runs + // (it early-returns when either side is empty). The row referenced + // by orders (O'Brien) is missing and must be fetched. + ["users", [{ id: "other-user", name: "Existing" }]], + ["orders", [{ id: 1, user_id: "O'Brien" }]], + ]); + + const relationships: Relationship[] = [ + { + name: "orders_user_id_fkey", + sourceTable: "orders", + sourceColumns: ["user_id"], + targetTable: "users", + targetColumns: ["id"], + onDelete: "NO ACTION", + onUpdate: "NO ACTION", + }, + ]; + + const result = await ensureReferentialIntegrity( + adapter, + sampledTables, + relationships, + ); + + // The missing-parent fetch must have been issued. Find it. + const parentFetch = adapter.calls.find( + (c) => + c.sql.includes('FROM "users"') && + c.sql.includes('"id" = $1'), + ); + expect(parentFetch).toBeDefined(); + + // The value must appear in params, NOT in the SQL string. + expect(parentFetch!.params).toEqual(["O'Brien"]); + expect(parentFetch!.sql).not.toContain("O'Brien"); + + // And the user row must have landed in the result, proving the query + // actually succeeded (the old behavior silently dropped it). + const users = result.get("users") || []; + expect(users.some((u) => u.id === "O'Brien")).toBe(true); + }); + + it("uses placeholders not string interpolation for composite foreign keys", async () => { + const adapter = new SpyAdapter(); + adapter.responses.set("composite_parent", [ + { tenant_id: "t1", entity_id: "e1", data: "ok" }, + ]); + + const sampledTables = new Map[]>([ + // Seed composite_parent with an unrelated row so the loop runs; + // the (t1, e1) key is missing and triggers the fetch path. + [ + "composite_parent", + [{ tenant_id: "other", entity_id: "other", data: "seed" }], + ], + [ + "composite_child", + [{ id: 1, p_tenant: "t1", p_entity: "e1" }], + ], + ]); + + const relationships: Relationship[] = [ + { + name: "composite_fk", + sourceTable: "composite_child", + sourceColumns: ["p_tenant", "p_entity"], + targetTable: "composite_parent", + targetColumns: ["tenant_id", "entity_id"], + onDelete: "NO ACTION", + onUpdate: "NO ACTION", + }, + ]; + + await ensureReferentialIntegrity(adapter, sampledTables, relationships); + + const parentFetch = adapter.calls.find((c) => + c.sql.includes('FROM "composite_parent"'), + ); + expect(parentFetch).toBeDefined(); + // Two placeholders, one per column + expect(parentFetch!.sql).toContain('"tenant_id" = $1'); + expect(parentFetch!.sql).toContain('"entity_id" = $2'); + expect(parentFetch!.params).toEqual(["t1", "e1"]); + }); + + it("does not interpolate raw values into the SQL text (defense against crafted payloads)", async () => { + // If a source DB had a hostile primary key like "'; DROP TABLE x; --" + // the pre-fix code would execute it. Post-fix, it must flow through + // params where it is inert text. + const adapter = new SpyAdapter(); + const hostile = "'; DROP TABLE users; --"; + adapter.responses.set("targets", [{ id: hostile, ok: true }]); + + const sampledTables = new Map[]>([ + // Seed targets with an unrelated row so the loop runs. + ["targets", [{ id: "benign", ok: true }]], + ["refs", [{ id: 1, target_id: hostile }]], + ]); + + const relationships: Relationship[] = [ + { + name: "refs_target_fkey", + sourceTable: "refs", + sourceColumns: ["target_id"], + targetTable: "targets", + targetColumns: ["id"], + onDelete: "NO ACTION", + onUpdate: "NO ACTION", + }, + ]; + + await ensureReferentialIntegrity(adapter, sampledTables, relationships); + + const parentFetch = adapter.calls.find((c) => + c.sql.includes('FROM "targets"'), + ); + expect(parentFetch).toBeDefined(); + // The hostile payload must NEVER appear inline in the SQL string. + expect(parentFetch!.sql).not.toContain("DROP TABLE"); + expect(parentFetch!.sql).not.toContain(hostile); + // It flows through params verbatim, where Postgres will treat it as + // a literal string value, not executable SQL. + expect(parentFetch!.params).toEqual([hostile]); + }); +}); diff --git a/packages/core/src/sampler/referential.ts b/packages/core/src/sampler/referential.ts index 23060a6..7d73ed4 100644 --- a/packages/core/src/sampler/referential.ts +++ b/packages/core/src/sampler/referential.ts @@ -5,6 +5,22 @@ const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/ // Columns already handled by formal FKs or that shouldn't be followed const SKIP_IMPLICIT_COLUMNS = new Set(["id", "user_id", "owner_id", "created_by"]); +/** + * Safely quote a SQL identifier (table or column name). + * + * Postgres binds values via $1,$2,... but identifiers cannot be parameterized. + * We wrap in double quotes and escape any embedded double quote by doubling it, + * per the SQL standard. This prevents an identifier containing quotes (e.g. a + * hostile or quirky catalog name) from breaking out of the quoting and turning + * into injectable SQL. The values in the queries themselves are always bound + * via the `params` array. + * + * Example: quoteIdent('weird"name') -> '"weird""name"' + */ +export function quoteIdent(name: string): string { + return '"' + name.replace(/"/g, '""') + '"'; +} + /** * Infer which table a column like `session_id` or `run_id` references. * Tries: session_id -> sessions, session_id -> session, analysis_run_id -> analysis_runs @@ -86,12 +102,16 @@ export async function ensureReferentialIntegrity( if (keyParts.length !== rel.targetColumns.length) continue; try { + // Identifiers are quoted (not parameterizable in Postgres); + // values go through $1,$2,... bind parameters. This is safe + // against a text PK like "O'Brien" and against crafted payloads. const conditions = rel.targetColumns - .map((col, i) => `"${col}" = '${keyParts[i]}'`) + .map((col, i) => `${quoteIdent(col)} = $${i + 1}`) .join(" AND "); const rows = await adapter.query( - `SELECT * FROM "${rel.targetTable}" WHERE ${conditions} LIMIT 1`, + `SELECT * FROM ${quoteIdent(rel.targetTable)} WHERE ${conditions} LIMIT 1`, + keyParts, ); if (rows.length > 0) { const existing = result.get(rel.targetTable) || []; @@ -120,15 +140,16 @@ export async function ensureReferentialIntegrity( if (!childFKValues.has(parentKey)) { try { + const values = rel.sourceColumns.map((_, i) => + String(parent[rel.targetColumns[i]] ?? ""), + ); const conditions = rel.sourceColumns - .map( - (col, i) => - `"${col}" = '${String(parent[rel.targetColumns[i]] ?? "")}'`, - ) + .map((col, i) => `${quoteIdent(col)} = $${i + 1}`) .join(" AND "); const rows = await adapter.query( - `SELECT * FROM "${rel.sourceTable}" WHERE ${conditions} LIMIT 1`, + `SELECT * FROM ${quoteIdent(rel.sourceTable)} WHERE ${conditions} LIMIT 1`, + values, ); if (rows.length > 0) { const existing = result.get(rel.sourceTable) || []; @@ -205,9 +226,15 @@ async function resolveImplicitReferences( for (const batch of idBatches) { try { - const idList = batch.map((id) => `'${id}'`).join(","); + // Build placeholder list ($1,$2,...) the same length as the batch. + // Each value is already guaranteed UUID-shaped by the gate above, + // but parameterization is the correct discipline regardless. + const placeholders = batch + .map((_, i) => `$${i + 1}`) + .join(","); const fetched = await adapter.query( - `SELECT * FROM "${targetTable}" WHERE id IN (${idList})`, + `SELECT * FROM ${quoteIdent(targetTable)} WHERE id IN (${placeholders})`, + batch, ); if (fetched.length > 0) { const existing = result.get(targetTable) || []; From e0a724d4ccf51c2b7bd2cf1c560dd2665aed8ca3 Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 6 Apr 2026 00:28:18 -0700 Subject: [PATCH 2/4] fix(core): parameterize remaining SQL injection sites in branching layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the sampler fix. A /ship specialist review flagged that the same class of string-interpolated SQL existed in the branching layer and on paths that receive user/agent input. Five more sites closed. Changes: - sql/identifiers.ts (new): extract quoteIdent to a shared module so it can be used by any file that builds dynamic SQL. The sampler re-exports it so the prior test imports still resolve. - branching/manager.ts:473 getBranchSample — the `table` argument comes directly from `sow branch sample ` (user argv) and from the `sow_branch_sample` MCP tool (agent-supplied). It was interpolated into sql.unsafe unsanitized. Now quoted via quoteIdent, and the limit is bound via $1 after numeric clamping. - branching/providers/supabase.ts:55 fetchAuthUserMappings — the IN(...) clause was built by single-quote-concatenating user ids from sampled source data. Now uses $1,$2,... placeholders with userIds as the params array to adapter.query(). - branching/supabase.ts RLS setup block — 8 sites interpolating tableName into DDL and information_schema WHERE clauses. tableName values without quoted identifier escaping could break out of the wrapping quotes. Now uses quoteIdent for all identifier positions and $1 bind params for all value positions in the information_schema lookups. - branching/supabase.ts createTestAuthUsers — user.id and user.email were interpolated into DELETE and INSERT statements inside single-quoted literals. Now all three call sites (delete-by-id, insert auth.users row, insert matching identity) bind values via $1,$2 placeholders. The `gen_random_uuid()` SQL function literal path is split from the parameterized-id path to keep the query text honest about which part is value vs SQL. All fixes follow the same pattern: quoteIdent for identifiers (which Postgres cannot parameterize), $N placeholders for values. Testing: - Full unit suite green (88/88). - Build clean across all three packages. - Lint clean on every touched file. - The sampler regression tests from the previous commit still pass (quoteIdent is re-exported via the sampler module). - Integration tests against the bugsterdb schema in __integration__ exercise the RLS and auth-user paths; they require a live Postgres and are NOT run here (separate vitest config). Future work: add spy-adapter tests for fetchAuthUserMappings specifically. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/branching/manager.ts | 11 +- .../core/src/branching/providers/supabase.ts | 8 +- packages/core/src/branching/supabase.ts | 130 +++++++++++++----- packages/core/src/sampler/referential.ts | 20 +-- packages/core/src/sql/identifiers.ts | 23 ++++ 5 files changed, 135 insertions(+), 57 deletions(-) create mode 100644 packages/core/src/sql/identifiers.ts diff --git a/packages/core/src/branching/manager.ts b/packages/core/src/branching/manager.ts index fa532c3..080c219 100644 --- a/packages/core/src/branching/manager.ts +++ b/packages/core/src/branching/manager.ts @@ -16,6 +16,7 @@ import { } from "./storage.js"; import { diffBranch } from "./diff.js"; import { loadProjectState } from "../config/loader.js"; +import { quoteIdent } from "../sql/identifiers.js"; import type { Branch, BranchOptions, CheckpointInfo, DiffResult } from "./types.js"; function resolveConnector(connectorName?: string): string { @@ -470,10 +471,16 @@ export async function getBranchSample( }); try { + // `table` comes from user/agent input via `sow branch sample
` + // or the `sow_branch_sample` MCP tool — we cannot trust it. Identifiers + // cannot be parameterized so we quote via the SQL-standard escape. The + // limit is numeric-clamped to [0, 100] then passed as $1. + const safeLimit = Math.min(Math.max(0, limit | 0), 100); const rows = await sql.unsafe( - `SELECT * FROM "${table}" LIMIT ${Math.min(limit, 100)}`, + `SELECT * FROM ${quoteIdent(table)} LIMIT $1`, + [safeLimit] as unknown as Parameters[1], ); - return rows.map((r: any) => ({ ...r })); + return rows.map((r: Record) => ({ ...r })); } finally { await sql.end(); } diff --git a/packages/core/src/branching/providers/supabase.ts b/packages/core/src/branching/providers/supabase.ts index 414f06d..2ecf421 100644 --- a/packages/core/src/branching/providers/supabase.ts +++ b/packages/core/src/branching/providers/supabase.ts @@ -52,9 +52,13 @@ async function fetchAuthUserMappings( ): Promise { if (userIds.length === 0) return []; try { - const idList = userIds.map((id) => `'${id}'`).join(","); + // Build $1,$2,... placeholders and bind userIds as parameters. The + // pre-fix code interpolated the ids inside single-quoted literals, + // which a source row with a quote or crafted payload could escape. + const placeholders = userIds.map((_, i) => `$${i + 1}`).join(","); const rows = await adapter.query<{ id: string; email: string }>( - `SELECT id::text, email FROM auth.users WHERE id IN (${idList})`, + `SELECT id::text, email FROM auth.users WHERE id IN (${placeholders})`, + userIds, ); return rows.map((row) => ({ id: row.id, diff --git a/packages/core/src/branching/supabase.ts b/packages/core/src/branching/supabase.ts index 9e71bb2..3ef60d9 100644 --- a/packages/core/src/branching/supabase.ts +++ b/packages/core/src/branching/supabase.ts @@ -2,6 +2,7 @@ import { execSync } from "node:child_process"; import { readFileSync } from "node:fs"; import { createConnection } from "node:net"; import postgres from "postgres"; +import { quoteIdent } from "../sql/identifiers.js"; const SUPABASE_DB_PORT = 54322; const SUPABASE_API_PORT = 54321; @@ -113,30 +114,47 @@ export async function loadIntoSupabase( for (const t of tables) { const tableName = t.tablename as string; + // tableName comes from the sandbox DB's pg_catalog. Even though we + // control the sandbox, a catalog row containing an unusual character + // (e.g. a double quote from a quoted DDL identifier in the source + // schema) must not break out of our identifier quoting. + const quotedTable = quoteIdent(tableName); try { + // Parameterize the value `tableName` in information_schema lookups. + // Identifiers (the policy name, the qualified table reference) are + // quoted via quoteIdent because SQL binds values only, not names. const cols = await sql.unsafe( - `SELECT column_name FROM information_schema.columns WHERE table_schema = 'public' AND table_name = '${tableName}' AND column_name IN ('user_id', 'owner_id', 'created_by')`, + "SELECT column_name FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name IN ('user_id', 'owner_id', 'created_by')", + [tableName] as unknown as Parameters[1], ); - await sql.unsafe(`ALTER TABLE public."${tableName}" ENABLE ROW LEVEL SECURITY`); + await sql.unsafe(`ALTER TABLE public.${quotedTable} ENABLE ROW LEVEL SECURITY`); if (cols.length > 0) { - const userCol = cols[0].column_name; - await sql.unsafe(`CREATE POLICY "sow_owner" ON public."${tableName}" FOR ALL USING ("${userCol}" = auth.uid())`); + const userCol = cols[0].column_name as string; + const quotedUserCol = quoteIdent(userCol); + await sql.unsafe( + `CREATE POLICY "sow_owner" ON public.${quotedTable} FOR ALL USING (${quotedUserCol} = auth.uid())`, + ); } else { const hasProjId = await sql.unsafe( - `SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' AND table_name = '${tableName}' AND column_name = 'project_id' LIMIT 1`, + "SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name = 'project_id' LIMIT 1", + [tableName] as unknown as Parameters[1], ); if (hasProjId.length > 0) { - await sql.unsafe(`CREATE POLICY "sow_project" ON public."${tableName}" FOR ALL USING (project_id IN (SELECT id FROM public.projects WHERE user_id = auth.uid()))`); + await sql.unsafe( + `CREATE POLICY "sow_project" ON public.${quotedTable} FOR ALL USING (project_id IN (SELECT id FROM public.projects WHERE user_id = auth.uid()))`, + ); } else { - await sql.unsafe(`CREATE POLICY "sow_open" ON public."${tableName}" FOR ALL USING (auth.role() = 'authenticated')`); + await sql.unsafe( + `CREATE POLICY "sow_open" ON public.${quotedTable} FOR ALL USING (auth.role() = 'authenticated')`, + ); } } } catch { try { - await sql.unsafe(`ALTER TABLE public."${tableName}" DISABLE ROW LEVEL SECURITY`); + await sql.unsafe(`ALTER TABLE public.${quotedTable} DISABLE ROW LEVEL SECURITY`); } catch { /* ignore */ } } } @@ -228,43 +246,81 @@ export async function createTestAuthUsers( try { for (const user of users) { - const useId = user.id - ? `'${user.id}'::uuid` - : "gen_random_uuid()"; - try { - // Remove existing entries for this ID or email to avoid conflicts + // Remove existing entries for this ID or email to avoid conflicts. + // user.id flows through $1 (cast server-side to uuid), not into + // the SQL text. Empty/malformed ids raise a cast error which is + // caught below, which is the intended behavior. if (user.id) { - await sql.unsafe(`DELETE FROM auth.identities WHERE user_id = '${user.id}'`); - await sql.unsafe(`DELETE FROM auth.users WHERE id = '${user.id}'`); + await sql.unsafe( + "DELETE FROM auth.identities WHERE user_id = $1::uuid", + [user.id] as unknown as Parameters[1], + ); + await sql.unsafe( + "DELETE FROM auth.users WHERE id = $1::uuid", + [user.id] as unknown as Parameters[1], + ); } - await sql.unsafe(` - INSERT INTO auth.users ( - instance_id, id, aud, role, email, encrypted_password, - email_confirmed_at, raw_app_meta_data, raw_user_meta_data, - created_at, updated_at, confirmation_token, email_change, - email_change_token_new, recovery_token - ) VALUES ( - '00000000-0000-0000-0000-000000000000', - ${useId}, 'authenticated', 'authenticated', - '${user.email}', - crypt('password123', gen_salt('bf')), - now(), - '{"provider":"email","providers":["email"]}', - '{}', - now(), now(), '', '', '', '' - ) - `); - - // Create matching identity for login - await sql.unsafe(` + // When user.id is empty, we want `gen_random_uuid()` as the id. + // That's SQL, not a value, so it cannot be parameterized directly. + // Branch on whether we have an id: either bind $1::uuid, or use + // the SQL function literal. + if (user.id) { + await sql.unsafe( + ` + INSERT INTO auth.users ( + instance_id, id, aud, role, email, encrypted_password, + email_confirmed_at, raw_app_meta_data, raw_user_meta_data, + created_at, updated_at, confirmation_token, email_change, + email_change_token_new, recovery_token + ) VALUES ( + '00000000-0000-0000-0000-000000000000', + $1::uuid, 'authenticated', 'authenticated', + $2, + crypt('password123', gen_salt('bf')), + now(), + '{"provider":"email","providers":["email"]}', + '{}', + now(), now(), '', '', '', '' + ) + `, + [user.id, user.email] as unknown as Parameters[1], + ); + } else { + await sql.unsafe( + ` + INSERT INTO auth.users ( + instance_id, id, aud, role, email, encrypted_password, + email_confirmed_at, raw_app_meta_data, raw_user_meta_data, + created_at, updated_at, confirmation_token, email_change, + email_change_token_new, recovery_token + ) VALUES ( + '00000000-0000-0000-0000-000000000000', + gen_random_uuid(), 'authenticated', 'authenticated', + $1, + crypt('password123', gen_salt('bf')), + now(), + '{"provider":"email","providers":["email"]}', + '{}', + now(), now(), '', '', '', '' + ) + `, + [user.email] as unknown as Parameters[1], + ); + } + + // Create matching identity for login — bind the email value. + await sql.unsafe( + ` INSERT INTO auth.identities (id, user_id, provider_id, identity_data, provider, last_sign_in_at, created_at, updated_at) SELECT gen_random_uuid(), id, id, format('{"sub":"%s","email":"%s"}', id::text, email)::jsonb, 'email', now(), now(), now() - FROM auth.users WHERE email = '${user.email}' - `); + FROM auth.users WHERE email = $1 + `, + [user.email] as unknown as Parameters[1], + ); } catch { // Skip users that can't be created } diff --git a/packages/core/src/sampler/referential.ts b/packages/core/src/sampler/referential.ts index 7d73ed4..c26371a 100644 --- a/packages/core/src/sampler/referential.ts +++ b/packages/core/src/sampler/referential.ts @@ -1,26 +1,14 @@ import type { DatabaseAdapter, Relationship } from "../types.js"; +import { quoteIdent } from "../sql/identifiers.js"; + +// Re-export so existing test imports keep working. +export { quoteIdent }; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; // Columns already handled by formal FKs or that shouldn't be followed const SKIP_IMPLICIT_COLUMNS = new Set(["id", "user_id", "owner_id", "created_by"]); -/** - * Safely quote a SQL identifier (table or column name). - * - * Postgres binds values via $1,$2,... but identifiers cannot be parameterized. - * We wrap in double quotes and escape any embedded double quote by doubling it, - * per the SQL standard. This prevents an identifier containing quotes (e.g. a - * hostile or quirky catalog name) from breaking out of the quoting and turning - * into injectable SQL. The values in the queries themselves are always bound - * via the `params` array. - * - * Example: quoteIdent('weird"name') -> '"weird""name"' - */ -export function quoteIdent(name: string): string { - return '"' + name.replace(/"/g, '""') + '"'; -} - /** * Infer which table a column like `session_id` or `run_id` references. * Tries: session_id -> sessions, session_id -> session, analysis_run_id -> analysis_runs diff --git a/packages/core/src/sql/identifiers.ts b/packages/core/src/sql/identifiers.ts new file mode 100644 index 0000000..e2cc85e --- /dev/null +++ b/packages/core/src/sql/identifiers.ts @@ -0,0 +1,23 @@ +/** + * Safely quote a SQL identifier (table, column, schema name). + * + * Postgres binds values via $1, $2, ... placeholders but identifiers + * cannot be parameterized — they are part of the query structure. We + * wrap in double quotes and escape any embedded double quote by doubling + * it, per the SQL standard. This prevents an identifier containing + * quotes (e.g. a hostile catalog row, or a user-supplied table name + * reaching `sow branch sample`) from breaking out of the quoting and + * turning into injectable SQL. + * + * Values in queries are always bound via the `params` array of + * `sql.unsafe(query, params)` or `adapter.query(sql, params)`, never + * interpolated into the query string. + * + * @example + * quoteIdent("users") // => '"users"' + * quoteIdent('weird"name') // => '"weird""name"' + * quoteIdent('x"; DROP x; --') // => '"x""; DROP x; --"' (inert) + */ +export function quoteIdent(name: string): string { + return '"' + name.replace(/"/g, '""') + '"'; +} From 11e0615911c1ab6d132f65cff3732aad42a772ea Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 6 Apr 2026 00:33:03 -0700 Subject: [PATCH 3/4] fix(core): adversarial review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review pass on the fix/sql-injection-sampler branch surfaced four actionable issues. Addressed here. 1. quoteIdent hardening: - Throws on empty identifier (would have produced the invalid SQL token `""` and errored at execution time far from the call site). - Throws on embedded NUL byte (libpq rejects these at protocol level with a confusing error; fail loudly here instead). 2. Kill the quoteIdent re-export from sampler/referential.ts. The test file now imports quoteIdent directly from ../sql/identifiers.js so there is one canonical import path and tests cannot silently diverge from production. 3. branching/manager.ts getBranchSample limit clamping: previously used `limit | 0` which coerced undefined/NaN to 0 and silently returned empty result sets. Now uses an explicit Number.isFinite check and falls back to the documented default of 5, then clamps to [1, 100]. Restores pre-fix semantics for edge inputs without reintroducing any injection surface (the clamped value is still bound via $1). 4. branching/supabase.ts RLS setup: hoisted the two information_schema lookups OUTSIDE the try/catch block. Previously, a transient read error during introspection could end up in the fallback path that DISABLES row level security on the table — a real security regression vector even though the pre-fix code had the same shape. Now the try/catch only wraps the DDL that the fallback is actually designed to recover from (ENABLE RLS + CREATE POLICY). Tests: - 89/89 unit tests passing (up from 88; quoteIdent empty-string test flipped to two throw tests). - Build clean. - Lint clean on every touched file. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/branching/manager.ts | 8 +++- packages/core/src/branching/supabase.ts | 44 +++++++++++-------- packages/core/src/sampler/referential.test.ts | 14 +++--- packages/core/src/sampler/referential.ts | 3 -- packages/core/src/sql/identifiers.ts | 9 ++++ 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/packages/core/src/branching/manager.ts b/packages/core/src/branching/manager.ts index 080c219..e084ea6 100644 --- a/packages/core/src/branching/manager.ts +++ b/packages/core/src/branching/manager.ts @@ -474,8 +474,12 @@ export async function getBranchSample( // `table` comes from user/agent input via `sow branch sample
` // or the `sow_branch_sample` MCP tool — we cannot trust it. Identifiers // cannot be parameterized so we quote via the SQL-standard escape. The - // limit is numeric-clamped to [0, 100] then passed as $1. - const safeLimit = Math.min(Math.max(0, limit | 0), 100); + // limit is numeric-clamped to [1, 100] then passed as $1. We preserve the + // documented default of 5 for non-finite inputs. + const rawLimit = typeof limit === "number" && Number.isFinite(limit) + ? Math.floor(limit) + : 5; + const safeLimit = Math.min(Math.max(1, rawLimit), 100); const rows = await sql.unsafe( `SELECT * FROM ${quoteIdent(table)} LIMIT $1`, [safeLimit] as unknown as Parameters[1], diff --git a/packages/core/src/branching/supabase.ts b/packages/core/src/branching/supabase.ts index 3ef60d9..75e75a4 100644 --- a/packages/core/src/branching/supabase.ts +++ b/packages/core/src/branching/supabase.ts @@ -119,15 +119,24 @@ export async function loadIntoSupabase( // (e.g. a double quote from a quoted DDL identifier in the source // schema) must not break out of our identifier quoting. const quotedTable = quoteIdent(tableName); - try { - // Parameterize the value `tableName` in information_schema lookups. - // Identifiers (the policy name, the qualified table reference) are - // quoted via quoteIdent because SQL binds values only, not names. - const cols = await sql.unsafe( - "SELECT column_name FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name IN ('user_id', 'owner_id', 'created_by')", + + // Introspection queries run outside the try/catch below so that a + // transient read error cannot end up in the policy-disable fallback. + // Values are bound via $1; identifiers use quoteIdent for DDL. + const cols = await sql.unsafe( + "SELECT column_name FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name IN ('user_id', 'owner_id', 'created_by')", + [tableName] as unknown as Parameters[1], + ); + + let hasProjId: unknown[] = []; + if (cols.length === 0) { + hasProjId = await sql.unsafe( + "SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name = 'project_id' LIMIT 1", [tableName] as unknown as Parameters[1], ); + } + try { await sql.unsafe(`ALTER TABLE public.${quotedTable} ENABLE ROW LEVEL SECURITY`); if (cols.length > 0) { @@ -136,23 +145,20 @@ export async function loadIntoSupabase( await sql.unsafe( `CREATE POLICY "sow_owner" ON public.${quotedTable} FOR ALL USING (${quotedUserCol} = auth.uid())`, ); + } else if (hasProjId.length > 0) { + await sql.unsafe( + `CREATE POLICY "sow_project" ON public.${quotedTable} FOR ALL USING (project_id IN (SELECT id FROM public.projects WHERE user_id = auth.uid()))`, + ); } else { - const hasProjId = await sql.unsafe( - "SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name = 'project_id' LIMIT 1", - [tableName] as unknown as Parameters[1], + await sql.unsafe( + `CREATE POLICY "sow_open" ON public.${quotedTable} FOR ALL USING (auth.role() = 'authenticated')`, ); - - if (hasProjId.length > 0) { - await sql.unsafe( - `CREATE POLICY "sow_project" ON public.${quotedTable} FOR ALL USING (project_id IN (SELECT id FROM public.projects WHERE user_id = auth.uid()))`, - ); - } else { - await sql.unsafe( - `CREATE POLICY "sow_open" ON public.${quotedTable} FOR ALL USING (auth.role() = 'authenticated')`, - ); - } } } catch { + // Policy creation failed (e.g. the table has a conflicting existing + // policy, or ENABLE RLS failed). Disable RLS so the dev can still + // use the sandbox table. This is a dev-sandbox fallback, not a + // production concession — branches are sanitized and ephemeral. try { await sql.unsafe(`ALTER TABLE public.${quotedTable} DISABLE ROW LEVEL SECURITY`); } catch { /* ignore */ } diff --git a/packages/core/src/sampler/referential.test.ts b/packages/core/src/sampler/referential.test.ts index b5121ea..ad2cbef 100644 --- a/packages/core/src/sampler/referential.test.ts +++ b/packages/core/src/sampler/referential.test.ts @@ -1,7 +1,5 @@ -import { - ensureReferentialIntegrity, - quoteIdent, -} from "./referential.js"; +import { ensureReferentialIntegrity } from "./referential.js"; +import { quoteIdent } from "../sql/identifiers.js"; import type { DatabaseAdapter, Relationship, @@ -41,8 +39,12 @@ describe("quoteIdent", () => { ); }); - it("handles empty string", () => { - expect(quoteIdent("")).toBe('""'); + it("throws on empty string (would produce an invalid identifier)", () => { + expect(() => quoteIdent("")).toThrow(/cannot be empty/); + }); + + it("throws on NUL byte (libpq rejects these at a distant layer)", () => { + expect(() => quoteIdent("users\0evil")).toThrow(/NUL byte/); }); }); diff --git a/packages/core/src/sampler/referential.ts b/packages/core/src/sampler/referential.ts index c26371a..253deb4 100644 --- a/packages/core/src/sampler/referential.ts +++ b/packages/core/src/sampler/referential.ts @@ -1,9 +1,6 @@ import type { DatabaseAdapter, Relationship } from "../types.js"; import { quoteIdent } from "../sql/identifiers.js"; -// Re-export so existing test imports keep working. -export { quoteIdent }; - const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; // Columns already handled by formal FKs or that shouldn't be followed diff --git a/packages/core/src/sql/identifiers.ts b/packages/core/src/sql/identifiers.ts index e2cc85e..16f6b1c 100644 --- a/packages/core/src/sql/identifiers.ts +++ b/packages/core/src/sql/identifiers.ts @@ -19,5 +19,14 @@ * quoteIdent('x"; DROP x; --') // => '"x""; DROP x; --"' (inert) */ export function quoteIdent(name: string): string { + if (name.length === 0) { + throw new Error("quoteIdent: identifier cannot be empty"); + } + // NUL is explicitly forbidden in Postgres identifiers and can cause + // protocol-level confusion. Fail loudly rather than let libpq reject it + // at a point far from the call site. + if (name.includes("\0")) { + throw new Error("quoteIdent: identifier cannot contain NUL byte"); + } return '"' + name.replace(/"/g, '""') + '"'; } From 08eebb1a2658d0b70ef6866b948ecf88351cb831 Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 6 Apr 2026 00:41:52 -0700 Subject: [PATCH 4/4] =?UTF-8?q?fix(core):=20codex=20adversarial=20follow-u?= =?UTF-8?q?ps=20=E2=80=94=20batching,=20UUID=20filter,=20per-table=20RLS?= =?UTF-8?q?=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second adversarial pass (Codex) surfaced three more issues. Addressed. 1. fetchAuthUserMappings: batch and UUID-filter before querying. - Batch size 1000 keeps us well under Postgres's 65,535 bind-param limit for full-copy snapshots with many auth users. - Filter userIds by UUID_RE before binding: the upstream heuristic in extractUserIds() only checks `length > 10`, so a non-UUID value reaching the IN clause would cause a cast error that the broad catch swallowed, silently dropping ALL mappings. Now we skip non-UUID ids before the query runs. - A single batch failure no longer nukes the whole mapping set — other batches still contribute their results. 2. supabase.ts RLS loop: per-table introspection scope. - The previous fix hoisted info_schema lookups outside the try to avoid the RLS-disable fallback firing on transient reads. But that made a catalog blip abort the entire loadIntoSupabase run. - New shape: two separate try blocks per table. (a) Introspection try: if this fails, `continue` to next table without touching RLS. Fail-safe: unconfigured tables remain locked, not disabled. (b) DDL try: if ENABLE RLS or CREATE POLICY fails, fall back to DISABLE RLS so the dev sandbox stays usable. Scope is now limited to actual DDL errors where the fallback is intended. - Transient errors no longer abort the whole load. - Security-regression vector (disable RLS on read error) is closed. 3. getBranchSample: allow limit=0. - Previous clamp was [1, 100], which silently bumped a caller's intentional `LIMIT 0` to 1 row (a behavior regression — LIMIT 0 is valid SQL and a legitimate probe). Clamp is now [0, 100]. Non-finite inputs still fall back to the documented default of 5. Tests: 89/89 passing. Build clean. Lint clean. Known non-blocking (documented in PR body): pre-existing `keyStr.split("|")` and `fkValue.includes("null")` fragility in the formal-FK path of referential.ts. Not introduced by this PR; tracked for a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/branching/manager.ts | 7 ++- .../core/src/branching/providers/supabase.ts | 55 +++++++++++++------ packages/core/src/branching/supabase.ts | 51 ++++++++++------- 3 files changed, 74 insertions(+), 39 deletions(-) diff --git a/packages/core/src/branching/manager.ts b/packages/core/src/branching/manager.ts index e084ea6..2e4b707 100644 --- a/packages/core/src/branching/manager.ts +++ b/packages/core/src/branching/manager.ts @@ -474,12 +474,13 @@ export async function getBranchSample( // `table` comes from user/agent input via `sow branch sample
` // or the `sow_branch_sample` MCP tool — we cannot trust it. Identifiers // cannot be parameterized so we quote via the SQL-standard escape. The - // limit is numeric-clamped to [1, 100] then passed as $1. We preserve the - // documented default of 5 for non-finite inputs. + // limit is numeric-clamped to [0, 100] then passed as $1. A non-finite + // input (undefined, NaN) falls back to the documented default of 5. + // LIMIT 0 is legal SQL and a valid request (empty result set). const rawLimit = typeof limit === "number" && Number.isFinite(limit) ? Math.floor(limit) : 5; - const safeLimit = Math.min(Math.max(1, rawLimit), 100); + const safeLimit = Math.min(Math.max(0, rawLimit), 100); const rows = await sql.unsafe( `SELECT * FROM ${quoteIdent(table)} LIMIT $1`, [safeLimit] as unknown as Parameters[1], diff --git a/packages/core/src/branching/providers/supabase.ts b/packages/core/src/branching/providers/supabase.ts index 2ecf421..7be8d08 100644 --- a/packages/core/src/branching/providers/supabase.ts +++ b/packages/core/src/branching/providers/supabase.ts @@ -46,28 +46,49 @@ function extractUserIds(tables: { rows: Record[] }[]): string[] return Array.from(ids); } +const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +// Batch size keeps us well under Postgres's 65,535 bind-parameter limit +// and gives the query planner a reasonable prepared-statement shape. +const AUTH_FETCH_BATCH = 1000; + async function fetchAuthUserMappings( adapter: DatabaseAdapter, userIds: string[], ): Promise { - if (userIds.length === 0) return []; - try { - // Build $1,$2,... placeholders and bind userIds as parameters. The - // pre-fix code interpolated the ids inside single-quoted literals, - // which a source row with a quote or crafted payload could escape. - const placeholders = userIds.map((_, i) => `$${i + 1}`).join(","); - const rows = await adapter.query<{ id: string; email: string }>( - `SELECT id::text, email FROM auth.users WHERE id IN (${placeholders})`, - userIds, - ); - return rows.map((row) => ({ - id: row.id, - email: row.email, - sanitizedEmail: transformValue(row.email, "email") as string, - })); - } catch { - return []; + // Only query for strictly UUID-shaped ids. Non-UUID values in the + // upstream heuristic (`length > 10`) would otherwise cause the whole + // batch to fail on a Postgres cast error and silently return empty. + const validIds = userIds.filter((id) => UUID_RE.test(id)); + if (validIds.length === 0) return []; + + const mappings: AuthUserMapping[] = []; + + for (let offset = 0; offset < validIds.length; offset += AUTH_FETCH_BATCH) { + const batch = validIds.slice(offset, offset + AUTH_FETCH_BATCH); + // Build $1,$2,... placeholders and bind ids as parameters. The pre-fix + // code interpolated ids inside single-quoted literals, which a source + // row with a quote or crafted payload could escape. + const placeholders = batch.map((_, i) => `$${i + 1}`).join(","); + try { + const rows = await adapter.query<{ id: string; email: string }>( + `SELECT id::text, email FROM auth.users WHERE id IN (${placeholders})`, + batch, + ); + for (const row of rows) { + mappings.push({ + id: row.id, + email: row.email, + sanitizedEmail: transformValue(row.email, "email") as string, + }); + } + } catch { + // A single batch failure shouldn't nuke the others — continue. + } } + + return mappings; } export class SupabaseBranchProvider implements BranchProvider { diff --git a/packages/core/src/branching/supabase.ts b/packages/core/src/branching/supabase.ts index 75e75a4..bb8a2d2 100644 --- a/packages/core/src/branching/supabase.ts +++ b/packages/core/src/branching/supabase.ts @@ -118,29 +118,42 @@ export async function loadIntoSupabase( // control the sandbox, a catalog row containing an unusual character // (e.g. a double quote from a quoted DDL identifier in the source // schema) must not break out of our identifier quoting. - const quotedTable = quoteIdent(tableName); - - // Introspection queries run outside the try/catch below so that a - // transient read error cannot end up in the policy-disable fallback. - // Values are bound via $1; identifiers use quoteIdent for DDL. - const cols = await sql.unsafe( - "SELECT column_name FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name IN ('user_id', 'owner_id', 'created_by')", - [tableName] as unknown as Parameters[1], - ); - + let quotedTable: string; + let cols: { column_name: string }[]; let hasProjId: unknown[] = []; - if (cols.length === 0) { - hasProjId = await sql.unsafe( - "SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name = 'project_id' LIMIT 1", + + // Per-table introspection. A transient failure here (connection blip, + // catalog contention, quoteIdent rejecting a degenerate name) skips + // this ONE table without affecting the others. Critically, this does + // NOT disable RLS on failure — unconfigured tables remain locked + // (fail-safe) rather than falling open. Callers that need write + // access to a skipped table can retry via `sow branch reset`. + try { + quotedTable = quoteIdent(tableName); + cols = (await sql.unsafe( + "SELECT column_name FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name IN ('user_id', 'owner_id', 'created_by')", [tableName] as unknown as Parameters[1], - ); + )) as { column_name: string }[]; + + if (cols.length === 0) { + hasProjId = await sql.unsafe( + "SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' AND table_name = $1 AND column_name = 'project_id' LIMIT 1", + [tableName] as unknown as Parameters[1], + ); + } + } catch { + // Introspection failed for this table — skip without touching RLS. + // This is the fail-safe branch: if we can't read the shape, we + // don't trust ourselves to configure access, and leaving RLS in + // whatever state the restore put it in is safer than disabling. + continue; } try { await sql.unsafe(`ALTER TABLE public.${quotedTable} ENABLE ROW LEVEL SECURITY`); if (cols.length > 0) { - const userCol = cols[0].column_name as string; + const userCol = cols[0].column_name; const quotedUserCol = quoteIdent(userCol); await sql.unsafe( `CREATE POLICY "sow_owner" ON public.${quotedTable} FOR ALL USING (${quotedUserCol} = auth.uid())`, @@ -155,10 +168,10 @@ export async function loadIntoSupabase( ); } } catch { - // Policy creation failed (e.g. the table has a conflicting existing - // policy, or ENABLE RLS failed). Disable RLS so the dev can still - // use the sandbox table. This is a dev-sandbox fallback, not a - // production concession — branches are sanitized and ephemeral. + // Policy creation failed (e.g. conflicting existing policy). Disable + // RLS so the dev can still use the sandbox table. This is a + // dev-sandbox fallback, not a production concession — branches are + // sanitized and ephemeral. try { await sql.unsafe(`ALTER TABLE public.${quotedTable} DISABLE ROW LEVEL SECURITY`); } catch { /* ignore */ }