diff --git a/.changeset/rewriter-never-drops-ciphertext.md b/.changeset/rewriter-never-drops-ciphertext.md index b3577f8b4..66ad0f209 100644 --- a/.changeset/rewriter-never-drops-ciphertext.md +++ b/.changeset/rewriter-never-drops-ciphertext.md @@ -24,6 +24,26 @@ The sweep also refuses to rewrite a column the migration corpus already gives an encrypted type, so changing a column's encrypted domain no longer drops a column full of ciphertext. Skipped statements report why they were left alone. +The sweep is now fail-closed about the columns it does not recognise at all. +Previously a column missing from the corpus index was assumed to be plaintext +and rewritten; absence is not evidence, and the declaration can simply live in a +migration directory the sweep never reads — the wizard ships scanning three of +them and indexes each separately. Such a statement is now reported for review +rather than rewritten, so the ADD+DROP+RENAME no longer drops a column that the +migration corpus itself shows already holds ciphertext. That is a guarantee +about what the corpus says, not about the database: the sweep reasons entirely +from migration files, and a database that has drifted from its migration +history is outside what it can see. `stash encrypt cutover` is the sharpest +example — it renames columns directly in the database and never writes drizzle +SQL, so the corpus can still describe a column as plaintext after cutover has +made it ciphertext; the same is true of any change made by hand via psql or the +Supabase dashboard. If your migration history is squashed, the column's +`CREATE TABLE` lives outside the directory being swept, or the database has +simply drifted from what the migrations describe, you will see the statement +flagged instead of repaired: check the column's current type in the database +and either apply the rewrite by hand on an empty table, or use the staged +`stash encrypt` lifecycle. + An unreadable migration directory (`EACCES`) is reported rather than silently treated as empty, and the wizard's `Run the migration now?` prompt defaults to No whenever the sweep rewrote anything, flagged anything, or could not check a diff --git a/docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md b/docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md new file mode 100644 index 000000000..f44fa80f4 --- /dev/null +++ b/docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md @@ -0,0 +1,203 @@ +# A-2 — fail-closed inversion for the encrypted ALTER COLUMN rewrite + +**Date:** 2026-07-24 +**Addresses:** A-2 in `.work/2026-07-24-eql-v2-removal-verification.md` +**Base:** `05b7bf07` (A-1/A-3 tokenizer fix) +**Affects:** `packages/cli/src/commands/db/rewrite-migrations.ts`, `packages/wizard/src/lib/rewrite-migrations.ts` (the same file twice) + +## Problem + +`rewriteEncryptedAlterColumns` rewrites an in-place `ALTER COLUMN … SET DATA TYPE +` into an ADD + DROP + RENAME sequence. That sequence is +equivalent to DROP + ADD: it fixes the column's type but destroys its contents. + +The sweep decides whether to rewrite by consulting a corpus-wide index of columns +the migrations give an *encrypted* type (`indexEncryptedColumns`). A column found +there is skipped as `already-encrypted`, because dropping it would destroy +ciphertext with no plaintext left to backfill from. **Everything else is +rewritten** — including a column the corpus never mentions at all. + +That default is fail-open. A column absent from the index is not known to be +plaintext; it is simply unknown. Two reproduced triggers, both from the +verification doc: + +- **(a) Lone ALTER, no source declaration.** The corpus contains the ALTER and + nothing that declares the column. Rewritten today: 1 live `DROP COLUMN`. +- **(b) Cross-directory split.** The declaration lives in `drizzle/` and the + ALTER in `migrations/`. The index is built per directory, so the sweep of + `migrations/` never sees the declaration. Rewritten today: 1 live `DROP + COLUMN` — and the dropped column is already typed `eql_v3_text_eq`, i.e. + ciphertext. + +Trigger (b) is not contrived. `post-agent.ts:163` ships with +`DRIZZLE_OUT_DIRS = ['drizzle', 'migrations', 'src/db/migrations']`, so scanning +multiple directories with a per-directory index *is* the default configuration. + +## Decision + +Invert the default. Rewrite only a column the corpus positively declares and does +not give an encrypted type. Anything unknown is skipped and reported. + +Two alternatives were considered and rejected: + +- **Widen the index across directories** (build one corpus index in + `sweepMigrationDirs` and pass it into each per-directory rewrite). Fixes (b) + with a sharper `already-encrypted` message, but unioning unrelated migration + histories can over-detect *plaintext*, which is itself fail-open. Rejected: + fail-closed already makes (b) safe, and this trades a better message for a new + fail-open surface. +- **Require the declaration in the same file.** A column created in + `0001_init.sql` and altered in `0007_encrypt.sql` is the normal case, so this + would stop rewriting anything. + +## Mechanism + +The encrypted side is already precise: it matches against the known domain list +in `ENCRYPTED_DOMAIN`. So "plaintext" needs no type classification — it is the +residue. A column the corpus **declares** but that is not in the encrypted set is +plaintext. + +That turns "what type is this column?" (a SQL parse) into "does a declaration for +this column exist?" (a name match in a position the code already isolates). No +comma splitting, no paren-depth tracking, no quote state machine. + +`indexEncryptedColumns` becomes `indexColumnDeclarations`, one traversal +returning `{ encrypted, declared }`. `encrypted` is populated exactly as today — +the existing broad scan is deliberately over-detecting and must not regress. +`declared` is populated from two positions: + +1. **Inside a `CREATE TABLE` body**, already captured as group 3 of + `CREATE_TABLE_RE`: scan `"([^"]+)"\s+["a-z]` for declared names. +2. **`ALTER TABLE … ADD COLUMN "col" `**: generalise + `ADD_ENCRYPTED_COLUMN_RE` to accept any type with the same tail. + +`RENAME COLUMN "a" TO "b"` propagates `declared` the way it already propagates +`encrypted`. + +The `\s+["a-z]` tail is what makes the name match a declaration rather than a +mention. A type token always begins with a letter or a double quote, so +`"email" text` and `"email" "public"."eql_v3_text_search"` match, while these do +not — the name is followed by `)`, `,` or an operator: + +```sql +PRIMARY KEY ("id", "name") +FOREIGN KEY ("org_id") REFERENCES "orgs"("id") +CHECK ("age" > 0) +UNIQUE("email") +``` + +**The scan must stay anchored to those two positions.** A whole-file scan would +let `ALTER COLUMN "email" SET DATA TYPE …` match its own `"email" SET` and +declare the very column it is asking about — a self-fulfilling fail-open. + +**Known false positive, accepted.** In +`CONSTRAINT "users_email_unique" UNIQUE("email")` the constraint *name* is +followed by whitespace and a letter, so it registers as a declared column. It is +inert unless a constraint name exactly equals the name of a column that is also +encrypted and undeclared; drizzle's `__unique` convention makes that +contrived. Documented in the docstring rather than engineered around. + +**Corpus-wide, not migration-ordered.** As with the existing encrypted index, a +declaration in a *later* migration than the ALTER still counts. This is a +pre-existing property, not introduced here. + +### Decision order in the rewrite callback + +Unchanged first step, two steps after it: + +1. `isInsideCommentOrString(original, offset)` → return the match untouched. +2. In `encrypted` → `skip(…, 'already-encrypted')`. +3. Not in `declared` → `skip(…, 'source-unknown')`. +4. Otherwise rewrite. + +Step 2 must precede step 3 so a column that is both declared and encrypted — the +output of a previous sweep, which emits `ADD COLUMN ` plus +`RENAME TO ` — still reports `already-encrypted`, the more specific +and more actionable reason. + +`isInsideCommentOrString` is not touched: it is the subject of A-1/A-3, already +landed in the base commit. + +## Skip reason + +`SkipReason` gains `'source-unknown'`. `describeSkipReason` has no `default` arm, +so the compiler forces the new arm to be written. All three consumers +(`install.ts:660`, `eql/migration.ts:248`, `post-agent.ts:192`) render +`describeSkipReason(reason)` verbatim and need no edit. + +The text names both causes, because the user's remedy differs: + +> the sweep could not find where this column was declared in this migration +> directory, so it cannot tell a plaintext column (safe to rewrite) from one that +> already holds ciphertext (where the rewrite would DROP it). Usually the +> column's `CREATE TABLE` / `ADD COLUMN` lives in a different directory, or the +> migration history was squashed. Check the column's current type in the +> database: if it is plaintext and the table is empty, apply the +> ADD/DROP/RENAME by hand; if it already holds ciphertext, use the staged +> `stash encrypt` lifecycle instead + +## Blast radius + +This is a behaviour change for correct corpora, not only for the bug. Any project +whose swept directory does not contain the column's declaration — squashed +migrations, a baseline pulled with `drizzle-kit pull`, a schema bootstrapped from +a hand-written SQL file — stops being rewritten and starts being flagged for +review. That is the trade being bought, and it is the correct direction: the cost +of a false skip is a statement the user fixes by hand, and the cost of a false +rewrite is irrecoverable data. + +## Testing + +Most existing tests in both files use a bare ALTER with no `CREATE` anywhere and +assert a rewrite; under the inversion they would all become `source-unknown`. +Rather than rewrite each fixture, add a helper that writes a `0000_init.sql` +declaring the plaintext source column. It sorts first and is never rewritten, so +`rewritten`/`skipped` assertions stay as they are and the diff is one line per +test. + +New cases, in **both** test files: + +- Lone ALTER, no declaration anywhere → `skipped` with `source-unknown`, file + unchanged on disk, zero `DROP COLUMN`. +- Declaration in a sibling file in the same directory → rewritten (the + non-regression case). +- Declaration living in the `options.skip` file → still counts as declared. +- `RENAME COLUMN` propagates declared-ness from the original name. +- A column named only inside `PRIMARY KEY (…)` / `REFERENCES "t"("col")` → + **not** declared, so `source-unknown`. +- A column that is both declared and encrypted → still `already-encrypted`, not + `source-unknown` (pins the ordering of steps 2 and 3). + +Wizard file only: + +- The cross-directory split from the verification doc, driven through + `sweepMigrationDirs`: encrypted declaration in `drizzle/`, ALTER in + `migrations/` → `source-unknown`, zero `DROP COLUMN` in the output. + +## Dual-copy sync + +The two files are the same file twice, differing only in the wizard's +`sweepMigrationDirs` / `DirRewriteResult`, its attribution string +(`@cipherstash/wizard` vs `stash`), and a few comment blocks. Apply one fix +twice, then diff the two whitespace-normalised files to confirm nothing new +diverged. + +## Docs and changeset + +- `skills/stash-cli/SKILL.md:393` and `skills/stash-drizzle/SKILL.md:64` both + state that each such statement is rewritten. Both need the qualifier that the + rewrite now requires the column's declaration to be present in the swept + directory, and otherwise flags the statement for review. +- Extend `.changeset/rewriter-never-drops-ciphertext.md` rather than adding a + second changeset: it already ships the sibling `already-encrypted` behaviour + for the same component in the same release, and two changesets would describe + overlapping behaviour. Both `stash` and `@cipherstash/wizard` are already + listed. + +## Out of scope + +- Any change to `isInsideCommentOrString` (A-1/A-3, landed in the base commit). +- Sharing a corpus index across directories in `sweepMigrationDirs` (rejected + above). +- The near-miss scan (`NEAR_MISS_RE`) and its `unrecognised-form` reason, which + are unaffected. diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 4f449965e..275ee5852 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -15,7 +15,28 @@ describe('rewriteEncryptedAlterColumns', () => { fs.rmSync(tmpDir, { recursive: true, force: true }) }) + /** + * Declare `columns` on `tableRef` as PLAINTEXT, in a migration that sorts + * before every fixture below. + * + * The sweep is fail-closed: it rewrites a column only when the corpus shows + * the column exists and is not already encrypted. A fixture that is just an + * ALTER declares nothing, so it is `source-unknown` by design — a test that + * exercises the REWRITE has to supply the `CREATE TABLE` a real drizzle + * corpus would carry. + * + * `tableRef` is written exactly as it appears in the ALTER, so a pgSchema() + * table passes `'"app"."users"'` and the declaration lands on the same key. + */ + const declarePlaintext = (tableRef: string, ...columns: string[]): void => { + const file = path.join(tmpDir, '0000_declare.sql') + const existing = fs.existsSync(file) ? fs.readFileSync(file, 'utf-8') : '' + const defs = columns.map((column) => `"${column}" text`).join(', ') + fs.writeFileSync(file, `${existing}CREATE TABLE ${tableRef} (${defs});\n`) + } + it('rewrites an in-place ALTER COLUMN with the bare type name', async () => { + declarePlaintext('"transactions"', 'amount') const original = `ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE eql_v2_encrypted;\n` const filePath = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(filePath, original) @@ -37,6 +58,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the schema-qualified form produced by drizzle-kit', async () => { + declarePlaintext('"users"', 'email') const original = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v2_encrypted";\n' const filePath = path.join(tmpDir, '0003_alter.sql') @@ -54,6 +76,7 @@ describe('rewriteEncryptedAlterColumns', () => { it('rewrites a schema-qualified table produced by pgSchema()', async () => { // drizzle-kit emits `"app"."users"` for a table declared in a pgSchema(); // the old `\s+` between the table and ALTER COLUMN could never cross the `.`. + declarePlaintext('"app"."users"', 'email') const original = 'ALTER TABLE "app"."users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' const filePath = path.join(tmpDir, '0014_qualified.sql') @@ -75,6 +98,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the "undefined" schema form drizzle-kit emits for bare custom types', async () => { + declarePlaintext('"transactions"', 'amount') const original = 'ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE "undefined"."eql_v2_encrypted";\n' const filePath = path.join(tmpDir, '0005_undef.sql') @@ -90,6 +114,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the double-quoted form produced by stack 0.15.0', async () => { + declarePlaintext('"transactions"', 'description') const original = 'ALTER TABLE "transactions" ALTER COLUMN "description" SET DATA TYPE "undefined".""public"."eql_v2_encrypted"";\n' const filePath = path.join(tmpDir, '0006_double.sql') @@ -117,6 +142,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('skips the file passed in options.skip', async () => { + declarePlaintext('"t"', 'c') const install = path.join(tmpDir, '0000_install-eql.sql') const alter = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(install, 'CREATE SCHEMA eql_v2;\n') @@ -169,6 +195,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(V3_DOMAINS)('rewrites an ALTER COLUMN to %s', async (domain) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0007_v3.sql') fs.writeFileSync( filePath, @@ -196,6 +223,7 @@ describe('rewriteEncryptedAlterColumns', () => { ...V3_DOMAINS, 'eql_v2_encrypted', ])('extracts the bare domain %s from a mangled ALTER', async (domain) => { + declarePlaintext('"t"', 'c') const filePath = path.join(tmpDir, '0015_drift.sql') fs.writeFileSync( filePath, @@ -243,6 +271,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(MANGLED_FORMS)('rewrites the v3 %s form', async (_label, emitted) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0008_form.sql') fs.writeFileSync( filePath, @@ -265,6 +294,7 @@ describe('rewriteEncryptedAlterColumns', () => { '"undefined"."public.eql_v2_encrypted"', ], ])('rewrites the previously unmatched v2 %s form', async (_label, emitted) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0009_v2form.sql') fs.writeFileSync( filePath, @@ -281,6 +311,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('names the target domain in the guidance comment', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0010_comment.sql') fs.writeFileSync( filePath, @@ -295,6 +326,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('notes that constraints/defaults/indexes are not carried over', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0016_constraints.sql') fs.writeFileSync( filePath, @@ -309,6 +341,7 @@ describe('rewriteEncryptedAlterColumns', () => { it('does not terminate the commented UPDATE placeholder with a semicolon', async () => { // A runner that naively splits on `;` must not cut mid-comment. + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0017_semicolon.sql') fs.writeFileSync( filePath, @@ -328,6 +361,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('separates ADD/DROP/RENAME with --> statement-breakpoint, one exec stmt per chunk', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0018_breakpoint.sql') fs.writeFileSync( filePath, @@ -353,6 +387,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites each statement to its own domain when v2 and v3 are mixed', async () => { + declarePlaintext('"a"', 'x', 'y') const filePath = path.join(tmpDir, '0011_mixed.sql') fs.writeFileSync( filePath, @@ -449,6 +484,92 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].statement).toBe(statement) }) + // A statement the STRICT matcher already matched but skipped (here: + // source-unknown) is left on disk unchanged, so it still contains + // `SET DATA TYPE` and the broad near-miss scan finds it again. Before the + // preamble regex stripped a leading block comment, that second pass reported + // a DIFFERENT statement string (comment glued to the front) than the strict + // pass's `match.trim()`, so the dedup key never matched and the same + // statement came back twice: once correctly as `source-unknown`, once as + // `unrecognised-form` — contradictory advice (look for a hand-authored + // `USING` clause) for a statement that has none. + // + // The block comment must NOT sit at the very start of the file — that is + // the one shape a previous, narrower version of the preamble regex happened + // to handle. A realistic migration file has a preceding statement, so + // NEAR_MISS_RE's match starts at THAT statement's `;`, dragging the newline + // before the comment in too; a regex that only strips a comment anchored to + // the very start of the match fails here. + it('reports a block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0040_block-preamble.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + '/* note */', + alterSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // Same bug, but the comment sits on the ALTER's own line rather than its + // own — the preceding statement's `;` still starts the near-miss match + // before the (indented) comment, not at it. + it('reports an indented block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0041_block-preamble-indented.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + ` /* note */ ${alterSql}`, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // Same bug again, this time on the OTHER correct reason a near-miss can + // carry: the column is already encrypted, not merely undeclared. + it('reports a block-comment-prefixed statement once, not twice, for an already-encrypted column', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0042_block-preamble-encrypted.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" "public"."eql_v3_text_eq");', + '/* note */', + alterSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + it('reports a near-miss without a preceding statement-breakpoint marker', async () => { const statement = 'ALTER TABLE "users" ALTER COLUMN "meta" SET DATA TYPE eql_v3_json USING (meta)::jsonb;' @@ -495,6 +616,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('reports no skipped statements when the strict rewrite fully handled the file', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0021_handled.sql') fs.writeFileSync( filePath, @@ -556,9 +678,10 @@ describe('rewriteEncryptedAlterColumns', () => { const filePath = path.join(tmpDir, '0030_nested.sql') fs.writeFileSync(filePath, original) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) }) @@ -577,6 +700,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The comment scan must not be fooled by `--` inside a string literal, or // it would skip a live statement and leave broken SQL to fail at migrate. it('still rewrites an ALTER that follows a "--" inside a string literal', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0030_literal.sql') fs.writeFileSync( filePath, @@ -793,6 +917,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The scoping matters: encrypting `contacts.email` must not be blocked by // an unrelated `users.email` that happens to share a column name. it('scopes the check to the table', async () => { + declarePlaintext('"contacts"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), 'ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -830,6 +955,7 @@ describe('rewriteEncryptedAlterColumns', () => { // A commented-out ADD never ran, so it says nothing about the live schema. it('ignores an encrypted ADD COLUMN that is commented out', async () => { + declarePlaintext('"users"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), '-- ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -868,9 +994,73 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(skipped[0]?.reason).toBe('already-encrypted') }) + + // Ordering pin: this column is BOTH declared plaintext (0000) and made + // encrypted by a previous sweep (0001). `already-encrypted` is the more + // specific reason and must win over `source-unknown`. + it('reports already-encrypted even when the column was also declared plaintext', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_swept.sql'), + [ + 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_eq";', + '--> statement-breakpoint', + 'ALTER TABLE "users" DROP COLUMN "email";', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + + // A commented-out encrypted column line inside an otherwise LIVE + // CREATE TABLE must still count as encrypted. The comment check applies + // only to the DECLARED scan (over-detecting plaintext costs data); the + // ENCRYPTED scan never re-checks comments inside a live CREATE TABLE, so + // this stays over-detecting — the safe direction — exactly as it was + // before the corpus learned to track declarations at all. + it('still counts a commented-out encrypted column inside a live CREATE TABLE as encrypted', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer,', + '\t-- "email" "public"."eql_v3_text_eq",', + '\t"email" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) }) it('handles multiple ALTER statements in one file', async () => { + declarePlaintext('"a"', 'x', 'y') const original = [ 'ALTER TABLE "a" ALTER COLUMN "x" SET DATA TYPE eql_v2_encrypted;', 'ALTER TABLE "a" ALTER COLUMN "y" SET DATA TYPE eql_v2_encrypted;', @@ -891,6 +1081,7 @@ describe('rewriteEncryptedAlterColumns', () => { // Regression pin, not a bug fix — the matchers carry `/gi`, so a // hand-lowercased migration is rewritten just like drizzle-kit's output. it('rewrites a lowercase alter table ... set data type', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0034_lowercase.sql') fs.writeFileSync( filePath, @@ -908,4 +1099,235 @@ describe('rewriteEncryptedAlterColumns', () => { expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') expect(updated).not.toMatch(/set data type/i) }) + + // A-2: the sweep is FAIL-CLOSED. A column the corpus never declares is + // UNKNOWN, not plaintext — it may already hold ciphertext, with its + // declaration sitting in a migration directory this sweep never sees. + describe('columns the corpus does not declare', () => { + it('refuses to rewrite a column the corpus never declares', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + it('rewrites a column declared plaintext by an ADD COLUMN', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_add.sql'), + 'ALTER TABLE "users" ADD COLUMN "email" text;\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // The skipped file is still part of the corpus: a column's current type + // comes from the migrations that ran before this one, edit-eligible or not. + it('counts a declaration living in the file passed to options.skip', async () => { + const install = path.join(tmpDir, '0000_install-eql.sql') + fs.writeFileSync( + install, + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + const alter = path.join(tmpDir, '0002_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns( + tmpDir, + { + skip: install, + }, + ) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + it('follows a RENAME when deciding a column is declared', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email_address" text);', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email_address" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // A name inside a table/key constraint is a MENTION, not a declaration. + // Here every mention is followed by `)` or `,`, which the declaration + // regex's tail alone already rejects. A mention followed by a SQL keyword + // instead (e.g. `CHECK ("email" IS NOT NULL)`) is a separate case, closed + // by the regex's keyword lookahead and pinned by its own tests below. + // Counting either would put the rewrite back on the fail-open path. + it('does not treat a name inside PRIMARY KEY / REFERENCES as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "sessions" (', + '\t"id" integer,', + '\t"user_id" integer,', + '\tPRIMARY KEY ("id", "email"),', + '\tFOREIGN KEY ("user_id") REFERENCES "users"("email")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "sessions" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A column line commented out INSIDE a live CREATE TABLE never ran, so it + // declares nothing. + it('does not count a column commented out inside a live CREATE TABLE', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\t-- "email" text,', + '\t"name" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('source-unknown') + }) + + // A CHECK predicate mentioning a column has the same `"name" ` + // shape as a declaration — `"email" IS` — but IS is a predicate keyword, + // not a type token. Without the keyword lookahead this would read as a + // declaration and rewrite the column, dropping any ciphertext it already + // holds via a declaration this sweep never sees. + it('does not treat a CHECK predicate mention as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "c1" CHECK ("email" IS NOT NULL)', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A constraint's NAME can coincide with a column's name — drizzle's + // `
__unique` convention makes this contrived, but the keyword + // lookahead closes it regardless: a constraint name is always followed + // immediately by its constraint-type keyword (UNIQUE here), which the + // lookahead excludes. + it('does not let a same-named constraint declare the column', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "email" UNIQUE("id")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // The keyword lookahead is pinned to a word boundary specifically so it + // does not eat real type names that merely START WITH a blocked + // keyword's letters: `interval`/`inet` both start "in" (colliding with + // IN) but must still count as genuine declarations. + it('still declares a column whose type name starts with a blocked keyword', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "events" ("email" interval, "note" inet);\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + [ + 'ALTER TABLE "events" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + 'ALTER TABLE "events" ALTER COLUMN "note" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + }) }) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 550cb282a..cf3d0e0be 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -96,20 +96,52 @@ const NEAR_MISS_RE = /[^;]*?\bSET\s+DATA\s+TYPE\b[^;]*?\beql_v[23][a-z0-9_]*[^;]*?;/gi /** - * Blank lines and `--` line comments (including drizzle-kit's - * `--> statement-breakpoint`) at the head of a {@link NEAR_MISS_RE} match. + * Strips any run of blank lines, `--` line comments (including drizzle-kit's + * `--> statement-breakpoint`), and `/* … *\/` block comments — in any order, + * repeated as many times as they occur — from the head of a + * {@link NEAR_MISS_RE} match. * * That regex opens with a lazy `[^;]*?`, whose only left boundary is the * previous `;` — or the start of the file when there is no preceding statement. * So the raw match drags in every comment and blank line since then, and a - * near-miss in a file that opens with a comment block gets reported to the user - * with that whole block glued to its front. Strip the preamble so the statement - * we quote back reads as the offending statement alone. + * near-miss preceded by a comment block gets reported to the user with that + * whole block glued to its front. Strip the preamble so the statement we quote + * back reads as the offending statement alone. * - * Only line comments are handled — `/* … *\/` block comments are not something - * drizzle-kit emits, and a stray one costs cosmetics, not correctness. + * The block comment is matched as its OWN loop alternative rather than a + * single group anchored ahead of the line-comment loop, because the latter + * only works when the block comment sits at the very start of the file: in + * the far more common case — a preceding statement earlier in the same file — + * the match starts at THAT statement's `;`, so it opens with the newline + * after it, not with the comment. An anchored `^(?:/\*…\*\/\s*)?` can't match + * past that newline to reach the comment, so it silently matches nothing and + * leaves the comment attached to the reported statement. Folding the block + * comment into the repeating loop lets it match after any number of leading + * newlines/line-comments, in any interleaving. + * + * The block comment strip is NOT cosmetic: a statement the strict + * {@link ALTER_COLUMN_TO_ENCRYPTED_RE} matched but skipped (`already-encrypted` + * or `source-unknown`) is left on disk unchanged, so it still contains + * `SET DATA TYPE` and the broad scan below finds it again. Without stripping + * the block comment here, that second pass reports a DIFFERENT statement string + * (comment glued to the front) than the strict pass's `match.trim()`, so + * {@link rewriteEncryptedAlterColumns}'s dedup key never matches and the same + * statement is reported twice — once with the correct reason, once as + * `unrecognised-form`, whose guidance (look for a hand-authored `USING`) is + * wrong for a statement the strict matcher already matched. Stripping the + * comment here makes both passes agree on the statement text so the second + * report collapses into the first. + * + * Known residue, accepted rather than fixed: a NESTED closed block comment + * ahead of a live ALTER (`/* outer /* inner *\/ still *\/`) still + * double-reports. The block-comment alternative's `*?` is lazy, so it stops + * at the FIRST `*\/` — consuming only `/* outer /* inner *\/` — and leaves + * `` still *\/`` glued to the front of the next iteration, which the + * line-comment alternative doesn't recognise either. That residual text rides + * along into the `unrecognised-form` report. */ -const STATEMENT_PREAMBLE_RE = /^(?:[^\S\n]*(?:--[^\n]*)?\n)+/ +const STATEMENT_PREAMBLE_RE = + /^(?:\s*\/\*[\s\S]*?\*\/|[^\S\n]*(?:--[^\n]*)?\n)*/ /** Drop the leading blank/comment lines a `[^;]*?`-anchored match dragged in. */ function trimStatementPreamble(statement: string): string { @@ -251,6 +283,91 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( 'gi', ) +/** + * A column name in a DECLARATION position — `"email" text`, + * `"email" "public"."eql_v3_text_search"`, `"amount" numeric(10, 2)`. + * + * The `\s+["a-z]` tail is what separates a declaration from a MENTION. Most + * mentions are followed by `)`, `,`, or an operator, which the tail alone + * already rejects: + * + * ```sql + * PRIMARY KEY ("id", "name") + * FOREIGN KEY ("org_id") REFERENCES "orgs"("id") + * ``` + * + * But a mention inside a predicate or a table-level constraint is often + * followed by whitespace and a SQL KEYWORD — which has exactly the same + * `\s+[a-z]` shape as a real declaration, and drizzle-kit emits both forms + * inside a `CREATE TABLE` (a `CONSTRAINT … CHECK ()` body is + * user-authored, so the predicate form is reachable too): + * + * ```sql + * CHECK ("email" IS NOT NULL) + * CONSTRAINT "users_email_unique" UNIQUE ("email") + * ``` + * + * The negative lookahead rejects the keywords reachable this way: predicate + * keywords (`IS`, `IN`, `NOT`, `LIKE`, `ILIKE`, `BETWEEN`, `AND`, `OR`), + * ordering/collation (`COLLATE`, `ASC`, `DESC`, `NULLS`), and every + * table/column-constraint keyword (`UNIQUE`, `PRIMARY`, `FOREIGN`, `CHECK`, + * `EXCLUDE`, `REFERENCES`, `CONSTRAINT`, `USING`, `WITH`) — the last six also + * close the constraint-NAME case above, since Postgres always puts one of + * them immediately after a constraint's name. + * + * A real type token still matches because the lookahead is pinned to a word + * boundary (`\b`) right after each keyword, so it only rejects a keyword when + * that keyword is the WHOLE next word: `interval` and `inet` both start with + * `IN`'s letters, but their third character keeps them inside one word, so + * `\bIN\b` never matches there; the same reasoning clears `int`, + * `char`/`citext` against `CHECK`, and `eql_v2_encrypted`/`eql_v3_*` against + * `EXCLUDE`. + * + * Deliberately NOT pinned to the encrypted domains. A column the corpus + * declares but does not give an encrypted type is, by residue, plaintext — so + * the fail-closed rule needs no type classification and no SQL parsing, only + * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. + * + * **The residue claim depends on {@link MANGLED_TYPE_FORMS} covering every + * encrypted shape.** "By residue, plaintext" is only as good as the encrypted + * side's coverage: a declaration {@link ENCRYPTED_TYPE_REF} fails to recognise + * falls to the plaintext residue and gets rewritten. Two forms do this: a + * domain installed into a non-`public` schema (`"email" "app"."eql_v3_text_search"`, + * since the mangled forms only special-case the literal `public` schema), and + * an array of the domain (`ADD COLUMN "email" public.eql_v3_text_search[]`, + * since {@link ENCRYPTED_TYPE_REF}'s trailing delimiter lookahead does not + * include `[`). Neither is a layout EQL installs into or a shape drizzle-kit + * emits, and both behaved identically before this branch — so this is a + * documentation gap, not a regression this branch introduced. + * + * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: + * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, + * `OVERLAPS`, `ON`, …) still lets a bare mention through, e.g. + * `CHECK ("email" SIMILAR TO '...')`, or a `REFERENCES "email" ON DELETE + * CASCADE` where `"email"` names the referenced TABLE rather than the column + * being declared — `"id" integer REFERENCES "email" ON DELETE CASCADE` inside + * a `CREATE TABLE` body still registers `email` as declared. That one only + * bites when a column shares a referenced table's name, so like the rest of + * this residue it is inert unless that mention's name exactly matches a + * DIFFERENT column that is both encrypted and genuinely undeclared anywhere + * else in the corpus — contrived, and strictly narrower than the gap this + * replaces. + */ +const DECLARED_COLUMN_RE = + /"([^"]+)"\s+(?!(?:IS|IN|NOT|LIKE|ILIKE|BETWEEN|AND|OR|COLLATE|ASC|DESC|NULLS|UNIQUE|PRIMARY|FOREIGN|CHECK|EXCLUDE|REFERENCES|CONSTRAINT|USING|WITH)\b)["a-z]/gi + +/** + * `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. + * + * Needs no {@link DECLARED_COLUMN_RE}-style keyword lookahead: the token + * right after an ADD COLUMN's column name is always its type, so no SQL + * keyword can occupy that position. + */ +const ADD_COLUMN_RE = new RegExp( + String.raw`ALTER TABLE\s+${TABLE_REF}\s+ADD COLUMN\s+(?:IF NOT EXISTS\s+)?"([^"]+)"\s+["a-z]`, + 'gi', +) + /** Splits a `TABLE_REF` capture pair into its schema and table halves. */ function tableOf( first: string, @@ -263,42 +380,80 @@ function tableOf( : { schema: first, table: second } } -/** Identity of a column across the corpus, for {@link indexEncryptedColumns}. */ +/** Identity of a column across the corpus, for {@link indexColumnDeclarations}. */ function columnKey(table: string, column: string, schema?: string): string { return JSON.stringify([schema ?? '', table, column]) } +/** What the migration corpus says about the columns it mentions. */ +interface ColumnIndex { + /** Columns the corpus gives an ENCRYPTED type. Rewriting one drops ciphertext. */ + encrypted: Set + /** Columns the corpus DECLARES at all, whatever type it gives them. */ + declared: Set +} + /** - * Index every column the migration corpus gives an ENCRYPTED type, so the - * rewrite can tell the change it exists for (plaintext → encrypted) from one it - * must never touch (encrypted → encrypted). + * Index what the migration corpus knows about each column, so the rewrite can + * tell the change it exists for (plaintext → encrypted) from the two it must + * never make: encrypted → encrypted, and a change to a column it knows nothing + * about. * - * **Why (#772 review, W-3):** the strict matcher captures only the TARGET type. - * A column whose encrypted domain merely changes (`types.TextEq` → + * **Why `encrypted` (#772 review, W-3):** the strict matcher captures only the + * TARGET type. A column whose encrypted domain merely changes (`types.TextEq` → * `types.TextSearch`) matches just as well as a plaintext column, and the * ADD+DROP+RENAME then drops a column full of CIPHERTEXT — with no plaintext * left anywhere to backfill from, so unlike the plaintext case the data is not - * recoverable from the application at all. Changing an encrypted column's domain - * changes its index terms, so the data has to be re-encrypted through the client - * regardless; the sweep flags the statement and leaves it for the user. + * recoverable from the application at all. + * + * **Why `declared` (A-2):** absence from `encrypted` is not evidence of + * plaintext. It is evidence of nothing. The sweep runs per directory, and the + * shipped wizard default scans three of them, so a column's `CREATE TABLE` can + * simply live somewhere this sweep never reads — the column is then rewritten + * on an assumption, and the ADD+DROP+RENAME drops live ciphertext. Requiring a + * POSITIVE declaration makes the unknown case a flagged statement instead. + * + * Because the encrypted side is matched against the known domain list, the + * plaintext side needs no type classification: a column the corpus declares but + * does not give an encrypted type is plaintext by residue. * * The index is corpus-wide rather than ordered by migration: over-detecting - * "encrypted" costs a flagged statement the user must handle by hand, while - * under-detecting costs irrecoverable ciphertext. Only comment-free statements - * count, for the same reason {@link isInsideCommentOrString} exists. + * "encrypted" costs a flagged statement the user handles by hand, while + * under-detecting costs irrecoverable ciphertext. That asymmetry is why the + * two per-column scans inside a `CREATE TABLE` body are deliberately + * inconsistent about comments (see the comment at the loops themselves) — + * do not "fix" that inconsistency, it is the point. */ -function indexEncryptedColumns(contents: readonly string[]): Set { +function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const encrypted = new Set() + const declared = new Set() for (const sql of contents) { for (const created of sql.matchAll(CREATE_TABLE_RE)) { if (isInsideCommentOrString(sql, created.index)) continue const { schema, table } = tableOf(created[1], created[2]) - for (const column of created[3].matchAll( - CREATE_TABLE_ENCRYPTED_COLUMN_RE, - )) { + const body = created[3] + + // The body is scanned as its own document: a `--` earlier in it comments + // out the rest of that line, and a CREATE inside a block comment or a + // string literal was already skipped above. + // + // Asymmetric on purpose. ENCRYPTED does NOT re-check comments here: a + // commented-out encrypted column inside an otherwise live CREATE TABLE + // still counts, exactly as it did before this sweep learned to declare + // anything — over-detecting "encrypted" only costs a flagged statement. + // DECLARED DOES re-check: a commented-out plaintext line never ran, so + // it must not count as a declaration — over-detecting "declared" is + // what would put a truly-undeclared, possibly already-encrypted column + // back on the fail-open path. + for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { encrypted.add(columnKey(table, column[1], schema)) } + + for (const column of body.matchAll(DECLARED_COLUMN_RE)) { + if (isInsideCommentOrString(body, column.index)) continue + declared.add(columnKey(table, column[1], schema)) + } } for (const added of sql.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { @@ -307,19 +462,26 @@ function indexEncryptedColumns(contents: readonly string[]): Set { encrypted.add(columnKey(table, added[3], schema)) } + for (const added of sql.matchAll(ADD_COLUMN_RE)) { + if (isInsideCommentOrString(sql, added.index)) continue + const { schema, table } = tableOf(added[1], added[2]) + declared.add(columnKey(table, added[3], schema)) + } + // A rename carries the column's type with it — and `__cipherstash_tmp` // renamed onto the real name is exactly what a previous sweep of this very // directory emitted. Run after ADD so that tmp column is already indexed. for (const renamed of sql.matchAll(RENAME_COLUMN_RE)) { if (isInsideCommentOrString(sql, renamed.index)) continue const { schema, table } = tableOf(renamed[1], renamed[2]) - if (encrypted.has(columnKey(table, renamed[3], schema))) { - encrypted.add(columnKey(table, renamed[4], schema)) - } + const from = columnKey(table, renamed[3], schema) + const to = columnKey(table, renamed[4], schema) + if (encrypted.has(from)) encrypted.add(to) + if (declared.has(from)) declared.add(to) } } - return encrypted + return { encrypted, declared } } /** Why a recognised ALTER-to-encrypted statement was left alone. */ @@ -328,6 +490,8 @@ export type SkipReason = | 'unrecognised-form' /** The column already holds an encrypted domain; rewriting drops ciphertext. */ | 'already-encrypted' + /** The corpus never declares the column, so its current type is unknown. */ + | 'source-unknown' /** A statement the sweep recognised as ALTER-to-encrypted but did NOT rewrite. */ export interface SkippedAlter { @@ -342,7 +506,7 @@ export interface SkippedAlter { /** * One-line explanation of a {@link SkipReason}, for the CLI/wizard to print * next to the statement. Lives here so every caller says the same thing — the - * two reasons need very different action from the user, and a single generic + * three reasons need very different action from the user, and a single generic * "could not rewrite automatically" hides that. */ export function describeSkipReason(reason: SkipReason): string { @@ -351,6 +515,8 @@ export function describeSkipReason(reason: SkipReason): string { return "the column is ALREADY encrypted, so the ADD+DROP+RENAME rewrite would DROP the ciphertext with no plaintext left to backfill from. Changing an encrypted column's domain changes its index terms, so the data must be re-encrypted through the staged `stash encrypt` lifecycle" case 'unrecognised-form': return 'it falls outside the strict matcher (a hand-authored `SET DATA TYPE ... USING ...`, or a drizzle-kit form the sweep does not recognise) and an in-place cast to an encrypted domain fails at migrate time' + case 'source-unknown': + return "the sweep could not find where this column was declared in this migration directory, so it cannot tell a plaintext column (safe to rewrite) from one that already holds ciphertext (where the rewrite would DROP it). Usually the column's `CREATE TABLE` / `ADD COLUMN` lives in a different directory, or the migration history was squashed. Check the column's current type in the database: if it is plaintext and the table is empty, apply the ADD/DROP/RENAME by hand; if it already holds ciphertext, use the staged `stash encrypt` lifecycle instead" } } @@ -386,13 +552,16 @@ export interface RewriteResult { * * Returns {@link RewriteResult}: the files rewritten, plus `skipped` statements * left for a human — ones outside the strict matcher (a hand-authored - * `SET DATA TYPE … USING …;`, or a future drizzle-kit form), and ones targeting - * a column that is ALREADY encrypted, where the rewrite would drop ciphertext. - * Both are left untouched on disk and surfaced non-fatally so the caller can - * tell the user to review them, rather than silently shipping broken SQL or - * destroying data. Statements sitting inside a SQL comment — or inside a - * single-quoted string literal, where they are data rather than SQL — are inert - * and are neither rewritten nor reported. + * `SET DATA TYPE … USING …;`, or a future drizzle-kit form); ones targeting a + * column that is ALREADY encrypted, where the rewrite would drop ciphertext; + * and ones targeting a column the corpus never DECLARES anywhere, where the + * rewrite would be guessing at a type it cannot see (likely the most common + * skip on a real corpus, since the sweep only ever reads part of the + * migration history). All three are left untouched on disk and surfaced + * non-fatally so the caller can tell the user to review them, rather than + * silently shipping broken SQL or destroying data. Statements sitting inside a + * SQL comment — or inside a single-quoted string literal, where they are data + * rather than SQL — are inert and are neither rewritten nor reported. */ export async function rewriteEncryptedAlterColumns( outDir: string, @@ -432,7 +601,8 @@ export async function rewriteEncryptedAlterColumns( // Built from the WHOLE corpus, including `options.skip`: a column's current // type comes from the migrations that ran before this one, not just the files // we are allowed to edit. - const encryptedColumns = indexEncryptedColumns([...contents.values()]) + const { encrypted: encryptedColumns, declared: declaredColumns } = + indexColumnDeclarations([...contents.values()]) for (const [filePath, original] of contents) { if (options.skip && filePath === options.skip) continue @@ -463,6 +633,17 @@ export async function rewriteEncryptedAlterColumns( return match } + // Fail closed. Absence from `declaredColumns` does not mean the column + // is plaintext — it means the corpus never said. The declaration can + // sit in a directory this sweep does not read (the wizard ships with + // three candidates and indexes each separately), so rewriting on the + // assumption is how a live `DROP COLUMN` reaches a populated — possibly + // already-encrypted — column. Flag it and let the user look. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() // Unreachable — the outer regex only matches when a domain is present — // but leave the statement alone rather than emit a broken rewrite. diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index ba3f8d7d1..9f0fa1f90 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -217,6 +217,14 @@ describe('eqlMigrationCommand — Drizzle', () => { it('rewrites a sibling migration with a broken v3 ALTER COLUMN', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) + // The sweep is fail-closed: it rewrites a column only when the corpus + // positively declares it (and it isn't already encrypted). A real drizzle + // corpus carries this declaration in an earlier migration — supply it so + // the fixture matches what the sweep actually requires. + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) const sibling = join(out, '0001_encrypt-email.sql') writeFileSync( sibling, @@ -239,10 +247,17 @@ describe('eqlMigrationCommand — Drizzle', () => { it('does not rewrite the EQL install migration it just generated', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) - const generated = join(out, '0000_install-eql.sql') + // Fail-closed requires the corpus to positively declare the column before + // the sweep will touch it — supply the declaration a real drizzle corpus + // would carry, same as the sibling-rewrite test above. + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const generated = join(out, '0001_install-eql.sql') // A sibling carrying the SAME statement — the differential that proves the // sweep ran at all, rather than no-opping over the whole directory. - const sibling = join(out, '0001_encrypt-email.sql') + const sibling = join(out, '0002_encrypt-email.sql') const unsafeAlter = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' writeFileSync(sibling, unsafeAlter) diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index c13f83cef..8e7656d9f 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -159,6 +159,15 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { it('defaults to No, and says why, when a file was rewritten', async () => { fs.mkdirSync(path.join(cwd, 'drizzle')) + // The sweep is fail-closed: it rewrites a column only when the corpus + // positively declares it (and it isn't already encrypted). A real drizzle + // corpus carries this declaration in an earlier migration — supply it so + // the fixture matches what the sweep actually requires, and the ALTER + // below is genuinely rewritten rather than skipped as source-unknown. + fs.writeFileSync( + path.join(cwd, 'drizzle', '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) fs.writeFileSync( path.join(cwd, 'drizzle', '0001_encrypt.sql'), 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', @@ -169,6 +178,20 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(false) expect(String(options?.message)).toContain('DESTROYS data') + + // Assert the REWRITE actually happened, not just that the prompt defaulted + // to No — both this test and its `source-unknown` sibling below produce + // `initialValue: false` and a message containing "DESTROYS data" is the + // only thing that used to distinguish them, and that came from the same + // skipped-statement branch too. Without the 0000_declare.sql fixture the + // ALTER is skipped as source-unknown rather than rewritten, so pin the + // on-disk effect a genuine rewrite leaves behind. + const swept = fs.readFileSync( + path.join(cwd, 'drizzle', '0001_encrypt.sql'), + 'utf-8', + ) + expect(swept).toContain('DROP COLUMN') + expect(swept).not.toContain('SET DATA TYPE') }) it('defaults to No when a statement was flagged rather than rewritten', async () => { @@ -182,6 +205,11 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(false) + // Nothing was rewritten here — the statement was left on disk and merely + // flagged — so the prompt must not claim data destruction the way the + // genuinely-rewritten case above does. + expect(String(options?.message)).not.toContain('DESTROYS data') + expect(String(options?.message)).toContain('flagged for review') }) // A directory whose sweep threw contributes 0 to both totals, so a failed diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index f5ef3ee45..01804dae6 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -18,7 +18,28 @@ describe('rewriteEncryptedAlterColumns', () => { fs.rmSync(tmpDir, { recursive: true, force: true }) }) + /** + * Declare `columns` on `tableRef` as PLAINTEXT, in a migration that sorts + * before every fixture below. + * + * The sweep is fail-closed: it rewrites a column only when the corpus shows + * the column exists and is not already encrypted. A fixture that is just an + * ALTER declares nothing, so it is `source-unknown` by design — a test that + * exercises the REWRITE has to supply the `CREATE TABLE` a real drizzle + * corpus would carry. + * + * `tableRef` is written exactly as it appears in the ALTER, so a pgSchema() + * table passes `'"app"."users"'` and the declaration lands on the same key. + */ + const declarePlaintext = (tableRef: string, ...columns: string[]): void => { + const file = path.join(tmpDir, '0000_declare.sql') + const existing = fs.existsSync(file) ? fs.readFileSync(file, 'utf-8') : '' + const defs = columns.map((column) => `"${column}" text`).join(', ') + fs.writeFileSync(file, `${existing}CREATE TABLE ${tableRef} (${defs});\n`) + } + it('rewrites an in-place ALTER COLUMN with the bare v2 type name', async () => { + declarePlaintext('"transactions"', 'amount') const original = `ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE eql_v2_encrypted;\n` const filePath = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(filePath, original) @@ -42,6 +63,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites a bare v3 domain (the generation the wizard now scaffolds)', async () => { + declarePlaintext('"users"', 'email') const original = `ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n` const filePath = path.join(tmpDir, '0002_v3.sql') fs.writeFileSync(filePath, original) @@ -57,6 +79,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the schema-qualified form produced by drizzle-kit', async () => { + declarePlaintext('"users"', 'email') const original = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v3_text_search";\n' const filePath = path.join(tmpDir, '0003_alter.sql') @@ -74,6 +97,7 @@ describe('rewriteEncryptedAlterColumns', () => { it('rewrites a schema-qualified table produced by pgSchema()', async () => { // drizzle-kit emits `"app"."users"` for a table declared in a pgSchema(); // the old `\s+` between the table and ALTER COLUMN could never cross the `.`. + declarePlaintext('"app"."users"', 'email') const original = 'ALTER TABLE "app"."users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' const filePath = path.join(tmpDir, '0014_qualified.sql') @@ -95,6 +119,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the "undefined" schema form drizzle-kit emits for bare custom types', async () => { + declarePlaintext('"transactions"', 'amount') const original = 'ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE "undefined"."eql_v2_encrypted";\n' const filePath = path.join(tmpDir, '0005_undef.sql') @@ -110,6 +135,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the double-quoted form produced by stack 0.15.0', async () => { + declarePlaintext('"transactions"', 'description') const original = 'ALTER TABLE "transactions" ALTER COLUMN "description" SET DATA TYPE "undefined".""public"."eql_v2_encrypted"";\n' const filePath = path.join(tmpDir, '0006_double.sql') @@ -137,6 +163,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('skips the file passed in options.skip', async () => { + declarePlaintext('"t"', 'c') const install = path.join(tmpDir, '0000_install-eql.sql') const alter = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(install, 'CREATE SCHEMA eql_v2;\n') @@ -190,6 +217,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(V3_DOMAINS)('rewrites an ALTER COLUMN to %s', async (domain) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0007_v3.sql') fs.writeFileSync( filePath, @@ -217,6 +245,7 @@ describe('rewriteEncryptedAlterColumns', () => { ...V3_DOMAINS, 'eql_v2_encrypted', ])('extracts the bare domain %s from a mangled ALTER', async (domain) => { + declarePlaintext('"t"', 'c') const filePath = path.join(tmpDir, '0015_drift.sql') fs.writeFileSync( filePath, @@ -255,6 +284,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(MANGLED_FORMS)('rewrites the v3 %s form', async (_label, emitted) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0008_form.sql') fs.writeFileSync( filePath, @@ -271,6 +301,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('names the target domain in the guidance comment', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0010_comment.sql') fs.writeFileSync( filePath, @@ -284,6 +315,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('warns that the rewrite is data-destroying / empty-table-only', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0016_warn.sql') fs.writeFileSync( filePath, @@ -299,6 +331,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('separates ADD/DROP/RENAME with --> statement-breakpoint', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0018_breakpoint.sql') fs.writeFileSync( filePath, @@ -316,6 +349,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites each statement to its own domain when v2 and v3 are mixed', async () => { + declarePlaintext('"a"', 'x', 'y') const filePath = path.join(tmpDir, '0011_mixed.sql') fs.writeFileSync( filePath, @@ -370,6 +404,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('reports no skipped statements when the strict rewrite fully handled the file', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0021_handled.sql') fs.writeFileSync( filePath, @@ -405,6 +440,92 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].statement).toBe(statement) }) + // A statement the STRICT matcher already matched but skipped (here: + // source-unknown) is left on disk unchanged, so it still contains + // `SET DATA TYPE` and the broad near-miss scan finds it again. Before the + // preamble regex stripped a leading block comment, that second pass reported + // a DIFFERENT statement string (comment glued to the front) than the strict + // pass's `match.trim()`, so the dedup key never matched and the same + // statement came back twice: once correctly as `source-unknown`, once as + // `unrecognised-form` — contradictory advice (look for a hand-authored + // `USING` clause) for a statement that has none. + // + // The block comment must NOT sit at the very start of the file — that is + // the one shape a previous, narrower version of the preamble regex happened + // to handle. A realistic migration file has a preceding statement, so + // NEAR_MISS_RE's match starts at THAT statement's `;`, dragging the newline + // before the comment in too; a regex that only strips a comment anchored to + // the very start of the match fails here. + it('reports a block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0040_block-preamble.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + '/* note */', + alterSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // Same bug, but the comment sits on the ALTER's own line rather than its + // own — the preceding statement's `;` still starts the near-miss match + // before the (indented) comment, not at it. + it('reports an indented block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0041_block-preamble-indented.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + ` /* note */ ${alterSql}`, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // Same bug again, this time on the OTHER correct reason a near-miss can + // carry: the column is already encrypted, not merely undeclared. + it('reports a block-comment-prefixed statement once, not twice, for an already-encrypted column', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0042_block-preamble-encrypted.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" "public"."eql_v3_text_eq");', + '/* note */', + alterSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + it('reports a near-miss without a preceding statement-breakpoint marker', async () => { const statement = 'ALTER TABLE "users" ALTER COLUMN "meta" SET DATA TYPE eql_v3_json USING (meta)::jsonb;' @@ -487,9 +608,10 @@ describe('rewriteEncryptedAlterColumns', () => { const filePath = path.join(tmpDir, '0030_nested.sql') fs.writeFileSync(filePath, original) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) }) @@ -508,6 +630,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The comment scan must not be fooled by `--` inside a string literal, or // it would skip a live statement and leave broken SQL to fail at migrate. it('still rewrites an ALTER that follows a "--" inside a string literal', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0030_literal.sql') fs.writeFileSync( filePath, @@ -724,6 +847,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The scoping matters: encrypting `contacts.email` must not be blocked by // an unrelated `users.email` that happens to share a column name. it('scopes the check to the table', async () => { + declarePlaintext('"contacts"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), 'ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -761,6 +885,7 @@ describe('rewriteEncryptedAlterColumns', () => { // A commented-out ADD never ran, so it says nothing about the live schema. it('ignores an encrypted ADD COLUMN that is commented out', async () => { + declarePlaintext('"users"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), '-- ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -799,9 +924,73 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(skipped[0]?.reason).toBe('already-encrypted') }) + + // Ordering pin: this column is BOTH declared plaintext (0000) and made + // encrypted by a previous sweep (0001). `already-encrypted` is the more + // specific reason and must win over `source-unknown`. + it('reports already-encrypted even when the column was also declared plaintext', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_swept.sql'), + [ + 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_eq";', + '--> statement-breakpoint', + 'ALTER TABLE "users" DROP COLUMN "email";', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + + // A commented-out encrypted column line inside an otherwise LIVE + // CREATE TABLE must still count as encrypted. The comment check applies + // only to the DECLARED scan (over-detecting plaintext costs data); the + // ENCRYPTED scan never re-checks comments inside a live CREATE TABLE, so + // this stays over-detecting — the safe direction — exactly as it was + // before the corpus learned to track declarations at all. + it('still counts a commented-out encrypted column inside a live CREATE TABLE as encrypted', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer,', + '\t-- "email" "public"."eql_v3_text_eq",', + '\t"email" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) }) it('handles multiple ALTER statements in one file', async () => { + declarePlaintext('"a"', 'x', 'y') const original = [ 'ALTER TABLE "a" ALTER COLUMN "x" SET DATA TYPE eql_v3_text_search;', 'ALTER TABLE "a" ALTER COLUMN "y" SET DATA TYPE eql_v3_text_search;', @@ -822,6 +1011,7 @@ describe('rewriteEncryptedAlterColumns', () => { // Regression pin, not a bug fix — the matchers carry `/gi`, so a // hand-lowercased migration is rewritten just like drizzle-kit's output. it('rewrites a lowercase alter table ... set data type', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0034_lowercase.sql') fs.writeFileSync( filePath, @@ -839,13 +1029,250 @@ describe('rewriteEncryptedAlterColumns', () => { expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') expect(updated).not.toMatch(/set data type/i) }) + + // A-2: the sweep is FAIL-CLOSED. A column the corpus never declares is + // UNKNOWN, not plaintext — it may already hold ciphertext, with its + // declaration sitting in a migration directory this sweep never sees. + describe('columns the corpus does not declare', () => { + it('refuses to rewrite a column the corpus never declares', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + it('rewrites a column declared plaintext by an ADD COLUMN', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_add.sql'), + 'ALTER TABLE "users" ADD COLUMN "email" text;\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // The skipped file is still part of the corpus: a column's current type + // comes from the migrations that ran before this one, edit-eligible or not. + it('counts a declaration living in the file passed to options.skip', async () => { + const install = path.join(tmpDir, '0000_install-eql.sql') + fs.writeFileSync( + install, + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + const alter = path.join(tmpDir, '0002_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns( + tmpDir, + { + skip: install, + }, + ) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + it('follows a RENAME when deciding a column is declared', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email_address" text);', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email_address" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // A name inside a table/key constraint is a MENTION, not a declaration. + // Here every mention is followed by `)` or `,`, which the declaration + // regex's tail alone already rejects. A mention followed by a SQL keyword + // instead (e.g. `CHECK ("email" IS NOT NULL)`) is a separate case, closed + // by the regex's keyword lookahead and pinned by its own tests below. + // Counting either would put the rewrite back on the fail-open path. + it('does not treat a name inside PRIMARY KEY / REFERENCES as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "sessions" (', + '\t"id" integer,', + '\t"user_id" integer,', + '\tPRIMARY KEY ("id", "email"),', + '\tFOREIGN KEY ("user_id") REFERENCES "users"("email")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "sessions" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A column line commented out INSIDE a live CREATE TABLE never ran, so it + // declares nothing. + it('does not count a column commented out inside a live CREATE TABLE', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\t-- "email" text,', + '\t"name" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('source-unknown') + }) + + // A CHECK predicate mentioning a column has the same `"name" ` + // shape as a declaration — `"email" IS` — but IS is a predicate keyword, + // not a type token. Without the keyword lookahead this would read as a + // declaration and rewrite the column, dropping any ciphertext it already + // holds via a declaration this sweep never sees. + it('does not treat a CHECK predicate mention as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "c1" CHECK ("email" IS NOT NULL)', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A constraint's NAME can coincide with a column's name — drizzle's + // `
__unique` convention makes this contrived, but the keyword + // lookahead closes it regardless: a constraint name is always followed + // immediately by its constraint-type keyword (UNIQUE here), which the + // lookahead excludes. + it('does not let a same-named constraint declare the column', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "email" UNIQUE("id")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // The keyword lookahead is pinned to a word boundary specifically so it + // does not eat real type names that merely START WITH a blocked + // keyword's letters: `interval`/`inet` both start "in" (colliding with + // IN) but must still count as genuine declarations. + it('still declares a column whose type name starts with a blocked keyword', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "events" ("email" interval, "note" inet);\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + [ + 'ALTER TABLE "events" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + 'ALTER TABLE "events" ALTER COLUMN "note" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + }) }) describe('sweepMigrationDirs', () => { let tmpDir: string - const ALTER = - 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n' + const ALTER = [ + // Fail-closed: the sweep rewrites only a column the corpus DECLARES, and a + // real drizzle corpus carries the CREATE that declared it. + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);', + '--> statement-breakpoint', + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') /** Create `dir` under the sandbox, optionally seeding `name` with `sql`. */ const seedDir = (dir: string, name?: string, sql?: string): string => { @@ -945,4 +1372,35 @@ describe('sweepMigrationDirs', () => { expect(results[0].skipped).toHaveLength(1) expect(results[0].skipped[0].file).toBe(path.join(abs, '0001_using.sql')) }) + + // A-2 trigger (b). The wizard ships scanning three candidate directories and + // indexes each separately, so a declaration in `drizzle/` is invisible to the + // sweep of `migrations/`. That column is already ciphertext; rewriting it + // emits a live DROP COLUMN with nothing anywhere to backfill from. + it('does not rewrite an ALTER whose column is declared in a sibling directory', async () => { + seedDir( + 'drizzle', + '0000_create.sql', + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" "public"."eql_v3_text_eq");\n', + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const migrations = seedDir( + 'migrations', + '0001_domain-change.sql', + `${alterSql}\n`, + ) + const alter = path.join(migrations, '0001_domain-change.sql') + + const results = await sweepMigrationDirs(tmpDir, ['drizzle', 'migrations']) + + const swept = results.find((result) => result.dir === 'migrations') + expect(swept?.rewritten).toEqual([]) + expect(swept?.skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + }) }) diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index e4d4e2154..25c37b7da 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -82,12 +82,17 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { // scaffolds, and legacy eql_v2_encrypted. CIP-2991 + CIP-2994 + #693. const sweep = await rewriteEncryptedMigrations(cwd) - // A rewritten file is a DROP+ADD in disguise, and a flagged statement is one - // the sweep could not make safe at all. Either way the next keystroke can - // destroy data, so the prompt says so and defaults to NO — an - // `initialValue: true` immediately under a "do NOT run the migration" - // warning invites exactly the mistake the warning is about. - const unsafe = sweep.rewritten > 0 || sweep.skipped > 0 + // A rewritten file is a DROP+ADD in disguise — the next migrate destroys + // data on any table that already holds rows. A flagged statement never got + // that treatment: it is left on disk untouched, so nothing is destroyed by + // migrating, but a raw ALTER to an encrypted domain has no cast in + // Postgres and fails at migrate time until a human resolves it. Both + // default the prompt to NO — an `initialValue: true` immediately under + // either warning invites exactly the mistake the warning is about — but + // they need different words: claiming "DESTROYS data" for a migration + // that destroyed nothing is its own kind of wrong guidance. + const destructive = sweep.rewritten > 0 + const flaggedOnly = !destructive && sweep.skipped > 0 // A directory whose sweep threw contributes 0 to both totals, so on its own // it is indistinguishable from a clean sweep — except that it means the @@ -110,12 +115,14 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { } const shouldMigrate = await p.confirm({ - message: unsafe + message: destructive ? `Run the migration now? (${runner} drizzle-kit migrate) — see the warnings above: this migration DESTROYS data on any table that already holds rows` - : unverified - ? `Run the migration now? (${runner} drizzle-kit migrate) — the sweep could not check ${unverifiedCount} (${unverifiedList}); review those migrations before migrating, or you may apply broken/unsafe SQL` - : `Run the migration now? (${runner} drizzle-kit migrate)`, - initialValue: !unsafe && !unverified, + : flaggedOnly + ? `Run the migration now? (${runner} drizzle-kit migrate) — statement(s) were flagged for review above rather than rewritten; nothing was destroyed, but the raw ALTER will fail at migrate time until they're resolved` + : unverified + ? `Run the migration now? (${runner} drizzle-kit migrate) — the sweep could not check ${unverifiedCount} (${unverifiedList}); review those migrations before migrating, or you may apply broken/unsafe SQL` + : `Run the migration now? (${runner} drizzle-kit migrate)`, + initialValue: !destructive && !flaggedOnly && !unverified, }) if (!p.isCancel(shouldMigrate) && shouldMigrate) { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 73e1908b7..0255df6b5 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -104,20 +104,52 @@ const NEAR_MISS_RE = /[^;]*?\bSET\s+DATA\s+TYPE\b[^;]*?\beql_v[23][a-z0-9_]*[^;]*?;/gi /** - * Blank lines and `--` line comments (including drizzle-kit's - * `--> statement-breakpoint`) at the head of a {@link NEAR_MISS_RE} match. + * Strips any run of blank lines, `--` line comments (including drizzle-kit's + * `--> statement-breakpoint`), and `/* … *\/` block comments — in any order, + * repeated as many times as they occur — from the head of a + * {@link NEAR_MISS_RE} match. * * That regex opens with a lazy `[^;]*?`, whose only left boundary is the * previous `;` — or the start of the file when there is no preceding statement. * So the raw match drags in every comment and blank line since then, and a - * near-miss in a file that opens with a comment block gets reported to the user - * with that whole block glued to its front. Strip the preamble so the statement - * we quote back reads as the offending statement alone. + * near-miss preceded by a comment block gets reported to the user with that + * whole block glued to its front. Strip the preamble so the statement we quote + * back reads as the offending statement alone. * - * Only line comments are handled — `/* … *\/` block comments are not something - * drizzle-kit emits, and a stray one costs cosmetics, not correctness. + * The block comment is matched as its OWN loop alternative rather than a + * single group anchored ahead of the line-comment loop, because the latter + * only works when the block comment sits at the very start of the file: in + * the far more common case — a preceding statement earlier in the same file — + * the match starts at THAT statement's `;`, so it opens with the newline + * after it, not with the comment. An anchored `^(?:/\*…\*\/\s*)?` can't match + * past that newline to reach the comment, so it silently matches nothing and + * leaves the comment attached to the reported statement. Folding the block + * comment into the repeating loop lets it match after any number of leading + * newlines/line-comments, in any interleaving. + * + * The block comment strip is NOT cosmetic: a statement the strict + * {@link ALTER_COLUMN_TO_ENCRYPTED_RE} matched but skipped (`already-encrypted` + * or `source-unknown`) is left on disk unchanged, so it still contains + * `SET DATA TYPE` and the broad scan below finds it again. Without stripping + * the block comment here, that second pass reports a DIFFERENT statement string + * (comment glued to the front) than the strict pass's `match.trim()`, so + * {@link rewriteEncryptedAlterColumns}'s dedup key never matches and the same + * statement is reported twice — once with the correct reason, once as + * `unrecognised-form`, whose guidance (look for a hand-authored `USING`) is + * wrong for a statement the strict matcher already matched. Stripping the + * comment here makes both passes agree on the statement text so the second + * report collapses into the first. + * + * Known residue, accepted rather than fixed: a NESTED closed block comment + * ahead of a live ALTER (`/* outer /* inner *\/ still *\/`) still + * double-reports. The block-comment alternative's `*?` is lazy, so it stops + * at the FIRST `*\/` — consuming only `/* outer /* inner *\/` — and leaves + * `` still *\/`` glued to the front of the next iteration, which the + * line-comment alternative doesn't recognise either. That residual text rides + * along into the `unrecognised-form` report. */ -const STATEMENT_PREAMBLE_RE = /^(?:[^\S\n]*(?:--[^\n]*)?\n)+/ +const STATEMENT_PREAMBLE_RE = + /^(?:\s*\/\*[\s\S]*?\*\/|[^\S\n]*(?:--[^\n]*)?\n)*/ /** Drop the leading blank/comment lines a `[^;]*?`-anchored match dragged in. */ function trimStatementPreamble(statement: string): string { @@ -259,6 +291,91 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( 'gi', ) +/** + * A column name in a DECLARATION position — `"email" text`, + * `"email" "public"."eql_v3_text_search"`, `"amount" numeric(10, 2)`. + * + * The `\s+["a-z]` tail is what separates a declaration from a MENTION. Most + * mentions are followed by `)`, `,`, or an operator, which the tail alone + * already rejects: + * + * ```sql + * PRIMARY KEY ("id", "name") + * FOREIGN KEY ("org_id") REFERENCES "orgs"("id") + * ``` + * + * But a mention inside a predicate or a table-level constraint is often + * followed by whitespace and a SQL KEYWORD — which has exactly the same + * `\s+[a-z]` shape as a real declaration, and drizzle-kit emits both forms + * inside a `CREATE TABLE` (a `CONSTRAINT … CHECK ()` body is + * user-authored, so the predicate form is reachable too): + * + * ```sql + * CHECK ("email" IS NOT NULL) + * CONSTRAINT "users_email_unique" UNIQUE ("email") + * ``` + * + * The negative lookahead rejects the keywords reachable this way: predicate + * keywords (`IS`, `IN`, `NOT`, `LIKE`, `ILIKE`, `BETWEEN`, `AND`, `OR`), + * ordering/collation (`COLLATE`, `ASC`, `DESC`, `NULLS`), and every + * table/column-constraint keyword (`UNIQUE`, `PRIMARY`, `FOREIGN`, `CHECK`, + * `EXCLUDE`, `REFERENCES`, `CONSTRAINT`, `USING`, `WITH`) — the last six also + * close the constraint-NAME case above, since Postgres always puts one of + * them immediately after a constraint's name. + * + * A real type token still matches because the lookahead is pinned to a word + * boundary (`\b`) right after each keyword, so it only rejects a keyword when + * that keyword is the WHOLE next word: `interval` and `inet` both start with + * `IN`'s letters, but their third character keeps them inside one word, so + * `\bIN\b` never matches there; the same reasoning clears `int`, + * `char`/`citext` against `CHECK`, and `eql_v2_encrypted`/`eql_v3_*` against + * `EXCLUDE`. + * + * Deliberately NOT pinned to the encrypted domains. A column the corpus + * declares but does not give an encrypted type is, by residue, plaintext — so + * the fail-closed rule needs no type classification and no SQL parsing, only + * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. + * + * **The residue claim depends on {@link MANGLED_TYPE_FORMS} covering every + * encrypted shape.** "By residue, plaintext" is only as good as the encrypted + * side's coverage: a declaration {@link ENCRYPTED_TYPE_REF} fails to recognise + * falls to the plaintext residue and gets rewritten. Two forms do this: a + * domain installed into a non-`public` schema (`"email" "app"."eql_v3_text_search"`, + * since the mangled forms only special-case the literal `public` schema), and + * an array of the domain (`ADD COLUMN "email" public.eql_v3_text_search[]`, + * since {@link ENCRYPTED_TYPE_REF}'s trailing delimiter lookahead does not + * include `[`). Neither is a layout EQL installs into or a shape drizzle-kit + * emits, and both behaved identically before this branch — so this is a + * documentation gap, not a regression this branch introduced. + * + * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: + * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, + * `OVERLAPS`, `ON`, …) still lets a bare mention through, e.g. + * `CHECK ("email" SIMILAR TO '...')`, or a `REFERENCES "email" ON DELETE + * CASCADE` where `"email"` names the referenced TABLE rather than the column + * being declared — `"id" integer REFERENCES "email" ON DELETE CASCADE` inside + * a `CREATE TABLE` body still registers `email` as declared. That one only + * bites when a column shares a referenced table's name, so like the rest of + * this residue it is inert unless that mention's name exactly matches a + * DIFFERENT column that is both encrypted and genuinely undeclared anywhere + * else in the corpus — contrived, and strictly narrower than the gap this + * replaces. + */ +const DECLARED_COLUMN_RE = + /"([^"]+)"\s+(?!(?:IS|IN|NOT|LIKE|ILIKE|BETWEEN|AND|OR|COLLATE|ASC|DESC|NULLS|UNIQUE|PRIMARY|FOREIGN|CHECK|EXCLUDE|REFERENCES|CONSTRAINT|USING|WITH)\b)["a-z]/gi + +/** + * `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. + * + * Needs no {@link DECLARED_COLUMN_RE}-style keyword lookahead: the token + * right after an ADD COLUMN's column name is always its type, so no SQL + * keyword can occupy that position. + */ +const ADD_COLUMN_RE = new RegExp( + String.raw`ALTER TABLE\s+${TABLE_REF}\s+ADD COLUMN\s+(?:IF NOT EXISTS\s+)?"([^"]+)"\s+["a-z]`, + 'gi', +) + /** Splits a `TABLE_REF` capture pair into its schema and table halves. */ function tableOf( first: string, @@ -271,42 +388,80 @@ function tableOf( : { schema: first, table: second } } -/** Identity of a column across the corpus, for {@link indexEncryptedColumns}. */ +/** Identity of a column across the corpus, for {@link indexColumnDeclarations}. */ function columnKey(table: string, column: string, schema?: string): string { return JSON.stringify([schema ?? '', table, column]) } +/** What the migration corpus says about the columns it mentions. */ +interface ColumnIndex { + /** Columns the corpus gives an ENCRYPTED type. Rewriting one drops ciphertext. */ + encrypted: Set + /** Columns the corpus DECLARES at all, whatever type it gives them. */ + declared: Set +} + /** - * Index every column the migration corpus gives an ENCRYPTED type, so the - * rewrite can tell the change it exists for (plaintext → encrypted) from one it - * must never touch (encrypted → encrypted). + * Index what the migration corpus knows about each column, so the rewrite can + * tell the change it exists for (plaintext → encrypted) from the two it must + * never make: encrypted → encrypted, and a change to a column it knows nothing + * about. * - * **Why (#772 review, W-3):** the strict matcher captures only the TARGET type. - * A column whose encrypted domain merely changes (`types.TextEq` → + * **Why `encrypted` (#772 review, W-3):** the strict matcher captures only the + * TARGET type. A column whose encrypted domain merely changes (`types.TextEq` → * `types.TextSearch`) matches just as well as a plaintext column, and the * ADD+DROP+RENAME then drops a column full of CIPHERTEXT — with no plaintext * left anywhere to backfill from, so unlike the plaintext case the data is not - * recoverable from the application at all. Changing an encrypted column's domain - * changes its index terms, so the data has to be re-encrypted through the client - * regardless; the sweep flags the statement and leaves it for the user. + * recoverable from the application at all. + * + * **Why `declared` (A-2):** absence from `encrypted` is not evidence of + * plaintext. It is evidence of nothing. The sweep runs per directory, and the + * shipped wizard default scans three of them, so a column's `CREATE TABLE` can + * simply live somewhere this sweep never reads — the column is then rewritten + * on an assumption, and the ADD+DROP+RENAME drops live ciphertext. Requiring a + * POSITIVE declaration makes the unknown case a flagged statement instead. + * + * Because the encrypted side is matched against the known domain list, the + * plaintext side needs no type classification: a column the corpus declares but + * does not give an encrypted type is plaintext by residue. * * The index is corpus-wide rather than ordered by migration: over-detecting - * "encrypted" costs a flagged statement the user must handle by hand, while - * under-detecting costs irrecoverable ciphertext. Only comment-free statements - * count, for the same reason {@link isInsideCommentOrString} exists. + * "encrypted" costs a flagged statement the user handles by hand, while + * under-detecting costs irrecoverable ciphertext. That asymmetry is why the + * two per-column scans inside a `CREATE TABLE` body are deliberately + * inconsistent about comments (see the comment at the loops themselves) — + * do not "fix" that inconsistency, it is the point. */ -function indexEncryptedColumns(contents: readonly string[]): Set { +function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const encrypted = new Set() + const declared = new Set() for (const sql of contents) { for (const created of sql.matchAll(CREATE_TABLE_RE)) { if (isInsideCommentOrString(sql, created.index)) continue const { schema, table } = tableOf(created[1], created[2]) - for (const column of created[3].matchAll( - CREATE_TABLE_ENCRYPTED_COLUMN_RE, - )) { + const body = created[3] + + // The body is scanned as its own document: a `--` earlier in it comments + // out the rest of that line, and a CREATE inside a block comment or a + // string literal was already skipped above. + // + // Asymmetric on purpose. ENCRYPTED does NOT re-check comments here: a + // commented-out encrypted column inside an otherwise live CREATE TABLE + // still counts, exactly as it did before this sweep learned to declare + // anything — over-detecting "encrypted" only costs a flagged statement. + // DECLARED DOES re-check: a commented-out plaintext line never ran, so + // it must not count as a declaration — over-detecting "declared" is + // what would put a truly-undeclared, possibly already-encrypted column + // back on the fail-open path. + for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { encrypted.add(columnKey(table, column[1], schema)) } + + for (const column of body.matchAll(DECLARED_COLUMN_RE)) { + if (isInsideCommentOrString(body, column.index)) continue + declared.add(columnKey(table, column[1], schema)) + } } for (const added of sql.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { @@ -315,19 +470,26 @@ function indexEncryptedColumns(contents: readonly string[]): Set { encrypted.add(columnKey(table, added[3], schema)) } + for (const added of sql.matchAll(ADD_COLUMN_RE)) { + if (isInsideCommentOrString(sql, added.index)) continue + const { schema, table } = tableOf(added[1], added[2]) + declared.add(columnKey(table, added[3], schema)) + } + // A rename carries the column's type with it — and `__cipherstash_tmp` // renamed onto the real name is exactly what a previous sweep of this very // directory emitted. Run after ADD so that tmp column is already indexed. for (const renamed of sql.matchAll(RENAME_COLUMN_RE)) { if (isInsideCommentOrString(sql, renamed.index)) continue const { schema, table } = tableOf(renamed[1], renamed[2]) - if (encrypted.has(columnKey(table, renamed[3], schema))) { - encrypted.add(columnKey(table, renamed[4], schema)) - } + const from = columnKey(table, renamed[3], schema) + const to = columnKey(table, renamed[4], schema) + if (encrypted.has(from)) encrypted.add(to) + if (declared.has(from)) declared.add(to) } } - return encrypted + return { encrypted, declared } } /** Why a recognised ALTER-to-encrypted statement was left alone. */ @@ -336,6 +498,8 @@ export type SkipReason = | 'unrecognised-form' /** The column already holds an encrypted domain; rewriting drops ciphertext. */ | 'already-encrypted' + /** The corpus never declares the column, so its current type is unknown. */ + | 'source-unknown' /** A statement the sweep recognised as ALTER-to-encrypted but did NOT rewrite. */ export interface SkippedAlter { @@ -350,7 +514,7 @@ export interface SkippedAlter { /** * One-line explanation of a {@link SkipReason}, for the CLI/wizard to print * next to the statement. Lives here so every caller says the same thing — the - * two reasons need very different action from the user, and a single generic + * three reasons need very different action from the user, and a single generic * "could not rewrite automatically" hides that. */ export function describeSkipReason(reason: SkipReason): string { @@ -359,6 +523,8 @@ export function describeSkipReason(reason: SkipReason): string { return "the column is ALREADY encrypted, so the ADD+DROP+RENAME rewrite would DROP the ciphertext with no plaintext left to backfill from. Changing an encrypted column's domain changes its index terms, so the data must be re-encrypted through the staged `stash encrypt` lifecycle" case 'unrecognised-form': return 'it falls outside the strict matcher (a hand-authored `SET DATA TYPE ... USING ...`, or a drizzle-kit form the sweep does not recognise) and an in-place cast to an encrypted domain fails at migrate time' + case 'source-unknown': + return "the sweep could not find where this column was declared in this migration directory, so it cannot tell a plaintext column (safe to rewrite) from one that already holds ciphertext (where the rewrite would DROP it). Usually the column's `CREATE TABLE` / `ADD COLUMN` lives in a different directory, or the migration history was squashed. Check the column's current type in the database: if it is plaintext and the table is empty, apply the ADD/DROP/RENAME by hand; if it already holds ciphertext, use the staged `stash encrypt` lifecycle instead" } } @@ -394,13 +560,16 @@ export interface RewriteResult { * * Returns {@link RewriteResult}: the files rewritten, plus `skipped` statements * left for a human — ones outside the strict matcher (a hand-authored - * `SET DATA TYPE … USING …;`, or a future drizzle-kit form), and ones targeting - * a column that is ALREADY encrypted, where the rewrite would drop ciphertext. - * Both are left untouched on disk and surfaced non-fatally so the caller can - * tell the user to review them, rather than silently shipping broken SQL or - * destroying data. Statements sitting inside a SQL comment — or inside a - * single-quoted string literal, where they are data rather than SQL — are inert - * and are neither rewritten nor reported. + * `SET DATA TYPE … USING …;`, or a future drizzle-kit form); ones targeting a + * column that is ALREADY encrypted, where the rewrite would drop ciphertext; + * and ones targeting a column the corpus never DECLARES anywhere, where the + * rewrite would be guessing at a type it cannot see (likely the most common + * skip on a real corpus, since the sweep only ever reads part of the + * migration history). All three are left untouched on disk and surfaced + * non-fatally so the caller can tell the user to review them, rather than + * silently shipping broken SQL or destroying data. Statements sitting inside a + * SQL comment — or inside a single-quoted string literal, where they are data + * rather than SQL — are inert and are neither rewritten nor reported. */ export async function rewriteEncryptedAlterColumns( outDir: string, @@ -440,7 +609,8 @@ export async function rewriteEncryptedAlterColumns( // Built from the WHOLE corpus, including `options.skip`: a column's current // type comes from the migrations that ran before this one, not just the files // we are allowed to edit. - const encryptedColumns = indexEncryptedColumns([...contents.values()]) + const { encrypted: encryptedColumns, declared: declaredColumns } = + indexColumnDeclarations([...contents.values()]) for (const [filePath, original] of contents) { if (options.skip && filePath === options.skip) continue @@ -471,6 +641,17 @@ export async function rewriteEncryptedAlterColumns( return match } + // Fail closed. Absence from `declaredColumns` does not mean the column + // is plaintext — it means the corpus never said. The declaration can + // sit in a directory this sweep does not read (the wizard ships with + // three candidates and indexes each separately), so rewriting on the + // assumption is how a live `DROP COLUMN` reaches a populated — possibly + // already-encrypted — column. Flag it and let the user look. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() // Unreachable — the outer regex only matches when a domain is present — // but leave the statement alone rather than emit a broken rewrite. diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index b35c9be4f..aa1c03c27 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -392,6 +392,8 @@ Pass exactly one of `--drizzle` / `--prisma`. The generated migration also insta After writing the migration, `--drizzle` sweeps the output directory for sibling migrations containing an in-place `ALTER COLUMN … SET DATA TYPE ` — drizzle-kit emits these when you change a plaintext column to an encrypted one, and Postgres rejects them (there is no cast from `text`/`numeric` to an EQL type). Each is rewritten into an `ADD COLUMN` + `DROP` + `RENAME` sequence and the rewritten files are listed. This is equivalent to DROP+ADD — it fixes the type but does **not** preserve data — so it is safe **only on an empty table**. On a populated table the new column starts NULL and the old one is dropped in the same migration, destroying the plaintext; do not run it there. Use the staged `stash encrypt` path (add → backfill → cutover → drop) instead. +The sweep is fail-closed: it rewrites a statement only when the same directory also contains the `CREATE TABLE` or `ADD COLUMN` that declared the column, and the column is not already encrypted **in the corpus**. That is a guarantee about what the migration files say, not about the live database — the sweep never queries the database, so a column that has drifted from its migration history (for example, renamed directly by `stash encrypt cutover`, or altered by hand via psql or the Supabase dashboard) can already hold ciphertext while the corpus still describes it as plaintext. A statement it cannot place — the declaration lives in another migration directory, or the history was squashed — is listed as needing review rather than rewritten, because a column of unknown type may already hold ciphertext that the rewrite's `DROP COLUMN` would destroy. Either way, check the column's actual type in the database before applying a flagged or corpus-cleared rewrite by hand. + #### `eql upgrade` The install SQL is safe to re-run — columns and data survive — but it is not fully idempotent: it begins with `DROP SCHEMA IF EXISTS eql_v3 CASCADE`, which cascade-drops any **functional indexes** built on the `eql_v3` extractors (see `stash-indexing`). After an upgrade, recreate them: migration runners skip migrations already recorded as applied, so add a *new* migration that re-issues the `CREATE INDEX` statements (or run the DDL directly), then `ANALYZE` the affected tables. `upgrade` checks the current version, re-runs the install SQL, and reports the new one. If EQL isn't installed it points you at `eql install`. Same `--supabase`, `--exclude-operator-family`, `--eql-version`, `--latest`, `--dry-run`, `--database-url` flags. diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index f52025bb4..50ae36fd2 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -61,7 +61,7 @@ stash eql migration --drizzle --supabase # also grants eql_v3 to anon/authenti The generated migration also installs the `cs_migrations` tracking schema, so a single `drizzle-kit migrate` covers everything `stash encrypt …` needs — no out-of-band `stash eql install`. EQL v3 ships one SQL bundle for every target including Supabase; `--supabase` only adds the PostgREST/RLS role grants (harmless when you connect directly as `postgres`). Requires `drizzle-kit` installed and configured. -**Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) The `stash eql migration --drizzle` sweep repairs the invalid statement — the `stash-cli` skill covers what it rewrites and the rule that matters: the repair is data-destroying, so it is safe **only on an empty table**. For a table with live data, do **not** apply the swept migration; follow the staged flow in **Migrating an Existing Column to Encrypted** below instead. +**Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) The `stash eql migration --drizzle` sweep repairs the invalid statement — the `stash-cli` skill covers what it rewrites and the rule that matters: the repair is data-destroying, so it is safe **only on an empty table**. It repairs only what it can place: the swept directory must also contain the migration that declared the column. If it does not, the statement is flagged for review instead of rewritten. For a table with live data, do **not** apply the swept migration; follow the staged flow in **Migrating an Existing Column to Encrypted** below instead. ### Column Storage