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..c1ea01f48 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,40 @@ 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 + + // 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 + // 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 +724,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 +859,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') + }) +})