fix(frontend): stop one undecryptable secret blanking the environment page - #1004
Open
omlahore wants to merge 4 commits into
Open
fix(frontend): stop one undecryptable secret blanking the environment page#1004omlahore wants to merge 4 commits into
omlahore wants to merge 4 commits into
Conversation
Switching environments keeps this page mounted; only `data` changes. The environment's decryption keys are derived asynchronously from the new data's wrapped seed, but the secrets-decrypting effect fires on the same render that `data` changes, before that derivation has any chance to resolve. It ran with the new environment's ciphertext and the previous environment's still-current envKeys, which decryptAsymmetric rejects. That rejection went to `decryptSecrets().then(...)` with no `.catch()`, an unhandled rejection, so `setDecrypting(false)` was never reached and the page was stuck on "Decrypting..." until reloaded. Switching to a third environment before the second's key derivation resolved could also let it land after the third's, pairing envKeys with the wrong data even once the promise settled. The GetSecrets query above also polls every 5s, so keying the key derivation off `data` directly would re-derive and clear envKeys on every idle poll tick, not just on real environment switches. Track which environment's wrapped seed the current envKeys were derived from instead, so an unrelated poll refresh of the same environment is a no-op. The decrypting effect independently checks envKeys against that same tracked seed before running, rather than assuming the deriving effect's state update is visible to it in the same pass, since the two effects' execution order relative to a state update from one of them is not something to build correctness on. It also gets the missing `.catch()`. Fixes the page getting stuck on "Decrypting..." after switching environments, and the narrower case of a secret from one environment being decrypted with another environment's keys during a fast multi-hop switch.
…hrow Hoisting the wrappedSeed read out of the async function to compare it against the ref changed what an empty environmentKeys array does: it was a rejected promise from inside the async fn, and became a synchronous throw from the effect body, which unmounts the page via the error boundary. Read it with optional chaining and bail instead, so this fix does not alter that failure mode as a side effect.
Switching A -> B and back to A before B's derivation resolved left the ref holding A's seed while envKeys was null, so the effect early-returned on the seed match and never re-derived. Also trims the comments in this file and the test suite.
… page The decrypt pass ran every secret through a single Promise.all, so one row with unreadable ciphertext rejected the whole batch and no user of that environment ever got past the skeleton loaders. Use Promise.allSettled, render the rows that decrypted, log each failure and surface a count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #948.
The problem
decryptSecretsruns every secret through a singlePromise.all. One row whose ciphertext cannot be decoded rejects the whole batch, sosetServerSecrets/setSecretsLoadednever run and every member of that app sits on skeleton loaders indefinitely.The reporter's instance hit this with a value truncated to 16384 characters at write time, months before anyone noticed. The truncation left the base64 segment at
length % 4 === 1, whichsodium.from_base64rejects withinvalid input.#970 added a
.catchon this chain, which stopped the unhandled rejection and releaseddecrypting. It does not fix this issue:setSecretsLoaded(true)is still never reached, so the skeletons stay. The two changes are complementary, which is the other reason this is stacked rather than standalone.The change
Promise.allSettledfor both the static and dynamic passes, then:console.errorper failed row, including the secret id, so the bad row is identifiableThe count matters because of the failure mode described in the issue: the CLI already skips undecodable rows silently,
phase secrets listshowed N-1 with no warning, and that silence is what hid the problem for months. Dropping the rows without saying so would reproduce that in the console.What I did not decide
How a failed row should look is a design call, so I left it out. A greyed row with an "undecryptable" marker in place of dropping it entirely would be better than a toast, and I will add it if you say what it should look like.
Verification
New test at
frontend/tests/utils/crypto/undecryptableSecret.test.tsreproduces the reported corruption (truncating a real ciphertext tolength % 4 === 1) and asserts both halves:Promise.allover the same three rows rejects,Promise.allSettledreturns 2 fulfilled and 1 rejected with the readable value intact.tsc --noEmitclean. Full suite 19 files / 387 tests passing.