Fix batched encrypted event records reuse per-record randomization metadata - #7124
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 946e53d The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The regression test is well-targeted, but the PR title says "Fix … reuse …" while the actual implementation fix is not present. Merging now would land a failing test on main and leave the AES-GCM IV-reuse vulnerability unfixed.
Reviewed changes
This run reviewed the single commit 8a674cc7a80b2c9e5dadd581b439113f5e07f040, which adds a focused reproduction test in packages/effect/test/unstable/eventlog/EventLog.test.ts that intercepts subtle.encrypt calls and asserts each batched entry receives a distinct AES-GCM IV. I ran the focused reproduction command and confirmed it fails against the current EventLogEncryption implementation as expected.
⚠️ Implementation fix is missing
EventLogEncryption.encrypt still generates one IV per batch and reuses it for every entry in packages/effect/src/unstable/eventlog/EventLogEncryption.ts. The new test correctly fails against this code. Before merging, the PR needs the actual fix plus the changes that ripple from it:
- Generate a unique IV per entry inside
makeEncryptionSubtle.encrypt. - Change the service return type from
{ iv, encryptedEntries }to a per-entry shape, e.g.ReadonlyArray<{ readonly iv: Uint8Array; readonly encryptedEntry: Uint8Array }>. - Update callers/consumers:
EventLogRemote.makeEncrypted,EventLogServerEncrypted, andSqlEventLogServerEncrypted. - Update the wire schemas (
WriteEntries,EncryptedEntry,EncryptedRemoteEntry) so each entry carries its own IV. - Update the existing
"encrypts and decrypts entries"test and any other code that destructures the old batch-leveliv. - Add a changeset, because the public
EventLogEncryptionshape and protocol message schemas change.
Technical details
# Missing fix and ripple scope
## Affected sites
- `packages/effect/src/unstable/eventlog/EventLogEncryption.ts:104-123` — still reuses one `iv` for the whole batch.
- `packages/effect/src/unstable/eventlog/EventLogMessage.ts:270-275` — `WriteEntries` carries a single top-level `iv`.
- `packages/effect/src/unstable/eventlog/EventLogRemote.ts:308-321` — `makeEncrypted` maps the single `iv` to all entries.
- `packages/effect/src/unstable/eventlog/EventLogServerEncrypted.ts:69-76` — `onWrite` stores `request.iv` for every persisted entry.
- `packages/effect/src/unstable/eventlog/SqlEventLogServerEncrypted.ts:220-244` — row encoding/decoding assumes one `iv` per entry already persisted in `entry.iv`.
## Required outcome
- `EventLogEncryption.encrypt` returns per-entry IVs.
- The wire protocol and storage layers preserve one IV per encrypted entry.
- All existing tests still pass after the API change.
- A changeset documents the breaking API/protocol change.
## Suggested approach
Return `ReadonlyArray<{ readonly iv: Uint8Array; readonly encryptedEntry: Uint8Array }>` from `encrypt`, then move `iv` into `EncryptedEntry` and `EncryptedRemoteEntry` so the protocol message schemas and SQL row shapes stay consistent.ℹ️ Nitpicks
- Consider marking the PR as draft while the fix is pending, or update the title to reflect that this is the reproduction-only stage.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
This run reviewed the new commit 20d5d8a42 since the prior Pullfrog review at 8a674cc7a. It adds the missing implementation fix for the AES-GCM IV-reuse issue and updates the ripple sites.
- Fixed per-entry AES-GCM IV generation:
makeEncryptionSubtle.encryptnow generates a fresh 12-byte IV inside the per-entrymapcallback and returnsReadonlyArray<{ iv, encryptedEntry }>. - Updated wire/storage schemas:
WriteEntriesno longer carries a top-leveliv;EncryptedEntrynow includes the per-entry IV, matching the existing per-entryivinEncryptedRemoteEntryandPersistedEntry. - Updated callers/consumers:
EventLogRemote.makeEncrypted,EventLogServerEncrypted.onWrite, andSqlEventLogServerEncrypted(already row-per-entry) use the per-entry IV. - Updated tests and added a changeset: existing tests destructure the new return shape; the new regression test asserts distinct IVs per batch entry;
.changeset/wise-bats-encrypt.mddocuments the patch.
Validation run: focused regression test, full EventLog.test.ts, SqlEventLogServerEncrypted.test.ts, pnpm lint, and pnpm check all pass.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the new commit 71086fc since the prior Pullfrog review at 20d5d8a4. It addresses the remaining style feedback from the previous review.
- Avoided
async/awaitin Effect code: the per-entry encryption callback inEventLogEncryption.makeEncryptionSubtlenow usescrypto.subtle.encrypt(...).then(...)instead of anasyncfunction, matching repository conventions. - Security semantics unchanged: a fresh 12-byte IV is still generated inside each
mapcallback, so every encrypted event-log entry receives a distinct AES-GCM nonce. - Validation: the focused regression test, full
EventLog.test.ts,SqlEventLogServerEncrypted.test.ts,pnpm lint, andpnpm checkall pass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the new commit 946e53d ("Strengthen event log IV regression coverage") since the prior Pullfrog review at 71086fc.
- Replaced the Proxy-based distinct-IV test with a stronger merged round-trip test in
EventLog.test.ts: it encrypts two entries, assertsassert.notDeepEqual(encrypted[0].iv, encrypted[1].iv), and completes a fullencrypt→decryptround-trip asserting the recovered entry ids match. The assertion genuinely fails if a shared per-batch IV were reintroduced, so it remains a real regression guard. - Tightened the
EncryptedEntryJSDoc to mention the per-entry initialization vector. - Expanded the changeset to note that encrypted clients and servers must be upgraded together because the
WriteEntrieswire shape changed. - Verified all callers of
encryptconsume the per-entry{ iv, encryptedEntry }shape with no stale references to the old batch-leveliv/encryptedEntries.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
2 participants
Summary
encrypt generates one 12-byte IV per batch and passes that same IV and key to every subtle.encrypt call in the batch. EventLogRemote and encrypted servers carry/copy that one IV to every entry.
Important
This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.
Batched encrypted event records reuse per-record randomization metadata
Module:
packages/effect/src/unstable/eventlog/EventLogEncryption.tsAudit ID:
relsem-eventlog-record-randomizationSeverity / confidence: high / high
What happens
encrypt generates one 12-byte IV per batch and passes that same IV and key to every subtle.encrypt call in the batch. EventLogRemote and encrypted servers carry/copy that one IV to every entry.
Why it happens
The wire/API shape returns one iv next to encryptedEntries, and encryption maps all encoded entries while closing over the single iv instead of generating a unique nonce per ciphertext.
Expected behavior
EventLogEncryption is the confidentiality/integrity service for encrypted event-log replication and uses AES-GCM under an identity-derived key.
Relevant implementation
These links and excerpts are pinned to audit base
b206fa5d7655c1634c9993410a9203f6616a5ca2.packages/effect/src/unstable/eventlog/EventLogEncryption.ts:1View problematic code at
packages/effect/src/unstable/eventlog/EventLogEncryption.ts:1View exact lines on GitHub
Reproduction
Observed failure: Independently rerun; failed at the intended semantic assertion.
Implementation handoff
The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.
Audit provenance
b206fa5d7655c1634c9993410a9203f6616a5ca2b206fa5d7655c1634c9993410a9203f6616a5ca2relsem-eventlog-record-randomizationCloses EFF-559