[Antriksh1984]

Fixes #620.

One corrupt encrypted password or recovery-code field previously aborted an authorized secrets listing. The three RPCs now preserve rows, return a non-null blank password for corrupt data, and use the existing recovery-code fallback. Authorization, sharing precedence, pagination and RPC schemas are unchanged. Rotation remains strict; no live database deployment is included.

Original implementation and pgTAP coverage: @Antriksh1984.

Review repairs by @kshivang: preserve installed-client non-null password decoding, propagate missing/empty/malformed key and systemic privilege/decoder/internal failures, report SQLSTATE-only warnings, register the internal helper in security audits, and add fault regression cases. Desktop regression coverage is retained from @Rushikeshiitb's #621 and its @kshivang review repairs. @akashjadon04's independent #652 implementation is also acknowledged as related provenance; its coverage was consolidated into #621 previously.

#621 (currently draft, outside this review assignment) and closed #652 address the same #620 listing behavior. Neither is closed or changed by this PR review. #647 is a distinct merged random-IV encryption change, fully retained; v2 does not provide authenticated encryption. A valid-but-wrong key cannot reliably be distinguished from damaged ciphertext in that format.

Validation on repair b3197c5: disposable local PostgreSQL applied the migration chain and passed 860 assertions / 35 suites. Fixture generation/check and diff whitespace checks pass. Focused desktop decoding tests, ktlintCheck and detekt PASS through the shared build lock. The original catch-all helper fails all five systemic-error mutation assertions; supplemental malformed/NULL-key probes pass. All ten hosted current-head checks PASS (run 34912766436). Claude completed the requested original-head review; all substantive findings are resolved in this repair with evidence: #668 (comment) .

No UI change; no visual pass claimed, app launch, new tab or live database operation. Merged into dev as f1eaba0 after current-head checks and review gates passed under the shared merge lock.

[kshivang]

@claude Please review the current head 762c752, including listing authorization, fail-soft decryption, migration ordering and interactions with #621, #652 and #647.

[claude]

Claude finished @kshivang's task in 8m 18s —— View job

---

Review of 762c752 - fail-soft secret listing decryption

• Read the new migration and pgTAP test
• Compare the three RPC bodies against their current definitions (authorization, filters, paging)
• Check try_decrypt_text shape, privileges, and the key-rotation sweep claim
• Check migration ordering / interactions with fix(security): randomize the IV for encrypt_text/decrypt_text #647 (fix(security): don't let one undecryptable row abort the secrets listing #621 / Fix fail-soft secret listing decryption #652 not reachable - see note)
• Post review

The SQL half of this is careful work and the design rationale in the header holds up under checking. But the fix does not reach the user: the host client still fails the whole page on the blanked row, which is the exact symptom #620 describes. Details below.

---

🔴 The blanked row still takes down the whole page, in the Kotlin decoder

password becomes NULL on the wire. On the client:

• composeApp/src/commonMain/kotlin/ai/rever/boss/services/supabase/models/SecretModels.kt:14 - SecretEntry.password: String, non-nullable, no default
• SecretModels.kt:158 - SecretEntryWithSharing.password: String, same
• SecretService.kt:92, :134, :335, :379 - all four listing decodes are atomic decodeFromJsonElement<List<…>>, not decodeListRecovering (which is used only at :511 for getSecretShares)
• SupabaseJson.kt:35-36 states the semantics that make this bite: "Null coercion helps only properties with defaults … required fields without defaults still fail." coerceInputValues substitutes the default value; a non-nullable property with no default cannot be coerced and throws.

So a corrupt row's "password": null raises a SerializationException, sanitizeSupabaseFailure turns it into SupabaseFailure, and the caller gets Result.failure for the entire list. SecretDecodingTest.kt:122-123 already spells out why that is fatal: "kotlinx does not decode partially - it is every row or an exception - so '2' is what separates a working list from the empty panel."

Net effect of the PR as it stands: Postgres no longer aborts, and the client aborts instead. The user still sees every secret fail to load. The DB change is necessary but not sufficient, and the two halves have to ship together.

Note recovery_codes is fine - SecretMetadata.recoveryCodes: List<String> = emptyList() has a default, and the server sends [] rather than null anyway.

Also worth deciding deliberately rather than by accident: once the client accepts it, password = "" (or null) is indistinguishable from a legitimately empty password. A separate "could not decrypt" signal is what stops a user "fixing" a corrupt row by overwriting it.

Fix this →

---

🟠 The new decryption oracle is not in the standing guards

try_decrypt_text is locked down correctly in the migration (:94-96, exactly mirroring safe_decrypt_twofa_secret at 20260909000000:50-51), and the 20260912120000 event trigger independently revokes public, anon at DDL end and raises 42501 if anon EXECUTE survives. Good.

What is missing is the durable pin. Two standing lists enumerate internal-only crypto routines by signature, and neither names the new function:

• supabase/audit/identity_disclosure_audit.sql:224-230 - CHECK 5, which db_access_audit_test.sql.in:6-9 asserts HEALTHY in CI
• supabase/tests/explicit_anon_and_org_visibility_test.sql:5-10 - the anon/authenticated denial sweep

So the lockdown is proven only by this PR's own new test file. Adding public.try_decrypt_text(text) to CHECK 5 gets it a permanent CI gate. public.safe_decrypt_twofa_secret(text) is absent from both lists too - same omission, pre-existing.

Fix this →

---

🟠 Nothing anywhere records that a row failed to decrypt

try_decrypt_text returns NULL and emits nothing - no RAISE WARNING, no count, no marker in the payload. That diverges from this repo's own fail-soft convention: decodeListRecovering logs rows_dropped/rows_total precisely so a silent fail-soft cannot hide a real fault (SupabaseJson.kt:156-163).

The operator consequence lands squarely on the rotation interaction. ops/rotate_master_encryption_key.sql:180-189 is the only other thing that reads these columns, and its pre-flight for secrets.password_encrypted maps to raw public.decrypt_text (:54), which raises rather than returning NULL - so the count-based message ("N rows of x.y cannot be read through z") never prints for a password; the rotation aborts with a bare invalid byte sequence for encoding UTF8 instead. Pre-existing, but this PR makes it the first place anyone learns about the corruption, because the UI now hides it.

Two suggestions:

1. RAISE WARNING with SQLSTATE only, never SQLERRM - the message on a UTF-8 failure can quote partially decrypted bytes, and AGENTS.md's sanitizeSupabaseFailure rule exists for exactly that.
2. Re-raise insufficient_privilege (42501) rather than returning NULL. WHEN OTHERS already excludes query_canceled, but it does swallow a permission error - so a mis-applied grant migration would silently blank every password in the UI instead of failing loudly. Data corruption should fail soft; a configuration error should not. (Same critique applies to the two existing wrappers, so this is optional / a follow-up.)

Fix this →

---

🟡 safe_decrypt_recovery_codes is being re-adopted in the state it was abandoned in

Routing recovery codes back through it is right, and the PR's archaeology checks out (20251023000005:236-237 does say "Used internally by get_user_secrets functions", and 20260907000000/20260909000000 really do call raw decrypt_text with the ad-hoc CASE). Two things about that function are worth cleaning up in the same change, now that it is the canonical read path again:

• service_role still holds EXECUTE. 20251023000014:383 granted it; 20260909120000:8 revokes only public, anon, authenticated. safe_decrypt_twofa_secret and the new try_decrypt_text both revoke service_role explicitly, with the reasoning written out at 20260909000000:48-49 ("Service-key callers also use an authorized RPC, not this raw decrypt helper"). This one is the odd one out.
• No SET search_path. It is SECURITY DEFINER with a mutable search path (20251023000005:286-288). Low risk in practice - the body fully qualifies public.decrypt_text and jsonb/text resolve from pg_catalog - but it is what Supabase's own linter flags, and adding the pin is safe for exactly that reason.

Fix this →

---

🟡 One inaccuracy in the header, and the header is the documentation

Lines 11-12 attribute a possible "HMAC/padding mismatch" to the v2 envelope. 20260914000000:46-49 explicitly leaves defect 3 (no MAC/AEAD) open - there is no HMAC in the stored envelope at all (the per-row HMACs in that migration are backfill-verification scaffolding, dropped at commit). The real v2 failure modes are a short or garbled envelope, an AES padding error, and the UTF-8 decode failure you already name.

Fix this →

---

✅ What I verified and found correct

Bodies are faithful. I diffed all three against their live definitions. get_user_secrets and search_user_secrets are byte-identical to 20260907000000:69-166 apart from the two decrypt calls and one dropped inline comment above the WHERE; get_user_secrets_with_shared matches 20260909000000:105-210 the same way. Grep confirms nothing between 20260909000000 and this migration redefines them, so "the current definitions" is accurate. Signatures, RETURNS TABLE lists, DISTINCT ON precedence, ORDER BY s.created_at DESC, s.id DESC, STABLE SECURITY DEFINER, SET search_path TO '' and the owner are all preserved.

Type equivalence on recovery codes. safe_decrypt_recovery_codes returns jsonb and answers '[]' on NULL input, so it is behaviourally identical to the CASE WHEN … IS NOT NULL THEN decrypt_text(…)::jsonb ELSE '[]' END it replaces, plus the fail-soft. This is the kind of thing that silently turns a JSON array into a JSON string if the wrapper had returned text, and it does not.

Listing authorization is untouched and still correct. get_user_secrets/search_user_secrets gate on s.user_id = auth.uid() OR (s.org_id IS NOT NULL AND is_org_member(s.org_id)); is_user_admin feeds only can_manage, never row visibility. with_shared's five sources keep the expiry predicate on all three share-derived branches, and DISTINCT ON (a.id) ORDER BY a.id, a.is_owner DESC, a.priority still resolves owner-before-share. No widening.

The naming rationale is real and already CI-enforced. ops/rotate_master_encryption_key.sql:123-135 sweeps pg_proc for proname like 'safe\_decrypt\_%' and raises No rotation adapter for public.% for any unmapped match, and master_key_rotation_test.sql.in inlines that whole script (prepare-db-access-tests.py:31) and runs it under supabase test db. So naming this safe_decrypt_text would have failed CI, not merely been untidy. Good call, and the header's explanation of why is worth keeping.

Keeping raw decrypt_text in the ops script is right, not an oversight. try_decrypt_text is strictly decrypt_text plus NULL-on-error, so verifying a rotation with the strict form is the fail-closed choice the script wants - unlike the safe_decrypt_* cases, where the wrapper tolerates a format the plain function does not. Worth a one-line note in that script's cols comment (:48-52), which now reads as "the function the APPLICATION uses" and is no longer literally true for password_encrypted.

Grants and the event triggers interact cleanly. CREATE OR REPLACE FUNCTION carries the tag CREATE FUNCTION, so enforce_explicit_anon_grants fires on all four definitions; it revokes only public, anon, CREATE OR REPLACE preserves existing ACL entries, and the migration restating the three RPC grants at :316-318 is harmless belt-and-braces (the comment says as much). The migration lands after 20260912120000, so the stricter, fails-closed version of the trigger is the one in force.

Ordering. 20260914010000 > 20260914000000 (#647), so decrypt_text is already the v2-dispatching version and the backfill has run before try_decrypt_text is created - though nothing here actually depends on the backfill, only on decrypt_text existing. No timestamp collision with anything in supabase/migrations. CREATE OR REPLACE rather than editing the historical files is the right call and the header's reasoning about checksum drift is correct.

Test quality. Genuinely good: the corrupt fixtures are the same two values proven to fail try_decrypt_text earlier in the file rather than a third unverified shape (v2:dGVzdA== decodes to 4 bytes, so the 16-byte IV split leaves an empty body and decrypt_iv raises - the claim holds); has_function_privilege('role', …) is used rather than depending on the connection role; the throws_ok(…, '42501') complements the privilege assertions with a real call; and because every RPC assertion runs under SET LOCAL ROLE authenticated, the test implicitly proves the grants survived the replace. lives_ok + row count + both the good and the bad row's values, per RPC, means swapping any single call back to decrypt_text fails a named test. The vault fixture's create-or-update shape matches master_key_rotation_test.sql.in:15-24, including the reason it exists.

Performance. The BEGIN/EXCEPTION block is a subtransaction per row per field, so a 50-row page now opens ~100 where it opened ~50. Read-only subtransactions acquire no XID, so there is no subxid-cache overflow concern; this is the same cost the recovery-codes wrapper has always paid.

---

Notes on scope

• fix(security): don't let one undecryptable row abort the secrets listing #621 and Fix fail-soft secret listing decryption #652: I could not read these. This environment has no network and no gh/API access, and neither number appears anywhere in the tree (the only in-repo hits for #621 and #652 are unrelated - #621 in docs/release-notes/v8.16.36.md:14 is the terminal-to-plugin move). If you tell me what they cover, or they land as files on a branch I can see, I will check the interaction specifically. fix(security): randomize the IV for encrypt_text/decrypt_text #647 I could review fully, since it is 20260914000000 in-tree, and it is covered above.
• I did not execute anything: no local Postgres/pgTAP, and gh/python3/network calls are not permitted for this run. Everything above is read-through against the schema, the prior migrations, the ops script, the CI workflow and the Kotlin client. The pgTAP job is build.yml:395-429, and supabase test db picks the new file up automatically - it is not a generated *_test.sql.in suite, so prepare-db-access-tests.py --check will not complain about it.

Summary

The migration itself is well-reasoned and I would take it. But it should not merge as the fix for #620 on its own - the client-side password: String non-nullable-without-default is the second half of the same defect, and without it the user-visible behaviour does not change. The standing-audit entry for try_decrypt_text and some signal when a row is blanked are the other two I would want before calling this closed.
| branch fix/failsoft-secret-listing-decryption

[kshivang]

@Antriksh1984 Thanks for the focused listing recovery change and real encrypted/corrupt pgTAP fixtures. I repaired the end-to-end contract in b3197c5 on your original branch, preserving your authorship.

Reconciled the completed Claude review of 762c752 against this repair:

• NULL password decoder failure: fixed at all three SQL RPC boundaries with COALESCE(..., ''), so already-installed clients benefit. Retained fix(security): don't let one undecryptable row abort the secrets listing #621's desktop tests proving NULL fails and mixed good/blank rows decode in both models, with explicit credit in the description. Changing models to nullable would not repair installed clients.
• Standing crypto audit omission: added try_decrypt_text(text) to both audit/denial lists. Existing helper grants remain restricted for anon, authenticated and service_role; direct authenticated execution is tested.
• Silent fallback/systemic failure masking: SQLSTATE-only warning, no SQLERRM or data. Missing/empty/malformed keys are validated outside the handler; privilege, undefined-function and internal errors propagate. Five fault assertions fail under the original catch-all helper and pass after repair; supplemental NULL/malformed-key probes also pass.
• HMAC documentation: corrected. Merged fix(security): randomize the IV for encrypt_text/decrypt_text #647 v2 uses a random IV, without authenticated encryption.
• Optional distinct UI failure flag: deferred as an additive product contract, not required for this compatible listing repair. The SQL warning supplies the operational signal; the existing Secret Manager edit action is disabled for a blank password (SecretManagerContent.kt, password.isNotBlank guards). No automatic writeback is introduced. Blank remains indistinguishable from an intentionally empty stored value; this limitation is explicit.
• Optional expansion to pre-existing TOTP guards/rotation comments: existing TOTP ACL and strict rotation behavior are unchanged and covered by the full database suite; no broader crypto refactor is included.

Independently verified #621/#652 overlap and #647/current migration ordering, which Claude could not fetch. #621 remains an outside-assignment draft; #652 was already consolidated there and closed. Neither was mutated. Their contributor/test provenance is explicit in this PR description. All three normalized RPC bodies match current dev apart from intended decrypt substitutions; authorization, sharing, ordering and output schemas are preserved.

Validation: 860 assertions / 35 disposable PostgreSQL suites pass, plus the two key probes and mutation evidence. Current-head hosted database, quality, compatibility, shell, version and AI checks pass; platform builds are still running. No live database changes or visual pass claimed. Will merge only to dev after the remaining gates pass under the shared merge lock.