Skip to content

Commit cc001e0

Browse files
committed
fix(cli, migrate): address CodeRabbit feedback on encryption-migrations
Aggregates the actionable items from CodeRabbit's review of #357. None introduce new behaviour; each closes a specific defect. Security / correctness: - drizzle-helper: replace `execSync` with `spawnSync` so caller-provided migration names can't escape into the shell. - migrate/backfill: drop the value preview from the leak-guard error message — the path that hits this branch is precisely where the encryption client passed plaintext through, so the value is sensitive. - cli/backfill: emit a generic catch-block error instead of bubbling `error.message` (same plaintext-leak risk via upstream library exception text). Schema-qualified table names: - requireTable: fall back to unqualified-name lookup so `--table public.users` resolves to a schema whose `tableName === 'users'`. - status: use lastIndexOf('.') when splitting `${table}.${column}` keys so schema-qualified table names don't drop the column segment. - cutover.buildRenameMigrationSql + drop.dropSql: split `schema.table` into separate quoted identifiers so the generated `ALTER TABLE` is valid and the `information_schema` probe matches. EQL state / SQL: - push, activate, cutover, eql.discardPendingConfig, status: schema- qualify `eql_v2_configuration` as `public.eql_v2_configuration` so a custom `search_path` doesn't shadow or hide the table. - countEncryptedWithActiveConfig: return `bigint` instead of `number` — the BIGINT row count silently truncates past `Number.MAX_SAFE_INTEGER` on the exact large tables this is meant to sanity-check. Process / lifecycle hygiene: - Every CLI handler that did `process.exit(1)` inside a try/catch with an async `finally`: replace with an `exitCode` flag and a single `if (exitCode) process.exit(exitCode)` after the finally, so `client.end()` actually runs on the error path. - backfill: move `pool.connect()` inside the same try/finally that registers the SIGINT handlers, so handlers/pool are cleaned up if connection acquisition fails. - cutover: catch proxy `reloadConfig()` failures separately and warn — reload runs after the cutover transaction commits, so a transient Proxy connectivity blip shouldn't make the outer catch report the whole cutover as failed. Schema validation: - manifest: replace `castAs: z.string()` with a `z.enum(...)` of the supported EQL types so a typo (`timestampz`, `strnig`) fails at manifest read instead of at use-time. Doc + skill accuracy: - setup-prompt: drop the incorrect "no read-path code change" claim from the migrate-existing-column flow. Add an explicit step 6 pointing at `decryptModel` / `encryptedSupabase` — without it the agent ships read paths returning raw ciphertext to end users. - stash-drizzle: `createProtectOperators` → `createEncryptionOperators` (the only correct name; every other reference in the file already uses it). - stash-encryption: fix the `runBackfill` example's parameter names (`table` → `tableName`, `column` → `schemaColumnKey/plaintextColumn/ encryptedColumn`, `client` → `encryptionClient`, plus the missing `tableSchema` and `pkColumn`). - design doc: cutover section now reflects the shipped flow (`migrate_config()` + `activate_config()` inside the transaction alongside the rename), and the read-path claim matches the skill. Test infrastructure: - backfill.integration.test.ts: import `dotenv/config` so PG_TEST_URL loads from `.env`; add `dotenv` to devDependencies. Deferred (separate follow-up entry): - The cutover preflight only verifies one column's phase, but the underlying EQL function promotes the whole pending config in one call — already tracked as item 3.8 in the working follow-ups doc.
1 parent 552bc58 commit cc001e0

19 files changed

Lines changed: 191 additions & 88 deletions

File tree

docs/plans/encryption-migrations.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,14 +139,19 @@ For each column in `backfilled` phase, in a single transaction:
139139
BEGIN;
140140
-- Renames <col> -> <col>_plaintext, <col>_encrypted -> <col>
141141
SELECT eql_v2.rename_encrypted_columns();
142+
-- Promote the pending config that was registered by the most recent
143+
-- `stash db push` (which the user ran after switching the schema to
144+
-- declare the encrypted column under its final name).
145+
SELECT eql_v2.migrate_config(); -- pending → encrypting
146+
SELECT eql_v2.activate_config(); -- encrypting → active (prior active → inactive)
142147
COMMIT;
143148

144149
-- If Proxy URL is configured, force refresh
145150
\c <proxy_url>
146151
SELECT eql_v2.reload_config();
147152
```
148153

149-
Record `cut_over` event. App's existing `SELECT email FROM users` now returns the encrypted column (decrypted transparently by Proxy or client-side by Stack). No app code change required for reads — this is the big payoff of the rename approach.
154+
Record `cut_over` event. App's existing `SELECT email FROM users` returns the encrypted column post-cutover; reading code paths must decrypt via the encryption client (e.g. `decryptModel(rows[0], usersTable)` in Drizzle, or the equivalent `encryptedSupabase` wrapper in Supabase) so the call site receives plaintext. No write-path code change beyond removing the dual-write logic — that comes after cutover.
150155

151156
### 6. `stash encrypt drop`
152157

packages/cli/src/commands/db/activate.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,20 @@ export async function activateCommand(
3535
databaseUrlFlag: options.databaseUrl,
3636
})
3737
const client = new pg.Client({ connectionString: stashConfig.databaseUrl })
38+
let exitCode = 0
3839

3940
try {
4041
await client.connect()
4142

4243
const pending = await client.query<{ exists: boolean }>(
43-
"SELECT EXISTS(SELECT 1 FROM eql_v2_configuration WHERE state = 'pending') AS exists",
44+
"SELECT EXISTS(SELECT 1 FROM public.eql_v2_configuration WHERE state = 'pending') AS exists",
4445
)
4546
if (pending.rows[0]?.exists !== true) {
4647
p.log.error(
4748
'No pending EQL configuration to activate. Run `stash db push` first to register a change.',
4849
)
49-
process.exit(1)
50+
exitCode = 1
51+
return
5052
}
5153

5254
await client.query('BEGIN')
@@ -63,8 +65,9 @@ export async function activateCommand(
6365
p.outro('Done.')
6466
} catch (error) {
6567
p.log.error(error instanceof Error ? error.message : 'Activation failed.')
66-
process.exit(1)
68+
exitCode = 1
6769
} finally {
6870
await client.end()
6971
}
72+
if (exitCode) process.exit(exitCode)
7073
}

packages/cli/src/commands/db/push.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,16 @@ export async function pushCommand(options: {
7777
}
7878

7979
const client = new pg.Client({ connectionString: config.databaseUrl })
80+
let exitCode = 0
8081

8182
try {
8283
s.start('Connecting to Postgres...')
8384
await client.connect()
8485
s.stop('Connected to Postgres.')
8586

86-
s.start('Checking eql_v2_configuration state...')
87+
s.start('Checking public.eql_v2_configuration state...')
8788
const activeResult = await client.query<{ exists: boolean }>(
88-
"SELECT EXISTS(SELECT 1 FROM eql_v2_configuration WHERE state = 'active') AS exists",
89+
"SELECT EXISTS(SELECT 1 FROM public.eql_v2_configuration WHERE state = 'active') AS exists",
8990
)
9091
const hasActive = activeResult.rows[0]?.exists === true
9192
s.stop(
@@ -99,7 +100,7 @@ export async function pushCommand(options: {
99100
// Proxy reading the active config. Insert directly as `active`.
100101
s.start('Writing initial active configuration...')
101102
await client.query(
102-
"INSERT INTO eql_v2_configuration (state, data) VALUES ('active', $1)",
103+
"INSERT INTO public.eql_v2_configuration (state, data) VALUES ('active', $1)",
103104
[eqlConfig],
104105
)
105106
s.stop('Active configuration written.')
@@ -118,7 +119,7 @@ export async function pushCommand(options: {
118119
try {
119120
await discardPendingConfig(client)
120121
await client.query(
121-
"INSERT INTO eql_v2_configuration (state, data) VALUES ('pending', $1)",
122+
"INSERT INTO public.eql_v2_configuration (state, data) VALUES ('pending', $1)",
122123
[eqlConfig],
123124
)
124125
await client.query('COMMIT')
@@ -151,8 +152,9 @@ export async function pushCommand(options: {
151152
p.log.error(
152153
error instanceof Error ? error.message : 'Failed to push configuration.',
153154
)
154-
process.exit(1)
155+
exitCode = 1
155156
} finally {
156157
await client.end()
157158
}
159+
if (exitCode) process.exit(exitCode)
158160
}

packages/cli/src/commands/encrypt/backfill.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,13 @@ export async function backfillCommand(options: BackfillCommandOptions) {
116116
p.log.warn('Interrupt received; finishing current chunk and exiting.')
117117
controller.abort()
118118
}
119-
process.on('SIGINT', onSignal)
120-
process.on('SIGTERM', onSignal)
121119

122-
const db = await pool.connect()
120+
let db: pg.PoolClient | undefined
121+
let exitCode = 0
123122
try {
123+
process.on('SIGINT', onSignal)
124+
process.on('SIGTERM', onSignal)
125+
db = await pool.connect()
124126
const pkColumn =
125127
options.pkColumn ?? (await detectPkColumn(db, options.table))
126128

@@ -249,14 +251,23 @@ export async function backfillCommand(options: BackfillCommandOptions) {
249251
`Backfill complete. ${result.rowsProcessed.toLocaleString()} rows encrypted.`,
250252
)
251253
} catch (error) {
252-
p.log.error(error instanceof Error ? error.message : 'Backfill failed.')
253-
process.exit(1)
254+
// Generic message only — `error.message` may include plaintext sample
255+
// values bubbled up from the encryption pipeline (e.g. the leak guard
256+
// in @cipherstash/migrate now emits type-only diagnostics, but
257+
// upstream libraries can still embed offending input in their
258+
// exception text). Preserve exit behaviour but stop the message path
259+
// from leaking sensitive data.
260+
p.log.error(
261+
`Backfill failed${error instanceof Error && /^[\w. -]+$/.test(error.name) ? ` (${error.name})` : ''}. Re-run with diagnostic logging if you need details.`,
262+
)
263+
exitCode = 1
254264
} finally {
255265
process.off('SIGINT', onSignal)
256266
process.off('SIGTERM', onSignal)
257-
db.release()
267+
db?.release()
258268
await pool.end()
259269
}
270+
if (exitCode) process.exit(exitCode)
260271
}
261272

262273
/**

packages/cli/src/commands/encrypt/context.ts

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -146,19 +146,30 @@ export async function loadEncryptionContext(): Promise<EncryptionContext> {
146146
* context. Exits the process with code `1` if the table is not declared
147147
* in the user's encryption client file — without this schema, backfill
148148
* cannot call `bulkEncryptModels`.
149+
*
150+
* Accepts schema-qualified inputs (`public.users`) and falls back to the
151+
* unqualified name when no exact match is found — `EncryptedTable.tableName`
152+
* is typically declared without a schema, so `public.users` should still
153+
* resolve to a table whose `tableName === 'users'`.
149154
*/
150155
export function requireTable(
151156
ctx: EncryptionContext,
152157
tableName: string,
153158
): EncryptedTableLike {
154-
const table = ctx.tables.get(tableName)
155-
if (!table) {
156-
const available = Array.from(ctx.tables.keys()).join(', ') || '(none)'
157-
console.error(
158-
`Error: Table "${tableName}" was not found in the encryption client exports.\n` +
159-
`Available: ${available}`,
160-
)
161-
process.exit(1)
159+
const direct = ctx.tables.get(tableName)
160+
if (direct) return direct
161+
162+
const dot = tableName.lastIndexOf('.')
163+
if (dot >= 0) {
164+
const unqualified = tableName.slice(dot + 1)
165+
const fallback = ctx.tables.get(unqualified)
166+
if (fallback) return fallback
162167
}
163-
return table
168+
169+
const available = Array.from(ctx.tables.keys()).join(', ') || '(none)'
170+
console.error(
171+
`Error: Table "${tableName}" was not found in the encryption client exports.\n` +
172+
`Available: ${available}`,
173+
)
174+
process.exit(1)
164175
}

packages/cli/src/commands/encrypt/cutover.ts

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
5656

5757
const config = await loadStashConfig()
5858
const client = new pg.Client({ connectionString: config.databaseUrl })
59+
let exitCode = 0
5960

6061
try {
6162
await client.connect()
@@ -65,7 +66,8 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
6566
p.log.error(
6667
`Cannot cut over: ${options.table}.${options.column} is in phase '${state?.phase ?? '—'}'. Must be 'backfilled'.`,
6768
)
68-
process.exit(1)
69+
exitCode = 1
70+
return
6971
}
7072

7173
// Verify a pending EQL config exists. cutover assumes the user has
@@ -74,13 +76,14 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
7476
// db push writes that as pending, and cutover transitions
7577
// pending → encrypting → active alongside the physical rename.
7678
const pending = await client.query<{ exists: boolean }>(
77-
"SELECT EXISTS(SELECT 1 FROM eql_v2_configuration WHERE state = 'pending') AS exists",
79+
"SELECT EXISTS(SELECT 1 FROM public.eql_v2_configuration WHERE state = 'pending') AS exists",
7880
)
7981
if (pending.rows[0]?.exists !== true) {
8082
p.log.error(
8183
'No pending EQL configuration. Update your schema to point at the encrypted column (drop the `_encrypted` suffix), then run `stash db push` to register the pending change before cutting over.',
8284
)
83-
process.exit(1)
85+
exitCode = 1
86+
return
8487
}
8588

8689
// Full lifecycle in one transaction:
@@ -142,13 +145,26 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
142145
}
143146
}
144147

148+
// Proxy reload runs *after* the cutover transaction has committed.
149+
// Any error from here on is post-commit cosmetic — the rename and
150+
// config promotion are durable. Catch the proxy reload separately so
151+
// a transient Proxy connectivity blip doesn't make the outer catch
152+
// exit(1), which would falsely tell automation the cutover failed
153+
// and encourage unsafe retries.
145154
const proxyUrl = options.proxyUrl ?? process.env.CIPHERSTASH_PROXY_URL
146155
if (proxyUrl) {
147156
const proxy = new pg.Client({ connectionString: proxyUrl })
148157
try {
149158
await proxy.connect()
150-
await reloadConfig(proxy)
151-
p.log.success('Proxy config reloaded.')
159+
try {
160+
await reloadConfig(proxy)
161+
p.log.success('Proxy config reloaded.')
162+
} catch (err) {
163+
const reason = err instanceof Error ? err.message : String(err)
164+
p.log.warn(
165+
`Proxy config reload failed (${reason}). The cutover itself is committed and durable; Proxy will pick up the new config on its next 60s refresh.`,
166+
)
167+
}
152168
} finally {
153169
await proxy.end()
154170
}
@@ -163,10 +179,11 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
163179
)
164180
} catch (error) {
165181
p.log.error(error instanceof Error ? error.message : 'Cut-over failed.')
166-
process.exit(1)
182+
exitCode = 1
167183
} finally {
168184
await client.end()
169185
}
186+
if (exitCode) process.exit(exitCode)
170187
}
171188

172189
/**
@@ -176,8 +193,20 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
176193
* block does nothing), but on a fresh restore the rename hasn't run yet
177194
* (so the block performs the swap). Same migration file, both behaviours,
178195
* idempotent.
196+
*
197+
* Splits `schema.table` into separate identifiers so the generated SQL is
198+
* correct regardless of whether the user passed `users` or `public.users`.
179199
*/
180200
function buildRenameMigrationSql(table: string, column: string): string {
201+
const dot = table.indexOf('.')
202+
const [schema, tableName] =
203+
dot >= 0 ? [table.slice(0, dot), table.slice(dot + 1)] : [null, table]
204+
205+
const qualified = schema ? `"${schema}"."${tableName}"` : `"${tableName}"`
206+
const schemaPredicate = schema
207+
? `table_schema = '${schema.replace(/'/g, "''")}' AND `
208+
: ''
209+
181210
return `-- Generated by stash encrypt cutover.
182211
-- Records the rename that eql_v2.rename_encrypted_columns() performed
183212
-- so Drizzle's snapshot stays in sync. Idempotent: a no-op on the DB
@@ -186,11 +215,11 @@ DO $$
186215
BEGIN
187216
IF EXISTS (
188217
SELECT 1 FROM information_schema.columns
189-
WHERE table_name = '${table}'
218+
WHERE ${schemaPredicate}table_name = '${tableName.replace(/'/g, "''")}'
190219
AND column_name = '${column}_encrypted'
191220
) THEN
192-
ALTER TABLE "${table}" RENAME COLUMN "${column}" TO "${column}_plaintext";
193-
ALTER TABLE "${table}" RENAME COLUMN "${column}_encrypted" TO "${column}";
221+
ALTER TABLE ${qualified} RENAME COLUMN "${column}" TO "${column}_plaintext";
222+
ALTER TABLE ${qualified} RENAME COLUMN "${column}_encrypted" TO "${column}";
194223
END IF;
195224
END $$;
196225
`

packages/cli/src/commands/encrypt/drizzle-helper.ts

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execSync } from 'node:child_process'
1+
import { spawnSync } from 'node:child_process'
22
import { existsSync } from 'node:fs'
33
import { readdir, writeFile } from 'node:fs/promises'
44
import { join, resolve } from 'node:path'
@@ -37,23 +37,19 @@ export async function scaffoldDrizzleMigration(opts: {
3737
}
3838

3939
// `drizzle-kit generate --custom` scaffolds an empty migration file with
40-
// the right prefix and records the journal/snapshot entry. Stderr is
41-
// captured so a missing-drizzle-kit error surfaces cleanly.
42-
try {
43-
execSync(`npx drizzle-kit generate --custom --name=${opts.name}`, {
44-
stdio: 'pipe',
45-
encoding: 'utf-8',
46-
})
47-
} catch (error) {
48-
const stderr =
49-
error !== null &&
50-
typeof error === 'object' &&
51-
'stderr' in error &&
52-
typeof error.stderr === 'string'
53-
? error.stderr.trim()
54-
: undefined
40+
// the right prefix and records the journal/snapshot entry. spawnSync
41+
// (not execSync) so opts.name can't escape into the shell — names like
42+
// `cutover_T_C` are agent-controlled and could in principle include
43+
// special characters.
44+
const cp = spawnSync(
45+
'npx',
46+
['drizzle-kit', 'generate', '--custom', `--name=${opts.name}`],
47+
{ stdio: 'pipe', encoding: 'utf-8' },
48+
)
49+
if (cp.error || cp.status !== 0) {
50+
const stderr = typeof cp.stderr === 'string' ? cp.stderr.trim() : undefined
5551
throw new Error(
56-
`Failed to scaffold Drizzle migration: ${stderr ?? (error instanceof Error ? error.message : String(error))}`,
52+
`Failed to scaffold Drizzle migration: ${stderr ?? cp.error?.message ?? 'unknown error'}`,
5753
)
5854
}
5955

packages/cli/src/commands/encrypt/drop.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ export async function dropCommand(options: DropCommandOptions) {
5252

5353
const config = await loadStashConfig()
5454
const client = new pg.Client({ connectionString: config.databaseUrl })
55+
let exitCode = 0
5556

5657
try {
5758
await client.connect()
@@ -61,10 +62,16 @@ export async function dropCommand(options: DropCommandOptions) {
6162
p.log.error(
6263
`Cannot generate drop migration: ${options.table}.${options.column} is in phase '${state?.phase ?? '—'}'. Must be 'cut-over'.`,
6364
)
64-
process.exit(1)
65+
exitCode = 1
66+
return
6567
}
6668

67-
const dropSql = `-- Generated by stash encrypt drop\n-- Drops the plaintext column now that ${options.table}.${options.column} is encrypted.\n\nALTER TABLE "${options.table}" DROP COLUMN "${options.column}_plaintext";\n`
69+
const dot = options.table.indexOf('.')
70+
const qualifiedTable =
71+
dot >= 0
72+
? `"${options.table.slice(0, dot)}"."${options.table.slice(dot + 1)}"`
73+
: `"${options.table}"`
74+
const dropSql = `-- Generated by stash encrypt drop\n-- Drops the plaintext column now that ${options.table}.${options.column} is encrypted.\n\nALTER TABLE ${qualifiedTable} DROP COLUMN "${options.column}_plaintext";\n`
6875

6976
const cwd = process.cwd()
7077
const migrationsDir = options.migrationsDir ?? 'drizzle'
@@ -119,8 +126,9 @@ export async function dropCommand(options: DropCommandOptions) {
119126
p.log.error(
120127
error instanceof Error ? error.message : 'Drop generation failed.',
121128
)
122-
process.exit(1)
129+
exitCode = 1
123130
} finally {
124131
await client.end()
125132
}
133+
if (exitCode) process.exit(exitCode)
126134
}

packages/cli/src/commands/encrypt/plan.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ export async function planCommand() {
2424
}
2525

2626
const client = new pg.Client({ connectionString: config.databaseUrl })
27+
let exitCode = 0
2728
try {
2829
await client.connect()
2930
const state = await latestByColumn(client)
@@ -51,8 +52,9 @@ export async function planCommand() {
5152
p.outro('Done.')
5253
} catch (error) {
5354
p.log.error(error instanceof Error ? error.message : 'Plan failed.')
54-
process.exit(1)
55+
exitCode = 1
5556
} finally {
5657
await client.end()
5758
}
59+
if (exitCode) process.exit(exitCode)
5860
}

0 commit comments

Comments
 (0)