[akashjadon04]

Fixes #620

Summary

• Add a per-field decryption boundary that returns NULL for corrupt ciphertext and emits a SQLSTATE-only warning.
• Keep the desktop client's non-null password contract by returning an empty string for a bad encrypted password, while preserving every valid row.
• Apply the behavior to owned, searched, and directly shared secret listings, including recovery-code handling.
• Add database regression coverage plus desktop decoding coverage.

Verification

• git diff --check
• Database and Kotlin test suites are included, but this machine has no local Supabase/Postgres service or Java runtime to execute them here.

Review focus

Please verify the fail-soft behavior remains scoped to listing reads and that no secret ciphertext or error details are exposed in logs.

[kshivang]

@claude Please review this assigned PR for correctness and compare its implementation/coverage with #621. In particular check its migration timestamp against merged #647 and its broad error handler versus systemic key/configuration failures. This is a single review request; consolidation is being evaluated with contributor credit and retained tests.

[claude]

Claude finished @kshivang's task in 6m 42s —— View job

---

Review: fail-soft secret listing decryption

• Read the PR diff (migration, pgTAP suite, desktop test)
• Check the migration timestamp against what is already on dev
• Evaluate the broad error handler against systemic key/configuration failure
• Compare with fix(security): don't let one undecryptable row abort the secrets listing #621 — blocked, see note at the end
• Post review

---

1. Blocker: the migration version collides with one already on dev

origin/dev already contains supabase/migrations/20260914000000_randomize_secret_encryption_iv.sql (the #618 IV work, merged via the PR you referenced). This PR adds supabase/migrations/20260914000000_fail_soft_secret_listing.sql — the same 20260914000000 version prefix.

The Supabase CLI keys supabase_migrations.schema_migrations on that numeric prefix, so the two files are the same migration as far as history is concerned. Two bad outcomes, depending on the path:

• supabase db start / db reset (which is what database-tests in build.yml:415 runs before supabase test db) applies both files and inserts both versions — a primary-key conflict on the history table.
• supabase db push against a project where fix(security): randomize the IV for encrypt_text/decrypt_text #647 has already been applied sees 20260914000000 as already applied and skips this file silently. The fix never reaches production and nothing reports that.

Ordering between the two is also only defined by the rest of the filename (fail_soft… sorts before randomize…), which is not something to rely on. Bump this to a later distinct version, e.g. 20260915000000_fail_soft_secret_listing.sql.

Fix this →

Related: the branch is cut from main, not dev, so it has not been exercised against the v2: envelope that decrypt_text grows in that IV migration. The corrupt fixture ('not-valid-ciphertext') still raises on both the legacy and the v2: dispatch path, so the pgTAP suite should survive the rebase — but it has not actually been run that way.

2. High: WHEN OTHERS turns a systemic key failure into silent mass blanking

try_decrypt_text (20260914000000_fail_soft_secret_listing.sql:21-29) catches WHEN OTHERS around public.decrypt_text. That is not only "this row's ciphertext is corrupt". The same handler swallows:

• Missing or renamed Vault key. get_encryption_key() raises P0001 with the literal message "Encryption key not found in vault…" (20251023000005_encryption_functions.sql:64-66). Every row now returns '' and the RPC returns HTTP 200.
• Wrong key — a half-applied ops/rotate_master_encryption_key.sql, or a key whose encoding changed (still-open defect 2 of encrypt_text encrypts every stored secret with a fixed all-zero IV #618). Same outcome.
• extensions not reachable, resource errors, and anything else class-39/53.

So the failure mode flips from "loading secrets fails loudly" to "the user's entire vault renders with blank passwords and no error anywhere except a Postgres-log WARNING they cannot see." Two concrete harms:

• Write-back destroys ciphertext. password is the same field the edit form round-trips (SecretService.kt:250, update_secret). Saving any unrelated field on a blanked row re-encrypts '' over a recoverable ciphertext. That path lives in the plugin repo, so this PR cannot fix it — but it should not ship without that being checked, because it is unrecoverable.
• It removes the operator's only signal, while fix(security): randomize the IV for encrypt_text/decrypt_text #647 still hard-refuses on those rows. The IV backfill on dev raises 'IV upgrade refused: % unreadable rows…' for exactly the rows this change makes invisible. After this lands, a user sees nothing wrong and the next encryption migration will not deploy, with no way to identify which row to recover.

WHEN OTHERS does at least not swallow QUERY_CANCELED, and there is prior art in-repo (safe_decrypt_recovery_codes at 20251023000005_encryption_functions.sql:300, safe_decrypt_twofa_secret at 20260909000000_encrypt_totp.sql:34), so this is consistent rather than novel. But those cover a metadata sidecar; this one covers the primary field, for every listing path, and that is where the blast radius changes.

Two ways to close it, either acceptable:

• Narrow the handler to the per-row data faults. Corrupt ciphertext surfaces as decode's invalid-base64 (22023), pgcrypto's own decrypt error, and convert_from's invalid UTF-8 (22021) — roughly WHEN data_exception OR external_routine_invocation_exception. I am reasoning about pgcrypto's SQLSTATE from source rather than measurement, so pin the actual codes in the pgTAP suite rather than trusting that list; the point is that P0001 from get_encryption_key must fall through and keep raising.
• Or add a cheap precondition. PERFORM public.get_encryption_key(); at the top of each RPC keeps a missing Vault key loud with no SQLSTATE guesswork. It does not catch a wrong key; a canary decrypt would.

Either way the test suite is missing the case that matters most: there is no test asserting that a missing or wrong master key still fails loudly. That is the assertion that stops this helper being widened later.

Fix this →

3. Medium: '' is indistinguishable from an empty password

COALESCE(try_decrypt_text(...), '') is the right call given SecretEntry.password: String is non-null with no default (SecretModels.kt:14) — a null there would throw and empty the whole page, which is the exact trap AGENTS.md records. But the resulting value cannot be told apart from a secret whose password is genuinely empty, and it also changes a genuinely NULL password_encrypted into ''.

The UI therefore cannot say "this one could not be decrypted", which is what the user needs in order to act. Options, in order of preference:

• Add a password_decrypt_failed boolean output column. The supabaseJson decoder ignores unknown keys, so older clients are unaffected. Note this needs DROP FUNCTION + CREATE — CREATE OR REPLACE cannot change the RETURNS TABLE shape — and the grants at :234-239 must then be restated (they already are).
• Or make password: String? = null in SecretModels.kt, which is the convention AGENTS.md states for projected columns, and let null mean "undecryptable".

Same observation applies to recovery_codes returning '[]', but that is pre-existing safe_decrypt_recovery_codes behaviour and not this PR's to change.

4. Medium-low: the new decryption helper is not in the standing audit

supabase/audit/identity_disclosure_audit.sql:224-228 is the CI-gated allowlist for internal-only crypto routines — it already names get_encryption_key, encrypt_text, decrypt_text and safe_decrypt_recovery_codes, and db_access_audit_test.sql.in:6-10 gates CHECK 5 as HEALTHY. public.try_decrypt_text(text) is another decryption oracle and belongs in that array, so that a future migration granting it to authenticated fails the build rather than relying on this PR's one-off has_function_privilege assertion at secret_listing_decryption_resilience_test.sql:139. Adding an element does not change the test's plan count. (safe_decrypt_twofa_secret is missing from that list too — pre-existing, worth the same one-line fix.)

Fix this →

5. Low: the Kotlin test is close to tautological

SecretDecodingTest gains a case that builds a JSON array with "password": "" and asserts both models decode two rows. No client code changed in this PR, and that test passes identically before and after the migration — it pins kotlinx behaviour, not the fix. The load-bearing fact is the opposite one: password is non-null with no default, so a null from the RPC would fail the whole page. A test asserting that password: null fails to decode is what actually documents why the migration must COALESCE. Worth keeping if consolidated, but retarget it.

What is right, and verified

• Signature fidelity. All three RETURNS TABLE shapes match the current definitions column-for-column and in order (get_user_secrets/search_user_secrets against 20260907000000_secrets_paging_tiebreaker.sql:72-77, 124-129; get_user_secrets_with_shared against 20260909000000_encrypt_totp.sql:108-115), so CREATE OR REPLACE will not fail and no column is dropped. The twofa_secret asymmetry — present in with_shared, absent from the other two — is faithful to the existing definitions and not an omission.
• Coverage is complete for listing. Those three are the only routines that decrypt password_encrypted; there is no single-secret read left behind.
• Forward-migration rather than editing history, matching this repo's stated convention, with the reason written down.
• The helper is properly revoked from PUBLIC, anon, authenticated, service_role and the pgTAP suite proves the 42501 at :113-118.
• No exception text and no ciphertext in the RAISE WARNING — SQLSTATE only. That part of the ask is met.
• The pgTAP suite covers the direct-share path and asserts access metadata survives (:124-138), and plan(17) matches the 17 assertions (I counted).

On the #621 comparison

I could not do it. This run has no gh, no network fetch and only a shallow origin/dev, so #620, #621 and #647 are all unreadable from here — I identified the version collision from the file present in origin/dev's tree, not from #647 itself. Rather than guess at #621's contents, here is what a consolidation decision should check against this PR, given the above: whether it fails loudly on a systemic key failure (§2), whether it exposes per-row failure to the client (§3), whether it keeps the three RPC signatures byte-identical (this one does), and whether its pgTAP suite covers the share path (this one does). Re-run me with gh available, or paste #621's diff, and I will do the side-by-side.

Nothing was changed on the branch — this is review only, as asked.
• branch fix/fail-soft-secret-listing

[kshivang]

@akashjadon04 Your change correctly preserves the installed desktop password contract and adds useful assertions for valid search/shared passwords, recovery-code arrays, direct-share attribution and helper access denial. I compared the actual code: all three listing RPC bodies are equivalent to #621 after normalization; the desktop mixed-row case is already equivalently covered there.

I retained your complete 17-assertion SQL suite byte-for-byte in #621 (secret_listing_decryption_resilience_test.sql), credited you explicitly in its description, and added your Co-authored-by trailer in commit fd3d903. The retained suite passes within 872 assertions across 36 disposable PostgreSQL suites. Coverage details are therefore verified, not inferred from the shared issue number.

Replacement: #621 . It also fixes the shared catch-all handler problem: missing/malformed/empty keys and privilege/missing-function/internal errors propagate instead of presenting a successful blank page. It uses migration version 20260915000000, avoiding this PR's 20260914000000 collision with merged #647, and retains merged #651's migration repairs. No live database was changed.

Closing this PR as a duplicate with your implementation intent and additional tests preserved in the replacement. This is consolidation, not a claim that your useful contribution is unnecessary. #621 still requires its current-head platform CI and requested Claude review before a dev merge; older green checks are not being reused. Exactly one Claude review was requested here; no repeated review loop is needed for the superseded implementation.