feat(client): add E2EE support via WebRTC Encoded Transforms#2198
Open
feat(client): add E2EE support via WebRTC Encoded Transforms#2198
Conversation
Add end-to-end encryption for media tracks using a symmetric XOR transform applied to every encoded frame in a dedicated Web Worker. Uses RTCRtpScriptTransform (W3C standard) on browsers that support it (Safari, Firefox) and falls back to Insertable Streams (createEncodedStreams) on Chrome where RTCRtpScriptTransform is unreliable.
|
Contributor
|
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:
📝 WalkthroughWalkthroughAdds end-to-end encryption (E2EE) for WebRTC encoded frames: introduces an inline worker and Changes
Sequence DiagramssequenceDiagram
participant Client
participant Publisher
participant BasePeerConnection
participant RTCPeerConnection
participant EncryptionManager
participant Worker
participant RTCRtpSender
Client->>Publisher: publish(track, codec)
Publisher->>BasePeerConnection: createPeerConnection(config, e2ee?)
BasePeerConnection->>BasePeerConnection: clone RTCConfiguration
alt e2ee present and Chrome
BasePeerConnection->>BasePeerConnection: set encodedInsertableStreams = true
end
BasePeerConnection->>RTCPeerConnection: new RTCPeerConnection(modified config)
Publisher->>RTCRtpSender: addTransceiver -> sender
Publisher->>EncryptionManager: encrypt(sender, codec)
EncryptionManager->>Worker: postMessage(attach encode streams / transfer streams)
Worker->>RTCRtpSender: attach encode TransformStream
RTCRtpSender->>Worker: encoded frames -> Worker (transform)
Worker->>Worker: AES-128-GCM encrypt, append trailer
Worker->>RTCRtpSender: output encrypted frames
sequenceDiagram
participant Network
participant RTCRtpReceiver
participant Worker
participant EncryptionManager
participant Subscriber
participant Client
Network->>RTCRtpReceiver: RTP packets (encrypted payload)
RTCRtpReceiver->>Worker: encoded frames -> Worker (transform)
Worker->>Worker: validate trailer, extract keyIndex/counter
Worker->>Worker: AES-128-GCM decrypt, RBSP unescape if needed
Worker->>RTCRtpReceiver: decrypted frames
RTCRtpReceiver->>Subscriber: ontrack(receiver, stream)
Subscriber->>EncryptionManager: decrypt(receiver, userId)
EncryptionManager->>Worker: postMessage(attach decode streams)
Subscriber->>Client: deliver media track for playback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/client/src/rtc/Publisher.ts (1)
141-149: Consider wrappingcreateEncryptorin a try-catch.If
createEncryptorthrows (e.g., Worker instantiation failure, invalid key), the entireaddTransceiverflow will fail and prevent track publishing. A defensive try-catch with a warning log would allow graceful degradation.🛡️ Proposed defensive handling
const { encryptionKey } = this.clientPublishOptions || {}; if (encryptionKey) { if (supportsE2EE()) { - createEncryptor(transceiver.sender, encryptionKey); - this.logger.debug('E2EE encryptor attached to sender'); + try { + createEncryptor(transceiver.sender, encryptionKey); + this.logger.debug('E2EE encryptor attached to sender'); + } catch (err) { + this.logger.warn('Failed to attach E2EE encryptor', err); + } } else { this.logger.warn(`E2EE requested but not supported`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/Publisher.ts` around lines 141 - 149, Wrap the call to createEncryptor(transceiver.sender, encryptionKey) inside a try-catch so that exceptions from encryptor creation (e.g., Worker instantiation or invalid key) do not abort the addTransceiver/publish flow; on catch, log a warning via this.logger.warn including the error and continue without attaching the encryptor, leaving behavior unchanged when supportsE2EE() is false, and keep the debug log only on successful creation.packages/client/src/rtc/Subscriber.ts (1)
97-105: Consider wrappingcreateDecryptorin a try-catch for consistency with Publisher.Same reasoning as Publisher: if
createDecryptorthrows, the track handling will fail entirely. A defensive try-catch would allow the track to still be processed (unencrypted) rather than dropping it.🛡️ Proposed defensive handling
const { encryptionKey } = this.clientPublishOptions || {}; if (encryptionKey) { if (supportsE2EE()) { - createDecryptor(e.receiver, encryptionKey); - this.logger.debug('E2EE decryptor attached to receiver'); + try { + createDecryptor(e.receiver, encryptionKey); + this.logger.debug('E2EE decryptor attached to receiver'); + } catch (err) { + this.logger.warn('Failed to attach E2EE decryptor', err); + } } else { this.logger.warn(`E2EE requested but not supported`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/Subscriber.ts` around lines 97 - 105, The createDecryptor invocation in Subscriber (using createDecryptor(e.receiver, encryptionKey) guarded by supportsE2EE()) should be wrapped in a try-catch like Publisher to avoid losing the entire track if decryption initialization throws; update the block where this.clientPublishOptions?.encryptionKey is checked to call createDecryptor inside a try, log a warning or debug on failure via this.logger (e.g., this.logger.warn('E2EE decryptor failed to attach', error)), and continue processing the track if an exception occurs so unencrypted handling still proceeds.sample-apps/react/react-dogfood/lib/queryConfigParams.ts (1)
19-19: Security note: encryption key in URL query string.Exposing the encryption key via
?encryption_key=makes it visible in browser history, server logs, and referrer headers. This is acceptable for a dogfood/testing app, but ensure this pattern isn't replicated in production integrations where the key should be exchanged out-of-band.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/react-dogfood/lib/queryConfigParams.ts` at line 19, The code currently reads the encryption key from the URL query (encryptionKey: query['encryption_key']) which exposes secrets; change this to not pull sensitive keys from the query string — remove or default encryptionKey to undefined in queryConfigParams.ts and instead accept the key via a secure channel (e.g., an explicit config parameter passed from server-side code or process.env when in safe dev/dogfood only); if you must keep a dev-only shortcut, gate it behind a strict environment check (e.g., NODE_ENV === 'development' or a DOGFOOD flag) and document that production must supply keys out-of-band.packages/client/src/rtc/e2ee/index.ts (2)
31-63: Add error handling for pipe operations and consider key encoding.Two concerns with the worker implementation:
Missing error handling:
pipeTo()returns a Promise that can reject on stream errors. Silent failures during encryption/decryption would be difficult to debug.Key encoding:
charCodeAt()returns values > 255 for non-ASCII characters, whichsetInt8truncates. While this works symmetrically, it's worth documenting that keys should be ASCII-only, or encoding the key to bytes explicitly.♻️ Suggested improvement for error handling
function handleTransform({ readable, writable, key }) { - readable.pipeThrough(xorTransform(key)).pipeTo(writable); + readable.pipeThrough(xorTransform(key)).pipeTo(writable).catch((err) => { + console.error('[stream-video-e2ee] transform pipeline error:', err); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/e2ee/index.ts` around lines 31 - 63, The worker lacks stream error handling and uses charCodeAt which mishandles non-ASCII key bytes; update xorTransform to explicitly encode the key to a byte array (e.g., via TextEncoder) and use unsigned byte access (getUint8/setUint8 or Uint8Array) so key bytes are 0-255, and in handleTransform where readable.pipeTo(writable) is used (and in the onrtctransform/onmessage dispatch paths) attach a rejection handler (pipeTo(...).catch(err => { /* report via postMessage or console.error */ })) or await and try/catch to surface errors back to the main thread for debugging; reference functions: xorTransform, handleTransform, the self.onrtctransform handler, and self.onmessage handler.
83-99: Theoperationparameter is passed but unused in the worker.The
operationvalue ('encode'/'decode') is included in the message but the worker'shandleTransformfunction ignores it since XOR is symmetric. Consider either:
- Removing it to avoid confusion
- Adding a comment explaining it's reserved for future asymmetric transforms
This is cosmetic since the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/e2ee/index.ts` around lines 83 - 99, The worker message includes an unused 'operation' field which is confusing; update attachTransform (and its getWorker/w.postMessage call) to stop sending the operation value and remove it from the function signature, and also remove or stop depending on operation elsewhere; alternatively, if you prefer to keep the API, add a concise comment in attachTransform and in the worker's handleTransform noting that XOR is symmetric so 'operation' is currently unused and reserved for potential future asymmetric transforms (reference attachTransform, w.postMessage, and handleTransform).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/rtc/e2ee/index.ts`:
- Around line 94-95: The current code initializes piped with a Set (if ((piped
??= new Set()).has(target) return; piped.add(target);) but it should be a
WeakSet to avoid memory leaks for object targets; change the initializer from
new Set() to new WeakSet() and update the piped variable's declared type (where
it's defined) to WeakSet<typeof target> (or WeakSet<object>) so has/add calls on
piped remain valid.
- Around line 65-68: Change piped from a Set to a WeakSet to avoid retaining
strong references to RTCRtpSender/RTCRtpReceiver (declare piped as
WeakSet<RTCRtpSender | RTCRtpReceiver> and initialize it accordingly), and add
explicit cleanup to release worker resources by terminating/disposing the Worker
and clearing worker and workerUrl when the associated Call/Publisher/Subscriber
is disposed; specifically, in the code paths for Call.dispose, Publisher.dispose
and Subscriber.dispose (or any e2ee teardown function), call worker.terminate()
or the worker's disposal method if available, then set worker = undefined and
workerUrl = undefined and set piped = undefined to allow GC.
---
Nitpick comments:
In `@packages/client/src/rtc/e2ee/index.ts`:
- Around line 31-63: The worker lacks stream error handling and uses charCodeAt
which mishandles non-ASCII key bytes; update xorTransform to explicitly encode
the key to a byte array (e.g., via TextEncoder) and use unsigned byte access
(getUint8/setUint8 or Uint8Array) so key bytes are 0-255, and in handleTransform
where readable.pipeTo(writable) is used (and in the onrtctransform/onmessage
dispatch paths) attach a rejection handler (pipeTo(...).catch(err => { /* report
via postMessage or console.error */ })) or await and try/catch to surface errors
back to the main thread for debugging; reference functions: xorTransform,
handleTransform, the self.onrtctransform handler, and self.onmessage handler.
- Around line 83-99: The worker message includes an unused 'operation' field
which is confusing; update attachTransform (and its getWorker/w.postMessage
call) to stop sending the operation value and remove it from the function
signature, and also remove or stop depending on operation elsewhere;
alternatively, if you prefer to keep the API, add a concise comment in
attachTransform and in the worker's handleTransform noting that XOR is symmetric
so 'operation' is currently unused and reserved for potential future asymmetric
transforms (reference attachTransform, w.postMessage, and handleTransform).
In `@packages/client/src/rtc/Publisher.ts`:
- Around line 141-149: Wrap the call to createEncryptor(transceiver.sender,
encryptionKey) inside a try-catch so that exceptions from encryptor creation
(e.g., Worker instantiation or invalid key) do not abort the
addTransceiver/publish flow; on catch, log a warning via this.logger.warn
including the error and continue without attaching the encryptor, leaving
behavior unchanged when supportsE2EE() is false, and keep the debug log only on
successful creation.
In `@packages/client/src/rtc/Subscriber.ts`:
- Around line 97-105: The createDecryptor invocation in Subscriber (using
createDecryptor(e.receiver, encryptionKey) guarded by supportsE2EE()) should be
wrapped in a try-catch like Publisher to avoid losing the entire track if
decryption initialization throws; update the block where
this.clientPublishOptions?.encryptionKey is checked to call createDecryptor
inside a try, log a warning or debug on failure via this.logger (e.g.,
this.logger.warn('E2EE decryptor failed to attach', error)), and continue
processing the track if an exception occurs so unencrypted handling still
proceeds.
In `@sample-apps/react/react-dogfood/lib/queryConfigParams.ts`:
- Line 19: The code currently reads the encryption key from the URL query
(encryptionKey: query['encryption_key']) which exposes secrets; change this to
not pull sensitive keys from the query string — remove or default encryptionKey
to undefined in queryConfigParams.ts and instead accept the key via a secure
channel (e.g., an explicit config parameter passed from server-side code or
process.env when in safe dev/dogfood only); if you must keep a dev-only
shortcut, gate it behind a strict environment check (e.g., NODE_ENV ===
'development' or a DOGFOOD flag) and document that production must supply keys
out-of-band.
🪄 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: 34706f24-48cc-4daa-a7dc-7f51973b58e7
📒 Files selected for processing (9)
packages/client/src/rtc/BasePeerConnection.tspackages/client/src/rtc/Publisher.tspackages/client/src/rtc/Subscriber.tspackages/client/src/rtc/__tests__/Publisher.test.tspackages/client/src/rtc/__tests__/Subscriber.test.tspackages/client/src/rtc/__tests__/mocks/webrtc.mocks.tspackages/client/src/rtc/e2ee/index.tspackages/client/src/types.tssample-apps/react/react-dogfood/lib/queryConfigParams.ts
Comment on lines
+65
to
+68
| /** Tracks senders/receivers that already have encoded streams piped. */ | ||
| let piped: Set<RTCRtpSender | RTCRtpReceiver> | undefined; | ||
| let worker: Worker | undefined; | ||
| let workerUrl: string | undefined; |
Contributor
There was a problem hiding this comment.
Use WeakSet instead of Set to prevent memory leaks.
The piped variable holds strong references to RTCRtpSender/RTCRtpReceiver objects. These won't be garbage collected even after the peer connection is closed, causing a memory leak that grows with each call.
Additionally, there's no disposal mechanism for worker and workerUrl, which violates the coding guideline: "Always call dispose() on Call, Publisher, Subscriber when done for proper memory management."
🐛 Proposed fix using WeakSet and adding disposal
/** Tracks senders/receivers that already have encoded streams piped. */
-let piped: Set<RTCRtpSender | RTCRtpReceiver> | undefined;
+let piped: WeakSet<RTCRtpSender | RTCRtpReceiver> | undefined;
let worker: Worker | undefined;
let workerUrl: string | undefined;
+
+/**
+ * Disposes E2EE resources. Call when encryption is no longer needed.
+ */
+export const disposeE2EE = () => {
+ if (worker) {
+ worker.terminate();
+ worker = undefined;
+ }
+ if (workerUrl) {
+ URL.revokeObjectURL(workerUrl);
+ workerUrl = undefined;
+ }
+ piped = undefined;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Tracks senders/receivers that already have encoded streams piped. */ | |
| let piped: Set<RTCRtpSender | RTCRtpReceiver> | undefined; | |
| let worker: Worker | undefined; | |
| let workerUrl: string | undefined; | |
| /** Tracks senders/receivers that already have encoded streams piped. */ | |
| let piped: WeakSet<RTCRtpSender | RTCRtpReceiver> | undefined; | |
| let worker: Worker | undefined; | |
| let workerUrl: string | undefined; | |
| /** | |
| * Disposes E2EE resources. Call when encryption is no longer needed. | |
| */ | |
| export const disposeE2EE = () => { | |
| if (worker) { | |
| worker.terminate(); | |
| worker = undefined; | |
| } | |
| if (workerUrl) { | |
| URL.revokeObjectURL(workerUrl); | |
| workerUrl = undefined; | |
| } | |
| piped = undefined; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/rtc/e2ee/index.ts` around lines 65 - 68, Change piped
from a Set to a WeakSet to avoid retaining strong references to
RTCRtpSender/RTCRtpReceiver (declare piped as WeakSet<RTCRtpSender |
RTCRtpReceiver> and initialize it accordingly), and add explicit cleanup to
release worker resources by terminating/disposing the Worker and clearing worker
and workerUrl when the associated Call/Publisher/Subscriber is disposed;
specifically, in the code paths for Call.dispose, Publisher.dispose and
Subscriber.dispose (or any e2ee teardown function), call worker.terminate() or
the worker's disposal method if available, then set worker = undefined and
workerUrl = undefined and set piped = undefined to allow GC.
Comment on lines
+94
to
+95
| if ((piped ??= new Set()).has(target)) return; | ||
| piped.add(target); |
Contributor
There was a problem hiding this comment.
Update to WeakSet here as well.
Per the fix suggested above, this line should create a WeakSet instead of Set.
🐛 Proposed fix
- if ((piped ??= new Set()).has(target)) return;
+ if ((piped ??= new WeakSet()).has(target)) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ((piped ??= new Set()).has(target)) return; | |
| piped.add(target); | |
| if ((piped ??= new WeakSet()).has(target)) return; | |
| piped.add(target); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/rtc/e2ee/index.ts` around lines 94 - 95, The current code
initializes piped with a Set (if ((piped ??= new Set()).has(target) return;
piped.add(target);) but it should be a WeakSet to avoid memory leaks for object
targets; change the initializer from new Set() to new WeakSet() and update the
piped variable's declared type (where it's defined) to WeakSet<typeof target>
(or WeakSet<object>) so has/add calls on piped remain valid.
Split the monolithic e2ee/index.ts into three files: - compatibility.ts: supportsE2EE() stays in the main bundle - worker.ts: worker source string + lifecycle management - e2ee.ts: createEncryptor/createDecryptor API Publisher and Subscriber now dynamically import the heavy E2EE code, so consumers who don't use E2EE pay zero bundle cost. The browser build emits a separate e2ee chunk; node builds inline it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/client/src/rtc/e2ee/e2ee.ts (1)
7-11: Consider broadening the Chromium detection comment.The
navigator.userAgent?.includes('Chrome')check will match all Chromium-based browsers (Edge, Opera, Brave, etc.), not just Chrome. This is likely intentional since they share the same WebRTC implementation quirks, but a clarifying comment would help future maintainers.📝 Suggested documentation improvement
/** - * Chrome exposes RTCRtpScriptTransform, but it doesn't seem to work reliably. - * Use Insertable Streams (createEncodedStreams) there instead. + * Chromium-based browsers (Chrome, Edge, Opera, etc.) expose RTCRtpScriptTransform, + * but it doesn't work reliably. Use Insertable Streams (createEncodedStreams) instead. + * The "Chrome" UA substring check intentionally matches all Chromium derivatives. */ const shouldUseInsertableStreams = (): boolean =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/e2ee/e2ee.ts` around lines 7 - 11, The userAgent check in shouldUseInsertableStreams() currently uses navigator.userAgent?.includes('Chrome') which also matches other Chromium-based browsers (Edge, Brave, Opera); update the code by adding a clarifying comment above shouldUseInsertableStreams() stating that this check intentionally targets Chromium-based browsers (not only Google Chrome) because they share the same WebRTC/RTCRtpSender behavior (specifically the presence of createEncodedStreams on RTCRtpSender.prototype), so future maintainers understand the broader match and the rationale for keeping the existing navigator.userAgent check.packages/client/src/rtc/e2ee/worker.ts (1)
223-237: Add worker disposal mechanism to prevent memory leaks.The worker is lazily created but never cleaned up. Per coding guidelines, cleanup/disposal should be implemented for proper memory management. Also,
workerUrlis never revoked.♻️ Proposed addition for worker cleanup
let worker: Worker | undefined; let workerUrl: string | undefined; export const getWorker = () => { if (!worker) { if (!workerUrl) { const blob = new Blob([WORKER_SOURCE], { type: 'application/javascript', }); workerUrl = URL.createObjectURL(blob); } worker = new Worker(workerUrl, { name: 'stream-video-e2ee' }); } return worker; }; + +export const disposeWorker = () => { + if (worker) { + worker.terminate(); + worker = undefined; + } + if (workerUrl) { + URL.revokeObjectURL(workerUrl); + workerUrl = undefined; + } +};The
disposeWorkerfunction should then be called fromPublisher.dispose()/Subscriber.dispose()or a higher-level cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/e2ee/worker.ts` around lines 223 - 237, The module lazily creates a Worker in getWorker but never disposes it or revokes workerUrl, causing memory leaks; add a disposeWorker function that checks the module-scoped worker and workerUrl and if present calls worker.terminate(), URL.revokeObjectURL(workerUrl), and sets both worker and workerUrl to undefined, and invoke disposeWorker from higher-level cleanup such as Publisher.dispose() / Subscriber.dispose() to ensure proper teardown of the WORKER_SOURCE-backed worker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/rtc/e2ee/worker.ts`:
- Around line 126-132: The transform method currently bypasses encryption for
AV1 frames (check: transform function, condition codec === 'av1') which can
leave AV1 streams in plaintext; modify transform to emit a clear runtime warning
via the module's logger (or console.warn if no logger exists) whenever codec ===
'av1' and E2EE is enabled, and update the public API docs/comments to explicitly
state that AV1 frames are not encrypted by this E2EE implementation so users are
aware of the caveat.
- Around line 115-119: Add a disposal and recovery mechanism for the E2EE
worker: implement a dispose() that unregisters event listeners registered on the
worker (rtctransform, message), calls worker.terminate(), and calls
URL.revokeObjectURL(workerUrl); add error and 'onerror'/'onmessageerror'
handling in getWorker() so the worker can be reset/recreated on failure;
document via JSDoc on the module/getWorker()/xorPayload() that this
XOR-with-repeating-key implementation (and the 0xDEADBEEF frame marker) is a
reference/demo only and not cryptographically secure and recommend authenticated
encryption (e.g., IETF SFrame / AES‑GCM per RFC 9605) for production; finally
add a small recovery API (e.g., resetWorker()/ensureWorker()) to recreate the
worker after errors.
In `@packages/client/src/rtc/Subscriber.ts`:
- Around line 97-107: The dynamic import in Subscriber (where supportsE2EE()
leads to import('./e2ee/e2ee').then(({ createDecryptor }) => {
createDecryptor(e.receiver, encryptionKey); this.logger.debug('E2EE decryptor
attached to receiver'); })) lacks error handling; add a .catch handler on the
import promise to log the failure via this.logger.error (include the error and
context like "failed to load E2EE module" or "failed to initialize decryptor")
and, if appropriate, call any fallback or cleanup (e.g., notify caller or avoid
attaching decryptor) so unhandled promise rejections are prevented and failures
are visible.
---
Nitpick comments:
In `@packages/client/src/rtc/e2ee/e2ee.ts`:
- Around line 7-11: The userAgent check in shouldUseInsertableStreams()
currently uses navigator.userAgent?.includes('Chrome') which also matches other
Chromium-based browsers (Edge, Brave, Opera); update the code by adding a
clarifying comment above shouldUseInsertableStreams() stating that this check
intentionally targets Chromium-based browsers (not only Google Chrome) because
they share the same WebRTC/RTCRtpSender behavior (specifically the presence of
createEncodedStreams on RTCRtpSender.prototype), so future maintainers
understand the broader match and the rationale for keeping the existing
navigator.userAgent check.
In `@packages/client/src/rtc/e2ee/worker.ts`:
- Around line 223-237: The module lazily creates a Worker in getWorker but never
disposes it or revokes workerUrl, causing memory leaks; add a disposeWorker
function that checks the module-scoped worker and workerUrl and if present calls
worker.terminate(), URL.revokeObjectURL(workerUrl), and sets both worker and
workerUrl to undefined, and invoke disposeWorker from higher-level cleanup such
as Publisher.dispose() / Subscriber.dispose() to ensure proper teardown of the
WORKER_SOURCE-backed worker.
🪄 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: 72ec27ec-5586-4e79-9399-0b0d70c779d6
📒 Files selected for processing (7)
packages/client/rollup.config.mjspackages/client/src/rtc/Publisher.tspackages/client/src/rtc/Subscriber.tspackages/client/src/rtc/__tests__/Subscriber.test.tspackages/client/src/rtc/e2ee/compatibility.tspackages/client/src/rtc/e2ee/e2ee.tspackages/client/src/rtc/e2ee/worker.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/client/src/rtc/Publisher.ts
- packages/client/src/rtc/tests/Subscriber.test.ts
Comment on lines
+126
to
+132
| transform(frame, controller) { | ||
| // https://groups.google.com/g/discuss-webrtc/c/5CMOZ4JtERo | ||
| // https://issues.chromium.org/issues/40287616 | ||
| if (codec === 'av1') { | ||
| controller.enqueue(frame); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
AV1 frames pass through unencrypted - ensure this is clearly communicated to users.
When codec === 'av1', frames are enqueued without encryption. While this is documented in the JSDoc header, users configuring E2EE might not realize AV1 streams remain in plaintext. Consider:
- Logging a warning when AV1 is detected with E2EE enabled
- Adding this caveat to the public API documentation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/rtc/e2ee/worker.ts` around lines 126 - 132, The transform
method currently bypasses encryption for AV1 frames (check: transform function,
condition codec === 'av1') which can leave AV1 streams in plaintext; modify
transform to emit a clear runtime warning via the module's logger (or
console.warn if no logger exists) whenever codec === 'av1' and E2EE is enabled,
and update the public API docs/comments to explicitly state that AV1 frames are
not encrypted by this E2EE implementation so users are aware of the caveat.
Comment on lines
+97
to
+107
| const { encryptionKey } = this.clientPublishOptions || {}; | ||
| if (encryptionKey) { | ||
| if (supportsE2EE()) { | ||
| import('./e2ee/e2ee').then(({ createDecryptor }) => { | ||
| createDecryptor(e.receiver, encryptionKey); | ||
| this.logger.debug('E2EE decryptor attached to receiver'); | ||
| }); | ||
| } else { | ||
| this.logger.warn(`E2EE requested but not supported`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for the dynamic import.
The dynamic import lacks a .catch() handler. If the import fails (e.g., network error loading the chunk, or an exception in module initialization), it will result in an unhandled promise rejection with no indication to the user that E2EE failed to initialize.
🛡️ Proposed fix
const { encryptionKey } = this.clientPublishOptions || {};
if (encryptionKey) {
if (supportsE2EE()) {
- import('./e2ee/e2ee').then(({ createDecryptor }) => {
- createDecryptor(e.receiver, encryptionKey);
- this.logger.debug('E2EE decryptor attached to receiver');
- });
+ import('./e2ee/e2ee')
+ .then(({ createDecryptor }) => {
+ createDecryptor(e.receiver, encryptionKey);
+ this.logger.debug('E2EE decryptor attached to receiver');
+ })
+ .catch((err) => {
+ this.logger.error('Failed to attach E2EE decryptor', err);
+ });
} else {
this.logger.warn(`E2EE requested but not supported`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { encryptionKey } = this.clientPublishOptions || {}; | |
| if (encryptionKey) { | |
| if (supportsE2EE()) { | |
| import('./e2ee/e2ee').then(({ createDecryptor }) => { | |
| createDecryptor(e.receiver, encryptionKey); | |
| this.logger.debug('E2EE decryptor attached to receiver'); | |
| }); | |
| } else { | |
| this.logger.warn(`E2EE requested but not supported`); | |
| } | |
| } | |
| const { encryptionKey } = this.clientPublishOptions || {}; | |
| if (encryptionKey) { | |
| if (supportsE2EE()) { | |
| import('./e2ee/e2ee') | |
| .then(({ createDecryptor }) => { | |
| createDecryptor(e.receiver, encryptionKey); | |
| this.logger.debug('E2EE decryptor attached to receiver'); | |
| }) | |
| .catch((err) => { | |
| this.logger.error('Failed to attach E2EE decryptor', err); | |
| }); | |
| } else { | |
| this.logger.warn(`E2EE requested but not supported`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/rtc/Subscriber.ts` around lines 97 - 107, The dynamic
import in Subscriber (where supportsE2EE() leads to
import('./e2ee/e2ee').then(({ createDecryptor }) => {
createDecryptor(e.receiver, encryptionKey); this.logger.debug('E2EE decryptor
attached to receiver'); })) lacks error handling; add a .catch handler on the
import promise to log the failure via this.logger.error (include the error and
context like "failed to load E2EE module" or "failed to initialize decryptor")
and, if appropriate, call any fallback or cleanup (e.g., notify caller or avoid
attaching decryptor) so unhandled promise rejections are prevented and failures
are visible.
Replace standalone E2EE function exports and ClientPublishOptions.e2ee config with a single EncryptionManager class that owns the full E2EE lifecycle: key distribution, transform attachment, and worker management. - Add EncryptionManager with factory create(), static isSupported(), and instance methods: setKey, setSharedKey, removeKeys, encrypt, decrypt - Upgrade worker crypto from XOR to AES-128-GCM with per-user key store, shared key fallback, counter-based IVs, and codec-aware clear bytes - Add Call.setE2EEManager() for externally-provided manager injection - Pass manager reference through BasePeerConnectionOpts to Publisher/Subscriber - Delete e2ee.ts and compatibility.ts (absorbed into EncryptionManager) - Remove e2ee field from ClientPublishOptions - worker.ts now exports only WORKER_SOURCE (manager owns worker lifecycle)
- Drop frames when encryption key is missing instead of leaking plaintext - Drop frames when decryption key is missing instead of feeding ciphertext to the decoder - Expand clearBytes trailer field to 2 bytes (max 32767) to support H.264 frames with large SPS/PPS/SEI headers - Add key material length validation (must be exactly 16 bytes for AES-128) - Add worker error reporting to main thread via postMessage - Use scoped logger (videoLoggerSystem) instead of console.error - Add dispose guard to prevent double-termination - Block AV1 codec in encrypt() instead of silently passing through - Make dogfood applyQueryConfigParams async to ensure E2EE is initialized before join() - Upgrade sample app key derivation from naive byte cycling to PBKDF2 - Remove unused sharedKeyIndex variable - Simplify frame counter back to single uint32
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/client/src/rtc/e2ee/EncryptionManager.ts (1)
139-144: Document that rawKey is transferred and becomes unusable after the call.The
[rawKey]transfer array moves ownership to the worker, making the original ArrayBuffer detached (zero-length). Callers who try to reuse the buffer will get unexpected behavior.📝 Suggested JSDoc addition
* `@param` rawKey - 16-byte raw AES-128 key material. Transferred to the worker (zero-copy). + * The ArrayBuffer becomes detached after this call and cannot be reused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/rtc/e2ee/EncryptionManager.ts` around lines 139 - 144, The setKey method transfers ownership of rawKey via this.worker.postMessage(..., [rawKey]) which detaches the original ArrayBuffer; update the JSDoc for setKey (and mention validateKeyLength behavior) to clearly state that rawKey is transferred and will become unusable after the call and advise callers to pass a clone if they need to reuse it (or remove the transfer if non-destructive behavior is required); ensure the comment references setKey and the postMessage transfer semantics so future maintainers understand the detachment risk.packages/client/src/Call.ts (1)
2001-2011: Consider adding runtime validation for timing constraint.The JSDoc correctly documents that
setE2EEManagermust be called beforejoin(), but there's no runtime enforcement. If called afterjoin(), the manager won't be passed to the peer connections (they're already created), and E2EE silently won't work.Consider adding a warning or error when called after joining:
🛡️ Suggested defensive check
setE2EEManager = (e2ee: EncryptionManager) => { + if (this.state.callingState === CallingState.JOINED || + this.state.callingState === CallingState.JOINING) { + this.logger.warn( + 'setE2EEManager called after join - E2EE will not be active for this session' + ); + } this.e2eeManager = e2ee; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/Call.ts` around lines 2001 - 2011, setE2EEManager can be called after the call has already joined which silently makes E2EE ineffective; add a runtime check in the setE2EEManager method that detects whether the call has already created peer connections (e.g. check a join flag like this.joined or existing peer connection map/array such as this.peerConnections) and when true, either throw an Error or at minimum log a clear warning (e.g. "setE2EEManager must be called before join() — E2EE will not be applied") and avoid silently failing; still assign this.e2eeManager when safe, but ensure the check references setE2EEManager, join, this.e2eeManager and the join/peer-connection indicator so future callers get immediate feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/rtc/e2ee/worker.ts`:
- Around line 105-118: Frame counter resets on worker recreation causing
potential IV reuse for the same key; update buildIV and nextFrameCounter to
prevent reuse by adding a random nonce component to the IV (e.g., make IV_LEN
12, fill bytes 0-3 with crypto.getRandomValues(4) and bytes 4-7 or 8-11 with the
4-byte frame counter) so each worker instantiation produces distinct (key, IV)
pairs even if counters restart, and ensure buildIV uses the same byte layout as
nextFrameCounter; alternatively consider persisting frameCounters externally or
requiring key rotation on worker recreation and document that constraint in
comments near buildIV/nextFrameCounter.
In `@sample-apps/react/react-dogfood/pages/bare/join/`[callId].tsx:
- Around line 65-67: The call to applyQueryConfigParams using router.query
currently swallows E2EE initialization errors by only logging them; change this
to await applyQueryConfigParams(router.query) inside a try/catch (mirroring
MeetingUI.tsx) and on catch either set a visible error state (e.g., setInitError
/ render an error screen) or prevent proceeding to join (e.g., navigate away or
disable join) so the call does not continue without requested E2EE; ensure the
error handling path surfaces the error to the user instead of only
console.error.
---
Nitpick comments:
In `@packages/client/src/Call.ts`:
- Around line 2001-2011: setE2EEManager can be called after the call has already
joined which silently makes E2EE ineffective; add a runtime check in the
setE2EEManager method that detects whether the call has already created peer
connections (e.g. check a join flag like this.joined or existing peer connection
map/array such as this.peerConnections) and when true, either throw an Error or
at minimum log a clear warning (e.g. "setE2EEManager must be called before
join() — E2EE will not be applied") and avoid silently failing; still assign
this.e2eeManager when safe, but ensure the check references setE2EEManager,
join, this.e2eeManager and the join/peer-connection indicator so future callers
get immediate feedback.
In `@packages/client/src/rtc/e2ee/EncryptionManager.ts`:
- Around line 139-144: The setKey method transfers ownership of rawKey via
this.worker.postMessage(..., [rawKey]) which detaches the original ArrayBuffer;
update the JSDoc for setKey (and mention validateKeyLength behavior) to clearly
state that rawKey is transferred and will become unusable after the call and
advise callers to pass a clone if they need to reuse it (or remove the transfer
if non-destructive behavior is required); ensure the comment references setKey
and the postMessage transfer semantics so future maintainers understand the
detachment risk.
🪄 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: f60c8fa0-f27a-4082-b311-018d76a5c700
📒 Files selected for processing (13)
packages/client/index.tspackages/client/src/Call.tspackages/client/src/rtc/BasePeerConnection.tspackages/client/src/rtc/Publisher.tspackages/client/src/rtc/Subscriber.tspackages/client/src/rtc/__tests__/Publisher.test.tspackages/client/src/rtc/__tests__/Subscriber.test.tspackages/client/src/rtc/e2ee/EncryptionManager.tspackages/client/src/rtc/e2ee/worker.tspackages/client/src/rtc/types.tssample-apps/react/react-dogfood/components/MeetingUI.tsxsample-apps/react/react-dogfood/lib/queryConfigParams.tssample-apps/react/react-dogfood/pages/bare/join/[callId].tsx
✅ Files skipped from review due to trivial changes (3)
- packages/client/src/rtc/types.ts
- packages/client/src/rtc/tests/Publisher.test.ts
- packages/client/src/rtc/tests/Subscriber.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/client/src/rtc/Publisher.ts
- packages/client/src/rtc/BasePeerConnection.ts
- packages/client/src/rtc/Subscriber.ts
Comment on lines
+65
to
+67
| applyQueryConfigParams(_call, router.query).catch((e) => | ||
| console.error('Failed to apply query config params', e), | ||
| ); |
Contributor
There was a problem hiding this comment.
E2EE initialization failures are silently ignored.
Unlike MeetingUI.tsx which awaits applyQueryConfigParams within a try/catch and shows an error screen on failure, this bare join page only logs to console. If E2EE key derivation or manager creation fails, the call proceeds without encryption while the user may expect it to be enabled.
Consider either awaiting and handling the error to prevent joining without E2EE when it was explicitly requested, or documenting this as intentional behavior for the minimal "bare" implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample-apps/react/react-dogfood/pages/bare/join/`[callId].tsx around lines 65
- 67, The call to applyQueryConfigParams using router.query currently swallows
E2EE initialization errors by only logging them; change this to await
applyQueryConfigParams(router.query) inside a try/catch (mirroring
MeetingUI.tsx) and on catch either set a visible error state (e.g., setInitError
/ render an error screen) or prevent proceeding to join (e.g., navigate away or
disable join) so the call does not continue without requested E2EE; ensure the
error handling path surfaces the error to the user instead of only
console.error.
Add a standalone Vite + React sample app for testing E2EE with multiple participants in a single browser window. Each participant has their own StreamVideoClient, Call, and EncryptionManager with per-user keys. Demo features: - Add/remove up to 4 participants sharing the same call - Per-user key generation, manual key input (hex or passphrase), rotation - Automatic cross-participant key distribution - "Local only" mode to simulate key mismatch - Decryption failure toast notification with manual dismiss - Event log showing all key operations in real time SDK changes: - Subscriber uses participant.userId for decrypt key lookup instead of trackLookupPrefix, fixing per-user key matching - Worker posts throttled decryptionFailed messages on decrypt failure - EncryptionManager exposes onDecryptionFailed callback
…tices Centralize all E2EE key operations into e2ee/keys.ts with a pluggable SendKeyFn transport abstraction. Extract React state management into a useE2EEDemo hook, leaving App.tsx as a thin layout shell. - Make Call.e2eeManager public for direct access - Add E2EEParticipant interface (pure key state, no SDK refs) - Wrap ParticipantPanel in React.memo with stable callbacks - Rename ParticipantState to ParticipantSession - Fix side-effects-in-state-updater anti-pattern - Delete crypto.ts (superseded by e2ee/keys.ts)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
sample-apps/react/e2ee-demo/src/e2ee/keys.ts (1)
119-140: Consider documenting the hardcoded salt limitation.The PBKDF2 derivation uses a static salt
'stream-e2ee'. While acceptable for a demo, a production implementation should use a unique salt per user or session. Consider adding a brief comment noting this.📝 Suggested documentation addition
export const deriveKeyFromPassphrase = async ( passphrase: string, ): Promise<ArrayBuffer> => { const enc = new TextEncoder(); const baseKey = await crypto.subtle.importKey( 'raw', enc.encode(passphrase), 'PBKDF2', false, ['deriveBits'], ); + // Note: Static salt is acceptable for this demo. In production, use a + // unique salt per user/session stored alongside the derived key metadata. return crypto.subtle.deriveBits( { name: 'PBKDF2', salt: enc.encode('stream-e2ee'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/e2ee-demo/src/e2ee/keys.ts` around lines 119 - 140, The deriveKeyFromPassphrase function currently uses a hardcoded salt ('stream-e2ee') in the PBKDF2 parameters; add a concise comment above deriveKeyFromPassphrase noting that the static salt is only suitable for demo purposes and that production should use a unique, per-user or per-session random salt (persisted or transmitted alongside the derived key material) to prevent cross-user attacks and rainbow-table vulnerabilities; reference the function name deriveKeyFromPassphrase and the literal 'stream-e2ee' in the comment so reviewers can quickly find and understand the limitation.sample-apps/react/e2ee-demo/src/components/KeyControls.tsx (1)
13-29: Consider memoizingtoHexcomputation and stabilizing handlers.For consistency with coding guidelines,
toHex(currentKey)could be memoized withuseMemo, andhandleSetKeycould useuseCallback. However, since this is a demo app with limited re-render frequency, this is a minor optimization.♻️ Optional optimization
-import { useState } from 'react'; +import { useState, useMemo, useCallback } from 'react'; import { toHex } from '../e2ee/keys'; import './KeyControls.css'; // ... props interface ... export const KeyControls = ({ currentKey, keyIndex, color, onRotate, onSetKey, }: KeyControlsProps) => { const [input, setInput] = useState(''); const [localOnly, setLocalOnly] = useState(false); - const hex = toHex(currentKey); + const hex = useMemo(() => toHex(currentKey), [currentKey]); - const handleSetKey = () => { + const handleSetKey = useCallback(() => { const trimmed = input.trim(); if (!trimmed) return; onSetKey(trimmed, localOnly); setInput(''); - }; + }, [input, localOnly, onSetKey]);As per coding guidelines: "Use useMemo for expensive computations in React components" and "Use useCallback hook to stabilize function references passed to child components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/e2ee-demo/src/components/KeyControls.tsx` around lines 13 - 29, The component KeyControls computes hex with toHex(currentKey) on every render and defines handleSetKey inline; memoize the hex value using React's useMemo referencing currentKey and stabilize handleSetKey with useCallback referencing input, localOnly and onSetKey so the function identity is stable when passed to children; ensure imports include useMemo and useCallback and keep existing behavior (trim input, early return, call onSetKey(trimmed, localOnly), then setInput('')) while referencing the memoized hex variable.sample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.ts (2)
30-36: Token fetch lacks explicit error handling.If the fetch fails or returns non-JSON, the error will propagate up. While the caller has a try/catch, consider adding basic validation for clearer error messages in the demo.
♻️ Suggested improvement
const createTokenProvider = (userId: string) => async () => { const url = new URL(TOKEN_ENDPOINT); url.searchParams.set('api_key', API_KEY); url.searchParams.set('user_id', userId); - const { token } = await fetch(url).then((r) => r.json()); + const response = await fetch(url); + if (!response.ok) { + throw new Error(`Token fetch failed: ${response.status}`); + } + const { token } = await response.json(); return token as string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.ts` around lines 30 - 36, The createTokenProvider function lacks explicit error handling for network failures and non-OK/non-JSON responses; wrap the fetch call in a try/catch, validate response.ok and content-type before parsing, and throw descriptive errors including the URL/userId and status/text or parse error so callers get clearer messages (update createTokenProvider to catch fetch/network exceptions, check response.ok, inspect headers for application/json, attempt json() inside a try and throw a clear error if parsing fails).
270-273: Minor inconsistency in accessinge2eeManager.Line 272 accesses
target.call.e2eeManager?.dispose(), while the cleanup effect (line 61) accessesp.e2eeManager.dispose()directly. Sincetargetalready has thee2eeManagerreference, using it directly is clearer and avoids the optional chaining.♻️ Suggested fix
// SDK cleanup target.call.leave().catch(() => {}); - target.call.e2eeManager?.dispose(); + target.e2eeManager.dispose(); target.client.disconnectUser().catch(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.ts` around lines 270 - 273, The cleanup block is inconsistently accessing the E2EE manager; change the call from using optional chaining on the call object (target.call.e2eeManager?.dispose()) to use the direct reference already present on the target (target.e2eeManager.dispose()), mirroring the earlier cleanup pattern (p.e2eeManager.dispose()); ensure you call dispose() on target.e2eeManager and remove the unnecessary optional chaining so the code consistently uses the same E2EE manager reference.sample-apps/react/e2ee-demo/src/components/ParticipantPanel.tsx (2)
23-37: Consider memoizingCallUIto avoid unnecessary re-renders.
CallUIis rendered inside eachParticipantPanel. Since the parent is memoized and this component relies only on hook state, wrapping it withmemoensures it won't re-render when parent props change.♻️ Suggested refactor
-const CallUI = () => { +const CallUI = memo(function CallUI() { const { useCallCallingState } = useCallStateHooks(); const callingState = useCallCallingState(); if (callingState !== CallingState.JOINED) { return <div className="participant-panel__loading">Connecting...</div>; } return ( <StreamTheme> <PaginatedGridLayout /> <CallControls /> </StreamTheme> ); -}; +});Also update the import:
-import { memo, useCallback } from 'react'; +import { memo, useCallback } from 'react';Based on learnings: "Use React.memo for components rendered in large participant lists to optimize re-render performance"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/e2ee-demo/src/components/ParticipantPanel.tsx` around lines 23 - 37, Wrap the CallUI functional component with React.memo so it only re-renders when its hook-derived state changes: import React, { memo } (or add memo to the existing React import) and export/define CallUI as memo(CallUI) (or assign const MemoizedCallUI = memo(CallUI) and use that). Target the CallUI definition that uses useCallCallingState and is rendered inside ParticipantPanel so it doesn't re-render when parent props change; no other behavior changes to StreamTheme, PaginatedGridLayout, or CallControls are needed.
72-74: Truncation logic may produce misleading UI for short user IDs.If
userIdis shorter than 24 characters, the display will still append...even though nothing was truncated. Consider conditional truncation.♻️ Suggested fix
<span className="participant-panel__user-id" title={userId}> - {userId.slice(0, 24)}... + {userId.length > 24 ? `${userId.slice(0, 24)}...` : userId} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/e2ee-demo/src/components/ParticipantPanel.tsx` around lines 72 - 74, The span in ParticipantPanel.tsx currently always appends "..." to the displayed userId via {userId.slice(0, 24)}..., which is misleading when userId.length <= 24; update the rendering in the component (look for the element with className "participant-panel__user-id" in ParticipantPanel) to conditionally truncate and append "..." only when userId.length > 24, otherwise render the full userId (and preserve the title={userId} behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/rtc/__tests__/Subscriber.test.ts`:
- Around line 218-247: The test assumes the '123' trackLookupPrefix is absent in
shared state; make it deterministic by using a unique prefix that cannot be
registered elsewhere in tests (e.g. 'orphan-<unique>' or a random string) when
setting mediaStream.id and in the expected decrypt call. Concretely, update the
mediaStream.id assignment in the test that calls Subscriber and handleOnTrack to
use a unique prefix (replace '123:TRACK_TYPE_VIDEO' with something like
'orphan-XYZ:TRACK_TYPE_VIDEO') and update the
expect(e2eeMock.decrypt).toHaveBeenCalledWith(...) assertion to expect that same
unique prefix ('orphan-XYZ') instead of '123', so the fallback-to-trackId path
is deterministic; alternatively clear or reset the relevant lookup in shared
state before invoking subscriber['handleOnTrack'] if you prefer to keep a fixed
id.
In `@packages/client/src/rtc/e2ee/worker.ts`:
- Around line 52-54: The sharedKey fallback reuses the same AES-GCM key across
participants while buildIV() only uses the frame counter, causing nonce reuse;
fix by deriving a participant-scoped subkey or adding a sender-unique IV prefix
that both sender and receiver can reconstruct. Locate the sharedKey variable and
the buildIV() function and implement one of these: 1) derive per-participant AES
keys from the shared secret (e.g., HKDF using userId or senderId as info) and
replace use of sharedKey with that derived CryptoKey, or 2) prepend a
sender-unique prefix (derived from the shared secret and senderId) to the IV
generated by buildIV() so the full IV includes both the prefix and the frame
counter; ensure the receiver performs the same derivation when decrypting so IVs
and keys match.
- Around line 56-63: importKey is async and can complete out of order, causing
latestKeyIndex to be rolled back; serialize imports per participant by adding a
per-user pending promise/lock (e.g., a Map like pendingImports keyed by userId)
and chain await the previous import before executing the body of importKey (or
acquire a per-user mutex) so imports for the same user run sequentially;
additionally, when updating latestKeyIndex inside importKey, only advance it if
the incoming keyIndex is newer than the stored value (or use the serialized
ordering guarantee so simple assignment is safe), referencing importKey,
latestKeyIndex, and keyStore to locate where to apply the serialization and
conditional update.
- Around line 46-54: The three maps keyStore, latestKeyIndex, and frameCounters
(and their comments) must be keyed by sessionId instead of userId to avoid
cross-session collisions when a user has multiple devices; change their usage so
every get/set/delete uses sessionId as the unique identifier, update any code
that installs/reads keys or increments frame counters to pass sessionId, and
ensure session-scoped cleanup when a session ends; keep sharedKey semantics as a
true fallback but document it explicitly as shared across sessions only when
intended.
In `@sample-apps/react/e2ee-demo/src/App.css`:
- Line 11: The CSS custom property --font-mono in App.css uses mixed case font
family names which triggers Stylelint's value-keyword-case rule; update the
value of --font-mono (the font-family string assigned to the --font-mono
variable) to use lowercase keywords (e.g., 'sf mono', 'cascadia code', 'fira
code', consolas, monospace) so all font names are lowercase and the linter
passes.
In `@sample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.ts`:
- Around line 230-258: The async call to setE2EEKeyFromInput inside
setKeyFromInput can reject but has no error handling; update setKeyFromInput to
add a .catch handler (or use try/catch if converting to async) for the Promise
returned by setE2EEKeyFromInput(target.e2eeManager, ...), and in that handler
log the error via logEvent (or console/error logger), surface a user-visible
error state for the target (e.g., update participants state to reflect failure
or set an error message), and ensure any partial UI updates are not applied on
failure so the UI remains consistent.
---
Nitpick comments:
In `@sample-apps/react/e2ee-demo/src/components/KeyControls.tsx`:
- Around line 13-29: The component KeyControls computes hex with
toHex(currentKey) on every render and defines handleSetKey inline; memoize the
hex value using React's useMemo referencing currentKey and stabilize
handleSetKey with useCallback referencing input, localOnly and onSetKey so the
function identity is stable when passed to children; ensure imports include
useMemo and useCallback and keep existing behavior (trim input, early return,
call onSetKey(trimmed, localOnly), then setInput('')) while referencing the
memoized hex variable.
In `@sample-apps/react/e2ee-demo/src/components/ParticipantPanel.tsx`:
- Around line 23-37: Wrap the CallUI functional component with React.memo so it
only re-renders when its hook-derived state changes: import React, { memo } (or
add memo to the existing React import) and export/define CallUI as memo(CallUI)
(or assign const MemoizedCallUI = memo(CallUI) and use that). Target the CallUI
definition that uses useCallCallingState and is rendered inside ParticipantPanel
so it doesn't re-render when parent props change; no other behavior changes to
StreamTheme, PaginatedGridLayout, or CallControls are needed.
- Around line 72-74: The span in ParticipantPanel.tsx currently always appends
"..." to the displayed userId via {userId.slice(0, 24)}..., which is misleading
when userId.length <= 24; update the rendering in the component (look for the
element with className "participant-panel__user-id" in ParticipantPanel) to
conditionally truncate and append "..." only when userId.length > 24, otherwise
render the full userId (and preserve the title={userId} behavior).
In `@sample-apps/react/e2ee-demo/src/e2ee/keys.ts`:
- Around line 119-140: The deriveKeyFromPassphrase function currently uses a
hardcoded salt ('stream-e2ee') in the PBKDF2 parameters; add a concise comment
above deriveKeyFromPassphrase noting that the static salt is only suitable for
demo purposes and that production should use a unique, per-user or per-session
random salt (persisted or transmitted alongside the derived key material) to
prevent cross-user attacks and rainbow-table vulnerabilities; reference the
function name deriveKeyFromPassphrase and the literal 'stream-e2ee' in the
comment so reviewers can quickly find and understand the limitation.
In `@sample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.ts`:
- Around line 30-36: The createTokenProvider function lacks explicit error
handling for network failures and non-OK/non-JSON responses; wrap the fetch call
in a try/catch, validate response.ok and content-type before parsing, and throw
descriptive errors including the URL/userId and status/text or parse error so
callers get clearer messages (update createTokenProvider to catch fetch/network
exceptions, check response.ok, inspect headers for application/json, attempt
json() inside a try and throw a clear error if parsing fails).
- Around line 270-273: The cleanup block is inconsistently accessing the E2EE
manager; change the call from using optional chaining on the call object
(target.call.e2eeManager?.dispose()) to use the direct reference already present
on the target (target.e2eeManager.dispose()), mirroring the earlier cleanup
pattern (p.e2eeManager.dispose()); ensure you call dispose() on
target.e2eeManager and remove the unnecessary optional chaining so the code
consistently uses the same E2EE manager reference.
🪄 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: 2330f332-3438-4070-82fe-411a79bdab61
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
packages/client/src/Call.tspackages/client/src/rtc/Subscriber.tspackages/client/src/rtc/__tests__/Subscriber.test.tspackages/client/src/rtc/e2ee/EncryptionManager.tspackages/client/src/rtc/e2ee/worker.tssample-apps/react/e2ee-demo/index.htmlsample-apps/react/e2ee-demo/package.jsonsample-apps/react/e2ee-demo/src/App.csssample-apps/react/e2ee-demo/src/App.tsxsample-apps/react/e2ee-demo/src/components/EventLog.csssample-apps/react/e2ee-demo/src/components/EventLog.tsxsample-apps/react/e2ee-demo/src/components/Header.csssample-apps/react/e2ee-demo/src/components/Header.tsxsample-apps/react/e2ee-demo/src/components/KeyControls.csssample-apps/react/e2ee-demo/src/components/KeyControls.tsxsample-apps/react/e2ee-demo/src/components/ParticipantGrid.csssample-apps/react/e2ee-demo/src/components/ParticipantGrid.tsxsample-apps/react/e2ee-demo/src/components/ParticipantPanel.csssample-apps/react/e2ee-demo/src/components/ParticipantPanel.tsxsample-apps/react/e2ee-demo/src/config.tssample-apps/react/e2ee-demo/src/e2ee/keys.tssample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.tssample-apps/react/e2ee-demo/src/main.tsxsample-apps/react/e2ee-demo/src/types.tssample-apps/react/e2ee-demo/src/vite-env.d.tssample-apps/react/e2ee-demo/tsconfig.jsonsample-apps/react/e2ee-demo/tsconfig.node.jsonsample-apps/react/e2ee-demo/vite.config.ts
✅ Files skipped from review due to trivial changes (13)
- sample-apps/react/e2ee-demo/src/vite-env.d.ts
- sample-apps/react/e2ee-demo/vite.config.ts
- sample-apps/react/e2ee-demo/tsconfig.node.json
- sample-apps/react/e2ee-demo/index.html
- sample-apps/react/e2ee-demo/tsconfig.json
- sample-apps/react/e2ee-demo/src/components/EventLog.css
- sample-apps/react/e2ee-demo/src/components/ParticipantGrid.css
- sample-apps/react/e2ee-demo/src/components/Header.css
- sample-apps/react/e2ee-demo/package.json
- sample-apps/react/e2ee-demo/src/main.tsx
- sample-apps/react/e2ee-demo/src/components/KeyControls.css
- sample-apps/react/e2ee-demo/src/types.ts
- sample-apps/react/e2ee-demo/src/components/ParticipantPanel.css
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/client/src/rtc/Subscriber.ts
- packages/client/src/Call.ts
- packages/client/src/rtc/e2ee/EncryptionManager.ts
Comment on lines
+46
to
+54
| // Map<userId, Map<keyIndex, CryptoKey>> | ||
| const keyStore = new Map(); | ||
| // Map<userId, number> — latest key index for encoding | ||
| const latestKeyIndex = new Map(); | ||
| // Map<userId, number> — monotonic frame counter for encoding | ||
| const frameCounters = new Map(); | ||
| // Shared fallback: used when no per-user key is set for a given userId. | ||
| // Enables the simple "everyone uses the same key" scenario. | ||
| let sharedKey = null; // { key: CryptoKey, keyIndex: number } | null |
Contributor
There was a problem hiding this comment.
Key worker state by sessionId, not userId.
keyStore, latestKeyIndex, and frameCounters collapse every concurrent session for the same user into one slot. If the same user joins from two devices/tabs, later key installs overwrite earlier ones and both sessions start sharing key/counter state in this worker.
As per coding guidelines, "Participants should use sessionId as the unique identifier, not userId".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/rtc/e2ee/worker.ts` around lines 46 - 54, The three maps
keyStore, latestKeyIndex, and frameCounters (and their comments) must be keyed
by sessionId instead of userId to avoid cross-session collisions when a user has
multiple devices; change their usage so every get/set/delete uses sessionId as
the unique identifier, update any code that installs/reads keys or increments
frame counters to pass sessionId, and ensure session-scoped cleanup when a
session ends; keep sharedKey semantics as a true fallback but document it
explicitly as shared across sessions only when intended.
Comment on lines
+56
to
+63
| async function importKey(userId, keyIndex, rawKey) { | ||
| try { | ||
| const cryptoKey = await crypto.subtle.importKey( | ||
| 'raw', rawKey, { name: 'AES-GCM', length: 128 }, false, ['encrypt', 'decrypt'] | ||
| ); | ||
| if (!keyStore.has(userId)) keyStore.set(userId, new Map()); | ||
| keyStore.get(userId).set(keyIndex, cryptoKey); | ||
| latestKeyIndex.set(userId, keyIndex); |
Contributor
There was a problem hiding this comment.
Prevent older setKey imports from rolling the encoder back.
importKey() is async, and the message handler fire-and-forgets it. Two setKey messages for the same participant can resolve out of order, so Line 63 may move latestKeyIndex back to an older key after rotation.
💡 Minimal fix sketch
async function importKey(userId, keyIndex, rawKey) {
try {
const cryptoKey = await crypto.subtle.importKey(
'raw', rawKey, { name: 'AES-GCM', length: 128 }, false, ['encrypt', 'decrypt']
);
if (!keyStore.has(userId)) keyStore.set(userId, new Map());
keyStore.get(userId).set(keyIndex, cryptoKey);
- latestKeyIndex.set(userId, keyIndex);
+ const current = latestKeyIndex.get(userId);
+ if (current === undefined || keyIndex >= current) {
+ latestKeyIndex.set(userId, keyIndex);
+ }
} catch (e) {
self.postMessage({ type: 'error', message: 'Failed to import key for user ' + userId + ': ' + e.message });
}
}If key indices can wrap or arrive non-monotonically, serialize imports per participant instead of comparing indices.
Also applies to: 366-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/rtc/e2ee/worker.ts` around lines 56 - 63, importKey is
async and can complete out of order, causing latestKeyIndex to be rolled back;
serialize imports per participant by adding a per-user pending promise/lock
(e.g., a Map like pendingImports keyed by userId) and chain await the previous
import before executing the body of importKey (or acquire a per-user mutex) so
imports for the same user run sequentially; additionally, when updating
latestKeyIndex inside importKey, only advance it if the incoming keyIndex is
newer than the stored value (or use the serialized ordering guarantee so simple
assignment is safe), referencing importKey, latestKeyIndex, and keyStore to
locate where to apply the serialization and conditional update.
| --color-text-mono: #7dd3fc; | ||
| --radius-md: 8px; | ||
| --radius-sm: 4px; | ||
| --font-mono: 'SF Mono', 'Cascadia Code', 'Fira Code', Consolas, monospace; |
Contributor
There was a problem hiding this comment.
Fix font name casing for Stylelint compliance.
Stylelint reports value-keyword-case error. While CSS font-family values are case-insensitive, the linter expects lowercase for consistency.
🔧 Proposed fix
- --font-mono: 'SF Mono', 'Cascadia Code', 'Fira Code', Consolas, monospace;
+ --font-mono: 'SF Mono', 'Cascadia Code', 'Fira Code', consolas, monospace;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| --font-mono: 'SF Mono', 'Cascadia Code', 'Fira Code', Consolas, monospace; | |
| --font-mono: 'SF Mono', 'Cascadia Code', 'Fira Code', consolas, monospace; |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 11-11: Expected "Consolas" to be "consolas" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample-apps/react/e2ee-demo/src/App.css` at line 11, The CSS custom property
--font-mono in App.css uses mixed case font family names which triggers
Stylelint's value-keyword-case rule; update the value of --font-mono (the
font-family string assigned to the --font-mono variable) to use lowercase
keywords (e.g., 'sf mono', 'cascadia code', 'fira code', consolas, monospace) so
all font names are lowercase and the linter passes.
Comment on lines
+230
to
+258
| const setKeyFromInput = useCallback( | ||
| (targetUserId: string, input: string, localOnly: boolean) => { | ||
| const allParticipants = participantsRef.current; | ||
| const target = allParticipants.find((p) => p.userId === targetUserId); | ||
| if (!target) return; | ||
|
|
||
| setE2EEKeyFromInput( | ||
| target.e2eeManager, | ||
| target, | ||
| input, | ||
| allParticipants, | ||
| sendKey, | ||
| { localOnly }, | ||
| ).then(({ key, keyIndex }) => { | ||
| logEvent( | ||
| `${target.name} set key (#${keyIndex}): ${toHex(key).slice(0, 16)}...${localOnly ? ' [LOCAL ONLY]' : ''}`, | ||
| 'key-set', | ||
| ); | ||
|
|
||
| // Pure state update | ||
| setParticipants((prev) => | ||
| prev.map((p) => | ||
| p.userId === targetUserId ? { ...p, currentKey: key, keyIndex } : p, | ||
| ), | ||
| ); | ||
| }); | ||
| }, | ||
| [logEvent, sendKey], | ||
| ); |
Contributor
There was a problem hiding this comment.
Missing error handling for async key operation.
setE2EEKeyFromInput returns a Promise that could reject (e.g., on invalid passphrase derivation), but there's no .catch() handler. Errors will be silently swallowed, potentially leaving the UI in an inconsistent state.
🛡️ Suggested fix
setE2EEKeyFromInput(
target.e2eeManager,
target,
input,
allParticipants,
sendKey,
{ localOnly },
- ).then(({ key, keyIndex }) => {
+ )
+ .then(({ key, keyIndex }) => {
logEvent(
`${target.name} set key (#${keyIndex}): ${toHex(key).slice(0, 16)}...${localOnly ? ' [LOCAL ONLY]' : ''}`,
'key-set',
);
// Pure state update
setParticipants((prev) =>
prev.map((p) =>
p.userId === targetUserId ? { ...p, currentKey: key, keyIndex } : p,
),
);
+ })
+ .catch((err) => {
+ logEvent(`${target.name} failed to set key: ${err}`, 'error');
});
- });
},
[logEvent, sendKey],
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample-apps/react/e2ee-demo/src/hooks/useE2EEDemo.ts` around lines 230 - 258,
The async call to setE2EEKeyFromInput inside setKeyFromInput can reject but has
no error handling; update setKeyFromInput to add a .catch handler (or use
try/catch if converting to async) for the Promise returned by
setE2EEKeyFromInput(target.e2eeManager, ...), and in that handler log the error
via logEvent (or console/error logger), surface a user-visible error state for
the target (e.g., update participants state to reflect failure or set an error
message), and ensure any partial UI updates are not applied on failure so the UI
remains consistent.
When participant B joins, sendKey could not deliver A's key to B because B wasn't in participantsRef yet. Split the exchange: use sendKey for the outbound direction (A gets B's key) and set existing keys on B's e2eeManager directly.
…iliation When a track arrives before the participantJoined event, the receiver was never wired to the E2EE decryptor, leaving frames permanently encrypted. Store the RTCRtpReceiver with orphaned tracks and call decrypt() when the participant identity becomes known during reconciliation in the participant event handlers. Also adds a comprehensive test suite for EncryptionManager.
…porting - Derive IV prefix from SHA-256(rawKey + userId) to prevent IV reuse across users with shared keys and across key rotations - Add version byte to frame trailer (v1) for forward compatibility - Add perf-report toggle: worker reports encode/decode FPS when enabled - Expose setPerfReport() and onPerfReport callback on EncryptionManager Also updates the E2EE demo app: - Use ?environment=pronto instead of hardcoded mmhfdzb5evj2 API key - Add E2EE on/off toggle in header - Show perf metrics in the event log - Fix stale closure in addParticipant
Runtime toggle: - Add EncryptionManager.setEnabled() to toggle E2EE without reconnecting - Worker e2eeActive flag gates encoder only; decoder always decrypts encrypted frames (valid trailer + keys) and passes plaintext through - Demo app: per-participant and global E2EE toggles with key revocation Performance optimizations: - Synchronous IV prefix lookup on the hot path (eliminates one await per frame — no more unnecessary Promise/microtask overhead) - Pre-compute shared key IV prefixes at transform creation time - Zero-copy AAD via subarray instead of slice - Single-pass RBSP escape (removed counting pass)
- Rename worker.ts → e2ee-worker.ts and worker/ → e2ee-worker/ - Export bundled worker as a function instead of a template literal string - EncryptionManager creates Worker from e2eeWorker.toString() - Remove unnecessary tsconfig exclusion for worker directory
…rospection - Add codec selector (VP8/VP9/H.264/AV1) in e2ee-demo header - Add shared key support with passphrase input and per-user key overrides - Auto-hide decryption error banner on E2EE disable or decryption resume - Add onDecryptionResumed callback to EncryptionManager - Fix lazy IV prefix recomputation in encode/decode transforms - Add requestKeyDump() for worker key state introspection - Export KeyStateReport type from client package
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
1 participant
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.
💡 Overview
Add end-to-end encryption for media tracks using a symmetric XOR transform applied to every encoded frame in a dedicated Web Worker.
📝 Implementation notes
packages/client/src/rtc/e2ee/index.tsmodule with a single shared Worker that handles both encode and decode transformscreateEncodedStreams) on Chrome whereRTCRtpScriptTransformis unreliableonrtctransformevent for the standard API,onmessagefor Insertable StreamsDataView+charCodeAt, following the webrtc/samples referenceBasePeerConnectionconditionally setsencodedInsertableStreams: trueon the PC config for ChromePublisher.addTransceiverattaches encryptor to sender,Subscriber.handleOnTrackattaches decryptor to receiverWeakSetguard prevents double-piping oncreateEncodedStreams(which can only be called once per sender/receiver)encryptionKeyoption added toClientPublishOptionsand wired through the dogfood app via?encryption_key=query param🎫 Ticket: https://linear.app/stream/issue/XYZ-123
Summary by CodeRabbit
New Features
Tests
Chores