Repository navigation
perf(database): reuse the derived key when decrypting a stored value again - #3095
Conversation
|
@greptileai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
| await expect(decrypt(stored)).resolves.toBe("drive-config"); | ||
| await expect(decrypt(stored)).resolves.toBe("drive-config"); | ||
| await expect( | ||
| Promise.all([decrypt(stored), decrypt(stored)]), | ||
| ).resolves.toEqual(["drive-config", "drive-config"]); |
There was a problem hiding this comment.
Concurrent test skips pending keys
The concurrent calls start only after two completed calls have filled the cache. This checks reuse of a finished key, not sharing a pending derivation. A change that caches only finished keys would still pass, even if concurrent requests repeat the expensive work.
Move Promise.all([decrypt(stored), decrypt(stored)]) before the sequential calls and keep the one-call assertion on deriveKey.
| await expect(decrypt(stored)).resolves.toBe("drive-config"); | |
| await expect(decrypt(stored)).resolves.toBe("drive-config"); | |
| await expect( | |
| Promise.all([decrypt(stored), decrypt(stored)]), | |
| ).resolves.toEqual(["drive-config", "drive-config"]); | |
| await expect( | |
| Promise.all([decrypt(stored), decrypt(stored)]), | |
| ).resolves.toEqual(["drive-config", "drive-config"]); | |
| await expect(decrypt(stored)).resolves.toBe("drive-config"); | |
| await expect(decrypt(stored)).resolves.toBe("drive-config"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/unit/database-crypto.test.ts
Line: 19-23
Comment:
**Concurrent test skips pending keys**
The concurrent calls start only after two completed calls have filled the cache. This checks reuse of a finished key, not sharing a pending derivation. A change that caches only finished keys would still pass, even if concurrent requests repeat the expensive work.
Move `Promise.all([decrypt(stored), decrypt(stored)])` before the sequential calls and keep the one-call assertion on `deriveKey`.
```suggestion
await expect(
Promise.all([decrypt(stored), decrypt(stored)]),
).resolves.toEqual(["drive-config", "drive-config"]);
await expect(decrypt(stored)).resolves.toBe("drive-config");
await expect(decrypt(stored)).resolves.toBe("drive-config");
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Applied in 180b8c8. The concurrent decrypts now run first, against a cold cache, and deriveKey still has to be called exactly once. I checked it against a variant that only caches keys once they've finished deriving: that variant now fails this test.
|
@greptileai review |
|
@greptileai review |
decryptderives its AES key with PBKDF2-SHA256 at 100,000 iterations on every call. That's about 10 ms on an M-series Mac and more on a serverless CPU; 64 concurrent derivations take about 175 ms locally.The salt is stored with each ciphertext, so decrypting the same stored value again always derives exactly the same key. Google Drive does that constantly: every Drive storage access decrypts the integration's config, including each source-commit page and each
/api/storage/objectrequest the media server makes for a segment. A 120 s Drive-backed recording repeats the same derivation ~130 times.What changes
decryptgets its key from a small cache keyed byencryption key + salt. It's least-recently-used, holds 256 entries, and stores the non-extractableCryptoKeypromise, so concurrent decrypts of the same value share one derivation.encryptis unchanged. It still makes a fresh random salt and derives a new key every time, so nothing about stored ciphertexts changes.Tests (new
database-crypto.test.ts)crypto.subtle.deriveKeyonce. On main it's called four times, and the test fails there.The full web unit suite has 4,255 passing tests. The 15 failures are in
add-folder-videosandshare-stage-identity, which fail the same way on main and don't touch encryption.

Confidence Score: 5/5
The PR appears safe to merge, with the previous test gap fixed and no new blocking issues found.Summary
Reuses derived decryption keys in a bounded cache without changing stored ciphertexts.
Reviews (3) · Last reviewed commit: "test(database): check that concurrent de..." · Reviewed by Greptile