fix(core): parameterize SQL across sampler and branching layers

[faculopezscala]

Summary

Closes a SQL injection vulnerability in the database sampler and extends the same fix to six related sites in the branching layer.

The bug. sow connect runs a referential-integrity pass that builds SQL WHERE clauses by string-interpolating primary-key values from sampled source rows. Two concrete failure modes:

1. Benign: a text PK containing a single quote (e.g. "O'Brien") produced invalid SQL, 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 DB." A crafted PK value like '; DROP TABLE x; -- would execute against the source DB under whatever permissions the connection string has. Read-only in intent, not in SQL effect.

Scope expansion. A specialist review during /ship surfaced the same class of bug in several other files. Rather than ship a narrow fix that left five more injection sinks on main, the PR was expanded to close them all.

Commits (bisectable):

1. 63ef8d8 — Parameterize the sampler. DatabaseAdapter.query() accepted a params argument in its interface but silently dropped it (_params?: unknown[]) and called sql.unsafe(sqlStr) with no binding — the interface was a lie. Now it actually passes params through to postgres@3's sql.unsafe(query, parameters). Three dynamic-SQL call sites in sampler/referential.ts rewritten to use $1, $2, ... placeholders. 9 regression tests including the O'Brien single-quote crash.

2. e0a724d — Parameterize the branching layer.

   • Extract quoteIdent() to packages/core/src/sql/identifiers.ts as a shared module.
   • branching/manager.ts:getBranchSample — the table argument comes directly from sow branch sample <branch> <table> (user argv) AND the sow_branch_sample MCP tool (AI agent input). Now quoted via quoteIdent; limit bound via $1 after numeric clamping.
   • branching/providers/supabase.ts:fetchAuthUserMappings — the IN (...) clause was built by single-quote-concatenating user ids from sampled source data. Now uses $1, $2, ... placeholders.
   • branching/supabase.ts RLS setup — 8 sites interpolated tableName into DDL and information_schema queries. Values go through $1 binds; identifiers through quoteIdent.
   • branching/supabase.ts createTestAuthUsers — user.id and user.email were interpolated into DELETE and INSERT statements inside single-quoted literals. All bound now.

3. 11e0615 — Adversarial review follow-ups (Claude subagent).

   • quoteIdent throws on empty identifier and NUL byte.
   • Dropped the re-export of quoteIdent from referential.ts so tests import from the canonical module.
   • getBranchSample limit clamping uses an explicit Number.isFinite check so unset/NaN falls back to the documented default of 5 instead of silently returning zero rows.
   • branching/supabase.ts RLS introspection hoisted outside the DDL try block to prevent a transient read error from landing in the policy-disable fallback path (a real fail-open security vector).

4. 08eebb1 — Codex adversarial follow-ups.

   • fetchAuthUserMappings now batches ids (1000 per query) so large full-copy snapshots don't hit Postgres's 65,535 bind-param limit, AND filters by UUID regex before binding so one non-UUID value doesn't poison the whole batch via cast error. A single batch failure no longer nukes the other batches.
   • branching/supabase.ts RLS loop has a per-table introspection try block that continues on error (fail-safe: leaves RLS enabled) rather than aborting the whole loadIntoSupabase run. The DDL try block still has the dev-sandbox disable-RLS-on-policy-failure fallback.
   • getBranchSample clamp fixed to [0, 100] (was [1, 100] — LIMIT 0 is legal SQL and a valid caller intent).

Test Coverage

Tests: 79 → 89 (+10 new) — 7 for quoteIdent (including throw cases for empty + NUL byte), 3 regression tests for the sampler SQL shape + params.

CODE PATH COVERAGE
===========================
[M] packages/core/src/sql/identifiers.ts — quoteIdent
    ├── [★★★ TESTED] simple identifier
    ├── [★★★ TESTED] underscored column
    ├── [★★★ TESTED] embedded double quote escape
    ├── [★★★ TESTED] multiple embedded quotes
    ├── [★★★ TESTED] hostile payload defense
    ├── [★★★ TESTED] throws on empty string
    └── [★★★ TESTED] throws on NUL byte

[M] packages/core/src/sampler/referential.ts
    ├── [★★★ TESTED] single-col PK with single quote (O'Brien regression)
    ├── [★★★ TESTED] composite FK → $1 + $2 placeholders
    └── [★★★ TESTED] hostile payload stays in params, not in SQL text

[M] packages/core/src/adapters/postgres.ts — query() passthrough
    └── [GAP]        Direct unit test of the new params branch.
                     Covered indirectly by every sampler test via SpyAdapter.
                     Full integration lives in __integration__ (needs live PG).

[M] packages/core/src/branching/manager.ts:getBranchSample
    └── [GAP]        Function creates a live postgres client; unit-testing
                     requires refactor. Existing integration test covers it.

[M] packages/core/src/branching/providers/supabase.ts:fetchAuthUserMappings
    └── [GAP]        Module-private; tested indirectly via supabase integration.

[M] packages/core/src/branching/supabase.ts RLS + auth setup
    └── [GAP]        Uses raw postgres() client. Integration test in
                     __integration__ covers the end-to-end path.

─────────────────────────────────
COVERAGE: 9/13 tested ≈ 69% (above 60% minimum, below 80% target)
─────────────────────────────────

The four untested paths all share a common shape: they use quoteIdent/parameterized queries in the same pattern as the fully-tested sampler path, and quoteIdent itself is exhaustively tested.

Pre-Landing Review

Specialist review (4 specialists):

• Testing: 7 findings, 0 critical. Suggestions around SpyAdapter fragility and edge cases. Below the fix bar.
• Maintainability: 9 findings, 0 critical. DRY suggestions, moving quoteIdent to a shared module (done).
• Security: 6 CRITICAL findings — all addressed in commits e0a724d and 11e0615. This is why the PR expanded from 3 files to 7.
• Performance: 5 INFORMATIONAL findings, all net-neutral or net-positive. Parameterized queries enable plan caching.

PR Quality Score: 7.0/10

Adversarial Review

Two rounds of adversarial review from both Claude and Codex:

Round 1 — Claude adversarial subagent: 15 findings. Actionable items (4) addressed in commit 11e0615.

Round 2 — Codex adversarial + structured review: 5 findings. 3 actionable (batching, UUID filter, per-table scope) addressed in 08eebb1. 2 pre-existing issues documented below.

Codex structured review P1 gate: PASS. No remaining P1 issues.

Scope Drift

Scope Check: CLEAN (with deliberate expansion). The PR started as a narrow sampler fix and deliberately expanded to close the entire SQL injection category after the specialist review surfaced 5 more sites of the same class. Every file changed is directly related to the security fix.

Plan Completion

This PR implements Issue #2 of the 11-issue plan from /plan-eng-review. Issues #1 (template-DB reset), #3 (sanitizer warnings), #4 (JSONB), #5 (sow sandbox), #6 (env-patch), #7 (dynamic skip list), #8 (N+1 batching in sampler), #9 (fail-closed sanitization), #10 (Postgres type coverage), and #11 (release workflow refactor) ship as separate PRs per the plan's parallelization lanes.

Known Non-Blocking (documented, deferred)

These surfaced during adversarial review but are pre-existing and not introduced by this PR:

1. keyStr.split("|") fragility in the formal-FK path of sampler/referential.ts. A primary key value containing a literal | is silently mis-split and the row is dropped.
2. fkValue.includes("null") bug in the missing-parent detection. A legitimate key containing the substring "null" is falsely treated as a NULL FK.
3. String(parent[col] ?? "") NULL→"" coercion in the ensure-1-child-per-parent path. A NULL parent key becomes '' and can match empty-string children.
4. Pre-existing lint debt in sanitizer/rules.ts (regex escapes) and sanitizer/transformer.ts (unused import).

Test plan

• bunx vitest run — 89/89 tests passing (79 pre-existing + 10 new)
• bunx turbo build — all 3 packages build cleanly
• bunx eslint on every touched file — clean
• Post-merge: verify auto-published v0.1.14 artifact on npm
• Post-merge: run sow branch sample <branch> 'test"name' to manually verify the quoteIdent path

🤖 Generated with Claude Code