diff --git a/.changeset/staged-add-only-rewriter.md b/.changeset/staged-add-only-rewriter.md new file mode 100644 index 000000000..afb6e7047 --- /dev/null +++ b/.changeset/staged-add-only-rewriter.md @@ -0,0 +1,10 @@ +--- +'stash': patch +'@cipherstash/wizard': patch +--- + +The Drizzle migration rewriter now preserves the source column and adds a staged +encrypted twin instead of emitting destructive drop/rename SQL. When the sweep +cannot prove a source column's type or the encrypted twin already exists, the +CLI and wizard fail closed with a non-zero exit so the migration directory must +be reviewed before applying it. diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 4a0d231bd..7bb16dcdc 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -1,17 +1,30 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { describeSkipReason, rewriteEncryptedAlterColumns, } from '../commands/db/rewrite-migrations.js' +const fsPromisesWrite = vi.hoisted(() => ({ + real: (() => { + throw new Error('fsPromisesWrite.real not initialised') + }) as typeof import('node:fs/promises').writeFile, + spy: vi.fn(), +})) +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + fsPromisesWrite.real = actual.writeFile + return { ...actual, writeFile: fsPromisesWrite.spy } +}) + describe('rewriteEncryptedAlterColumns', () => { let tmpDir: string beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'stash-rewrite-')) + fsPromisesWrite.spy.mockImplementation(fsPromisesWrite.real) }) afterEach(() => { @@ -49,13 +62,13 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "transactions" ADD COLUMN "amount__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "transactions" ADD COLUMN "amount_encrypted" "public"."eql_v2_encrypted";', ) - expect(updated).toContain( + expect(updated).not.toContain( 'ALTER TABLE "transactions" DROP COLUMN "amount";', ) - expect(updated).toContain( - 'ALTER TABLE "transactions" RENAME COLUMN "amount__cipherstash_tmp" TO "amount";', + expect(updated).not.toContain( + 'ALTER TABLE "transactions" RENAME COLUMN "amount_encrypted" TO "amount";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -71,7 +84,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -91,11 +104,13 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') // Every emitted statement keeps the schema qualifier. expect(updated).toContain( - 'ALTER TABLE "app"."users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "app"."users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) - expect(updated).toContain('ALTER TABLE "app"."users" DROP COLUMN "email";') - expect(updated).toContain( - 'ALTER TABLE "app"."users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + expect(updated).not.toContain( + 'ALTER TABLE "app"."users" DROP COLUMN "email";', + ) + expect(updated).not.toContain( + 'ALTER TABLE "app"."users" RENAME COLUMN "email_encrypted" TO "email";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -111,7 +126,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "transactions" ADD COLUMN "amount__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "transactions" ADD COLUMN "amount_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -127,7 +142,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "transactions" ADD COLUMN "description__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "transactions" ADD COLUMN "description_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -210,11 +225,11 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - `ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."${domain}";`, + `ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."${domain}";`, ) - expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') - expect(updated).toContain( - 'ALTER TABLE "users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + expect(updated).not.toContain('ALTER TABLE "users" DROP COLUMN "email";') + expect(updated).not.toContain( + 'ALTER TABLE "users" RENAME COLUMN "email_encrypted" TO "email";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -238,7 +253,7 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - `ALTER TABLE "t" ADD COLUMN "c__cipherstash_tmp" "public"."${domain}";`, + `ALTER TABLE "t" ADD COLUMN "c_encrypted" "public"."${domain}";`, ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -285,7 +300,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -308,7 +323,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -328,7 +343,7 @@ describe('rewriteEncryptedAlterColumns', () => { expect(updated).not.toContain('eql_v2_encrypted') }) - it('notes that constraints/defaults/indexes are not carried over', async () => { + it('explains that the source column is preserved for the staged lifecycle', async () => { declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0016_constraints.sql') fs.writeFileSync( @@ -339,11 +354,12 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) const updated = fs.readFileSync(filePath, 'utf-8') - expect(updated).toContain('constraints, defaults, and indexes') + expect(updated).toContain('source column "email" is deliberately preserved') + expect(updated).toContain('staged `stash encrypt` lifecycle') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) }) - it('does not terminate the commented UPDATE placeholder with a semicolon', async () => { - // A runner that naively splits on `;` must not cut mid-comment. + it('does not emit an unusable SQL backfill placeholder', async () => { declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0017_semicolon.sql') fs.writeFileSync( @@ -354,16 +370,11 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) const updated = fs.readFileSync(filePath, 'utf-8') - const updateLine = updated - .split('\n') - .find( - (line) => line.includes('UPDATE') && line.includes('encrypted value'), - ) - expect(updateLine).toBeDefined() - expect(updateLine?.trimEnd().endsWith(';')).toBe(false) + expect(updated).not.toMatch(/^\s*(?:--\s*)?UPDATE\b/im) + expect(updated).toContain('encryptModel') }) - it('separates ADD/DROP/RENAME with --> statement-breakpoint, one exec stmt per chunk', async () => { + it('emits one executable ADD and no cutover statements', async () => { declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0018_breakpoint.sql') fs.writeFileSync( @@ -374,19 +385,14 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) const updated = fs.readFileSync(filePath, 'utf-8').trimEnd() - const chunks = updated.split('--> statement-breakpoint') - // Three executable statements: ADD, DROP, RENAME — one per chunk. - expect(chunks).toHaveLength(3) - for (const chunk of chunks) { - const execLines = chunk - .split('\n') - .map((line) => line.trim()) - .filter((line) => line.length > 0 && !line.startsWith('--')) - expect(execLines).toHaveLength(1) - } - expect(chunks[0]).toContain('ADD COLUMN') - expect(chunks[1]).toContain('DROP COLUMN') - expect(chunks[2]).toContain('RENAME COLUMN') + const execLines = updated + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith('--')) + expect(execLines).toEqual([ + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v2_encrypted";', + ]) + expect(updated).not.toContain('--> statement-breakpoint') }) it('rewrites each statement to its own domain when v2 and v3 are mixed', async () => { @@ -404,10 +410,10 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "a" ADD COLUMN "x__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "a" ADD COLUMN "x_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).toContain( - 'ALTER TABLE "a" ADD COLUMN "y__cipherstash_tmp" "public"."eql_v3_json";', + 'ALTER TABLE "a" ADD COLUMN "y_encrypted" "public"."eql_v3_json";', ) }) @@ -717,9 +723,9 @@ describe('rewriteEncryptedAlterColumns', () => { const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([filePath]) - expect(fs.readFileSync(filePath, 'utf-8')).toContain( - 'ALTER TABLE "users" DROP COLUMN "email";', - ) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) }) // An apostrophe inside a DOUBLE-QUOTED identifier is not a string @@ -909,12 +915,137 @@ describe('rewriteEncryptedAlterColumns', () => { const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([filePath]) - expect(fs.readFileSync(filePath, 'utf-8')).toContain('DROP COLUMN') + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toContain('DROP COLUMN') + }) + + it.each([ + ['tagged', 'price$usd$cents'], + ['untagged', 'price$$cents'], + ])('does not treat a %s dollar delimiter inside an unquoted identifier as a dollar-quoted body', async (_kind, identifier) => { + declarePlaintext('"users"', 'email') + const filePath = path.join(tmpDir, `0033_${_kind}-identifier.sql`) + fs.writeFileSync( + filePath, + [ + `SELECT ${identifier} FROM "prices";`, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + expect(skipped).toEqual([]) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toContain('SET DATA TYPE') + }) + }) + + describe('issue #811 dollar-quoted DDL regression', () => { + const domainChange = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_eq";' + + it('does not emit destructive SQL for the reported two-file DO $$ rename corpus', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("id" serial PRIMARY KEY NOT NULL, "email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" RENAME COLUMN "email" TO "email_old";', + ' ALTER TABLE "users" RENAME COLUMN "email_encrypted" TO "email";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'target-exists' }, + ]) + }) + + it('does not emit destructive SQL when an encrypted ADD COLUMN is inside DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" ADD COLUMN "email" "eql_v3_text_search";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + const updated = fs.readFileSync(change, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + }) + + it('does not emit destructive SQL for a rename inside a custom dollar tag', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + 'DO $stash$ BEGIN', + ' ALTER TABLE "users" RENAME COLUMN "email" TO "email_old";', + ' ALTER TABLE "users" RENAME COLUMN "email_encrypted" TO "email";', + 'END $stash$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'target-exists' }, + ]) + }) + + it('does not emit destructive SQL when an unterminated $$ hides a later encrypted declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'SELECT $$unterminated;', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + const updated = fs.readFileSync(change, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) }) }) - // ADD+DROP+RENAME on a column that is ALREADY encrypted drops CIPHERTEXT, and - // unlike the plaintext case there is nothing left anywhere to backfill from. + // A domain change on a column that is ALREADY encrypted needs staged + // re-encryption; there is no plaintext source to backfill from. describe('columns that are already encrypted', () => { it('refuses to rewrite a domain change on a column created encrypted', async () => { const create = path.join(tmpDir, '0000_create.sql') @@ -1201,6 +1332,25 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].reason).toBe('already-encrypted') }) + it('fails closed when the staged encrypted twin already exists', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("email" text, "email_encrypted" eql_v3_text_eq);\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(fs.readFileSync(alter, 'utf-8')).toBe(`${alterSql}\n`) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'target-exists' }, + ]) + }) + // #772 review, finding 3. The index reads CREATE TABLE, ADD COLUMN and // RENAME, but never the strict matcher's OWN target. So a corpus where an // earlier migration already converted the column — realistic wherever a @@ -1225,11 +1375,13 @@ describe('rewriteEncryptedAlterColumns', () => { // The first conversion is the legitimate plaintext -> encrypted one and // must still happen — flagging it too would be the naive fix. expect(rewritten).toEqual([first]) - expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN') - // The second targets ciphertext. Left byte-identical on disk. + expect(fs.readFileSync(first, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + // The second would add the same target again. Left byte-identical. expect(fs.readFileSync(second, 'utf-8')).toBe(`${secondSql}\n`) expect(skipped).toEqual([ - { file: second, statement: secondSql, reason: 'already-encrypted' }, + { file: second, statement: secondSql, reason: 'target-exists' }, ]) }) @@ -1251,11 +1403,12 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') - // Exactly one conversion was applied, and the v3 statement survives verbatim. - expect(updated.match(/DROP COLUMN/g)?.length).toBe(1) + // Exactly one target was staged, and the v3 statement survives verbatim. + expect(updated.match(/ADD COLUMN/g)?.length).toBe(1) + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) expect(updated).toContain(secondSql) expect(skipped).toEqual([ - { file: filePath, statement: secondSql, reason: 'already-encrypted' }, + { file: filePath, statement: secondSql, reason: 'target-exists' }, ]) }) @@ -1297,7 +1450,9 @@ describe('rewriteEncryptedAlterColumns', () => { const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([live]) - expect(fs.readFileSync(live, 'utf-8')).toContain('DROP COLUMN') + const updated = fs.readFileSync(live, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toContain('DROP COLUMN') }) // #772 review, finding 4. `columnKey` keys on the schema exactly as written, @@ -1485,11 +1640,46 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated.match(/ADD COLUMN/g)?.length).toBe(2) - expect(updated.match(/DROP COLUMN/g)?.length).toBe(2) + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) // Non-matching statement preserved expect(updated).toContain('CREATE INDEX "a_z" ON "a" ("z");') }) + it('reports files rewritten before a later write failure', async () => { + declarePlaintext('"users"', 'email', 'name') + const first = path.join(tmpDir, '0001_email.sql') + const failing = path.join(tmpDir, '0002_name.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + fs.writeFileSync( + failing, + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + fsPromisesWrite.spy.mockImplementation(async (file, data, options) => { + if (file === failing) { + fs.unlinkSync(failing) + fs.mkdirSync(failing) + } + return fsPromisesWrite.real(file, data, options) + }) + + try { + await rewriteEncryptedAlterColumns(tmpDir) + throw new Error('expected rewriteEncryptedAlterColumns to throw') + } catch (error) { + const partial = error as { rewritten?: string[]; skipped?: unknown[] } + expect(partial.rewritten).toEqual([first]) + expect(partial.skipped).toEqual([]) + expect(fs.readFileSync(first, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + } finally { + if (fs.statSync(failing).isDirectory()) fs.rmdirSync(failing) + } + }) + // 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 () => { @@ -1506,9 +1696,9 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toEqual([]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) - expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') + expect(updated).not.toContain('ALTER TABLE "users" DROP COLUMN "email";') expect(updated).not.toMatch(/set data type/i) }) @@ -1754,10 +1944,16 @@ describe('describeSkipReason', () => { it('describes already-encrypted as a re-encrypt-through-the-lifecycle action', () => { const text = describeSkipReason('already-encrypted') expect(text).toContain('ALREADY encrypted') - expect(text).toContain('DROP the ciphertext') + expect(text).toContain('re-encrypted') expect(text).toContain('`stash encrypt` lifecycle') }) + it('describes an existing target as a duplicate-column failure', () => { + const text = describeSkipReason('target-exists') + expect(text).toContain('already exists') + expect(text).toContain('another ADD COLUMN would fail') + }) + it('describes unrecognised-form as a hand-authored / unknown cast', () => { const text = describeSkipReason('unrecognised-form') expect(text).toContain('SET DATA TYPE ... USING') @@ -1768,11 +1964,14 @@ describe('describeSkipReason', () => { const text = describeSkipReason('source-unknown') expect(text).toContain('could not find where this column was declared') expect(text).toContain("Check the column's current type in the database") + expect(text).toContain('staged `stash encrypt` lifecycle') + expect(text).not.toContain('table is empty') }) it('gives each reason a distinct description', () => { const reasons = [ 'already-encrypted', + 'target-exists', 'unrecognised-form', 'source-unknown', ] as const diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 2e0c33c8b..db82b4519 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -258,6 +258,11 @@ function isInsideCommentOrString(sql: string, index: number): boolean { * dollar-quote opener. */ function dollarQuoteDelimiter(sql: string, open: number): string | undefined { + // PostgreSQL requires a dollar-quoted string following an identifier or + // keyword to be separated from it. `$` is legal inside an unquoted + // identifier, so `price$usd$cents` and `price$$cents` must not open phantom + // dollar-quoted bodies that hide later live SQL from the sweep. + if (/[A-Za-z0-9_$]/.test(sql[open - 1] ?? '')) return undefined DOLLAR_QUOTE_OPEN_RE.lastIndex = open return DOLLAR_QUOTE_OPEN_RE.exec(sql)?.[0] } @@ -319,8 +324,8 @@ const TABLE_REF = String.raw`"([^"]+)"(?:\."([^"]+)")?` * delimiter so a bare domain cannot match a prefix of a longer identifier. * * Two shapes {@link MANGLED_TYPE_FORMS} does not enumerate are folded in here so - * the encrypted index recognises them and the ADD+DROP+RENAME cannot drop their - * ciphertext: a domain in ANY quoted schema (`"app"."eql_v3_text_search"`, not + * the encrypted index recognises them and never mistakes their ciphertext for + * plaintext: a domain in ANY quoted schema (`"app"."eql_v3_text_search"`, not * just the literal `public` the mangled forms special-case), and a `[` in the * trailing lookahead so an ARRAY of the domain (`public.eql_v3_text_search[]`) * ends at a delimiter too. Both feed only the encrypted index — never the @@ -440,8 +445,8 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * covered explicitly by {@link ENCRYPTED_TYPE_REF} — a domain in a non-`public` * schema (`"email" "app"."eql_v3_text_search"`) and an array of the domain * (`"email" public.eql_v3_text_search[]`) — because both name ciphertext the - * corpus can see, so rewriting them is the exact drop this rule exists to - * prevent. The dependency itself remains: any encrypted shape a future EQL + * corpus can see, so treating them as plaintext would choose the wrong staged + * lifecycle. The dependency itself remains: any encrypted shape a future EQL * install introduces must be added to {@link ENCRYPTED_TYPE_REF} or it too will * fall to the plaintext residue. * @@ -494,7 +499,7 @@ function tableOf( * while hand-written SQL and this sweep's own {@link renderSafeAlter} output * are qualified. Keying on the literal text split one column across two keys, * so a column already encrypted under one spelling looked plaintext when - * altered under the other and the ADD+DROP+RENAME dropped its ciphertext + * altered under the other and bypassed the encrypted-domain guard * (#772 review, finding 4). * * Only the IMPLICIT schema collapses. `"app"."users"` is a genuinely different @@ -512,7 +517,7 @@ function columnKey(table: string, column: string, schema?: string): string { /** What the migration corpus says about the columns it mentions. */ interface ColumnIndex { - /** Columns the corpus gives an ENCRYPTED type. Rewriting one drops ciphertext. */ + /** Columns the corpus gives an ENCRYPTED type; these need re-encryption. */ encrypted: Set /** Columns the corpus DECLARES at all, whatever type it gives them. */ declared: Set @@ -520,23 +525,22 @@ interface ColumnIndex { /** * 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. + * tell the change it exists for (plaintext → staged encrypted twin) from the + * cases it must leave for review: encrypted → encrypted, a change to a column + * it knows nothing about, and a duplicate staged target. * * **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. + * correct migration requires decrypting and re-encrypting the existing + * ciphertext; it cannot use the plaintext-to-twin rewrite. * * **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. + * simply live somewhere this sweep never reads. Requiring a POSITIVE + * declaration makes the unknown case a flagged statement instead of choosing + * a lifecycle on an assumption. * * Because the encrypted side is matched against the known domain list, the * plaintext side needs no type classification: a column the corpus declares but @@ -618,8 +622,10 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { export type SkipReason = /** Outside the strict matcher — hand-authored `USING`, or an unknown form. */ | 'unrecognised-form' - /** The column already holds an encrypted domain; rewriting drops ciphertext. */ + /** The column already holds an encrypted domain and needs re-encryption. */ | 'already-encrypted' + /** The conventional encrypted twin already exists or was staged earlier. */ + | 'target-exists' /** The corpus never declares the column, so its current type is unknown. */ | 'source-unknown' @@ -636,17 +642,19 @@ 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 - * three reasons need very different action from the user, and a single generic + * reasons need different action from the user, and a single generic * "could not rewrite automatically" hides that. */ export function describeSkipReason(reason: SkipReason): string { switch (reason) { case 'already-encrypted': - 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" + return "the column is ALREADY encrypted and has no plaintext source for the normal backfill. Changing an encrypted column's domain changes its index terms, so the ciphertext must be decrypted and re-encrypted through a reviewed staged `stash encrypt` lifecycle" + case 'target-exists': + return 'the staged encrypted target already exists (or was added by an earlier rewrite), so another ADD COLUMN would fail. Review the existing encrypted twin and use the staged `stash encrypt` lifecycle without creating a duplicate column' 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" + return "the sweep could not find where this column was declared in this migration directory, so it cannot tell a plaintext column (which needs an encrypted twin and staged backfill) from one that already holds ciphertext (which needs staged re-encryption). 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, then use the staged `stash encrypt` lifecycle; never replace the source column in place" } } @@ -658,9 +666,14 @@ export interface RewriteResult { skipped: SkippedAlter[] } +interface RewriteSweepError extends Error { + rewritten?: string[] + skipped?: SkippedAlter[] +} + /** * Replace in-place `ALTER COLUMN ... SET DATA TYPE ` - * statements with an ADD + DROP + RENAME sequence. + * statements with a staged encrypted-column addition. * * **Why this exists (CIP-2991, CIP-2994, #693):** Postgres has no implicit cast * from `text`/`numeric` to an encrypted domain, so `ALTER COLUMN ... SET DATA @@ -668,29 +681,25 @@ export interface RewriteResult { * `cannot cast type ... to `. This applies equally to the single EQL v2 * type and the whole EQL v3 concrete-domain family. * - * The rewrite is an ADD+DROP+RENAME, which is **equivalent to DROP+ADD**: it - * makes the column type valid but does NOT preserve the column's data. It is - * therefore safe ONLY on an EMPTY table. On a populated table the new column - * starts NULL and the original is dropped in the same migration, so the - * plaintext is destroyed. The commented UPDATE is a placeholder that can never - * become real SQL (the encrypted value is the EQL envelope produced by ZeroKMS - * via the client — there is no expression Postgres can evaluate to fill it), so - * a populated table must instead use the staged EQL v3 lifecycle (add an - * encrypted twin → dual-write → backfill via `@cipherstash/stack`'s - * `encryptModel` → switch the application to the encrypted column by name → - * drop plaintext), which keeps both columns alive across deploys. Each - * rewritten file carries a header comment saying exactly this. + * The rewrite adds a separate encrypted column and preserves the source column. + * PostgreSQL cannot create the EQL envelope itself, so the application must use + * the staged EQL v3 lifecycle (add an encrypted twin → dual-write → backfill via + * `@cipherstash/stack`'s `encryptModel` → switch the application to the + * encrypted column by name → drop plaintext), which keeps both columns alive + * across deploys. The rewriter emits only the ADD: every later step is + * intentionally a separate, reviewed operation. * * 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); 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 + * column that is ALREADY encrypted and needs re-encryption; ones whose staged + * encrypted target already exists; 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 + * migration history). All are left untouched on disk and surfaced to the + * caller; CLI and wizard callers then fail non-zero rather than silently + * shipping broken SQL. 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. */ @@ -710,6 +719,7 @@ export async function rewriteEncryptedAlterColumns( const rewritten: string[] = [] const skipped: SkippedAlter[] = [] const seen = new Set() + const stagedTargets = new Set() /** Record a skip once — the strict pass and the broad scan can both find it. */ const skip = (file: string, statement: string, reason: SkipReason): void => { @@ -735,116 +745,102 @@ export async function rewriteEncryptedAlterColumns( const { encrypted: encryptedColumns, declared: declaredColumns } = indexColumnDeclarations([...contents.values()]) - for (const [filePath, original] of contents) { - if (options.skip && filePath === options.skip) continue - - // Reset the regex's lastIndex — it's stateful on /g - ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 - - const updated = original.replace( - ALTER_COLUMN_TO_ENCRYPTED_RE, - ( - match: string, - first: string, - second: string | undefined, - column: string, - mangledType: string, - offset: number, - ) => { - // Commented-out SQL never runs, and a multi-line replacement would only - // inherit the `-- ` on its first line — leaving the rest live. - if (isInsideCommentOrString(original, offset)) return match - - const { schema, table } = tableOf(first, second) - - // Already encrypted: the ADD+DROP+RENAME would drop the ciphertext and - // there is no plaintext left to backfill from. Flag, never guess. - if (encryptedColumns.has(columnKey(table, column, schema))) { - skip(filePath, match.trim(), 'already-encrypted') - return match - } + try { + for (const [filePath, original] of contents) { + if (options.skip && filePath === options.skip) continue + + // Reset the regex's lastIndex — it's stateful on /g + ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 + + const updated = original.replace( + ALTER_COLUMN_TO_ENCRYPTED_RE, + ( + match: string, + first: string, + second: string | undefined, + column: string, + mangledType: string, + offset: number, + ) => { + // Commented-out SQL never runs, and a multi-line replacement would only + // inherit the `-- ` on its first line — leaving the rest live. + if (isInsideCommentOrString(original, offset)) return match + + const { schema, table } = tableOf(first, second) + + // Already encrypted: an in-place domain conversion needs a staged + // re-encryption. Flag, never guess. + if (encryptedColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'already-encrypted') + return match + } + + // Fail closed. Absence from `declaredColumns` does not prove the source + // type, so leave the raw statement untouched and require review. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + + const target = columnKey(table, `${column}_encrypted`, schema) + if (declaredColumns.has(target) || stagedTargets.has(target)) { + skip(filePath, match.trim(), 'target-exists') + 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. + if (!domain) return match + + stagedTargets.add(target) + return renderSafeAlter(table, column, domain, schema) + }, + ) + + if (updated !== original) { + await writeFile(filePath, updated, 'utf-8') + rewritten.push(filePath) + } - // 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 + // Broad secondary scan on the POST-rewrite content: anything still carrying + // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict + // matcher. Record it for the fail-closed caller rather than leave the user shipping SQL + // that fails at migrate time. + for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { + const statement = trimStatementPreamble(nearMiss[0]) + // Anchor the comment test on the `SET DATA TYPE` itself: the match starts + // at the previous `;`, so its own offset sits before any preamble. + const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) + if ( + isInsideCommentOrString( + updated, + nearMiss.index + Math.max(keyword, 0), + ) + ) { + continue } - - 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. - if (!domain) return match - - // This statement converts the column, so from here on in the corpus it - // holds CIPHERTEXT. `indexColumnDeclarations` cannot know that: it - // reads CREATE TABLE, ADD COLUMN and RENAME, never the strict matcher's - // own target. Without this, a corpus carrying an earlier conversion - // (`... SET DATA TYPE eql_v2_encrypted` from a stack version that - // predates this sweep) leaves the column looking plaintext, and the - // NEXT domain change drops the ciphertext — `declared` cannot catch it, - // because the column really is declared, as plaintext, by the original - // CREATE TABLE. - // - // Recorded here rather than in the corpus-wide index on purpose: the - // index has no order, so it would flag THIS conversion — the legitimate - // plaintext -> encrypted one — as already-encrypted too. Files are - // walked in sorted order and matches within a file in source order, so - // "already converted" means "converted by a statement that runs before - // this one". - encryptedColumns.add(columnKey(table, column, schema)) - return renderSafeAlter(table, column, domain, schema) - }, - ) - - if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') - rewritten.push(filePath) - } - - // Broad secondary scan on the POST-rewrite content: anything still carrying - // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict - // matcher. Flag it — non-fatally — rather than leave the user shipping SQL - // that fails at migrate time. - for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { - const statement = trimStatementPreamble(nearMiss[0]) - // Anchor the comment test on the `SET DATA TYPE` itself: the match starts - // at the previous `;`, so its own offset sits before any preamble. - const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) - if ( - isInsideCommentOrString(updated, nearMiss.index + Math.max(keyword, 0)) - ) { - continue + skip(filePath, statement, 'unrecognised-form') } - skip(filePath, statement, 'unrecognised-form') } + } catch (error) { + if (error instanceof Error) { + const partial = error as RewriteSweepError + partial.rewritten = rewritten + partial.skipped = skipped + } + throw error } return { rewritten, skipped } } /** - * The rewrite is identical for v2 and v3, and the ADD+DROP+RENAME sequence is - * equivalent to DROP+ADD: it makes the column type valid but does NOT preserve - * the column's data. It is therefore 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, so the plaintext is destroyed. - * - * The commented UPDATE is only a placeholder — it can never become real SQL. The - * encrypted value is the EQL envelope produced by ZeroKMS via the client - * (`encryptModel` / `bulkEncryptModels`); there is no expression Postgres can - * evaluate to fill it. (v3 stores that envelope as jsonb rather than v2's - * composite, but this is equally true on both surfaces.) - * - * So the guidance does NOT tell the user to backfill and run this migration — - * that would still lose data. It points a populated table at the staged EQL v3 - * lifecycle (add an encrypted twin → dual-write → backfill → switch the - * application to the encrypted column by name → drop plaintext), which keeps - * both columns alive across deploys. + * The source column remains live. The encrypted value is an EQL envelope + * produced by ZeroKMS, so PostgreSQL cannot backfill it with an SQL expression. + * The generated comment directs the user to the staged EQL v3 lifecycle, whose + * later backfill, application switch, and plaintext drop are explicit + * operations outside this automatic rewrite. */ function renderSafeAlter( table: string, @@ -852,25 +848,27 @@ function renderSafeAlter( domain: string, schema?: string, ): string { - const tmp = `${column}__cipherstash_tmp` + const encrypted = `${column}_encrypted` // Preserve the schema qualifier drizzle-kit emitted for pgSchema() tables so // the rewritten statements target the same object. const qualifiedTable = schema ? `"${schema}"."${table}"` : `"${table}"` return [ '-- Rewritten by stash: in-place ALTER COLUMN cannot cast to', - `-- ${domain}. This ADD+DROP+RENAME equals DROP+ADD and is safe ONLY if`, - `-- ${qualifiedTable} is empty. On a populated table it DESTROYS existing "${column}"`, - '-- data (the new column starts NULL) — do NOT run it there. Use the staged', - '-- EQL v3 path instead: add an encrypted twin -> dual-write -> backfill via', - '-- encryptModel -> switch the app to the encrypted column -> drop plaintext.', - '-- NOTE: constraints, defaults, and indexes on the original column are NOT', - '-- carried over by this ADD/DROP/RENAME — re-add any NOT NULL, DEFAULT,', - '-- UNIQUE, or index definitions manually.', - `ALTER TABLE ${qualifiedTable} ADD COLUMN "${tmp}" "public"."${domain}";`, - `-- UPDATE ${qualifiedTable} SET "${tmp}" = /* encrypted value for ${column} */ NULL`, - '--> statement-breakpoint', - `ALTER TABLE ${qualifiedTable} DROP COLUMN "${column}";`, - '--> statement-breakpoint', - `ALTER TABLE ${qualifiedTable} RENAME COLUMN "${tmp}" TO "${column}";`, + `-- ${domain}. The source column "${column}" is deliberately preserved.`, + '-- Use the staged `stash encrypt` lifecycle: dual-write to this new column,', + "-- backfill it via @cipherstash/stack's encryptModel, then", + '-- switch the application to the encrypted column by name and drop the', + '-- plaintext column — both separately reviewed steps taken only after the', + '-- backfill is complete.', + // The domain is emitted as `"public".""` unconditionally: EQL + // installs its domains into `public` (both `stash eql install` and the + // adapters' baseline migrations do), and the qualifier makes the rewrite + // independent of the session `search_path`. It is an ASSUMPTION, not + // something read back from the matched SQL — the `schema` capture above is + // the TABLE's schema (from a pgSchema() table) and says nothing about where + // the domain lives. If EQL ever supports installing into a non-`public` + // schema, this needs the install schema threaded in, here and in the + // sibling `packages/wizard/src/lib/rewrite-migrations.ts`. + `ALTER TABLE ${qualifiedTable} ADD COLUMN "${encrypted}" "public"."${domain}";`, ].join('\n') } diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index c0193b665..6b21ffcfc 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -7,7 +7,7 @@ import { writeFileSync, } from 'node:fs' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { join, resolve } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { CliExit } from '../../../cli/exit.js' import { messages } from '../../../messages.js' @@ -66,6 +66,35 @@ vi.mock('node:fs', async (importOriginal) => { return { ...actual, default: actual, writeFileSync: fsWrite.spy } }) +// Pin the detected package manager so the argv assertion below can name the +// exact runner prefix. Detection walks the real filesystem, so without this the +// expected argv would have to be computed from `execArgv` — which makes the +// assertion tautological and unable to catch a regression back to the +// download-and-run (`dlx`) form. Everything else in `utils.js` stays real. +vi.mock('@/commands/init/utils.js', async (importOriginal) => ({ + ...(await importOriginal()), + detectPackageManager: () => 'pnpm', +})) + +// The sweep stays REAL by default — every other sweep test drives it through +// actual SQL on disk. The spy exists so the "sweep threw" branch can be reached +// with a throw the sweep itself never produces (a bare string, `null`), which is +// the case the partial-result reporting has to survive without masking. +const rewriteMock = vi.hoisted(() => ({ + real: (() => { + throw new Error( + 'rewriteMock.real not initialised: rewrite-migrations mock factory did not run', + ) + }) as typeof import('../../db/rewrite-migrations.js').rewriteEncryptedAlterColumns, + spy: vi.fn(), +})) +vi.mock('../../db/rewrite-migrations.js', async (importOriginal) => { + const actual = + await importOriginal() + rewriteMock.real = actual.rewriteEncryptedAlterColumns + return { ...actual, rewriteEncryptedAlterColumns: rewriteMock.spy } +}) + // `printNextSteps` lives in the install module, which drags in `pg`. Stub it; // the two helpers we reuse (`findGeneratedMigration`, `cleanupMigrationFile`) // stay real and act on the tmpdir. @@ -76,6 +105,7 @@ vi.mock('../../db/install.js', async (importOriginal) => { beforeEach(() => { fsWrite.spy.mockImplementation(fsWrite.real) + rewriteMock.spy.mockImplementation(rewriteMock.real) }) afterEach(() => { vi.clearAllMocks() @@ -170,6 +200,32 @@ describe('eqlMigrationCommand — Drizzle', () => { expect(spawnMock).not.toHaveBeenCalled() }) + it.each([ + ['command substitution', 'a$(whoami)'], + ['backticks', 'a`id`'], + ['a path separator', '../escape'], + // `''` is not nullish, so it slips past `options.name ?? DEFAULT` and hits + // the regex, where `+` rejects it — `--name ''` aborts, it does NOT fall + // back to `install-eql`. The one input where "empty" and "absent" diverge. + ['an empty string', ''], + ])('rejects %s in --name', async (_label, name) => { + await expect( + eqlMigrationCommand({ drizzle: true, name, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + expect(spawnMock).not.toHaveBeenCalled() + }) + + // Ordering invariant: name validation sits ABOVE the dry-run preview, so a + // bad name aborts before anything is rendered. Move the validation below the + // preview and an unvalidated name ships into the note — with no other test + // combining the two, that refactor would pass CI. + it('rejects an unsafe name in a dry run too (validation precedes the preview)', async () => { + await expect( + eqlMigrationCommand({ drizzle: true, name: 'x; ls', dryRun: true }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.note).not.toHaveBeenCalled() + }) + it('dry run neither spawns drizzle-kit nor creates the out directory', async () => { const out = join(tmp, 'drizzle') await eqlMigrationCommand({ drizzle: true, dryRun: true, out }) @@ -183,6 +239,28 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) + it('includes --out in the dry-run preview', async () => { + const out = join(tmp, 'custom-out') + await eqlMigrationCommand({ drizzle: true, dryRun: true, out }) + expect(spawnMock).not.toHaveBeenCalled() + expect(clack.note).toHaveBeenCalledWith( + expect.stringContaining(`--out=${out}`), + 'Dry Run', + ) + }) + + // Widest blast radius of the flag handling: because `--out` is ALWAYS + // appended to drizzle-kit's argv, a flag-less invocation silently overrides + // the project's drizzle.config.ts `out` with `/drizzle`. The dry-run + // preview reaches that arm without spawning or touching the filesystem. + it('defaults --out to an absolute drizzle/ when the flag is omitted', async () => { + await eqlMigrationCommand({ drizzle: true, dryRun: true }) + expect(clack.note).toHaveBeenCalledWith( + expect.stringContaining(`--out=${resolve('drizzle')}`), + 'Dry Run', + ) + }) + it('dry run says the grants would be included under --supabase', async () => { await eqlMigrationCommand({ drizzle: true, @@ -207,11 +285,24 @@ describe('eqlMigrationCommand — Drizzle', () => { await eqlMigrationCommand({ drizzle: true, name: 'add-eql', out }) - // argv array, never a shell string — the name/out are discrete tokens. - const [, argv] = spawnMock.mock.calls[0] - expect(argv).toContain('drizzle-kit') - expect(argv).toContain('--name=add-eql') - expect(argv).toContain(`--out=${out}`) + expect(spawnMock).toHaveBeenCalledTimes(1) + const [command, argv] = spawnMock.mock.calls[0] + // The whole argv, exactly — not `toContain` checks, which would still pass + // if the runner prefix (`exec`) were dropped and drizzle-kit ran under the + // wrong resolver. Three things at once: name and out are discrete inert + // tokens in an array, never interpolated into a shell string; `--out` is + // actually passed, so drizzle-kit writes where step 2 then looks; and the + // project-local `exec` form (not `dlx`) is asserted, so a regression back to + // download-and-run — which resolves a different drizzle.config.ts — fails. + expect(command).toBe('pnpm') + expect(argv).toEqual([ + 'exec', + 'drizzle-kit', + 'generate', + '--custom', + '--name=add-eql', + `--out=${out}`, + ]) const written = readFileSync(join(out, '0000_add-eql.sql'), 'utf-8') expect(written).toContain('EQL v3 schema creation') @@ -221,8 +312,8 @@ describe('eqlMigrationCommand — Drizzle', () => { // drizzle-kit emits an un-runnable in-place `ALTER COLUMN ... SET DATA TYPE` // when a plaintext column is changed to an encrypted one. `eql install // --drizzle` has always swept the out directory for these; the v3 - // migration-first path must do the same, or a v3 user is left with a broken - // migration and nothing to fix it (#693). + // migration-first path must do the same, but the rewrite is now add-only and + // fails closed when it cannot prove the source column. it('rewrites a sibling migration with a broken v3 ALTER COLUMN', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) @@ -248,9 +339,18 @@ describe('eqlMigrationCommand — Drizzle', () => { const rewritten = readFileSync(sibling, 'utf-8') expect(rewritten).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) expect(rewritten).not.toContain('SET DATA TYPE') + // The add-only invariant, at the COMMAND level. Both rewriter unit suites + // pin it too, but only this asserts what the command actually leaves on + // disk — the wiring between them is where a regression would hide. + expect(rewritten).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + // The success line is the user's only signal that the sweep did anything. + const info = clack.log.info.mock.calls.map((c) => String(c[0])) + expect(info.some((msg) => msg.includes('Rewrote 1 migration file'))).toBe( + true, + ) }) it('does not rewrite the EQL install migration it just generated', async () => { @@ -311,15 +411,17 @@ describe('eqlMigrationCommand — Drizzle', () => { return { status: 0, stdout: '', stderr: '' } }) - await eqlMigrationCommand({ drizzle: true, out }) + await expect( + eqlMigrationCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) // The near-miss statement is left untouched... expect(readFileSync(sibling, 'utf-8')).toContain('SET DATA TYPE') - // ...and the closing note warns the sweep did not fully complete. - const warned = clack.log.warn.mock.calls.map((c) => String(c[0])) - expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe( - true, + // ...and the command fails closed with the sweep error. + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('unsafe or unverified SQL'), ) + expect(clack.outro).toHaveBeenCalledWith('Migration aborted.') }) // A column the corpus never declares is fail-closed to `source-unknown`: left @@ -340,12 +442,15 @@ describe('eqlMigrationCommand — Drizzle', () => { return { status: 0, stdout: '', stderr: '' } }) - await eqlMigrationCommand({ drizzle: true, out }) + await expect( + eqlMigrationCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) // Left exactly as written — a source-unknown statement is never rewritten. expect(readFileSync(sibling, 'utf-8')).toBe(unsafeAlter) // The per-statement report reached the user with the source-unknown - // remediation (the whole point of the reason), not a crash into the catch. + // remediation (the whole point of the reason), and the command failed + // closed instead of continuing. const stepped = clack.log.step.mock.calls.map((c) => String(c[0])) expect(stepped.some((msg) => msg.includes(sibling))).toBe(true) expect( @@ -353,10 +458,36 @@ describe('eqlMigrationCommand — Drizzle', () => { msg.includes("Check the column's current type in the database"), ), ).toBe(true) - // And the closing note warns the sweep did not fully complete. - const warned = clack.log.warn.mock.calls.map((c) => String(c[0])) - expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe( - true, + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('unsafe or unverified SQL'), + ) + }) + + // The catch reads `rewritten` / `skipped` off the thrown value to report the + // work a partial sweep did complete (#786). A throw that is not an object — + // or not one carrying those arrays — must fall through to the plain "could + // not sweep" message and still fail closed, not crash on a property read. + it.each([ + null, + undefined, + 'rewrite failed', + ])('handles a non-object sweep failure without masking it: %s', async (failure) => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0000_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + rewriteMock.spy.mockRejectedValueOnce(failure) + + await expect( + eqlMigrationCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.warn).toHaveBeenCalledWith( + expect.stringContaining('Could not sweep'), + ) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('unsafe or unverified SQL'), ) }) @@ -368,6 +499,59 @@ describe('eqlMigrationCommand — Drizzle', () => { expect(clack.log.error).toHaveBeenCalledWith('boom') }) + it('reports the spawn error when drizzle-kit cannot be launched', async () => { + // spawnSync's ENOENT shape: null status, no captured stderr, `error` set. + // `result.stderr?.trim()` is undefined, so the message falls through to the + // second arm (`result.error?.message`) — the realistic "drizzle-kit isn't + // installed" case. If the `?.` on stderr were dropped, this shape would + // throw a TypeError instead of reporting. + spawnMock.mockReturnValue({ + status: null, + stdout: null, + stderr: null, + error: Object.assign(new Error('spawnSync pnpm ENOENT'), { + code: 'ENOENT', + }), + }) + + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith('spawnSync pnpm ENOENT') + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining('Make sure drizzle-kit is installed'), + ) + }) + + it('falls back to the exit status when there is no stderr or spawn error', async () => { + // Third arm of the fallback chain: non-zero status, empty stderr, no + // `error`. The user still gets a message instead of a blank error line. + spawnMock.mockReturnValue({ status: 2, stdout: '', stderr: '' }) + + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith( + 'drizzle-kit exited with status 2.', + ) + }) + + it('points at --out when the generated migration is not where we looked', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + // drizzle-kit "succeeds" but wrote to its own drizzle.config.ts `out`, not + // ours, so step 2 finds nothing and must abort with the remediation hint + // rather than a bare "could not find a migration". + spawnMock.mockReturnValue({ status: 0, stdout: '', stderr: '' }) + + await expect( + eqlMigrationCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.info).toHaveBeenCalledWith( + 'If your drizzle.config.ts writes elsewhere, pass --out so it matches.', + ) + }) + it('removes the scaffolded migration when writing the SQL fails', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index e68cb0b44..ff0633bcc 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -9,6 +9,7 @@ import { printNextSteps, SAFE_MIGRATION_NAME } from '@/commands/db/install.js' import { describeSkipReason, rewriteEncryptedAlterColumns, + type SkippedAlter, } from '@/commands/db/rewrite-migrations.js' import { detectPackageManager, @@ -21,6 +22,32 @@ import { messages } from '@/messages.js' const DEFAULT_MIGRATION_NAME = 'install-eql' const DEFAULT_DRIZZLE_OUT = 'drizzle' +/** + * What {@link rewriteEncryptedAlterColumns} attaches to the error when the + * sweep throws partway through: the files it had already rewritten, and the + * statements it had already flagged. Reported so a partial sweep names the + * work it did rather than looking like it never ran (#786). + */ +interface PartialRewriteResult { + rewritten?: string[] + skipped?: SkippedAlter[] +} + +/** + * The sweep can also fail with a non-`Error` throw — a string, `null`, anything + * — in which case there is no partial result to report. Narrow rather than cast + * so those cases fall through to the plain "could not sweep" message instead of + * crashing on a property read of a non-object. + */ +function isPartialRewriteResult(value: unknown): value is PartialRewriteResult { + if (typeof value !== 'object' || value === null) return false + const partial = value as Record + return ( + (partial.rewritten === undefined || Array.isArray(partial.rewritten)) && + (partial.skipped === undefined || Array.isArray(partial.skipped)) + ) +} + /** Find the most recently generated Drizzle migration matching the name. */ export async function findGeneratedMigration( outDir: string, @@ -255,15 +282,10 @@ async function generateDrizzleEqlMigration( // Step 4 — sweep for sibling migrations drizzle-kit emitted with an in-place // `ALTER COLUMN ... SET DATA TYPE `. Those fail in Postgres // (no implicit cast from text/numeric to an EQL domain), so rewrite them into - // an ADD+DROP+RENAME sequence that is runnable. That sequence is equivalent to - // DROP+ADD — safe on an EMPTY table but data-destroying on a populated one — - // so the rewritten file carries a comment steering populated tables to the - // staged `stash encrypt` path. `eql install --drizzle` has always done this - // for v2; without it the v3 migration-first path leaves the user with broken - // SQL and no repair (#693). - // Whether the sweep failed outright or left near-misses it couldn't rewrite. - // Either way the user must review sibling migrations before running migrate, - // so surface it again at the closing note (below) — not just inline here. + // a staged encrypted-column addition that preserves the source column. + // Whether the sweep failed outright or left near-misses it couldn't rewrite, + // the user must review sibling migrations before running migrate, so surface + // it again at the closing note (below) — not just inline here. let sweepIncomplete = false try { const { rewritten, skipped } = await rewriteEncryptedAlterColumns(outDir, { @@ -271,7 +293,7 @@ async function generateDrizzleEqlMigration( }) if (rewritten.length > 0) { p.log.info( - `Rewrote ${rewritten.length} migration file(s) into a runnable ADD+DROP+RENAME for encrypted columns (safe on empty tables; see each file's header before running against populated data):`, + `Rewrote ${rewritten.length} migration file(s) to add staged encrypted columns while preserving the source columns:`, ) for (const file of rewritten) p.log.step(` - ${file}`) } @@ -288,6 +310,24 @@ async function generateDrizzleEqlMigration( } catch (error) { // Advisory: the install migration itself is already written and valid. sweepIncomplete = true + const partial: PartialRewriteResult = isPartialRewriteResult(error) + ? error + : {} + if (partial.rewritten && partial.rewritten.length > 0) { + p.log.info( + `Rewrote ${partial.rewritten.length} migration file(s) before the sweep stopped:`, + ) + for (const file of partial.rewritten) p.log.step(` - ${file}`) + } + if (partial.skipped && partial.skipped.length > 0) { + p.log.warn( + `Found ${partial.skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, + ) + for (const { file, statement, reason } of partial.skipped) { + p.log.step(` - ${file}: ${statement}`) + p.log.step(` ${describeSkipReason(reason)}`) + } + } p.log.warn( `Could not sweep ${outDir} for unsafe ALTER COLUMN statements: ${ error instanceof Error ? error.message : String(error) @@ -295,12 +335,14 @@ async function generateDrizzleEqlMigration( ) } - p.log.success(`Migration created: ${migrationPath}`) if (sweepIncomplete) { - p.log.warn( - `The ALTER COLUMN sweep did not fully complete — review the sibling migrations in ${outDir} before running drizzle-kit migrate, or you may apply broken/unsafe SQL.`, + p.log.error( + `The ALTER COLUMN sweep found unsafe or unverified SQL. The generated migration remains at ${migrationPath}, but review the sibling migrations in ${outDir} and use the staged stash encrypt flow before running drizzle-kit migrate.`, ) + if (!embedded) p.outro('Migration aborted.') + throw new CliExit(1) } + p.log.success(`Migration created: ${migrationPath}`) p.note( `Run your Drizzle migrations to install EQL v3:\n\n ${execCommand(pm)} drizzle-kit migrate`, 'Next Steps', diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index 0816129a6..799eda070 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -145,7 +145,7 @@ describe('runPostAgentSteps execution commands', () => { // The sweep's own warning says "do NOT run the migration" on a populated table. // Defaulting the very next prompt to Yes invites the mistake the warning exists // to prevent — so a sweep that touched anything flips the default to No. -describe('drizzle migrate prompt after a destructive rewrite', () => { +describe('drizzle migrate prompt after a staged rewrite', () => { let cwd: string const runDrizzle = () => @@ -163,8 +163,8 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { /** * Make `dir` a drizzle-kit OUTPUT directory. The sweep now requires the * `meta/_journal.json` drizzle-kit maintains, because `migrations/` and - * `src/db/migrations/` are generic names other tools use and this sweep - * emits `DROP COLUMN`. + * `src/db/migrations/` are generic names other tools use, so the wizard must + * not edit them unless drizzle-kit's journal proves ownership. */ const makeDrizzleOut = (dir: string): string => { const abs = path.join(cwd, dir) @@ -198,18 +198,15 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(true) - // The clean arm is the last of a 4-way nested ternary; the negative - // assertions that guard the other arms' wording never run against this one. - // A mis-nested arm here would ship a destruction/flag/unchecked warning to a - // user with nothing to sweep, and asserting only `initialValue` would miss - // it — so pin that the clean message carries none of those phrases. + // Pin that the clean message carries none of the stale destructive or + // fail-closed wording. const message = String(options?.message) expect(message).not.toContain('DESTROYS data') expect(message).not.toContain('flagged for review') expect(message).not.toContain('could not check') }) - it('defaults to No, and says why, when a file was rewritten', async () => { + it('defaults to Yes and explains the staged addition when a file was rewritten', async () => { makeDrizzleOut('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 @@ -228,59 +225,59 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { await runDrizzle() 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. + expect(options?.initialValue).toBe(true) + expect(String(options?.message)).toContain('staged encrypted columns') + expect(String(options?.message)).toContain('preserves the source column') + + // Pin the on-disk effect so a skipped source-unknown statement cannot make + // this prompt test pass accidentally. const swept = fs.readFileSync( path.join(cwd, 'drizzle', '0001_encrypt.sql'), 'utf-8', ) - expect(swept).toContain('DROP COLUMN') + expect(swept).toContain('ADD COLUMN "email_encrypted"') + expect(swept).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) expect(swept).not.toContain('SET DATA TYPE') }) - it('defaults to No when a statement was flagged rather than rewritten', async () => { + it('fails before prompting when a statement was flagged rather than rewritten', async () => { + const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) makeDrizzleOut('drizzle') fs.writeFileSync( path.join(cwd, 'drizzle', '0001_using.sql'), 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search USING (email)::eql_v3_text_search;\n', ) - await runDrizzle() - - 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') + await expect(runDrizzle()).rejects.toThrow('unsafe or unverified SQL') + expect(p.confirm).not.toHaveBeenCalled() + expect( + warn.mock.calls.filter(([message]) => + String(message).includes('rewrite left alone'), + ), + ).toHaveLength(1) + warn.mockRestore() }) // A directory whose sweep threw contributes 0 to both totals, so a failed // sweep used to be indistinguishable from a clean one: prompt defaulting to // Yes over migrations nobody checked. Unknown is not the same as safe. - it('defaults to No when a directory could not be swept at all', async () => { + it('fails before prompting when a directory could not be swept at all', async () => { + const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) // A directory named `*.sql` makes readFile throw EISDIR mid-sweep. makeDrizzleOut('drizzle') fs.mkdirSync(path.join(cwd, 'drizzle', '0001_alter.sql')) - await runDrizzle() - - const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] - expect(options?.initialValue).toBe(false) - // Nothing is known about that directory, so the prompt must not claim the - // migration destroys data — only that it went unchecked. - expect(String(options?.message)).not.toContain('DESTROYS data') - expect(String(options?.message)).toContain('drizzle/') - expect(String(options?.message)).toContain('could not check 1 directory') + await expect(runDrizzle()).rejects.toThrow('unsafe or unverified SQL') + expect(p.confirm).not.toHaveBeenCalled() + expect( + warn.mock.calls.filter(([message]) => + String(message).includes('Could not rewrite migrations'), + ), + ).toHaveLength(1) + expect(warn).not.toHaveBeenCalledWith( + expect.stringContaining('Could not fully rewrite migrations'), + ) + warn.mockRestore() }) // `error` is built as `err instanceof Error ? err.message : String(err)`, and @@ -293,11 +290,8 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { { dir: 'drizzle', rewritten: [], skipped: [], error: '' }, ]) - await runDrizzle() - - const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] - expect(options?.initialValue).toBe(false) - expect(String(options?.message)).toContain('could not check 1 directory') + await expect(runDrizzle()).rejects.toThrow('unsafe or unverified SQL') + expect(p.confirm).not.toHaveBeenCalled() }) // The wizard ships scanning drizzle/, migrations/ and src/db/migrations/ and @@ -332,18 +326,18 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { path.join(cwd, 'migrations', '0001_encrypt.sql'), 'utf-8', ) - expect(swept).toContain('DROP COLUMN') + expect(swept).toContain('ADD COLUMN "email_encrypted"') + expect(swept).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) expect(swept).not.toContain('SET DATA TYPE') const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] - expect(options?.initialValue).toBe(false) - expect(String(options?.message)).toContain('DESTROYS data') + expect(options?.initialValue).toBe(true) + expect(String(options?.message)).toContain('staged encrypted columns') }) // The other half of the test above. `migrations/` is swept when it is a // drizzle output directory — and must NOT be when it belongs to Knex, // node-pg-migrate, Flyway or hand-rolled psql, all of which use that name. - // The wizard was never pointed at such a directory, and this sweep emits - // DROP COLUMN (#772 review, finding 5). + // The wizard was never pointed at such a directory (#772 review, finding 5). it('leaves a migrations/ directory that is not a drizzle output untouched', async () => { makeDrizzleOut('drizzle') fs.writeFileSync( @@ -398,19 +392,17 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { 'ALTER TABLE "orders" ALTER COLUMN "total" SET DATA TYPE eql_v3_text_search;\n' fs.writeFileSync(flagged, flaggedSql) - await runDrizzle() + await expect(runDrizzle()).rejects.toThrow('unsafe or unverified SQL') // drizzle/ was rewritten... const rewritten = fs.readFileSync( path.join(cwd, 'drizzle', '0001_encrypt.sql'), 'utf-8', ) - expect(rewritten).toContain('DROP COLUMN') + expect(rewritten).toContain('ADD COLUMN "email_encrypted"') + expect(rewritten).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) // ...and migrations/ was left untouched, flagged rather than rewritten. expect(fs.readFileSync(flagged, 'utf-8')).toBe(flaggedSql) - // A rewrite anywhere makes the prompt destructive and defaults it to No. - const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] - expect(options?.initialValue).toBe(false) - expect(String(options?.message)).toContain('DESTROYS data') + expect(p.confirm).not.toHaveBeenCalled() }) }) diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 5cefe86e2..aaecaa957 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1,18 +1,31 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { describeSkipReason, rewriteEncryptedAlterColumns, sweepMigrationDirs, } from '../lib/rewrite-migrations.js' +const fsPromisesWrite = vi.hoisted(() => ({ + real: (() => { + throw new Error('fsPromisesWrite.real not initialised') + }) as typeof import('node:fs/promises').writeFile, + spy: vi.fn(), +})) +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + fsPromisesWrite.real = actual.writeFile + return { ...actual, writeFile: fsPromisesWrite.spy } +}) + describe('rewriteEncryptedAlterColumns', () => { let tmpDir: string beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-rewrite-')) + fsPromisesWrite.spy.mockImplementation(fsPromisesWrite.real) }) afterEach(() => { @@ -50,13 +63,13 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "transactions" ADD COLUMN "amount__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "transactions" ADD COLUMN "amount_encrypted" "public"."eql_v2_encrypted";', ) - expect(updated).toContain( + expect(updated).not.toContain( 'ALTER TABLE "transactions" DROP COLUMN "amount";', ) - expect(updated).toContain( - 'ALTER TABLE "transactions" RENAME COLUMN "amount__cipherstash_tmp" TO "amount";', + expect(updated).not.toContain( + 'ALTER TABLE "transactions" RENAME COLUMN "amount_encrypted" TO "amount";', ) expect(updated).not.toContain('SET DATA TYPE') // Wizard-branded header. @@ -74,7 +87,7 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -90,7 +103,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -110,11 +123,13 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') // Every emitted statement keeps the schema qualifier. expect(updated).toContain( - 'ALTER TABLE "app"."users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "app"."users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) - expect(updated).toContain('ALTER TABLE "app"."users" DROP COLUMN "email";') - expect(updated).toContain( - 'ALTER TABLE "app"."users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + expect(updated).not.toContain( + 'ALTER TABLE "app"."users" DROP COLUMN "email";', + ) + expect(updated).not.toContain( + 'ALTER TABLE "app"."users" RENAME COLUMN "email_encrypted" TO "email";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -130,7 +145,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "transactions" ADD COLUMN "amount__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "transactions" ADD COLUMN "amount_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -146,7 +161,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "transactions" ADD COLUMN "description__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "transactions" ADD COLUMN "description_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -230,11 +245,11 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - `ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."${domain}";`, + `ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."${domain}";`, ) - expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') - expect(updated).toContain( - 'ALTER TABLE "users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + expect(updated).not.toContain('ALTER TABLE "users" DROP COLUMN "email";') + expect(updated).not.toContain( + 'ALTER TABLE "users" RENAME COLUMN "email_encrypted" TO "email";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -258,7 +273,7 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - `ALTER TABLE "t" ADD COLUMN "c__cipherstash_tmp" "public"."${domain}";`, + `ALTER TABLE "t" ADD COLUMN "c_encrypted" "public"."${domain}";`, ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -296,7 +311,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) expect(updated).not.toContain('SET DATA TYPE') }) @@ -315,7 +330,7 @@ describe('rewriteEncryptedAlterColumns', () => { expect(updated).toContain('-- eql_v3_integer_ord.') }) - it('warns that the rewrite is data-destroying / empty-table-only', async () => { + it('explains that the source column is preserved for the staged lifecycle', async () => { declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0016_warn.sql') fs.writeFileSync( @@ -326,14 +341,13 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) const updated = fs.readFileSync(filePath, 'utf-8') - expect(updated).toContain('safe ONLY if') - expect(updated).toContain('constraints, defaults, and indexes') - expect(updated).toContain('EQL v3 path') - expect(updated).toContain('dual-write') - expect(updated).toContain('switch the app to the encrypted column') + expect(updated).toContain('source column "email" is deliberately preserved') + expect(updated).toContain('staged `stash encrypt` lifecycle') + expect(updated).toContain('switch the application to the') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) }) - it('separates ADD/DROP/RENAME with --> statement-breakpoint', async () => { + it('emits one executable ADD and no cutover statements', async () => { declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0018_breakpoint.sql') fs.writeFileSync( @@ -344,11 +358,14 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) const updated = fs.readFileSync(filePath, 'utf-8').trimEnd() - const chunks = updated.split('--> statement-breakpoint') - expect(chunks).toHaveLength(3) - expect(chunks[0]).toContain('ADD COLUMN') - expect(chunks[1]).toContain('DROP COLUMN') - expect(chunks[2]).toContain('RENAME COLUMN') + const execLines = updated + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith('--')) + expect(execLines).toEqual([ + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', + ]) + expect(updated).not.toContain('--> statement-breakpoint') }) it('rewrites each statement to its own domain when v2 and v3 are mixed', async () => { @@ -366,10 +383,10 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "a" ADD COLUMN "x__cipherstash_tmp" "public"."eql_v2_encrypted";', + 'ALTER TABLE "a" ADD COLUMN "x_encrypted" "public"."eql_v2_encrypted";', ) expect(updated).toContain( - 'ALTER TABLE "a" ADD COLUMN "y__cipherstash_tmp" "public"."eql_v3_json";', + 'ALTER TABLE "a" ADD COLUMN "y_encrypted" "public"."eql_v3_json";', ) }) @@ -647,9 +664,9 @@ describe('rewriteEncryptedAlterColumns', () => { const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([filePath]) - expect(fs.readFileSync(filePath, 'utf-8')).toContain( - 'ALTER TABLE "users" DROP COLUMN "email";', - ) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) }) // An apostrophe inside a DOUBLE-QUOTED identifier is not a string @@ -839,12 +856,137 @@ describe('rewriteEncryptedAlterColumns', () => { const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([filePath]) - expect(fs.readFileSync(filePath, 'utf-8')).toContain('DROP COLUMN') + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toContain('DROP COLUMN') + }) + + it.each([ + ['tagged', 'price$usd$cents'], + ['untagged', 'price$$cents'], + ])('does not treat a %s dollar delimiter inside an unquoted identifier as a dollar-quoted body', async (_kind, identifier) => { + declarePlaintext('"users"', 'email') + const filePath = path.join(tmpDir, `0033_${_kind}-identifier.sql`) + fs.writeFileSync( + filePath, + [ + `SELECT ${identifier} FROM "prices";`, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + expect(skipped).toEqual([]) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toContain('SET DATA TYPE') }) }) - // ADD+DROP+RENAME on a column that is ALREADY encrypted drops CIPHERTEXT, and - // unlike the plaintext case there is nothing left anywhere to backfill from. + describe('issue #811 dollar-quoted DDL regression', () => { + const domainChange = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_eq";' + + it('does not emit destructive SQL for the reported two-file DO $$ rename corpus', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("id" serial PRIMARY KEY NOT NULL, "email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" RENAME COLUMN "email" TO "email_old";', + ' ALTER TABLE "users" RENAME COLUMN "email_encrypted" TO "email";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'target-exists' }, + ]) + }) + + it('does not emit destructive SQL when an encrypted ADD COLUMN is inside DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" ADD COLUMN "email" "eql_v3_text_search";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + const updated = fs.readFileSync(change, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + }) + + it('does not emit destructive SQL for a rename inside a custom dollar tag', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + 'DO $stash$ BEGIN', + ' ALTER TABLE "users" RENAME COLUMN "email" TO "email_old";', + ' ALTER TABLE "users" RENAME COLUMN "email_encrypted" TO "email";', + 'END $stash$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'target-exists' }, + ]) + }) + + it('does not emit destructive SQL when an unterminated $$ hides a later encrypted declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'SELECT $$unterminated;', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + const updated = fs.readFileSync(change, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + }) + }) + + // A domain change on a column that is ALREADY encrypted needs staged + // re-encryption; there is no plaintext source to backfill from. describe('columns that are already encrypted', () => { it('refuses to rewrite a domain change on a column created encrypted', async () => { const create = path.join(tmpDir, '0000_create.sql') @@ -1131,6 +1273,25 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].reason).toBe('already-encrypted') }) + it('fails closed when the staged encrypted twin already exists', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("email" text, "email_encrypted" eql_v3_text_eq);\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(fs.readFileSync(alter, 'utf-8')).toBe(`${alterSql}\n`) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'target-exists' }, + ]) + }) + // #772 review, finding 3. The index reads CREATE TABLE, ADD COLUMN and // RENAME, but never the strict matcher's OWN target. So a corpus where an // earlier migration already converted the column — realistic wherever a @@ -1155,11 +1316,13 @@ describe('rewriteEncryptedAlterColumns', () => { // The first conversion is the legitimate plaintext -> encrypted one and // must still happen — flagging it too would be the naive fix. expect(rewritten).toEqual([first]) - expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN') - // The second targets ciphertext. Left byte-identical on disk. + expect(fs.readFileSync(first, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + // The second would add the same target again. Left byte-identical. expect(fs.readFileSync(second, 'utf-8')).toBe(`${secondSql}\n`) expect(skipped).toEqual([ - { file: second, statement: secondSql, reason: 'already-encrypted' }, + { file: second, statement: secondSql, reason: 'target-exists' }, ]) }) @@ -1181,11 +1344,12 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([filePath]) const updated = fs.readFileSync(filePath, 'utf-8') - // Exactly one conversion was applied, and the v3 statement survives verbatim. - expect(updated.match(/DROP COLUMN/g)?.length).toBe(1) + // Exactly one target was staged, and the v3 statement survives verbatim. + expect(updated.match(/ADD COLUMN/g)?.length).toBe(1) + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) expect(updated).toContain(secondSql) expect(skipped).toEqual([ - { file: filePath, statement: secondSql, reason: 'already-encrypted' }, + { file: filePath, statement: secondSql, reason: 'target-exists' }, ]) }) @@ -1227,7 +1391,9 @@ describe('rewriteEncryptedAlterColumns', () => { const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([live]) - expect(fs.readFileSync(live, 'utf-8')).toContain('DROP COLUMN') + const updated = fs.readFileSync(live, 'utf-8') + expect(updated).toContain('ADD COLUMN "email_encrypted"') + expect(updated).not.toContain('DROP COLUMN') }) // #772 review, finding 4. `columnKey` keys on the schema exactly as written, @@ -1415,11 +1581,46 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated.match(/ADD COLUMN/g)?.length).toBe(2) - expect(updated.match(/DROP COLUMN/g)?.length).toBe(2) + expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) // Non-matching statement preserved expect(updated).toContain('CREATE INDEX "a_z" ON "a" ("z");') }) + it('reports files rewritten before a later write failure', async () => { + declarePlaintext('"users"', 'email', 'name') + const first = path.join(tmpDir, '0001_email.sql') + const failing = path.join(tmpDir, '0002_name.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + fs.writeFileSync( + failing, + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + fsPromisesWrite.spy.mockImplementation(async (file, data, options) => { + if (file === failing) { + fs.unlinkSync(failing) + fs.mkdirSync(failing) + } + return fsPromisesWrite.real(file, data, options) + }) + + try { + await rewriteEncryptedAlterColumns(tmpDir) + throw new Error('expected rewriteEncryptedAlterColumns to throw') + } catch (error) { + const partial = error as { rewritten?: string[]; skipped?: unknown[] } + expect(partial.rewritten).toEqual([first]) + expect(partial.skipped).toEqual([]) + expect(fs.readFileSync(first, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + } finally { + if (fs.statSync(failing).isDirectory()) fs.rmdirSync(failing) + } + }) + // 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 () => { @@ -1436,9 +1637,9 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toEqual([]) const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain( - 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', ) - expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') + expect(updated).not.toContain('ALTER TABLE "users" DROP COLUMN "email";') expect(updated).not.toMatch(/set data type/i) }) @@ -1692,8 +1893,8 @@ describe('sweepMigrationDirs', () => { * * The `meta/_journal.json` is what makes it drizzle-kit's: the sweep now * requires it, because `migrations/` and `src/db/migrations/` are generic - * names that Knex, Flyway, node-pg-migrate and raw psql also use, and this - * sweep emits `DROP COLUMN`. Use {@link seedForeignDir} for the other case. + * names that Knex, Flyway, node-pg-migrate and raw psql also use. Use + * {@link seedForeignDir} for the other case. */ const seedDir = (dir: string, name?: string, sql?: string): string => { const abs = seedForeignDir(dir, name, sql) @@ -1807,7 +2008,7 @@ describe('sweepMigrationDirs', () => { // 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. + // must remain source-unknown rather than choosing a lifecycle by assumption. it('does not rewrite an ALTER whose column is declared in a sibling directory', async () => { seedDir( 'drizzle', @@ -1839,8 +2040,8 @@ describe('sweepMigrationDirs', () => { // names — Knex, node-pg-migrate, Flyway and hand-rolled psql all use them. // Sweeping every candidate that merely EXISTS meant a project whose drizzle // `out` is `drizzle/` but which also keeps a hand-maintained `migrations/` - // had that second directory rewritten into ADD+DROP+RENAME, in a directory - // this tool was never pointed at. The fail-closed `declared` rule does not + // had that second directory rewritten in a directory this tool was never + // pointed at. The fail-closed `declared` rule does not // help: a real migration history declares its own columns. // // drizzle-kit always writes `meta/_journal.json` into its output directory; @@ -1909,10 +2110,16 @@ describe('describeSkipReason', () => { it('describes already-encrypted as a re-encrypt-through-the-lifecycle action', () => { const text = describeSkipReason('already-encrypted') expect(text).toContain('ALREADY encrypted') - expect(text).toContain('DROP the ciphertext') + expect(text).toContain('re-encrypted') expect(text).toContain('`stash encrypt` lifecycle') }) + it('describes an existing target as a duplicate-column failure', () => { + const text = describeSkipReason('target-exists') + expect(text).toContain('already exists') + expect(text).toContain('another ADD COLUMN would fail') + }) + it('describes unrecognised-form as a hand-authored / unknown cast', () => { const text = describeSkipReason('unrecognised-form') expect(text).toContain('SET DATA TYPE ... USING') @@ -1923,11 +2130,14 @@ describe('describeSkipReason', () => { const text = describeSkipReason('source-unknown') expect(text).toContain('could not find where this column was declared') expect(text).toContain("Check the column's current type in the database") + expect(text).toContain('staged `stash encrypt` lifecycle') + expect(text).not.toContain('table is empty') }) it('gives each reason a distinct description', () => { const reasons = [ 'already-encrypted', + 'target-exists', 'unrecognised-form', 'source-unknown', ] as const diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index f34ff1c38..bccfd6997 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -69,48 +69,26 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { // text/numeric to an EQL domain). Covers the EQL v3 family the wizard now // scaffolds, and legacy eql_v2_encrypted. CIP-2991 + CIP-2994 + #693. const sweep = await rewriteEncryptedMigrations(cwd) + const staged = sweep.rewritten > 0 + const skipped = sweep.skipped > 0 + const unverified = sweep.failedDirs.length > 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 - // opposite: those migrations may still hold unrepaired `SET DATA TYPE` - // statements and nobody has looked. `stash eql migration` / `db install` - // treat "sweep failed outright" and "sweep left near-misses" as the same - // state for the same reason; unknown is not safe, so the default is NO here - // too. The wording differs from the destructive case on purpose: nothing is - // known about that directory, so claiming it destroys data would be a guess. - const unverifiedDirs = sweep.failedDirs - const unverified = unverifiedDirs.length > 0 - const unverifiedList = unverifiedDirs.map((dir) => `${dir}/`).join(', ') - const unverifiedCount = `${unverifiedDirs.length} director${ - unverifiedDirs.length === 1 ? 'y' : 'ies' - }` - if (unverified) { - p.log.warn( - `The ALTER COLUMN sweep did not fully complete — review the sibling migrations in ${unverifiedList} before running drizzle-kit migrate, or you may apply broken/unsafe SQL.`, + if (staged) { + p.log.info( + `Rewrote ${sweep.rewritten} migration file(s) in the drizzle output to add staged encrypted columns while preserving the source columns.`, + ) + } + if (skipped || unverified) { + throw new Error( + `The ALTER COLUMN sweep found unsafe or unverified SQL. The generated migration remains in ${cwd}, but review the sibling migrations before running drizzle-kit migrate.`, ) } const shouldMigrate = await p.confirm({ - 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` - : 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, + message: staged + ? `Run the migration now? (${runner} drizzle-kit migrate) — the generated migration adds staged encrypted columns and preserves the source column for the later backfill and application switch` + : `Run the migration now? (${runner} drizzle-kit migrate)`, + initialValue: true, }) if (!p.isCancel(shouldMigrate) && shouldMigrate) { @@ -188,12 +166,9 @@ async function rewriteEncryptedMigrations(cwd: string): Promise<{ if (rewritten.length > 0) { p.log.info( - `Rewrote ${rewritten.length} migration file(s) in ${dir}/ to use ADD+DROP+RENAME for encrypted columns.`, + `Rewrote ${rewritten.length} migration file(s) in ${dir}/ to add staged encrypted columns while preserving the source columns.`, ) for (const file of rewritten) p.log.step(` - ${file}`) - p.log.warn( - 'This rewrite is data-destroying — safe only on an EMPTY table. If any of these tables already have rows, do NOT run the migration; use the staged `stash encrypt` flow (add -> backfill via @cipherstash/stack -> cutover -> drop) instead. See the comments in the rewritten SQL.', - ) } if (skipped.length > 0) { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index a050b4d05..7a04f0e36 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -266,6 +266,11 @@ function isInsideCommentOrString(sql: string, index: number): boolean { * dollar-quote opener. */ function dollarQuoteDelimiter(sql: string, open: number): string | undefined { + // PostgreSQL requires a dollar-quoted string following an identifier or + // keyword to be separated from it. `$` is legal inside an unquoted + // identifier, so `price$usd$cents` and `price$$cents` must not open phantom + // dollar-quoted bodies that hide later live SQL from the sweep. + if (/[A-Za-z0-9_$]/.test(sql[open - 1] ?? '')) return undefined DOLLAR_QUOTE_OPEN_RE.lastIndex = open return DOLLAR_QUOTE_OPEN_RE.exec(sql)?.[0] } @@ -327,8 +332,8 @@ const TABLE_REF = String.raw`"([^"]+)"(?:\."([^"]+)")?` * delimiter so a bare domain cannot match a prefix of a longer identifier. * * Two shapes {@link MANGLED_TYPE_FORMS} does not enumerate are folded in here so - * the encrypted index recognises them and the ADD+DROP+RENAME cannot drop their - * ciphertext: a domain in ANY quoted schema (`"app"."eql_v3_text_search"`, not + * the encrypted index recognises them and never mistakes their ciphertext for + * plaintext: a domain in ANY quoted schema (`"app"."eql_v3_text_search"`, not * just the literal `public` the mangled forms special-case), and a `[` in the * trailing lookahead so an ARRAY of the domain (`public.eql_v3_text_search[]`) * ends at a delimiter too. Both feed only the encrypted index — never the @@ -448,8 +453,8 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * covered explicitly by {@link ENCRYPTED_TYPE_REF} — a domain in a non-`public` * schema (`"email" "app"."eql_v3_text_search"`) and an array of the domain * (`"email" public.eql_v3_text_search[]`) — because both name ciphertext the - * corpus can see, so rewriting them is the exact drop this rule exists to - * prevent. The dependency itself remains: any encrypted shape a future EQL + * corpus can see, so treating them as plaintext would choose the wrong staged + * lifecycle. The dependency itself remains: any encrypted shape a future EQL * install introduces must be added to {@link ENCRYPTED_TYPE_REF} or it too will * fall to the plaintext residue. * @@ -502,7 +507,7 @@ function tableOf( * while hand-written SQL and this sweep's own {@link renderSafeAlter} output * are qualified. Keying on the literal text split one column across two keys, * so a column already encrypted under one spelling looked plaintext when - * altered under the other and the ADD+DROP+RENAME dropped its ciphertext + * altered under the other and bypassed the encrypted-domain guard * (#772 review, finding 4). * * Only the IMPLICIT schema collapses. `"app"."users"` is a genuinely different @@ -520,7 +525,7 @@ function columnKey(table: string, column: string, schema?: string): string { /** What the migration corpus says about the columns it mentions. */ interface ColumnIndex { - /** Columns the corpus gives an ENCRYPTED type. Rewriting one drops ciphertext. */ + /** Columns the corpus gives an ENCRYPTED type; these need re-encryption. */ encrypted: Set /** Columns the corpus DECLARES at all, whatever type it gives them. */ declared: Set @@ -528,23 +533,22 @@ interface ColumnIndex { /** * 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. + * tell the change it exists for (plaintext → staged encrypted twin) from the + * cases it must leave for review: encrypted → encrypted, a change to a column + * it knows nothing about, and a duplicate staged target. * * **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. + * correct migration requires decrypting and re-encrypting the existing + * ciphertext; it cannot use the plaintext-to-twin rewrite. * * **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. + * simply live somewhere this sweep never reads. Requiring a POSITIVE + * declaration makes the unknown case a flagged statement instead of choosing + * a lifecycle on an assumption. * * Because the encrypted side is matched against the known domain list, the * plaintext side needs no type classification: a column the corpus declares but @@ -626,8 +630,10 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { export type SkipReason = /** Outside the strict matcher — hand-authored `USING`, or an unknown form. */ | 'unrecognised-form' - /** The column already holds an encrypted domain; rewriting drops ciphertext. */ + /** The column already holds an encrypted domain and needs re-encryption. */ | 'already-encrypted' + /** The conventional encrypted twin already exists or was staged earlier. */ + | 'target-exists' /** The corpus never declares the column, so its current type is unknown. */ | 'source-unknown' @@ -644,17 +650,19 @@ 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 - * three reasons need very different action from the user, and a single generic + * reasons need different action from the user, and a single generic * "could not rewrite automatically" hides that. */ export function describeSkipReason(reason: SkipReason): string { switch (reason) { case 'already-encrypted': - 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" + return "the column is ALREADY encrypted and has no plaintext source for the normal backfill. Changing an encrypted column's domain changes its index terms, so the ciphertext must be decrypted and re-encrypted through a reviewed staged `stash encrypt` lifecycle" + case 'target-exists': + return 'the staged encrypted target already exists (or was added by an earlier rewrite), so another ADD COLUMN would fail. Review the existing encrypted twin and use the staged `stash encrypt` lifecycle without creating a duplicate column' 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" + return "the sweep could not find where this column was declared in this migration directory, so it cannot tell a plaintext column (which needs an encrypted twin and staged backfill) from one that already holds ciphertext (which needs staged re-encryption). 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, then use the staged `stash encrypt` lifecycle; never replace the source column in place" } } @@ -666,9 +674,14 @@ export interface RewriteResult { skipped: SkippedAlter[] } +interface RewriteSweepError extends Error { + rewritten?: string[] + skipped?: SkippedAlter[] +} + /** * Replace in-place `ALTER COLUMN ... SET DATA TYPE ` - * statements with an ADD + DROP + RENAME sequence. + * statements with a staged encrypted-column addition. * * **Why this exists (CIP-2991, CIP-2994, #693):** Postgres has no implicit cast * from `text`/`numeric` to an encrypted domain, so `ALTER COLUMN ... SET DATA @@ -676,29 +689,25 @@ export interface RewriteResult { * `cannot cast type ... to `. This applies equally to the single EQL v2 * type and the whole EQL v3 concrete-domain family. * - * The rewrite is an ADD+DROP+RENAME, which is **equivalent to DROP+ADD**: it - * makes the column type valid but does NOT preserve the column's data. It is - * therefore safe ONLY on an EMPTY table. On a populated table the new column - * starts NULL and the original is dropped in the same migration, so the - * plaintext is destroyed. The commented UPDATE is a placeholder that can never - * become real SQL (the encrypted value is the EQL envelope produced by ZeroKMS - * via the client — there is no expression Postgres can evaluate to fill it), so - * a populated table must instead use the staged EQL v3 lifecycle (add an - * encrypted twin → dual-write → backfill via `@cipherstash/stack`'s - * `encryptModel` → switch the application to the encrypted column by name → - * drop plaintext), which keeps both columns alive across deploys. Each - * rewritten file carries a header comment saying exactly this. + * The rewrite adds a separate encrypted column and preserves the source column. + * PostgreSQL cannot create the EQL envelope itself, so the application must use + * the staged EQL v3 lifecycle (add an encrypted twin → dual-write → backfill via + * `@cipherstash/stack`'s `encryptModel` → switch the application to the + * encrypted column by name → drop plaintext), which keeps both columns alive + * across deploys. The rewriter emits only the ADD: every later step is + * intentionally a separate, reviewed operation. * * 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); 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 + * column that is ALREADY encrypted and needs re-encryption; ones whose staged + * encrypted target already exists; 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 + * migration history). All are left untouched on disk and surfaced to the + * caller; CLI and wizard callers then fail non-zero rather than silently + * shipping broken SQL. 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. */ @@ -718,6 +727,7 @@ export async function rewriteEncryptedAlterColumns( const rewritten: string[] = [] const skipped: SkippedAlter[] = [] const seen = new Set() + const stagedTargets = new Set() /** Record a skip once — the strict pass and the broad scan can both find it. */ const skip = (file: string, statement: string, reason: SkipReason): void => { @@ -743,93 +753,91 @@ export async function rewriteEncryptedAlterColumns( const { encrypted: encryptedColumns, declared: declaredColumns } = indexColumnDeclarations([...contents.values()]) - for (const [filePath, original] of contents) { - if (options.skip && filePath === options.skip) continue - - // Reset the regex's lastIndex — it's stateful on /g - ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 - - const updated = original.replace( - ALTER_COLUMN_TO_ENCRYPTED_RE, - ( - match: string, - first: string, - second: string | undefined, - column: string, - mangledType: string, - offset: number, - ) => { - // Commented-out SQL never runs, and a multi-line replacement would only - // inherit the `-- ` on its first line — leaving the rest live. - if (isInsideCommentOrString(original, offset)) return match - - const { schema, table } = tableOf(first, second) - - // Already encrypted: the ADD+DROP+RENAME would drop the ciphertext and - // there is no plaintext left to backfill from. Flag, never guess. - if (encryptedColumns.has(columnKey(table, column, schema))) { - skip(filePath, match.trim(), 'already-encrypted') - return match - } + try { + for (const [filePath, original] of contents) { + if (options.skip && filePath === options.skip) continue + + // Reset the regex's lastIndex — it's stateful on /g + ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 + + const updated = original.replace( + ALTER_COLUMN_TO_ENCRYPTED_RE, + ( + match: string, + first: string, + second: string | undefined, + column: string, + mangledType: string, + offset: number, + ) => { + // Commented-out SQL never runs, and a multi-line replacement would only + // inherit the `-- ` on its first line — leaving the rest live. + if (isInsideCommentOrString(original, offset)) return match + + const { schema, table } = tableOf(first, second) + + // Already encrypted: an in-place domain conversion needs a staged + // re-encryption. Flag, never guess. + if (encryptedColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'already-encrypted') + return match + } + + // Fail closed. Absence from `declaredColumns` does not prove the source + // type, so leave the raw statement untouched and require review. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + + const target = columnKey(table, `${column}_encrypted`, schema) + if (declaredColumns.has(target) || stagedTargets.has(target)) { + skip(filePath, match.trim(), 'target-exists') + 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. + if (!domain) return match + + stagedTargets.add(target) + return renderSafeAlter(table, column, domain, schema) + }, + ) + + if (updated !== original) { + await writeFile(filePath, updated, 'utf-8') + rewritten.push(filePath) + } - // 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 + // Broad secondary scan on the POST-rewrite content: anything still carrying + // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict + // matcher. Record it for the fail-closed caller rather than leave the user shipping SQL + // that fails at migrate time. + for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { + const statement = trimStatementPreamble(nearMiss[0]) + // Anchor the comment test on the `SET DATA TYPE` itself: the match starts + // at the previous `;`, so its own offset sits before any preamble. + const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) + if ( + isInsideCommentOrString( + updated, + nearMiss.index + Math.max(keyword, 0), + ) + ) { + continue } - - 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. - if (!domain) return match - - // This statement converts the column, so from here on in the corpus it - // holds CIPHERTEXT. `indexColumnDeclarations` cannot know that: it - // reads CREATE TABLE, ADD COLUMN and RENAME, never the strict matcher's - // own target. Without this, a corpus carrying an earlier conversion - // (`... SET DATA TYPE eql_v2_encrypted` from a stack version that - // predates this sweep) leaves the column looking plaintext, and the - // NEXT domain change drops the ciphertext — `declared` cannot catch it, - // because the column really is declared, as plaintext, by the original - // CREATE TABLE. - // - // Recorded here rather than in the corpus-wide index on purpose: the - // index has no order, so it would flag THIS conversion — the legitimate - // plaintext -> encrypted one — as already-encrypted too. Files are - // walked in sorted order and matches within a file in source order, so - // "already converted" means "converted by a statement that runs before - // this one". - encryptedColumns.add(columnKey(table, column, schema)) - return renderSafeAlter(table, column, domain, schema) - }, - ) - - if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') - rewritten.push(filePath) - } - - // Broad secondary scan on the POST-rewrite content: anything still carrying - // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict - // matcher. Flag it — non-fatally — rather than leave the user shipping SQL - // that fails at migrate time. - for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { - const statement = trimStatementPreamble(nearMiss[0]) - // Anchor the comment test on the `SET DATA TYPE` itself: the match starts - // at the previous `;`, so its own offset sits before any preamble. - const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) - if ( - isInsideCommentOrString(updated, nearMiss.index + Math.max(keyword, 0)) - ) { - continue + skip(filePath, statement, 'unrecognised-form') } - skip(filePath, statement, 'unrecognised-form') } + } catch (error) { + if (error instanceof Error) { + const partial = error as RewriteSweepError + partial.rewritten = rewritten + partial.skipped = skipped + } + throw error } return { rewritten, skipped } @@ -899,8 +907,8 @@ async function holdsSqlFiles(abs: string): Promise { * * **Why only drizzle-kit output directories:** the candidate list is a guess, * and two of its three entries are generic names that Knex, node-pg-migrate, - * Flyway and hand-rolled psql also use. This sweep emits `DROP COLUMN`, so - * rewriting a directory it was never pointed at is the worst thing it can do — + * Flyway and hand-rolled psql also use. Rewriting a directory it was never + * pointed at would still mutate migrations owned by another tool — * and the fail-closed `declared` rule is no defence there, because a real * migration history declares its own columns. Requiring the `meta/_journal.json` * that drizzle-kit maintains keeps the reach to directories drizzle-kit owns @@ -935,7 +943,13 @@ export async function sweepMigrationDirs( results.push({ dir, rewritten, skipped }) } catch (err) { const message = err instanceof Error ? err.message : String(err) - results.push({ dir, rewritten: [], skipped: [], error: message }) + const partial = err as Partial + results.push({ + dir, + rewritten: partial.rewritten ?? [], + skipped: partial.skipped ?? [], + error: message, + }) } } @@ -944,23 +958,11 @@ export async function sweepMigrationDirs( // #endregion wizard-only /** - * The rewrite is identical for v2 and v3, and the ADD+DROP+RENAME sequence is - * equivalent to DROP+ADD: it makes the column type valid but does NOT preserve - * the column's data. It is therefore 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, so the plaintext is destroyed. - * - * The commented UPDATE is only a placeholder — it can never become real SQL. The - * encrypted value is the EQL envelope produced by ZeroKMS via the client - * (`encryptModel` / `bulkEncryptModels`); there is no expression Postgres can - * evaluate to fill it. (v3 stores that envelope as jsonb rather than v2's - * composite, but this is equally true on both surfaces.) - * - * So the guidance does NOT tell the user to backfill and run this migration — - * that would still lose data. It points a populated table at the staged EQL v3 - * lifecycle (add an encrypted twin → dual-write → backfill → switch the - * application to the encrypted column by name → drop plaintext), which keeps - * both columns alive across deploys. + * The source column remains live. The encrypted value is an EQL envelope + * produced by ZeroKMS, so PostgreSQL cannot backfill it with an SQL expression. + * The generated comment directs the user to the staged EQL v3 lifecycle, whose + * later backfill, application switch, and plaintext drop are explicit + * operations outside this automatic rewrite. */ function renderSafeAlter( table: string, @@ -968,20 +970,18 @@ function renderSafeAlter( domain: string, schema?: string, ): string { - const tmp = `${column}__cipherstash_tmp` + const encrypted = `${column}_encrypted` // Preserve the schema qualifier drizzle-kit emitted for pgSchema() tables so // the rewritten statements target the same object. const qualifiedTable = schema ? `"${schema}"."${table}"` : `"${table}"` return [ '-- Rewritten by @cipherstash/wizard: in-place ALTER COLUMN cannot cast to', - `-- ${domain}. This ADD+DROP+RENAME equals DROP+ADD and is safe ONLY if`, - `-- ${qualifiedTable} is empty. On a populated table it DESTROYS existing "${column}"`, - '-- data (the new column starts NULL) — do NOT run it there. Use the staged', - '-- EQL v3 path instead: add an encrypted twin -> dual-write -> backfill via', - '-- encryptModel -> switch the app to the encrypted column -> drop plaintext.', - '-- NOTE: constraints, defaults, and indexes on the original column are NOT', - '-- carried over by this ADD/DROP/RENAME — re-add any NOT NULL, DEFAULT,', - '-- UNIQUE, or index definitions manually.', + `-- ${domain}. The source column "${column}" is deliberately preserved.`, + '-- Use the staged `stash encrypt` lifecycle: dual-write to this new column,', + "-- backfill it via @cipherstash/stack's encryptModel, then", + '-- switch the application to the encrypted column by name and drop the', + '-- plaintext column — both separately reviewed steps taken only after the', + '-- backfill is complete.', // The domain is emitted as `"public".""` unconditionally: EQL // installs its domains into `public` (both `stash eql install` and the // adapters' baseline migrations do), and the qualifier makes the rewrite @@ -991,11 +991,6 @@ function renderSafeAlter( // the domain lives. If EQL ever supports installing into a non-`public` // schema, this needs the install schema threaded in, here and in the // sibling `packages/cli/src/commands/db/rewrite-migrations.ts`. - `ALTER TABLE ${qualifiedTable} ADD COLUMN "${tmp}" "public"."${domain}";`, - `-- UPDATE ${qualifiedTable} SET "${tmp}" = /* encrypted value for ${column} */ NULL`, - '--> statement-breakpoint', - `ALTER TABLE ${qualifiedTable} DROP COLUMN "${column}";`, - '--> statement-breakpoint', - `ALTER TABLE ${qualifiedTable} RENAME COLUMN "${tmp}" TO "${column}";`, + `ALTER TABLE ${qualifiedTable} ADD COLUMN "${encrypted}" "public"."${domain}";`, ].join('\n') } diff --git a/scripts/__tests__/rewriter-copies-in-sync.test.mjs b/scripts/__tests__/rewriter-copies-in-sync.test.mjs index 213b1ffc5..96aae7b51 100644 --- a/scripts/__tests__/rewriter-copies-in-sync.test.mjs +++ b/scripts/__tests__/rewriter-copies-in-sync.test.mjs @@ -6,7 +6,7 @@ import { describe, expect, it } from 'vitest' const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') /** - * The destructive ALTER-COLUMN rewriter exists twice: `@cipherstash/wizard` + * The ALTER-COLUMN rewriter exists twice: `@cipherstash/wizard` * runs it after its agent edits a schema, and `stash eql migration --drizzle` * runs it over an explicit `--out`. Both are published, neither depends on the * other, and `packages/utils` is not a package while `@cipherstash/test-kit` is @@ -87,7 +87,7 @@ describe('the wizard and cli rewriter copies stay in sync', () => { wizard.join('\n'), `${WIZARD} and ${CLI} have drifted. Every fix to this rewriter must land ` + "in BOTH copies — a one-sided fix still passes that package's own suite, " + - 'and this rewriter emits DROP COLUMN. If the difference is intentional and ' + + 'and this rewriter now emits a staged encrypted-column addition. If the difference is intentional and ' + `wizard-only, move it inside the ${REGION_OPEN} region.`, ).toBe(cli.join('\n')) }) diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 44784cc2f..26f982302 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -376,7 +376,9 @@ stash eql migration --drizzle --supabase # also grant eql_v3 to anon/authentic Pass exactly one of `--drizzle` / `--prisma`. The generated migration also installs the `cs_migrations` tracking schema, so one `drizzle-kit migrate` covers everything `stash encrypt …` needs. -After writing the migration, `--drizzle` sweeps sibling migrations containing an in-place `ALTER COLUMN … SET DATA TYPE `. When the source declaration is provably plaintext, it replaces the ALTER with an `ADD COLUMN` + `DROP` + `RENAME` sequence and lists the rewritten files. This is equivalent to DROP+ADD: it is safe only on an empty table and does not preserve data, constraints, defaults, or indexes. On a populated table, do not run that rewrite; use the staged EQL v3 rollout (add an encrypted twin, dual-write, backfill, switch the application to the encrypted column by name, then drop plaintext). If the source type cannot be proven, the statement remains unchanged and the command warns that the migration directory needs review. +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 a staged `ADD COLUMN` for the encrypted twin, while preserving the source column, and the rewritten files are listed. The rewrite never emits `DROP COLUMN` or `RENAME COLUMN`. If the sweep cannot prove a column's source type, finds that the encrypted twin already exists, or encounters another unsafe form, it leaves that statement untouched and the command exits non-zero so you review the migration directory before running `drizzle-kit migrate`. Populated plaintext tables then take the staged EQL v3 rollout from there: dual-write, backfill, switch the application to the encrypted column by name, and drop plaintext only after verification. + +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 (altered by hand via psql or the Supabase dashboard, say) 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, and the command exits non-zero if any such statement remains. Check the column's actual type in the database before applying a flagged or corpus-cleared rewrite by hand. #### `eql upgrade`