feat(roomcrypto): direct AES-256-GCM room encryption; add client decoder#207
Merged
Conversation
Benchmark the current ECIES-style Encode path (P-256 ephemeral keygen + ECDH per message) and document a replacement scheme. - pkg/roomcrypto/bench_test.go: benchmarks for the full Encode path, the ECDH and ephemeral-keygen steps in isolation, X25519 reference, and the proposed symmetric-only hot path. - docs/superpowers/specs/2026-05-20-ecdh-performance-analysis-design.md: threat-model recap, forward-secrecy analysis (per-message FS is illusory under Valkey compromise), perf measurements, alternatives, and a design spec for a versioned symmetric AES key derived from the room private key via HKDF. ~436x faster per Encode in dev container. No production code changes in this commit.
Per follow-up, drop dual-scheme migration machinery entirely. Server and
chat-frontend ship the new HKDF-only versioned-symmetric-key scheme
together in a single PR; legacy ECIES code path is removed.
- Wire format collapses to {version, nonce, ciphertext}; the Scheme field
and EphemeralPublicKey field are removed.
- chat-frontend section added: new lib/roomcrypto module, new
subscribeToRoomKeyEvents API op, new RoomKeysContext, decryption in
the room-events dispatcher (decrypt before the reducer sees the event).
- New room-service RPC chat.user.{account}.request.rooms.keys for the
bootstrap path on (re)connect.
- Migration plan collapses to one deploy. Swift client is explicitly
out-of-scope and tracked separately.
No production code changes in this commit.
28 TDD tasks across seven phases: - Phase 1 (Tasks 1-9): pkg/roomcrypto rewrite + broadcast-worker migration - Phase 2 (Tasks 10-13): room-service keys-bootstrap RPC - Phase 3 (Tasks 14-17): chat-frontend lib/roomcrypto + cross-language fixture - Phase 4 (Tasks 18-21): chat-frontend api ops (subjects, types, subscribe, fetch) - Phase 5 (Tasks 22-24): chat-frontend RoomKeysContext - Phase 6 (Tasks 25-26): chat-frontend RoomEvents integration - Phase 7 (Tasks 27-28): docs/client-api.md + verification gate Each task is bite-sized with red/green/refactor/commit cadence and exact code blocks; the implementer should be able to follow without reading the source first.
…ate key length up-front Addresses Task 1 code review findings: - Encoder.Encode now wraps aeadFor errors with context per project rule. - evictLowestVersionLocked has an explicit policy + precondition comment. - aeadFor validates key length before the cache lookup so an invalid-length key is rejected even on a cache hit.
…CHE_SIZE, log cache size at startup Aligns with adjacent ROOM_META_CACHE_SIZE / USER_CACHE_SIZE naming convention and the existing pattern of logging each cache's size at startup. Addresses code review feedback on Task 7. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
… check, prune legacy benchmarks Task 8 removed the free roomcrypto.Encode function. Two follow-ups: - pkg/roomkeystore/keygen_test.go: the TestGenerateKeyPair_RoundTripWithRoomcrypto test exercised legacy ECIES encrypt+decrypt to verify pub/priv halves matched. Replace it with TestGenerateKeyPair_HalvesMatch, which derives the public key from the private scalar via crypto/ecdh and compares directly — a stronger and more focused check, and one that doesn't couple roomkeystore tests to roomcrypto. - pkg/roomcrypto/bench_test.go: drop BenchmarkEncode, BenchmarkEphemeralKeyGen, BenchmarkECDH, BenchmarkParsePub. They profiled the deleted legacy code. Keep BenchmarkSymmetricOnly, BenchmarkX25519* (reference comparisons used in the design spec) and BenchmarkEncoder_Encode (the new hot path). https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
Implements decryptRoomMessage(ciphertext, nonce, aesKey) that decrypts
server-produced {nonce, ciphertext} pairs using the AES-256-GCM key
from deriveAesKey. Uses .buffer.slice() workaround for BufferSource
compatibility. Exports from the roomcrypto barrel index.
https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
Adds RoomKeysProvider context that fetches the full key snapshot on mount via fetchRoomKeysBootstrap, subscribes to live key rotation events via subscribeToRoomKeyEvents, and exposes hasKey/decrypt to consumers. Fixes useNats() cast (same pattern as RoomEventsContext) so TypeScript sees the Nats interface from a .jsx context file. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
Plumbs the `decrypt` function from RoomKeysContext into useRoomSubscriptions so incoming encrypted events are decrypted before being dispatched to the reducer. Adds decryptAndDispatch helper covering encryptedMessage (new_message) and messageEdited.encryptedNewContent (edit) paths for both DM and channel subscriptions. When no key is available, decrypt returns null and the event passes through unchanged for the reducer's placeholder branch to handle. Also fixes MainApp.integration.test to mock @/context/RoomKeysContext so RoomEventsProvider doesn't throw when rendered without a real RoomKeysProvider. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
Final-review follow-ups:
- broadcast-worker/integration_test.go: thread roomcrypto.NewEncoder()
through NewHandler call sites (Task 7's signature change).
- pkg/roomkeysender/integration_test.go: rewrite the round-trip using
the new Encoder + inline HKDF/AES-GCM decrypt (Task 8 removed the
free roomcrypto.Encode function).
- room-service/handler.go: tighten natsListRoomKeys subject validation
to require the full chat.user.{account}.request.rooms.keys pattern.
https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates chat encryption from P-256 ECDH ephemeral keys to direct 32-byte symmetric AES-256-GCM keys, removing HKDF derivation and public-key transmission. Server-side encoding gains a cached ChangesSymmetric Encryption Redesign
Frontend Decryption and Key Management
Documentation and Specifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chat-frontend/src/api/_transport/subjects.test.js (1)
1-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMigrate this test off
.jsor align it with the js/jsx testing policy.This file is a
chat-frontend/src/**/*.test.jstest but does not use@testing-library/reactper repository test policy. For this non-React unit test, the lowest-friction fix is renaming it tosubjects.test.ts(same content), which keeps it typed and out of the js/jsx-specific rule.Proposed minimal fix
- chat-frontend/src/api/_transport/subjects.test.js + chat-frontend/src/api/_transport/subjects.test.tsAs per coding guidelines,
chat-frontend/src/**/*.test.{js,jsx}: "Test files must use vitest with jsdom environment and@testing-library/react— test files live next to source".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chat-frontend/src/api/_transport/subjects.test.js` around lines 1 - 153, This test file is a plain JS unit test that violates the js/jsx testing policy; rename chat-frontend/src/api/_transport/subjects.test.js to subjects.test.ts and keep the same test content (no code changes required) so it becomes a TypeScript test file; ensure the import line that pulls in the helpers (userRoomEvent, userRoomKey, roomEvent, memberAdd, ...) remains unchanged and that your Vitest/tsconfig test globs include .test.ts so the tests run under the non-React unit-test policy.
🧹 Nitpick comments (1)
pkg/subject/subject_test.go (1)
615-621: ⚡ Quick winAdd invalid-account test coverage for
RoomsKeysBootstrap.Please add negative cases (
*,>, empty) so this subject builder’s guard behavior is locked in, consistent with other account-token builders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/subject/subject_test.go` around lines 615 - 621, Add negative test coverage for RoomsKeysBootstrap by creating a test (e.g., TestRoomsKeysBootstrapInvalid) that verifies the builder rejects invalid account inputs "*" , ">" and "" in the same way other account-token builders do; call subject.RoomsKeysBootstrap with each invalid value and assert the expected failure mode used across the package (panic or error/empty result) so the guard behavior matches other builders and RoomsKeysBootstrapWildcard remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@chat-frontend/src/api/fetchRoomKeysBootstrap/index.ts`:
- Around line 6-10: The API op fetchRoomKeysBootstrap currently uses the
nonstandard signature and omits opts; change its signature to the canonical
three-argument form fetchRoomKeysBootstrap(nats, args, opts?) where nats is
Pick<Nats,'request'|'user'> and args contains the user (or account) payload,
then call request<RoomKeysResponse>(roomsKeysBootstrap(args.user.account), {},
opts) (i.e. forward the opts parameter as the third argument to request) so the
operation follows the repo contract and always passes opts through to the NATS
primitive.
In `@chat-frontend/src/api/subscribeToRoomKeyEvents/index.ts`:
- Around line 5-10: Change subscribeToRoomKeyEvents to follow the standard
operation signature operationName(nats, args, opts?) by accepting (nats: Nats,
args: { user: Nats['user'] }, opts?: SubscribeOptions) and forward opts to the
NATS primitive; specifically, update the function signature for
subscribeToRoomKeyEvents, extract subscribe and user from the nats arg (or use
args.user), call subscribe(userRoomKey(user.account), callback, opts) so the
third-argument shape is preserved, and keep the return type as NatsSubscription.
In `@chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js`:
- Around line 224-266: The decryptAndDispatch flow currently can throw during
decryptRef.current or JSON.parse and skip calling finalize, dropping events;
wrap each decryption/parse block in try/catch around decryptRef.current and
JSON.parse (for the full-message and messageEdited paths) and on any error call
finalize(evt) (i.e., the original unmodified event) and return, otherwise
proceed to set decoded and call finalize(decoded); apply the same pattern to the
other decryption locations referenced (the other blocks using decryptRef.current
and JSON.parse) so finalize is always invoked even if decryption or parsing
fails.
In `@chat-frontend/src/context/RoomKeysContext/RoomKeysContext.test.tsx`:
- Line 1: The test file RoomKeysContext.test.tsx violates the repo policy
requiring frontend tests to be .test.jsx; rename the file to
RoomKeysContext.test.jsx and remove any TypeScript-only syntax (type
annotations, interfaces, or .tsx-specific JSX typings) so it is valid JS/JSX,
adjust the import line (e.g., remove or change TS-specific imports like
type-only imports) and ensure the test uses plain JS (vitest imports remain),
then run the test suite to confirm no TS-only constructs remain in functions or
exports referenced by RoomKeysContext tests.
In `@chat-frontend/src/context/RoomKeysContext/RoomKeysContext.tsx`:
- Around line 78-86: The subscription callback for subscribeToRoomKeyEvents can
throw when b64decode(evt.privateKey) receives malformed input; wrap the decode
and subsequent dispatch in a try/catch inside the callback (the function
handling RoomKeyEvent), catch any decode/parsing errors, log or drop the bad
event (using the existing logger or console.error) and do not call
dispatch('KEY_RECEIVED') for that event; keep validation for evt.roomId and
evt.version as-is and ensure the catch only covers b64decode(...) and dispatch
to avoid crashing the subscription stream.
- Around line 78-87: The KEY_RECEIVED handler (`subscribeToRoomKeyEvents`
callback / reducer branch that processes `KEY_RECEIVED`) must evict any cached
derived AES CryptoKey for the exact (roomId, version) when the stored raw key
bytes are replaced; locate where you call `dispatch({ type: 'KEY_RECEIVED',
roomId, version, privateKey: b64decode(...) })` and ensure the code that updates
`aesKeyCacheRef` removes the entry for that roomId/version (e.g. delete or set
to undefined in `aesKeyCacheRef.current[roomId][version]`) whenever a different
`privateKey` is written, so stale `CryptoKey` objects are not reused; apply the
same eviction in the other identical handler referenced (lines ~104-113) so both
subscription paths clear per-version cache on key replacement.
In `@docs/superpowers/plans/2026-05-20-ecdh-performance-analysis.md`:
- Line 28: The env var name is inconsistent: update the parsing in
broadcast-worker/main.go (the line that currently references
ROOMCRYPTO_CACHE_SIZE) to use ROOM_CRYPTO_CACHE_SIZE to match the spec and the
config struct field RoomCryptoCacheSize, or alternatively add an explicit env
tag mapping for RoomCryptoCacheSize; ensure the parser and the config field
(RoomCryptoCacheSize) both use the same uppercase_with_underscores name so env
parsing is consistent.
In `@pkg/subject/subject.go`:
- Around line 186-191: RoomsKeysBootstrap currently builds a subject without
validating the account token; update RoomsKeysBootstrap to call the shared
validator isValidAccountToken(account) and return an error-safe or guarded value
(mirror other builders' behavior) when the token is invalid (i.e., reject "*"
and ">"); specifically, add the same guard logic used elsewhere in pkg/subject
to check account before calling fmt.Sprintf in RoomsKeysBootstrap so
wildcard-bearing tokens are blocked consistently.
In `@room-service/handler.go`:
- Around line 972-973: The reply currently echoes the raw NATS subject via
natsutil.ReplyError(m.Msg, fmt.Sprintf("invalid subject: %s", m.Msg.Subject));
change this to send a generic client-safe message (e.g., "invalid subject") and
move the detailed subject information into server-side logs instead (use the
existing logger or log.Printf to log m.Msg.Subject and context). Update the code
path that handles subject validation (where natsutil.ReplyError is called) to
log the raw subject and return the sanitized message to the client.
- Around line 1004-1005: handleListRoomKeys currently passes h.keyStore into
chunkedGetKeys without nil-checking, which can panic in the NATS handler; add a
guard at the start of handleListRoomKeys to verify h.keyStore != nil and handle
the missing keystore by returning a controlled error response (or logging and
early return) instead of calling chunkedGetKeys, and update any error path to
use the same NATS response pattern used elsewhere in this handler so callers
receive a proper error rather than causing a panic.
In `@room-service/store_mongo.go`:
- Around line 126-138: The ListSubscriptionsByAccount query filters on
"u.account" and "isSubscribed" but no index exists; update the store startup
index creation (EnsureIndexes or MongoStore constructor) to create a compound
index on the subscriptions collection for the keys {"u.account": 1,
"isSubscribed": 1} (non-unique) so ListSubscriptionsByAccount uses the index
during boot/reconnect; add the index creation call alongside the other indexes
and handle/report any error from EnsureIndexes.
---
Outside diff comments:
In `@chat-frontend/src/api/_transport/subjects.test.js`:
- Around line 1-153: This test file is a plain JS unit test that violates the
js/jsx testing policy; rename chat-frontend/src/api/_transport/subjects.test.js
to subjects.test.ts and keep the same test content (no code changes required) so
it becomes a TypeScript test file; ensure the import line that pulls in the
helpers (userRoomEvent, userRoomKey, roomEvent, memberAdd, ...) remains
unchanged and that your Vitest/tsconfig test globs include .test.ts so the tests
run under the non-React unit-test policy.
---
Nitpick comments:
In `@pkg/subject/subject_test.go`:
- Around line 615-621: Add negative test coverage for RoomsKeysBootstrap by
creating a test (e.g., TestRoomsKeysBootstrapInvalid) that verifies the builder
rejects invalid account inputs "*" , ">" and "" in the same way other
account-token builders do; call subject.RoomsKeysBootstrap with each invalid
value and assert the expected failure mode used across the package (panic or
error/empty result) so the guard behavior matches other builders and
RoomsKeysBootstrapWildcard remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5199829-66ec-44f5-b9d1-ccb68776ecc3
📒 Files selected for processing (48)
broadcast-worker/handler.gobroadcast-worker/handler_test.gobroadcast-worker/integration_test.gobroadcast-worker/main.gobroadcast-worker/testhelpers_test.gochat-frontend/scripts/gen-crypto-fixtures.gochat-frontend/src/App.jsxchat-frontend/src/api/_transport/subjects.test.jschat-frontend/src/api/_transport/subjects.tschat-frontend/src/api/fetchRoomKeysBootstrap/index.test.tschat-frontend/src/api/fetchRoomKeysBootstrap/index.tschat-frontend/src/api/index.tschat-frontend/src/api/subscribeToRoomKeyEvents/index.test.tschat-frontend/src/api/subscribeToRoomKeyEvents/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/MainApp.integration.test.jsxchat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsxchat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsxchat-frontend/src/context/RoomEventsContext/reducer.test.jschat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.jschat-frontend/src/context/RoomKeysContext/RoomKeysContext.test.tsxchat-frontend/src/context/RoomKeysContext/RoomKeysContext.tsxchat-frontend/src/context/RoomKeysContext/index.tsxchat-frontend/src/context/RoomKeysContext/reducer.test.tschat-frontend/src/context/RoomKeysContext/reducer.tschat-frontend/src/lib/roomcrypto/index.tschat-frontend/src/lib/roomcrypto/roomcrypto.test.tschat-frontend/src/lib/roomcrypto/roomcrypto.tschat-frontend/test/fixtures/encrypted-message.jsondocs/client-api.mddocs/superpowers/plans/2026-05-20-ecdh-performance-analysis.mddocs/superpowers/specs/2026-05-20-ecdh-performance-analysis-design.mdpkg/model/event.gopkg/model/model_test.gopkg/roomcrypto/bench_test.gopkg/roomcrypto/integration_test.gopkg/roomcrypto/roomcrypto.gopkg/roomcrypto/roomcrypto_test.gopkg/roomcrypto/testdata/decrypt.tspkg/roomkeysender/integration_test.gopkg/roomkeystore/keygen_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.go
Comment on lines
+6
to
+10
| export function fetchRoomKeysBootstrap( | ||
| { request, user }: Pick<Nats, 'request' | 'user'>, | ||
| ): Promise<RoomKeysResponse> { | ||
| return request<RoomKeysResponse>(roomsKeysBootstrap(user.account), {}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align this API op to the required (nats, args, opts?) contract and 3-arg request shape.
This operation currently bypasses the standard API signature and does not pass opts through to request(...). Please switch to the repo’s canonical API-op shape and forward opts to the NATS call (even when undefined) for consistency with the operation tests/contracts.
As per coding guidelines: “API operations must have … operationName(nats, args, opts?)” and “Always pass opts parameter through to NATS primitives … use three-argument shape.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chat-frontend/src/api/fetchRoomKeysBootstrap/index.ts` around lines 6 - 10,
The API op fetchRoomKeysBootstrap currently uses the nonstandard signature and
omits opts; change its signature to the canonical three-argument form
fetchRoomKeysBootstrap(nats, args, opts?) where nats is
Pick<Nats,'request'|'user'> and args contains the user (or account) payload,
then call request<RoomKeysResponse>(roomsKeysBootstrap(args.user.account), {},
opts) (i.e. forward the opts parameter as the third argument to request) so the
operation follows the repo contract and always passes opts through to the NATS
primitive.
Comment on lines
+5
to
+10
| export function subscribeToRoomKeyEvents( | ||
| { subscribe, user }: Pick<Nats, 'subscribe' | 'user'>, | ||
| callback: SubscriptionCallback, | ||
| ): NatsSubscription { | ||
| return subscribe(userRoomKey(user.account), callback) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the standard API operation signature and pass opts to subscribe.
Please update this op to match the required operationName(nats, args, opts?) signature and forward opts to the NATS primitive for consistency across src/api/*/index.ts.
As per coding guidelines: “API operations must have … operationName(nats, args, opts?)” and “Always pass opts parameter through to NATS primitives … use three-argument shape.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chat-frontend/src/api/subscribeToRoomKeyEvents/index.ts` around lines 5 - 10,
Change subscribeToRoomKeyEvents to follow the standard operation signature
operationName(nats, args, opts?) by accepting (nats: Nats, args: { user:
Nats['user'] }, opts?: SubscribeOptions) and forward opts to the NATS primitive;
specifically, update the function signature for subscribeToRoomKeyEvents,
extract subscribe and user from the nats arg (or use args.user), call
subscribe(userRoomKey(user.account), callback, opts) so the third-argument shape
is preserved, and keep the return type as NatsSubscription.
CI was failing on test-integration (pkg/roomkeysender) because the TS client at testdata/client.ts still did legacy ECIES decryption against ephemeralPublicKey on the wire. The Go-side encoder switched to HKDF-only in Task 9 (commit c71b480) but this sibling decoder was missed. Rewrite decryptMessage to take only the room private key, derive the AES key via HKDF-SHA-256(roomPriv, info="room-message-encryption-v2"), and AES-GCM-decrypt. Drop ephemeralPublicKey from the EncryptedMessage type and publicKey from the local RoomKeyEvent type. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
- useRoomSubscriptions: wrap decryptAndDispatch body in try/catch so a thrown decrypt or JSON.parse no longer drops the event silently. - RoomKeysContext: guard b64decode in subscription + bootstrap paths; evict aesKeyCacheRef entry on key replacement so a stale CryptoKey isn't reused with new key bytes. - pkg/subject.RoomsKeysBootstrap: validate account token via existing isValidAccountToken guard. - room-service: sanitize invalid-subject error reply, log details only; add compound mongo index for the new ListSubscriptionsByAccount query path. - chat-frontend: rename RoomKeysContext.test.tsx -> .test.jsx per incremental-migration convention; strip TS-only syntax. - docs: align ROOM_CRYPTO_CACHE_SIZE naming in the plan doc. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
…tion Two follow-ups from a /simplify review pass: - room-service/store_mongo.go: the new (u.account, isSubscribed) compound index serves a single query that filters isSubscribed=true. Add a partial filter expression so the index only carries active rows and doesn't bloat as unsubscribed entries accumulate. - chat-frontend/RoomKeysContext: the live key-event callback used to unconditionally evict the cached AES CryptoKey before dispatching. When the server rebroadcasts an unchanged key (e.g., on reconnect), the reducer no-ops via bytesEqual, but the eviction had already thrown away the derived key — forcing a redundant crypto.subtle deriveKey call on the next decrypt. Guard the eviction with the same bytesEqual check. Export bytesEqual from reducer.ts so both sites share the comparison.
Three small compactions, no behavior change: - pkg/roomcrypto/testdata/decrypt.ts: replace manual HMAC-based HKDF implementation with node:crypto's built-in hkdfSync. ~25 lines off. - pkg/roomcrypto/bench_test.go: drop BenchmarkSymmetricOnly (now subsumed by BenchmarkEncoder_Encode) and the X25519 reference benchmarks (their results are already captured in the design doc as rationale for rejecting curve-switching). Keep only the canonical hot-path benchmark. ~80 lines off. - chat-frontend lib/roomcrypto.test.ts: drop the two inline-encrypt round-trip tests; the committed cross-language fixture exercises the same chain more authoritatively against real Go-emitted ciphertext. Keep happy-path + error cases. ~50 lines off. Total reduction: ~155 lines of test/bench scaffolding.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/roomcrypto/testdata/decrypt.ts (1)
22-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the versioned envelope before decrypting.
This helper currently ignores
message.versionand lets malformednonce/ciphertextvalues fall through to generic GCM failures. Since it is the cross-language fixture validator, explicit contract checks here make regressions much easier to diagnose.Suggested guard rails
const p = JSON.parse(raw) as Payload const privateKey = Buffer.from(p.privateKey, 'base64') if (privateKey.length !== 32) throw new Error(`expected 32-byte private key, got ${privateKey.length}`) + if (p.message.version !== 2) throw new Error(`unsupported message version ${p.message.version}`) const aesKey = Buffer.from(hkdfSync('sha256', privateKey, Buffer.alloc(0), 'room-message-encryption-v2', 32)) const nonce = Buffer.from(p.message.nonce, 'base64') const ciphertext = Buffer.from(p.message.ciphertext, 'base64') + if (nonce.length !== 12) throw new Error(`expected 12-byte nonce, got ${nonce.length}`) + if (ciphertext.length < 16) throw new Error('ciphertext must include a 16-byte GCM tag')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/roomcrypto/testdata/decrypt.ts` around lines 22 - 32, Validate the versioned envelope before decrypting by checking p.message.version matches the expected version (e.g., 2 or "v2" consistent with your "room-message-encryption-v2" label) and fail fast if it doesn't; also assert decoded nonce and ciphertext lengths (nonce must be 12 bytes for AES-GCM and ciphertext must be at least 16 bytes to contain the auth tag) before slicing into tag/body. Add these checks near where Payload is parsed (around variables privateKey, aesKey, nonce, ciphertext) so you return/throw a clear error on invalid version or malformed inputs rather than letting hkdfSync/createDecipheriv produce generic GCM errors.pkg/roomkeysender/testdata/client.ts (1)
70-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope cached keys to the target room and wait for the matching version.
keySubjectis account-scoped, but this client caches keys only byversionand hard-fails if the message loop beats the background key consumer. That makes the test client incorrect once two rooms share a version number, and flaky whenever the encrypted message is processed before the corresponding key event is stored.💡 One way to make the client deterministic
// Store received keys indexed by version number. const keys = new Map<number, string>(); + const pending = new Map<number, (key: string) => void>(); const nc: NatsConnection = await connect({ servers: natsURL }); @@ (async () => { for await (const msg of keySub) { const evt: RoomKeyEvent = JSON.parse(new TextDecoder().decode(msg.data)); - keys.set(evt.version, evt.privateKey); + if (evt.roomId !== roomID) { + continue; + } + keys.set(evt.version, evt.privateKey); + pending.get(evt.version)?.(evt.privateKey); + pending.delete(evt.version); } })(); @@ const version = parseInt(versionStr, 10); - const privateKey = keys.get(version); - if (!privateKey) { - process.stderr.write(`no key found for version ${version}\n`); - process.exit(1); - } + const privateKey = + keys.get(version) ?? + (await new Promise<string>((resolve) => { + pending.set(version, resolve); + }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/roomkeysender/testdata/client.ts` around lines 70 - 108, The client currently caches keys in keys: Map<number,string> and indexes only by version, causing collisions across rooms and race failures when the message loop outruns the key consumer; change the key cache to include room scope (e.g., use a composite key like `${roomID}:${evt.version}`) and update all lookups (where version is read from X-Room-Key-Version) to use that composite key; additionally, make the message processing wait for the matching scoped key instead of hard-failing (e.g., poll/wait with a short timeout or await a promise resolved by the background key consumer) so decryptMessage is only called once the correct room-scoped privateKey is available; adjust references to keySubject, keySub, keys, and decryptMessage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/roomcrypto/testdata/decrypt.ts`:
- Around line 22-32: Validate the versioned envelope before decrypting by
checking p.message.version matches the expected version (e.g., 2 or "v2"
consistent with your "room-message-encryption-v2" label) and fail fast if it
doesn't; also assert decoded nonce and ciphertext lengths (nonce must be 12
bytes for AES-GCM and ciphertext must be at least 16 bytes to contain the auth
tag) before slicing into tag/body. Add these checks near where Payload is parsed
(around variables privateKey, aesKey, nonce, ciphertext) so you return/throw a
clear error on invalid version or malformed inputs rather than letting
hkdfSync/createDecipheriv produce generic GCM errors.
In `@pkg/roomkeysender/testdata/client.ts`:
- Around line 70-108: The client currently caches keys in keys:
Map<number,string> and indexes only by version, causing collisions across rooms
and race failures when the message loop outruns the key consumer; change the key
cache to include room scope (e.g., use a composite key like
`${roomID}:${evt.version}`) and update all lookups (where version is read from
X-Room-Key-Version) to use that composite key; additionally, make the message
processing wait for the matching scoped key instead of hard-failing (e.g.,
poll/wait with a short timeout or await a promise resolved by the background key
consumer) so decryptMessage is only called once the correct room-scoped
privateKey is available; adjust references to keySubject, keySub, keys, and
decryptMessage accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bada9d4-e474-4fd0-8e43-08be9e5f981d
📒 Files selected for processing (12)
chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.jschat-frontend/src/context/RoomKeysContext/RoomKeysContext.test.jsxchat-frontend/src/context/RoomKeysContext/RoomKeysContext.tsxchat-frontend/src/context/RoomKeysContext/reducer.tschat-frontend/src/lib/roomcrypto/roomcrypto.test.tsdocs/superpowers/plans/2026-05-20-ecdh-performance-analysis.mdpkg/roomcrypto/bench_test.gopkg/roomcrypto/testdata/decrypt.tspkg/roomkeysender/testdata/client.tspkg/subject/subject.goroom-service/handler.goroom-service/store_mongo.go
CodeRabbit flagged that the test decryptor lets malformed nonce/ciphertext fall through to generic AES-GCM errors. Cheap defensive checks make regressions easier to diagnose. Skipped the suggested "version === 2" guard — the version field is the room key version (0/1/7/...), not a scheme version. The "v2" suffix on the HKDF info string is a separate concept.
Was: NewHandler took *roomcrypto.Encoder as a parameter, threaded through 31 test call sites + main.go config. The "tunable cache size" env var was speculative — no realistic deployment needs to change it from the 4096 default (~128 KB of AEAD entries). Now: NewHandler constructs its own roomcrypto.NewEncoder() internally. Tests just call NewHandler(...) without the trailing argument. Drops: - encoder parameter from NewHandler signature - RoomCryptoCacheSize config field - ROOM_CRYPTO_CACHE_SIZE env var - startup log line - 31 trailing roomcrypto.NewEncoder() args in tests - roomcrypto imports in main.go and integration_test.go (no longer needed) Total: ~45 lines off the PR review surface. No behavior change.
…secret HKDF-only encryption never reads the public half of the room keypair — the AES key is derived directly from the 32-byte private scalar via HKDF. The public key was carried along only because the legacy ECIES scheme needed it and the storage layout still matched that shape. Cleanup: - RoomKeyEvent and RoomKeyPair drop the PublicKey field. - GenerateKeyPair() switches from ecdh.P256().GenerateKey() to crypto/rand.Read(32); P-256 group membership wasn't required for HKDF input keying material. - roomkeystore writers stop writing the "pub" Valkey field; readers stop reading it. Old rows still carry "pub" and are harmless — the field is ignored on read (forward-compat; no migration required). - Test helpers and tools/loadgen switch to crypto/rand for fixture keys. - docs/client-api.md §5.1 updated to describe the 32-byte secret instead of a P-256 keypair. crypto/ecdh is no longer imported anywhere in the Go tree. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
room-worker/handler.go (1)
291-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't reset the rotated key back to version
0in theErrNoCurrentKeyfallback.This path fans out
predictedVersion = currentPair.Version + 1first, but then persists the same secret withSet, which stamps version0. After that, survivors cache the key undervN+1while broadcast-worker readsv0, so the next encrypted message becomes undecryptable. PersistpredictedVersionhere as well, or delay fan-out until the stored version is known.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/handler.go` around lines 291 - 313, The bug is that when h.keyStore.Rotate returns ErrNoCurrentKey you call h.keyStore.Set(ctx, roomID, *newPair) which stores the key at version 0 while you already fan-out a VersionedKeyPair using predictedVersion (computed from currentPair.Version+1), causing a version mismatch; fix by persisting the same predictedVersion used in h.fanOutRoomKeyToSurvivors — e.g., instead of calling Set(ctx, roomID, *newPair) in the ErrNoCurrentKey branch, call the store API that accepts a versioned value (use roomkeystore.VersionedKeyPair{Version: predictedVersion, KeyPair: *newPair} or h.keyStore.SetVersioned/SetWithVersion if available) or modify h.keyStore.Set to accept a VersionedKeyPair so the stored version matches predictedVersion (alternatively, delay calling h.fanOutRoomKeyToSurvivors until after the key is successfully persisted).
♻️ Duplicate comments (1)
room-service/handler.go (1)
988-1005:⚠️ Potential issue | 🔴 CriticalGuard the optional keystore before bootstrapping room keys.
Handler.keyStoreis explicitly optional in this service, buthandleListRoomKeysstill passes it straight intochunkedGetKeys. When tests or a misconfigured instance register this endpoint withkeyStore == nil, the request path panics instead of returning a controlled error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-service/handler.go` around lines 988 - 1005, handleListRoomKeys calls chunkedGetKeys with the optional Handler.keyStore without checking for nil, which can panic; add a nil check for h.keyStore near the start of handleListRoomKeys (before calling chunkedGetKeys) and return a controlled error (e.g., fmt.Errorf("keystore not configured") or a wrapped error) so the handler returns an error instead of panicking; reference Handler.keyStore, handleListRoomKeys and chunkedGetKeys when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/client-api.md`:
- Line 1991: The documentation currently conflicts about DM/botDM key
availability; update the text around RoomKeyEvent and §5.2 to clarify that while
DM and botDM rooms receive a RoomKeyEvent at creation for consistency, that
event may contain only plaintext `message` with no `encryptedMessage` and
therefore may not carry persistent key material — clients must treat DM/botDM as
optional in key bootstrap and explicitly check the RoomKeyEvent payload
(presence of `encryptedMessage` or key fields) rather than assuming keys exist.
Reference RoomKeyEvent, DM, botDM, and the bootstrapping behavior described in
§5.2 when making this change.
In `@pkg/roomkeystore/roomkeystore.go`:
- Around line 226-231: In decodeKeyPair, ensure the "priv" entry exists and
decodes to exactly 32 bytes before returning a RoomKeyPair: check that
fields["priv"] is present (non-empty), decode it (as currently done), then
verify len(priv) == 32 and return an error if not; return a clear fmt.Errorf
message on missing or malformed private key so RoomKeyPair{PrivateKey: priv} is
only constructed for a valid 32-byte private key.
---
Outside diff comments:
In `@room-worker/handler.go`:
- Around line 291-313: The bug is that when h.keyStore.Rotate returns
ErrNoCurrentKey you call h.keyStore.Set(ctx, roomID, *newPair) which stores the
key at version 0 while you already fan-out a VersionedKeyPair using
predictedVersion (computed from currentPair.Version+1), causing a version
mismatch; fix by persisting the same predictedVersion used in
h.fanOutRoomKeyToSurvivors — e.g., instead of calling Set(ctx, roomID, *newPair)
in the ErrNoCurrentKey branch, call the store API that accepts a versioned value
(use roomkeystore.VersionedKeyPair{Version: predictedVersion, KeyPair: *newPair}
or h.keyStore.SetVersioned/SetWithVersion if available) or modify h.keyStore.Set
to accept a VersionedKeyPair so the stored version matches predictedVersion
(alternatively, delay calling h.fanOutRoomKeyToSurvivors until after the key is
successfully persisted).
---
Duplicate comments:
In `@room-service/handler.go`:
- Around line 988-1005: handleListRoomKeys calls chunkedGetKeys with the
optional Handler.keyStore without checking for nil, which can panic; add a nil
check for h.keyStore near the start of handleListRoomKeys (before calling
chunkedGetKeys) and return a controlled error (e.g., fmt.Errorf("keystore not
configured") or a wrapped error) so the handler returns an error instead of
panicking; reference Handler.keyStore, handleListRoomKeys and chunkedGetKeys
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e9c399e-b7d2-40a4-b2d0-fba4cfeaf2d2
📒 Files selected for processing (26)
broadcast-worker/handler.gobroadcast-worker/handler_test.gobroadcast-worker/testhelpers_test.godocs/client-api.mdpkg/model/event.gopkg/model/model_test.gopkg/roomcrypto/testdata/decrypt.tspkg/roomkeysender/integration_test.gopkg/roomkeysender/roomkeysender_test.gopkg/roomkeystore/adapter.gopkg/roomkeystore/doc.gopkg/roomkeystore/integration_test.gopkg/roomkeystore/keygen.gopkg/roomkeystore/keygen_test.gopkg/roomkeystore/roomkeystore.gopkg/roomkeystore/roomkeystore_test.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_publisher_test.gotools/loadgen/preset.gotools/loadgen/preset_test.gotools/loadgen/seed_test.go
💤 Files with no reviewable changes (4)
- room-worker/mock_publisher_test.go
- tools/loadgen/preset_test.go
- pkg/model/model_test.go
- pkg/roomkeystore/keygen_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/roomkeystore/doc.go
…validation
Three concrete bug fixes from CodeRabbit review:
1. room-worker/handler.go rotateAndFanOut: when keyStore.Rotate returns
ErrNoCurrentKey, the fallback used Set() which stamps v0 — but
fan-out had already committed survivors to predictedVersion =
currentPair.Version + 1, creating a version mismatch that would
render the next encrypted message undecryptable. Switch to
SetWithVersion(predictedVersion). Adds SetWithVersion to the
room-worker RoomKeyStore interface (already exists on the
pkg/roomkeystore implementation) and updates both mocks.
2. room-service/handler.go handleListRoomKeys: nil-check h.keyStore
before calling chunkedGetKeys. The keyStore field is documented as
optional (// tests may pass nil) and other handlers in the same
file (lines 353, 540) already guard before use. The new bootstrap
handler was inconsistent and would panic on a nil keystore.
3. pkg/roomkeystore decodeKeyPair: require the priv field to be
present and exactly 32 bytes before returning. Previously a
missing field decoded to []byte{} and produced a RoomKeyPair with
empty IKM, silently corrupting downstream HKDF derivations.
Returns a clear error on missing/malformed input.
Tests updated to use a 32-byte base64 priv where the legacy 3-byte
"AQID" placeholder was used only as a syntactically valid fixture.
CI failure on test-integration (room-worker): the prior commit (b87f84f) added a 32-byte length check in pkg/roomkeystore.decodeKeyPair to reject malformed Valkey rows. TestIntegration_CreateRoom_FansOutRoomKeyEvent was seeding the room key with a 17-byte placeholder ([]byte("private-key-bytes")), which the new validator correctly rejects on read. The test predates the length validation and was relying on the reader accepting whatever was written. Switch to a 32-byte secret so the fan-out path can decode the seeded key and the rest of the test runs as intended.
A second targeted bug-hunt found three real issues. All landed in the
new code introduced by this PR.
1. CRITICAL — room-service/store_mongo.go: ListSubscriptionsByAccount
filter `{u.account, isSubscribed: true}` excluded essentially every
real subscription. Subscription.IsSubscribed is set to true ONLY for
the human side of botDM rooms (see inbox-worker's
subscriptionIsSubscribed); channels, DMs, and the bot side leave it
false, and the bson `omitempty` tag means false rows don't even
persist the field. Result: the keys-bootstrap RPC returned 0 entries
for the common case, and every encrypted message rendered as the
[encrypted message] placeholder forever. Drop the isSubscribed
predicate; replace the partial-filter index with a plain
(u.account) index.
2. MAJOR — chat-frontend useRoomSubscriptions: events for the same
room could finalize out of order. decryptAndDispatch is async; an
encrypted new_message suspends on deriveAesKey + GCM.open, then a
plaintext mutation event (or another message in a different
key-cache state) finalizes synchronously and overtakes it. The
reducer's appendBounded is a pure append with no chronological
sort, so the message list ended up scrambled. Introduce a per-room
dispatch chain (Map<roomId, Promise>); every event for a room
chains off the prior one via .then(fn, fn) so order is preserved
even across the encrypted/plaintext divide.
3. MINOR — chat-frontend RoomKeysContext: when deriveAesKey rejected,
the rejected promise stayed cached in aesKeyCacheRef and every
subsequent decrypt for that (roomId, version) re-awaited it,
logged, and returned null. Recovery required a key replacement or
relogin. Clear the entry from the cache on catch — only when the
cached promise is still the one we awaited, to avoid evicting a
newer entry that arrived in the meantime.
…for rotateAndFanOut fallback
Closes the test-coverage gap that let two bugs reach PR review:
1. room-service/integration_test.go: real-Mongo test for
ListSubscriptionsByAccount. Seeds channel + DM + botDM (both
sides) subscriptions and asserts ALL are returned. The original
filter `{u.account, isSubscribed: true}` excluded everything
except the human side of a botDM; this test fails on that filter.
2. room-worker/handler_test.go: unit test for rotateAndFanOut's
ErrNoCurrentKey fallback. Mocks the keystore so Rotate returns
ErrNoCurrentKey and SetWithVersion expects the predicted version.
The earlier-reported version-mismatch bug (Set was called, stamps
v0, mismatches survivor's predictedVersion=N+1) is now pinned.
https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
The room secret is generated by crypto/rand and is exactly 32 bytes of
uniform random material — already a valid AES-256 key. HKDF here was a
no-op for confidentiality (the input is already uniform) and we don't
use the domain-separation property (no other keys are derived from the
room secret). Removing it shrinks the dep surface (drops
golang.org/x/crypto/hkdf), simplifies both the Go encoder and the Web
Crypto / Node decoders, and eliminates the misleading "deriveAesKey"
abstraction (nothing is being derived).
Trade-off: cheap scheme-versioning via the HKDF info string is gone. If
we ever switch AEAD (e.g. to ChaCha20-Poly1305) we'll need either a
wire-format scheme-version field or a re-keying migration. Both are
acceptable; we chose conceptual coherence over the unused versioning
hook.
- pkg/roomcrypto: Encoder.aeadFor uses roomPrivateKey directly via
aes.NewCipher. HKDF import dropped.
- pkg/roomcrypto/testdata/decrypt.ts: drop hkdfSync; use privateKey
directly as the AES key.
- chat-frontend lib/roomcrypto: rename deriveAesKey → importAesKey;
use crypto.subtle.importKey('raw', priv, AES-GCM, ...) directly.
- chat-frontend test fixture regenerated under the new scheme.
- pkg/roomkeysender testdata client.ts and broadcast-worker
testhelpers_test.go: same HKDF → direct-import change.
- docs/client-api.md §5.1: drop HKDF instructions.
https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js (1)
225-233: ⚡ Quick winPrune settled per-room chain entries to avoid session-long map growth.
dispatchChainskeeps one promise per room forever until full teardown. Deleting settled entries keeps memory bounded in long-lived sessions with many transient rooms.Proposed fix
const enqueueByRoom = (roomId, work) => { if (!roomId) { - work() - return + return Promise.resolve().then(work) } const prev = dispatchChains.get(roomId) ?? Promise.resolve() - const next = prev.then(work, work) + const next = prev + .then(work, work) + .finally(() => { + if (dispatchChains.get(roomId) === next) dispatchChains.delete(roomId) + }) dispatchChains.set(roomId, next) + return next }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js` around lines 225 - 233, dispatchChains holds a settled promise per room indefinitely; modify enqueueByRoom so that after creating `next` it registers a final cleanup to remove the map entry when the chain settles (e.g. attach a `.finally` or `.then(...).catch(...).finally(...)` that calls `dispatchChains.delete(roomId)`), ensuring you still set `dispatchChains.set(roomId, next)` before adding the cleanup and leaving behavior for the no-room path unchanged; reference `enqueueByRoom` and `dispatchChains` to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-05-20-ecdh-performance-analysis-design.md`:
- Around line 195-200: The spec is inconsistent: the implemented note says HKDF
was removed and that aesKey = roomPrivateKey (generated from crypto/rand) is
used directly, but later server/client algorithm sections still describe HKDF.
Update those algorithm descriptions to remove HKDF usage and instead state that
the 32-byte room secret (roomPrivateKey) is used directly as the AES-256-GCM key
(aesKey = roomPrivateKey), remove references to golang.org/x/crypto/hkdf and the
info string "room-message-encryption-v2", and ensure all examples and pseudocode
reflect the direct-key usage so the document consistently describes the same
encryption flow.
In `@room-service/integration_test.go`:
- Around line 1648-1660: Replace the loose membership check over wantRoomIDs and
Len==4 with an exact multiset equality assertion: collect the observed room IDs
from result (use the loop over result / sub.RoomID), build a map or sorted slice
of observed IDs, and assert that the observed set exactly equals the expected
set defined by wantRoomIDs (no duplicates, no missing entries). Use the same
symbols (result, sub.RoomID, wantRoomIDs) and keep the per-sub User.Account
assertion, but replace assert.True checks with a single
equality/assert.ElementsMatch (or equivalent) between observed IDs and the
expected slice to prevent duplicate-pass false positives.
---
Nitpick comments:
In `@chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js`:
- Around line 225-233: dispatchChains holds a settled promise per room
indefinitely; modify enqueueByRoom so that after creating `next` it registers a
final cleanup to remove the map entry when the chain settles (e.g. attach a
`.finally` or `.then(...).catch(...).finally(...)` that calls
`dispatchChains.delete(roomId)`), ensuring you still set
`dispatchChains.set(roomId, next)` before adding the cleanup and leaving
behavior for the no-room path unchanged; reference `enqueueByRoom` and
`dispatchChains` to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d0e6137-ce2b-4319-9417-29b1de14f25b
📒 Files selected for processing (32)
broadcast-worker/handler_test.gobroadcast-worker/testhelpers_test.gochat-frontend/scripts/gen-crypto-fixtures.gochat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.jschat-frontend/src/context/RoomKeysContext/RoomKeysContext.tsxchat-frontend/src/lib/roomcrypto/index.tschat-frontend/src/lib/roomcrypto/roomcrypto.test.tschat-frontend/src/lib/roomcrypto/roomcrypto.tschat-frontend/test/fixtures/encrypted-message.jsondocs/client-api.mddocs/superpowers/specs/2026-05-20-ecdh-performance-analysis-design.mdgo.modpkg/roomcrypto/bench_test.gopkg/roomcrypto/integration_test.gopkg/roomcrypto/roomcrypto.gopkg/roomcrypto/roomcrypto_test.gopkg/roomcrypto/testdata/decrypt.tspkg/roomkeysender/integration_test.gopkg/roomkeysender/testdata/client.tspkg/roomkeystore/keygen.gopkg/roomkeystore/roomkeystore.gopkg/roomkeystore/roomkeystore_test.goroom-service/handler.goroom-service/integration_test.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_publisher_test.goroom-worker/mock_store_test.goroom-worker/store.gotools/loadgen/preset.go
✅ Files skipped from review due to trivial changes (3)
- room-worker/mock_store_test.go
- chat-frontend/scripts/gen-crypto-fixtures.go
- chat-frontend/test/fixtures/encrypted-message.json
Comment on lines
+195
to
+200
| > **Implemented note (2026-05-21):** The HKDF step was subsequently removed. | ||
| > The 32-byte room secret is used directly as the AES-256-GCM key | ||
| > (`aesKey = roomPrivateKey`) — no key derivation. Since the secret is | ||
| > generated by `crypto/rand` it is already uniform random material; HKDF | ||
| > provided no confidentiality benefit. The `golang.org/x/crypto/hkdf` import | ||
| > is gone; the info string `"room-message-encryption-v2"` is retired. |
There was a problem hiding this comment.
Resolve internal algorithm mismatch across this spec.
This note says HKDF is removed, but later sections still define HKDF-based server/client algorithms, leaving the spec contradictory.
Proposed doc alignment
-### E. **HKDF-only versioned symmetric key** (recommended)
+### E. **Versioned symmetric key (implemented)** (recommended)-1. Look up (or compute and cache) `aesKey_v` for `(roomID, version)`. On
- cache miss:
- `aesKey_v = HKDF-SHA256(roomPrivateKey, salt=nil, info=[]byte("room-message-encryption-v2"))`,
+1. Look up (or compute and cache) `aesKey_v` for `(roomID, version)`. On
+ cache miss:
+ `aesKey_v = roomPrivateKey`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-20-ecdh-performance-analysis-design.md` around
lines 195 - 200, The spec is inconsistent: the implemented note says HKDF was
removed and that aesKey = roomPrivateKey (generated from crypto/rand) is used
directly, but later server/client algorithm sections still describe HKDF. Update
those algorithm descriptions to remove HKDF usage and instead state that the
32-byte room secret (roomPrivateKey) is used directly as the AES-256-GCM key
(aesKey = roomPrivateKey), remove references to golang.org/x/crypto/hkdf and the
info string "room-message-encryption-v2", and ensure all examples and pseudocode
reflect the direct-key usage so the document consistently describes the same
encryption flow.
…ce-analysis-diH42 # Conflicts: # pkg/model/event.go # pkg/model/model_test.go # pkg/roomkeystore/adapter.go # pkg/roomkeystore/integration_test.go # room-service/handler.go # room-service/handler_test.go # room-service/integration_test.go # room-service/store_mongo.go
hmchangw
changed the title
mliu33
reviewed
May 21, 2026
|
|
||
| Removed members keep prior keys for decrypting historical messages but cannot decrypt anything published after the rotation. | ||
|
|
||
| ### 5.2 Bootstrap room keys on (re)connect |
Collaborator
There was a problem hiding this comment.
No need to for this RPC. Client will use Get subscription list RPC which includes room key info. This subscription lists related RPC are not yet documented here.
| EphemeralPublicKey []byte `json:"ephemeralPublicKey"` // 65 bytes, uncompressed P-256 point | ||
| Nonce []byte `json:"nonce"` // 12 bytes, AES-GCM nonce | ||
| Ciphertext []byte `json:"ciphertext"` // encrypted content + 16-byte AES-GCM tag | ||
| EphemeralPublicKey []byte `json:"ephemeralPublicKey,omitempty"` // legacy scheme; empty on the new scheme |
Collaborator
There was a problem hiding this comment.
We don't need to care about backward compatibility. Better to remove legacy field ?
| hkdfReader := hkdf.New(sha256.New, sharedSecret, nil, []byte("room-message-encryption")) | ||
| if _, err := io.ReadFull(hkdfReader, aesKey); err != nil { | ||
| // Unreachable for SHA-256, but must be checked per project convention. | ||
| r := hkdf.New(sha256.New, roomPrivateKey, nil, []byte("room-message-encryption-v2")) |
Collaborator
There was a problem hiding this comment.
Can we define a constant for the info "room-message-encryption-v2" ?
| hkdfReader := hkdf.New(sha256.New, sharedSecret, nil, []byte("room-message-encryption")) | ||
| if _, err := io.ReadFull(hkdfReader, aesKey); err != nil { | ||
| // Unreachable for SHA-256, but must be checked per project convention. | ||
| r := hkdf.New(sha256.New, roomPrivateKey, nil, []byte("room-message-encryption-v2")) |
Collaborator
There was a problem hiding this comment.
I am wondering if we should just take 32bytes room private key as aes key and pass to aes.NewCipher. Not sure about the benefit and difference using HKDF.
I think that is a standard AES-GCM approach. Then, we don't need to derive the aes key from room private key every time. The performance will be better, and I guess security level is already good enough. What do u think ?
| return merged, nil | ||
| } | ||
|
|
||
| func (h *Handler) natsListRoomKeys(m otelnats.Msg) { |
Collaborator
There was a problem hiding this comment.
We can remove this method as frontend will not need it
| // Fan-out already committed survivors to predictedVersion; persist at | ||
| // the same version so broadcast-worker reads under the same key clients | ||
| // hold. Using Set here would stamp v0 and create a version mismatch. | ||
| if setErr := h.keyStore.SetWithVersion(ctx, roomID, *newPair, predictedVersion); setErr != nil { |
…lPublicKey PR review feedback from mliu33: the bootstrap RPC isn't needed because clients will receive room key material via the existing subscription.get* RPC family (user-service responsibility). Extending those RPCs to carry keys is separate, future work; until then RoomKeysContext populates from live RoomKeyEvent subscriptions only — reconnecting users will re-acquire keys when a rotation or membership change next fires for each room. Documented limitation. Also drops the legacy EphemeralPublicKey field from EncryptedMessage (was kept as omitempty for transitional compatibility; reviewer confirmed we don't need it). Removed: - room-service: natsListRoomKeys + handleListRoomKeys + RegisterCRUD subscribe; the ListSubscriptionsByAccount store method, its mongo impl, and its mock + integration test. - pkg/subject: RoomsKeysBootstrap + Wildcard builders and tests. - pkg/model: RoomsKeysEntry, RoomsKeysResponse, and their JSON test. - pkg/roomcrypto: EphemeralPublicKey field on EncryptedMessage. - chat-frontend: fetchRoomKeysBootstrap API op + its test; the roomsKeysBootstrap subject builder + test; RoomKeysEntry + RoomKeysResponse types; the BOOTSTRAP_LOADED reducer action + bootstrapped flag + corresponding tests; the bootstrap-fetch block in RoomKeysContext.tsx. - docs/client-api.md §5.2. https://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the per-message ECIES-style encryption in
pkg/roomcryptowith direct AES-256-GCM using a versioned 32-byte room secret. No ECDH, no ephemeral keys, no HKDF, no asymmetric crypto at any layer. The room secret IS the AES key.Implements the first chat-frontend decoder end-to-end so the in-repo client decrypts real broadcast messages instead of rendering the
[encrypted message]placeholder.What changed
Server (Go):
pkg/roomcrypto: newEncoderwith per-(roomId, version)AES-GCM cipher cache.Encode()is a thin wrapper: random 12-byte nonce +gcm.Seal. Per-message work is cache hit + nonce read + seal.pkg/roomkeystore: room secret is now a single 32-byte slice (RoomKeyPair.PrivateKey). Public-key field removed from storage, model, andRoomKeyEventwire payload. Generation viacrypto/rand.Read(32)— no curve operations. Existing Valkey rows that still carry the legacypubfield are ignored on read (forward-compat, no migration needed).pkg/model:RoomKeyEvent.PublicKeyremoved. NewRoomsKeysResponse/RoomsKeysEntrytypes for the bootstrap RPC.room-service: newchat.user.{account}.request.rooms.keysRPC (handleListRoomKeys) for clients to bootstrap their key cache on (re)connect.broadcast-worker: encoder constructed internally inNewHandler; both encrypt call sites switched toencoder.Encode(roomID, content, key.KeyPair.PrivateKey, key.Version).room-worker.rotateAndFanOut:ErrNoCurrentKeyfallback now usesSetWithVersion(predictedVersion)so survivors and Valkey stay version-aligned (fixes a latent mismatch bug).crypto/ecdhandgolang.org/x/crypto/hkdfdirect imports removed.Client (chat-frontend):
src/lib/roomcrypto/: new pure-utility module —b64decode,importAesKey(Web CryptoimportKey('raw', priv, AES-GCM)),decryptRoomMessage(crypto.subtle.decrypt).src/context/RoomKeysContext/: provider + reducer that bootstraps room keys viafetchRoomKeysBootstrap()on mount, subscribes to liveRoomKeyEvents, caches the derivedCryptoKeyper(roomId, version)in a ref, and exposesdecrypt({roomId, version, nonceB64, ciphertextB64}).src/api/: new opsfetchRoomKeysBootstrap,subscribeToRoomKeyEvents; new wire typesRoomKeyEvent,RoomKeysEntry,RoomKeysResponse; new subject builders.src/context/RoomEventsContext/useRoomSubscriptions.js: per-room promise chain serializes dispatch so encrypted and plaintext events for the same room arrive at the reducer in order.test/fixtures/encrypted-message.json: committed cross-language fixture (Go encoder → TS decoder) so wire-format drift breaks tests, not production.Docs:
docs/client-api.md§5.1: describes the 32-byte secret + AES-256-GCM scheme.docs/superpowers/specs/...md: design rationale (kept as historical artifact; algorithm has since been simplified further in code).Performance
BenchmarkEncoder_Encodeon Intel Xeon @ 2.80 GHz, Go 1.25.10:Encode(ECDH + keygen + HKDF + GCM)Encoder.Encode(steady state, cache warm)~245× faster per message. ECDH was 74% of the old cost; the cache amortizes it to zero. At 10K msg/s that's ~87% of one core → ~0.2%.
Wire format
encryptedMessageenvelope is now{version, nonce, ciphertext}. TheephemeralPublicKeyfield is removed.Migration
Server-only deploy. No re-keying required:
pubfield; readers ignore it.Out of scope
handleListRoomKeysreturns only same-site keys; documented limitation).Test plan
make lintcleanmake test— 47 packages, race detector enabledmake test-integration— Mongo + Valkey + NATS testcontainersnpm run typecheckcleannpm test— 587/587 frontend testsmake sast-goseccleanListSubscriptionsByAccountreturns all sub types regardless ofIsSubscribedrotateAndFanOutErrNoCurrentKeyfallback usesSetWithVersionwith the predicted versionhttps://claude.ai/code/session_01Egg36LodN4cWkCVCss6mqa
Summary by CodeRabbit
New Features
Documentation
{v, nonce, ciphertext, ephemeralPublicKey}to{version, nonce, ciphertext}.