fix(security): randomize the IV for encrypt_text/decrypt_text

[Antriksh1984]

Summary

encrypt_text called the 3-arg extensions.encrypt(plaintext, key, 'aes'), which pgcrypto documents as using an all-zero IV. Encryption was therefore a pure function of (plaintext, key): two equal passwords produced byte-identical ciphertext. Because create_secret encrypts server-side and RLS lets a user read their own password_encrypted back, any signed-in user was an encryption oracle - grouping a dump of secrets by ciphertext names every set of users sharing a password, from a dump alone, with no key required. The same defect applied to recovery_codes_encrypted and (#417) twofa_secret.

This closes BossConsole#618's first of three named defects.

What changed

• encrypt_text now draws a fresh random 16-byte IV per call (extensions.gen_random_bytes) and encrypts via extensions.encrypt_iv, storing 'v2:' || base64(iv || ciphertext) - the same versioned-envelope convention 20260909000000_encrypt_totp.sql established for TOTP secrets.
• decrypt_text dispatches on the 'v2:' prefix, or falls back to the original zero-IV path unchanged for anything not yet migrated - no flag day, nothing else about either function's contract changes.
• A backfill converts existing secrets.password_encrypted, secret_metadata.recovery_codes_encrypted, and secret_metadata.twofa_secret rows, guarded by NOT LIKE 'v2:%' so a partial or repeated run is a no-op. The twofa_secret backfill briefly disables encrypt_twofa_secret_trigger (mirroring what ops/rotate_master_encryption_key.sql already does), since that trigger rejects any write already carrying its own 'v1:' prefix.

Deliberately not in scope

BossConsole#618 names two more defects, neither touched here, and both explained in the migration's own header comment:

• Key truncation (encryption_key::bytea truncates to the string's first 32 characters, i.e. 128 bits if hex-encoded, not the documented 256). ops/rotate_master_encryption_key.sql's own comment records that a deployed key may be hex or base64, and this migration has no way to tell which is live in any given project's Vault - guessing wrong would make get_encryption_key()'s result unusable and break encryption/decryption for every row the moment this deploys. That needs an operator who knows the live encoding, not a guess baked into a migration.
• No MAC/AEAD (a flipped ciphertext byte surfaces only as a UTF-8 decode error later, never as an authentication failure). Adding one is its own wire-format decision.

ops/rotate_master_encryption_key.sql will also need a follow-up before it is next run: it manipulates raw ciphertext bytes assuming the old no-IV, no-prefix layout, and after this backfill every mapped column is 'v2:'-framed. Flagged in the migration header; not actioned here.

Two follow-up commits fix problems hosted CI caught that local review couldn't, since this environment has no Postgres/Supabase CLI to test against directly:

• pg_catalog.substring(x from y) is a syntax error - the SQL-standard FROM/FOR trailing syntax is only recognized on the bare function name, matching how 20260909000000_encrypt_totp.sql already calls it. Fixed at all three call sites in decrypt_text and the twofa_secret backfill.
• ops/rotate_master_encryption_key.sql's re-encrypt loop called extensions.decrypt/encrypt directly on raw ciphertext bytes, assuming the old zero-IV, no-prefix layout - exactly the gap this migration's header already flagged as a follow-up, just needed sooner than expected, since the pre-existing master_key_rotation_test.sql exercises that script directly. Added a pg_temp helper that re-encrypts a mapped column's value under the new key while preserving whichever envelope it already carries ('v2:' gets a fresh IV; a legacy row, including the still-unmigrated qbo_token_state/google_token_state columns, stays zero-IV).

Test plan

• Added supabase/tests/secret_encryption_iv_randomization_test.sql (pgTAP): asserts two encryptions of the same plaintext now differ, a v2:-enveloped value round-trips, a hand-constructed legacy zero-IV ciphertext still decrypts, and the backfill UPDATE both converts a legacy row and is idempotent on a second run.
• I could not run Postgres or the Supabase CLI in this environment (no supabase/docker binary available in this sandbox), so all three commits were written and reviewed by hand rather than executed locally, taking hosted CI's Database (pgTAP) check as the actual verification gate - which is how the two follow-up commits above got caught and fixed.

Review repairs and contributor credit

Original IV randomization, legacy-read compatibility, backfill and rotation implementation: @Antriksh1984. Review repair ba3d46fdf by @kshivang changes plaintext encoding to convert_to(..., 'utf8') so the backfill preserves literal backslashes, and adds real populated migration replay plus mixed legacy/v2 rotation tests.

Validation: isolated Supabase PostgreSQL 17, 826 assertions across 34 suites passed. Real migration replay covers all three fields, exact backslash/Unicode preservation, idempotency and restored TOTP trigger behavior. Replaying with the original encoding fails with invalid input syntax for type bytea, confirming the regression. Fixture generation/check and git diff --check pass. Final-head hosted CI passed all ten checks: https://github.com/risa-labs-inc/BossConsole/actions/runs/34822090604. Merged into dev as 7dfb90e5a76301f34d9bf98e8f370b67d3dd197e after the locked refresh and reviewed-head check.

Cross-group context: draft #619 proposes an authenticated envelope also called v2:. Its wire format is incompatible with this narrower IV-only envelope; a future integration must use a distinct version and migrate explicitly. #619 is read-only context here, not a consolidation target. #621's resilient-listing change and merged #417/#423's TOTP/access-control work remain distinct. No live database deployment or key rotation was performed.

Latest review repair 095ec0c723976da5eab1c8f61cecea60ce20a957 makes the complete backfill atomic, diagnoses unreadable columns before mutation, and refuses an absent/disabled TOTP trigger with a restore-first message. All eight findings from the completed Claude review are addressed or explicitly assessed in the review-resolution comment.

[kshivang]

@claude Please review current head d32a200 against dev, focusing on legacy/v1/v2 encryption migration compatibility, TOTP triggers, key rotation, and pgTAP regression coverage. Isolated tests only; no live database operations.

[claude]

Claude finished @kshivang's task in 12m 3s —— View job

---

Review of d32a200 against dev

• Read the three changed files at head
• Check legacy / v1 / v2 envelope compatibility across readers and writers
• Check the TOTP trigger interaction and the backfill's trigger disable
• Check ops/rotate_master_encryption_key.sql and the suite that executes it
• Check pgTAP regression coverage
• Post review

No live database was used - this is source analysis only, per the request.

Verdict: the cryptographic change is correct. The envelope is unambiguous, the fallback is sound, and the rotation helper handles both shapes. Everything I found is about migration robustness and test coverage, not about the cipher work.

---

What I verified as correct

• v2: cannot collide with a legacy value. : is not in the base64 alphabet, so no pre-migration ciphertext can ever match LIKE 'v2:%'. The dispatch is safe in both directions.
• substring(x FROM y) really is safe - but not for the reason the comment at 20260914000000_randomize_secret_encryption_iv.sql:94-98 gives. The grammar rewrites the FROM/FOR form through SystemFuncName(), which hard-codes pg_catalog.substring; search_path never gets a say. That distinction matters here, because these are SECURITY DEFINER functions whose search_path lists public ahead of pg_catalog (pre-existing, inherited from 20251023000005), so any other unqualified call would resolve out of public first. The comment as written could lead a future editor to believe search_path protects unqualified calls in this file. Everything else in both bodies is schema-qualified, so there is no live exposure.
• NULL propagation is preserved. 'v2:' || encode(NULL) is NULL, matching the old encode(NULL). Callers that store NULL for "not set" are unaffected.
• The long test key works, and confirms defect 2 is genuinely untouched. pgcrypto's combo_init truncates to px_cipher_key_size before px_cipher_init, so the 36-character fixture key and the existing 34-character TOTP fixture key both silently become 32 bytes. Consistent with leaving key truncation out of scope.
• No privilege regression from the replacement. CREATE OR REPLACE preserves the ACL, and the enforce_explicit_anon_grants event trigger (as rewritten by 20260912120000) only re-revokes public, anon - it no longer materialises PUBLIC-derived access as a grant. encrypt_text/decrypt_text stay revoked per 20260909120000.
• Nothing outside the database reads these columns. No Kotlin or Edge Function decrypts password_encrypted / recovery_codes_encrypted / twofa_secret, and no index, constraint or query depends on ciphertext equality - so randomizing the IV breaks no reader.
• Rotation helper arithmetic is right. Prefix stripping is ''→substr(col,1) and 'v1:'→substr(col,4); pg_temp exists by the time rotate_ciphertext is created (rot_fp is created at step 1, before it); the helper is dropped at the end of each run, so the five sequential \ir invocations in master_key_rotation_test.sql.in all work; and both throws_ok guards raise before the helper is created, so they leave nothing behind. A v1: row whose inner payload is still legacy (rotating a database where this migration has not been applied) takes the legacy branch correctly, so the ops script is backwards compatible with a pre-migration database.

---

Findings

1. The legacy branch of pg_temp.rotate_ciphertext is now unreachable in the rotation suite. supabase/ops/rotate_master_encryption_key.sql:261-265

master_key_rotation_test.sql.in builds every fixture row through public.encrypt_text (create_secret, the four qbo_token_state columns, the two google_token_state columns) and the TOTP row through the trigger, which also calls encrypt_text. After this migration all of those are v2:, so the suite now exercises only the v2 branch. The legacy branch is the one that will actually run in production against qbo_token_state and google_token_state, which the backfill deliberately does not convert - and it has no coverage at all. One hand-built zero-IV broker row (the exact expression the new test file already uses at secret_encryption_iv_randomization_test.sql:35-38) plus an assertion that it survives rotation and stays legacy-framed would close it. Fix this →

2. Nothing asserts encrypt_twofa_secret_trigger is enabled after migrations. supabase/migrations/20260914000000_randomize_secret_encryption_iv.sql:146,153

This PR is the first migration to disable that trigger and re-enable it. The only standing check is has_trigger(...) at secret_totp_encryption_test.sql:18, which reads pg_trigger existence and says nothing about tgenabled. So a migration that disables the trigger and fails to re-enable it - this one, or any future one - produces a fully green pgTAP run while every subsequent TOTP write stores the raw base32 seed in plaintext. That is exactly the failure 20260909000000 exists to prevent, and it is currently invisible. One assertion (is((select tgenabled from pg_trigger where tgname = 'encrypt_twofa_secret_trigger' and tgrelid = 'public.secret_metadata'::regclass), 'O')) pins it. Fix this →

3. The DISABLE/ENABLE TRIGGER pair is unguarded, unlike the precedent it cites. …20260914000000…sql:146,153

The header says it mirrors ops/rotate_master_encryption_key.sql, but that script wraps both halves in a to_regclass + pg_trigger existence check (:217-226, :281-290) and this does not. Two consequences:

• ALTER TABLE ... DISABLE TRIGGER <name> raises if the trigger is absent, so the migration hard-fails on any database where an operator dropped it - which 20260909000000's own header (:12-16) describes operators doing for restores.
• If the trigger is already disabled when the migration runs, the unconditional ENABLE at the end turns on a trigger the operator deliberately turned off. (The ops script shares this second flaw, so this is about not spreading it.)

Copying the existence guard costs four lines and matches the established pattern.

4. decrypt_text raises inside the backfill, so an undecryptable row fails the deploy opaquely - with the trigger already off. …20260914000000…sql:128-151

The backfill calls plain public.decrypt_text on all three columns. This codebase treats undecryptable rows as a live possibility rather than a hypothetical: safe_decrypt_recovery_codes exists for exactly that, and the ops script's step 0 (:176-190) pre-checks every mapped column through its own read path specifically so that "an already-corrupt row would only surface at step 5 ... pointing at the wrong thing". Here one such row aborts the migration with a bare Wrong key or corrupt data, naming neither table nor column nor id.

It also interacts with the trigger window. Whether the whole file is atomic depends on the apply path (the Supabase CLI's multi-statement file send lands in an implicit transaction block; psql -f without --single-transaction does not), and the ops script's header is explicit that this is why it is "ONE DO block, so it is atomic no matter how the caller wraps statements". Under a non-atomic apply, a failure in the twofa_secret UPDATE leaves encrypt_twofa_secret_trigger disabled, and because ALTER TABLE ... DISABLE TRIGGER only holds its ShareRowExclusiveLock for the duration of the statement, concurrent application writes during the backfill would store plaintext seeds.

Cheapest fix that closes both: a step-0-style pre-check that counts undecryptable rows per column and raises a message naming the column, placed before any trigger is touched - plus wrapping the backfill in a single DO block so the trigger window cannot outlive a failure. Worth noting there is also a trigger-free formulation for the twofa statement (SET twofa_secret = public.decrypt_text(substring(twofa_secret from 4)), letting the enabled trigger re-apply the envelope; the BEFORE trigger rewrites NEW before the tuple is formed, so no plaintext reaches WAL) - the trade-off being that it silently writes plaintext if the trigger happens to be disabled, which is why the explicit-disable version is defensible. I would not change the approach, only make the window atomic. Fix this →

5. The new pgTAP file covers only the least risky of the three backfill statements. supabase/tests/secret_encryption_iv_randomization_test.sql:60-88

It replays the secrets.password_encrypted UPDATE, which is the simple one. Uncovered: the recovery_codes_encrypted statement, and the twofa_secret statement - which is the only one with a trigger interaction, the only one that rewrites a nested envelope, and therefore the only one where getting the substring offset or the NOT LIKE guard wrong is plausible. Worth adding: a v1:-framed legacy row, converted by that statement to v1:v2:…, asserted idempotent on a second run, and asserted to round-trip through public.safe_decrypt_twofa_secret - that last one is the app's actual read path for the nested form and nothing tests it against a v2: payload today.

6. v2: now overloads a prefix namespace that already had a different meaning. …20260914000000…sql:71, supabase/tests/master_key_rotation_test.sql.in:104-112

Stored TOTP values are now v1:v2:<base64> - an outer envelope version from 20260909000000 and an inner one from this migration, two independent version namespaces sharing one syntax. The rotation suite makes the hazard concrete: it uses a literal 'v2:' outer prefix as its stand-in for "an unrecognised storage envelope" and asserts rotation refuses. That still passes (the guard accepts only v1:%), but the fixture now reads as though v2: were a real outer envelope, and a future reviewer reconciling the two is being set up to fail. Nothing here needs a wire-format change; a sentence at each prefix definition stating the two namespaces are independent, and switching the negative fixture to something that cannot be confused (v9:), would settle it.

7. Six broker columns stay deterministic at rest, permanently, with no follow-up named. …20260914000000…sql:47-49

qbo_token_state.{client_id,client_secret,refresh_token,access_token}_enc and google_token_state.{private_key,access_token}_enc are written through encrypt_text, so new writes are v2 - but existing rows keep the zero-IV format forever, because nothing converts them and rotation now deliberately preserves whichever shape it finds. The header mentions them only in passing as "still-unmigrated", which reads like a timing statement rather than a permanent state. These are broker tokens rather than user-chosen passwords, so the shared-value oracle argument is much weaker - but either extend the backfill (guarded by to_regclass, since the tables are optional deployments) or say plainly in the header that those rows are never converted and why that is acceptable.

8. Nit - unconditional vault.create_secret in the new test. secret_encryption_iv_randomization_test.sql:10

This matches secret_totp_encryption_test.sql:6, and there is no key in seed.sql, so it is green today. But master_key_rotation_test.sql.in:14-25 carries a comment recording that the unconditional form "made the whole suite fail in exactly that case" once a seed, migration or previous run held the name, since vault.secrets.name is unique. The conditional create-or-update form is three lines and immune.

---

Scope calls I agree with: leaving key truncation to an operator who knows the live encoding, leaving AEAD as its own wire-format decision, and not rolling back a partial backfill. The NOT LIKE 'v2:%' idempotency guard is the right shape and the test proves it.
· branch fix/secret-encryption-deterministic-iv

[kshivang]

Claude review resolution for d32a200, implemented in ba3d46f and 095ec0c:

1. Legacy rotation coverage: added a hand-built zero-IV broker value alongside v2 values, asserting plaintext preservation, unchanged legacy framing, v2 framing and a fresh IV.
2. Enabled trigger: real migration replay now asserts tgenabled = 'O', rejects imported envelopes and verifies subsequent plaintext writes encrypt/decrypt.
3. Missing/disabled trigger: deliberately fail closed, with an actionable restore-first error before backfill. Skipping a missing trigger would leave subsequent TOTP writes unencrypted. The prerequisite TOTP migration installs this trigger; its restore instructions say disable/re-enable, not leave it missing. Silently enabling an operator-disabled trigger is also avoided. Optional broker tables in the rotation script are a different contract from mandatory core secret storage.
4. Atomicity/diagnostics: all backfills and trigger ALTERs now run in one DO block, including under psql without --single-transaction. A preflight counts unreadable legacy values per column and raises a recovery hint without values/keys. Regression coverage proves corrupt TOTP is preserved and the trigger stays enabled.
5. Populated backfill coverage: the actual migration is replayed twice over password/recovery/TOTP fixtures, with exact backslash/Unicode preservation and byte-identical idempotency.
6. Namespace clarity: documented independent inner encryption/outer TOTP versions; unknown outer-version rotation fixture is now v9:.
7. Broker scope: clarified that existing broker values are not upgraded by this migration or rotation; future application writes use v2, and an operator-owned backfill is needed for the external schemas. No live broker operation was performed.
8. Test fixture key: both new suites now create or update their transaction-local fixture key.

Also corrected the substring comment to explain parser resolution rather than claiming search_path protection. The original backslash bug was independently reproduced and repaired. 826 assertions / 34 isolated PostgreSQL 17 suites PASS, including all new regressions and rotation tests; fixture generation/check and diff whitespace checks pass. Current-head CI still must pass; no additional Claude request is needed for these direct resolutions.