From 1267087d554e31f4778370fdc44812d0e42b4798 Mon Sep 17 00:00:00 2001 From: Om Date: Wed, 12 Aug 2026 18:05:35 +0530 Subject: [PATCH 1/2] 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 8551f630a..650fd3981 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 87f065cf25f2c1db8eb983ee181918d1de1fb805 Mon Sep 17 00:00:00 2001 From: Om Date: Wed, 12 Aug 2026 18:34:06 +0530 Subject: [PATCH 2/2] 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 650fd3981..c1ea01f48 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