From 895363fa9d89c5df85fe014036cf88da603907c6 Mon Sep 17 00:00:00 2001 From: Om Date: Wed, 12 Aug 2026 18:05:35 +0530 Subject: [PATCH 1/4] fix(frontend): stop environment switches from decrypting with stale keys 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. --- .../[environment]/[[...path]]/page.tsx | 88 +++++++++++++++---- .../utils/crypto/environmentKeyRace.test.ts | 52 +++++++++++ 2 files changed, 124 insertions(+), 16 deletions(-) create mode 100644 frontend/tests/utils/crypto/environmentKeyRace.test.ts diff --git a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx index f5c96a194..a05ae0e4c 100644 --- a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx +++ b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx @@ -677,10 +677,33 @@ export default function EnvironmentPath({ [deleteFolder, params.environment, secretPath] ) + // wrappedSeed identifies which environment's keys are currently derived. + // GetSecrets polls every 5s (see the useQuery below), so `data` gets a new + // object reference on every poll tick even when nothing changed; keying off + // wrappedSeed rather than `data` itself means an unrelated poll refresh of + // the same environment does not re-trigger key derivation or clear envKeys. + const derivedForSeedRef = useRef(null) + useEffect(() => { - const initEnvKeys = async () => { - const wrappedSeed = data.environmentKeys[0].wrappedSeed + if (!data || !keyring) return + + const wrappedSeed = data.environmentKeys[0].wrappedSeed + if (derivedForSeedRef.current === wrappedSeed) return + + // Switching environments (e.g. via the environment tabs) keeps this page + // mounted and only changes `data`, so a slower-resolving key derivation + // for an environment the user has since navigated away from must not be + // allowed to land after a newer one; `ignore` covers that regardless of + // resolution order. Clearing envKeys here is a display nicety, not the + // race guard itself: it stops the previous environment's already-decrypted + // secrets from staying on screen while this one's keys are still deriving. + // The decryptSecrets effect below does its own check against + // derivedForSeedRef, so it never runs against a mismatched envKeys/data + // pair even if it fires before this line's update is visible to it. + let ignore = false + setEnvKeys(null) + const initEnvKeys = async () => { const userKxKeys = { publicKey: await getUserKxPublicKey(keyring!.publicKey), privateKey: await getUserKxPrivateKey(keyring!.privateKey), @@ -694,18 +717,36 @@ export default function EnvironmentPath({ ) const { publicKey, privateKey } = await envKeyring(seed) - setEnvKeys({ - publicKey, - privateKey, - salt, - }) + if (!ignore) { + derivedForSeedRef.current = wrappedSeed + setEnvKeys({ + publicKey, + privateKey, + salt, + }) + } } - if (data && keyring) initEnvKeys() + initEnvKeys() + + return () => { + ignore = true + } }, [data, keyring]) useEffect(() => { - if (data && envKeys) { + // This is the actual guard against decrypting one environment's secrets + // with another environment's keys. envKeys can be one render behind data + // changing, since the effect above derives it asynchronously, so this + // effect must not trust that envKeys already matches data just because + // both are non-null; it checks against derivedForSeedRef directly instead + // of relying on the ordering of the two effects. + const currentWrappedSeed = data?.environmentKeys[0]?.wrappedSeed + const envKeysAreCurrent = + currentWrappedSeed !== undefined && derivedForSeedRef.current === currentWrappedSeed + + if (data && envKeys && envKeysAreCurrent) { + let ignore = false setDecrypting(true) const decryptSecrets = async () => { const decryptedStaticSecrets = await Promise.all( @@ -811,13 +852,28 @@ export default function EnvironmentPath({ return { decryptedStaticSecrets, decryptedDynamicSecrets } } - decryptSecrets().then((decryptedSecrets) => { - setServerSecrets(decryptedSecrets.decryptedStaticSecrets) - setClientSecrets(decryptedSecrets.decryptedStaticSecrets) - setDynamicSecrets(decryptedSecrets.decryptedDynamicSecrets) - setDecrypting(false) - setSecretsLoaded(true) - }) + decryptSecrets() + .then((decryptedSecrets) => { + if (ignore) return + setServerSecrets(decryptedSecrets.decryptedStaticSecrets) + setClientSecrets(decryptedSecrets.decryptedStaticSecrets) + setDynamicSecrets(decryptedSecrets.decryptedDynamicSecrets) + setDecrypting(false) + setSecretsLoaded(true) + }) + .catch((error) => { + // A decrypt call rejects if envKeys ever gets paired with data from + // a different environment (see the guard in the effect above). This + // used to be an unhandled rejection that left `decrypting` stuck at + // true, so the page never recovered from the race without a reload. + if (ignore) return + console.error('Failed to decrypt secrets:', error) + setDecrypting(false) + }) + + return () => { + ignore = true + } } }, [envKeys, data]) diff --git a/frontend/tests/utils/crypto/environmentKeyRace.test.ts b/frontend/tests/utils/crypto/environmentKeyRace.test.ts new file mode 100644 index 000000000..58870528b --- /dev/null +++ b/frontend/tests/utils/crypto/environmentKeyRace.test.ts @@ -0,0 +1,52 @@ +/** + * @jest-environment node + */ + +/* + 👆 + overrides: testEnvironment: 'jsdom' in jest.config.js + to fix: ReferenceError: TextDecoder is not defined +*/ + +/* + Regression test for the environment-switch race described in the PR that + added this file. It does not mount the page component (that needs Apollo, + Next navigation and the keyring context, none of which are set up in this + suite); instead it proves the property the fix in + app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx + relies on, using the real crypto primitives: decryptAsymmetric rejects, + rather than silently returning garbage, when the keypair does not match the + ciphertext's session. + + That is why the original unguarded effect (pairing one environment's `data` + with another environment's `envKeys` while the correct keys were still + being derived) surfaced as a promise rejection, and why the missing + `.catch()` on that call turned it into an unhandled rejection that left + `decrypting` stuck at `true` with no way to recover short of a reload. +*/ + +import { decryptAsymmetric, encryptAsymmetric, randomKeyPair } from '@/utils/crypto' + +const toHexKeyPair = (keyPair: { publicKey: Uint8Array; privateKey: Uint8Array }) => ({ + publicKey: Buffer.from(keyPair.publicKey).toString('hex'), + privateKey: Buffer.from(keyPair.privateKey).toString('hex'), +}) + +describe("Environment key race (decrypting one environment's secrets with another's keys)", () => { + test('decrypting with a mismatched keypair rejects rather than returning garbage', async () => { + const envA = toHexKeyPair(await randomKeyPair()) + const envB = toHexKeyPair(await randomKeyPair()) + + const ciphertext = await encryptAsymmetric('super-secret-value', envA.publicKey) + + // This is the exact operation the page performs when envKeys still holds + // environment B's keys while data has already updated to environment A's + // secrets (or vice versa): decrypting A's ciphertext with B's keypair. + await expect(decryptAsymmetric(ciphertext, envB.privateKey, envB.publicKey)).rejects.toBeDefined() + + // Decrypting with the matching keypair still works, so the rejection + // above is specifically about the key mismatch, not a broken fixture. + const decrypted = await decryptAsymmetric(ciphertext, envA.privateKey, envA.publicKey) + expect(decrypted).toBe('super-secret-value') + }) +}) From da0984a54283669edbc1232ddf852f43670f7fcd Mon Sep 17 00:00:00 2001 From: Om Date: Wed, 12 Aug 2026 18:34:06 +0530 Subject: [PATCH 2/4] fix(frontend): keep empty environmentKeys a rejected promise, not a throw 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. --- .../environments/[environment]/[[...path]]/page.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx index a05ae0e4c..e80936378 100644 --- a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx +++ b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx @@ -687,7 +687,14 @@ export default function EnvironmentPath({ useEffect(() => { if (!data || !keyring) return - const wrappedSeed = data.environmentKeys[0].wrappedSeed + // Optional chaining rather than a bare index: this read used to sit inside + // the async function below, where an empty environmentKeys would surface + // as a rejected promise. Hoisting it up here to compare against the ref + // would otherwise turn that same case into a synchronous throw from the + // effect body, which takes the page down via the error boundary. Keeping + // the old failure mode rather than changing it as a side effect. + const wrappedSeed = data.environmentKeys[0]?.wrappedSeed + if (!wrappedSeed) return if (derivedForSeedRef.current === wrappedSeed) return // Switching environments (e.g. via the environment tabs) keeps this page From 4ca41e1d43dec42cc78e4c1c9b0afcfa50810ece Mon Sep 17 00:00:00 2001 From: Om Date: Wed, 2 Sep 2026 14:02:48 +0530 Subject: [PATCH 3/4] fix(frontend): clear the derived-seed ref alongside envKeys 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. --- .../[environment]/[[...path]]/page.tsx | 30 ++++++------------- .../utils/crypto/environmentKeyRace.test.ts | 24 ++++----------- 2 files changed, 14 insertions(+), 40 deletions(-) diff --git a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx index e80936378..a9ff08ddf 100644 --- a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx +++ b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx @@ -677,37 +677,25 @@ export default function EnvironmentPath({ [deleteFolder, params.environment, secretPath] ) - // wrappedSeed identifies which environment's keys are currently derived. - // GetSecrets polls every 5s (see the useQuery below), so `data` gets a new - // object reference on every poll tick even when nothing changed; keying off - // wrappedSeed rather than `data` itself means an unrelated poll refresh of - // the same environment does not re-trigger key derivation or clear envKeys. + // Tracks which environment's keys are derived. Keyed off wrappedSeed rather + // than `data`, which gets a new reference on every 5s poll tick. const derivedForSeedRef = useRef(null) useEffect(() => { if (!data || !keyring) return - // Optional chaining rather than a bare index: this read used to sit inside - // the async function below, where an empty environmentKeys would surface - // as a rejected promise. Hoisting it up here to compare against the ref - // would otherwise turn that same case into a synchronous throw from the - // effect body, which takes the page down via the error boundary. Keeping - // the old failure mode rather than changing it as a side effect. + // Optional chaining keeps an empty environmentKeys a rejected promise + // rather than a synchronous throw from the effect body. const wrappedSeed = data.environmentKeys[0]?.wrappedSeed if (!wrappedSeed) return if (derivedForSeedRef.current === wrappedSeed) return - // Switching environments (e.g. via the environment tabs) keeps this page - // mounted and only changes `data`, so a slower-resolving key derivation - // for an environment the user has since navigated away from must not be - // allowed to land after a newer one; `ignore` covers that regardless of - // resolution order. Clearing envKeys here is a display nicety, not the - // race guard itself: it stops the previous environment's already-decrypted - // secrets from staying on screen while this one's keys are still deriving. - // The decryptSecrets effect below does its own check against - // derivedForSeedRef, so it never runs against a mismatched envKeys/data - // pair even if it fires before this line's update is visible to it. + // `ignore` stops a slower derivation for an environment the user has left + // from landing after a newer one. The ref is cleared with envKeys so the + // two cannot disagree: on A -> B -> A before B resolves, a stale ref would + // match on the return to A and the effect would never re-derive. let ignore = false + derivedForSeedRef.current = null setEnvKeys(null) const initEnvKeys = async () => { diff --git a/frontend/tests/utils/crypto/environmentKeyRace.test.ts b/frontend/tests/utils/crypto/environmentKeyRace.test.ts index 58870528b..c9b14607d 100644 --- a/frontend/tests/utils/crypto/environmentKeyRace.test.ts +++ b/frontend/tests/utils/crypto/environmentKeyRace.test.ts @@ -9,20 +9,9 @@ */ /* - Regression test for the environment-switch race described in the PR that - added this file. It does not mount the page component (that needs Apollo, - Next navigation and the keyring context, none of which are set up in this - suite); instead it proves the property the fix in - app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx - relies on, using the real crypto primitives: decryptAsymmetric rejects, - rather than silently returning garbage, when the keypair does not match the - ciphertext's session. - - That is why the original unguarded effect (pairing one environment's `data` - with another environment's `envKeys` while the correct keys were still - being derived) surfaced as a promise rejection, and why the missing - `.catch()` on that call turned it into an unhandled rejection that left - `decrypting` stuck at `true` with no way to recover short of a reload. + Proves the property the environment-switch fix relies on: decryptAsymmetric + rejects, rather than returning garbage, when the keypair does not match the + ciphertext. */ import { decryptAsymmetric, encryptAsymmetric, randomKeyPair } from '@/utils/crypto' @@ -39,13 +28,10 @@ describe("Environment key race (decrypting one environment's secrets with anothe const ciphertext = await encryptAsymmetric('super-secret-value', envA.publicKey) - // This is the exact operation the page performs when envKeys still holds - // environment B's keys while data has already updated to environment A's - // secrets (or vice versa): decrypting A's ciphertext with B's keypair. + // What the page did when envKeys still held B's keys and data was A's. await expect(decryptAsymmetric(ciphertext, envB.privateKey, envB.publicKey)).rejects.toBeDefined() - // Decrypting with the matching keypair still works, so the rejection - // above is specifically about the key mismatch, not a broken fixture. + // The matching keypair still works, so the rejection is the mismatch. const decrypted = await decryptAsymmetric(ciphertext, envA.privateKey, envA.publicKey) expect(decrypted).toBe('super-secret-value') }) From a3b882a3c0b9ee82c204269102b3f5a839ab73b3 Mon Sep 17 00:00:00 2001 From: Om Date: Sun, 6 Sep 2026 04:20:13 +0530 Subject: [PATCH 4/4] fix(frontend): trim the explanatory comments to one or two lines The comments on the two effects and on the decrypt catch had grown to four and six lines each, restating the reasoning the code already shows. Cut each to the part that is not obvious from reading it: why the ref is cleared with envKeys, why being non-null is not proof envKeys matches data, and what the catch is for. Same for the block comment on the race test. --- .../[environment]/[[...path]]/page.tsx | 27 +++++++------------ .../utils/crypto/environmentKeyRace.test.ts | 11 ++++---- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx index a9ff08ddf..e11d2ecbc 100644 --- a/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx +++ b/frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx @@ -677,23 +677,20 @@ export default function EnvironmentPath({ [deleteFolder, params.environment, secretPath] ) - // Tracks which environment's keys are derived. Keyed off wrappedSeed rather - // than `data`, which gets a new reference on every 5s poll tick. + // Which environment's keys are derived. Keyed off wrappedSeed, not `data`, + // which gets a fresh reference on every poll tick. const derivedForSeedRef = useRef(null) useEffect(() => { if (!data || !keyring) return - // Optional chaining keeps an empty environmentKeys a rejected promise - // rather than a synchronous throw from the effect body. + // Optional chaining: an empty environmentKeys must not throw synchronously. const wrappedSeed = data.environmentKeys[0]?.wrappedSeed if (!wrappedSeed) return if (derivedForSeedRef.current === wrappedSeed) return - // `ignore` stops a slower derivation for an environment the user has left - // from landing after a newer one. The ref is cleared with envKeys so the - // two cannot disagree: on A -> B -> A before B resolves, a stale ref would - // match on the return to A and the effect would never re-derive. + // `ignore` drops a derivation the user has already navigated away from. + // The ref is cleared with envKeys so a cancelled pass cannot leave it stale. let ignore = false derivedForSeedRef.current = null setEnvKeys(null) @@ -730,12 +727,8 @@ export default function EnvironmentPath({ }, [data, keyring]) useEffect(() => { - // This is the actual guard against decrypting one environment's secrets - // with another environment's keys. envKeys can be one render behind data - // changing, since the effect above derives it asynchronously, so this - // effect must not trust that envKeys already matches data just because - // both are non-null; it checks against derivedForSeedRef directly instead - // of relying on the ordering of the two effects. + // envKeys can be one render behind data, so being non-null is not proof it + // belongs to this environment. Check the derived seed rather than the order. const currentWrappedSeed = data?.environmentKeys[0]?.wrappedSeed const envKeysAreCurrent = currentWrappedSeed !== undefined && derivedForSeedRef.current === currentWrappedSeed @@ -857,10 +850,8 @@ export default function EnvironmentPath({ setSecretsLoaded(true) }) .catch((error) => { - // A decrypt call rejects if envKeys ever gets paired with data from - // a different environment (see the guard in the effect above). This - // used to be an unhandled rejection that left `decrypting` stuck at - // true, so the page never recovered from the race without a reload. + // Without this the mismatch rejection was unhandled and left + // `decrypting` stuck at true until a reload. if (ignore) return console.error('Failed to decrypt secrets:', error) setDecrypting(false) diff --git a/frontend/tests/utils/crypto/environmentKeyRace.test.ts b/frontend/tests/utils/crypto/environmentKeyRace.test.ts index c9b14607d..ac7491eba 100644 --- a/frontend/tests/utils/crypto/environmentKeyRace.test.ts +++ b/frontend/tests/utils/crypto/environmentKeyRace.test.ts @@ -8,11 +8,8 @@ to fix: ReferenceError: TextDecoder is not defined */ -/* - Proves the property the environment-switch fix relies on: decryptAsymmetric - rejects, rather than returning garbage, when the keypair does not match the - ciphertext. -*/ +// decryptAsymmetric must reject on a mismatched keypair, not return garbage. +// That is the property the environment-switch fix relies on. import { decryptAsymmetric, encryptAsymmetric, randomKeyPair } from '@/utils/crypto' @@ -29,7 +26,9 @@ describe("Environment key race (decrypting one environment's secrets with anothe const ciphertext = await encryptAsymmetric('super-secret-value', envA.publicKey) // What the page did when envKeys still held B's keys and data was A's. - await expect(decryptAsymmetric(ciphertext, envB.privateKey, envB.publicKey)).rejects.toBeDefined() + await expect( + decryptAsymmetric(ciphertext, envB.privateKey, envB.publicKey) + ).rejects.toBeDefined() // The matching keypair still works, so the rejection is the mismatch. const decrypted = await decryptAsymmetric(ciphertext, envA.privateKey, envA.publicKey)