Open
Conversation
… with queueJob for better timeout management
…urity and async handling
…improved performance and consistency
📝 WalkthroughWalkthroughThis pull request converts the codebase from synchronous Node crypto primitives to async WebCrypto APIs. It replaces synchronous functions ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
⚔️ Resolve merge conflicts
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/curve.js (1)
122-134:⚠️ Potential issue | 🟡 MinorAdd documentation explaining the
isInitparameter's security implications.The
isInit ? true : curveJs.verify(...)pattern at line 133 intentionally bypasses signature verification during session initialization (confirmed by git commit "remove unnecessary verify in session creation"). However, there is no comment explaining when this bypass is appropriate or why it's safe in the Signal protocol context. Add an inline comment documenting this behavior to prevent future maintainers from misinterpreting it as a security bug.Additionally, note that this function is declared
asyncbut returns a synchronous value—consider whether this aligns with the module's stated goal of maintaining an async interface for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/curve.js` around lines 122 - 134, The verifySignature function currently bypasses actual signature checking when isInit is true (returning isInit ? true : curveJs.verify(...)), which needs an inline comment documenting why bypassing verification is safe during session initialization and under what Signal-protocol conditions this is allowed; add an explicit comment next to the isInit branch referencing verifySignature, isInit, and curveJs.verify that states the intended behavior, security rationale, and any assumptions (e.g., this path is only used during initial session setup where verification is performed elsewhere). Also clarify the async signature: either remove the async keyword from verifySignature if it is always synchronous, or add a short inline comment explaining that the function is intentionally async to preserve an async interface contract across the module. Ensure the changes are local to the verifySignature function and its surrounding comments so maintainers can easily find and understand the rationale.
🧹 Nitpick comments (2)
src/session_cipher.js (1)
227-233: UseObject.prototype.hasOwnProperty.callhere.Calling
hasOwnPropertydirectly on a mutable plain object is what ESLint is flagging, and it can be shadowed by an own property. Use the prototype form instead.Small fix
- if (!chain.messageKeys.hasOwnProperty(message.counter)) { + if (!Object.prototype.hasOwnProperty.call(chain.messageKeys, message.counter)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session_cipher.js` around lines 227 - 233, Replace the direct hasOwnProperty call on the mutable plain object to avoid shadowing: in the decrypt path where you call maybeStepRatchet, getChain, and fillMessageKeys (look for methods maybeStepRatchet, getChain, fillMessageKeys and the variable chain.messageKeys in session_cipher.js), change the check "chain.messageKeys.hasOwnProperty(message.counter)" to use Object.prototype.hasOwnProperty.call(chain.messageKeys, message.counter) so the prototype method is used safely.src/session_record.js (1)
273-273: Remove the checked-in commented logs.If these messages still matter, route them through a debug logger or flag. Leaving disabled
console.*lines in the session lifecycle just adds dead code.Also applies to: 281-281, 301-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session_record.js` at line 273, Remove the checked-in commented console.* lines in src/session_record.js (the commented "console.info('Closing session:', session)" and the other commented console lines) — either delete them entirely or replace them with calls to the project's debug/logger API (e.g., logger.debug or processLogger.debug) inside the session lifecycle functions that manage closing/saving sessions so the messages are emitted only when debug logging is enabled; ensure no stray commented console.* statements remain in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/crypto.js`:
- Around line 85-96: The code in src/crypto.js exposes sensitive MAC material in
error strings when verifying MACs; update the checks around calculateMAC, mac,
and calculatedMac to throw generic errors without including MAC hex values or
key-derived data—e.g. replace the detailed messages in the length check and
verification failure with non-sensitive messages like "Bad MAC length" and "Bad
MAC" (or a single generic "Bad MAC") and ensure no code path logs mac or
calculatedMac; keep the validation logic (calculateMAC(...).subarray(...), the
length comparison, and the constant-time XOR loop) unchanged but remove any
inclusion of mac or calculatedMac in thrown errors or logs.
In `@src/curve.js`:
- Around line 10-12: Remove the unused constant PUBLIC_KEY_DER_PREFIX from
src/curve.js: locate the declaration PUBLIC_KEY_DER_PREFIX and delete it (and
any related comments or references if present) so the file no longer contains an
unused Buffer constant leftover from the WebCrypto migration.
In `@src/keyhelper.js`:
- Line 17: The change made generateSignedPreKey and generatePreKey to be async
(returning Promises) which breaks the public API; restore backward compatibility
by reverting them to synchronous exports: remove the async keyword and ensure
both generateSignedPreKey and generatePreKey return plain objects with the same
properties (e.g., .keyPair and .signature) instead of a Promise, or
alternatively add new async-named variants (e.g., generateSignedPreKeyAsync /
generatePreKeyAsync) and keep the original function names synchronous; update
the module exports so callers of generateSignedPreKey and generatePreKey
continue to receive plain objects (also apply the same fix to the other
occurrence at the second function on line 36).
In `@src/queue_job.js`:
- Around line 21-37: withTimeout currently rejects immediately on timeout which
lets _asyncQueueExecutor advance while the original job.awaitable (the
underlying fn) is still running; change withTimeout so that when the timeout
fires it records that the call has timed out (e.g., create a TimeoutError or set
a timedOut flag) but does not resolve/reject the returned Promise until the
underlying fn() settles, then reject with the TimeoutError; ensure you still
clear the timer and clean up on fn() resolution/rejection and preserve the
original behavior when fn() finishes before ms expires—this keeps
_asyncQueueExecutor and job.awaitable serialization intact while providing a
clear timeout signal to callers.
---
Outside diff comments:
In `@src/curve.js`:
- Around line 122-134: The verifySignature function currently bypasses actual
signature checking when isInit is true (returning isInit ? true :
curveJs.verify(...)), which needs an inline comment documenting why bypassing
verification is safe during session initialization and under what
Signal-protocol conditions this is allowed; add an explicit comment next to the
isInit branch referencing verifySignature, isInit, and curveJs.verify that
states the intended behavior, security rationale, and any assumptions (e.g.,
this path is only used during initial session setup where verification is
performed elsewhere). Also clarify the async signature: either remove the async
keyword from verifySignature if it is always synchronous, or add a short inline
comment explaining that the function is intentionally async to preserve an async
interface contract across the module. Ensure the changes are local to the
verifySignature function and its surrounding comments so maintainers can easily
find and understand the rationale.
---
Nitpick comments:
In `@src/session_cipher.js`:
- Around line 227-233: Replace the direct hasOwnProperty call on the mutable
plain object to avoid shadowing: in the decrypt path where you call
maybeStepRatchet, getChain, and fillMessageKeys (look for methods
maybeStepRatchet, getChain, fillMessageKeys and the variable chain.messageKeys
in session_cipher.js), change the check
"chain.messageKeys.hasOwnProperty(message.counter)" to use
Object.prototype.hasOwnProperty.call(chain.messageKeys, message.counter) so the
prototype method is used safely.
In `@src/session_record.js`:
- Line 273: Remove the checked-in commented console.* lines in
src/session_record.js (the commented "console.info('Closing session:', session)"
and the other commented console lines) — either delete them entirely or replace
them with calls to the project's debug/logger API (e.g., logger.debug or
processLogger.debug) inside the session lifecycle functions that manage
closing/saving sessions so the messages are emitted only when debug logging is
enabled; ensure no stray commented console.* statements remain in the file.
🪄 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: 32a518a9-8bab-41a2-ae30-7838a6fa2747
📒 Files selected for processing (9)
package.jsonsrc/crypto.jssrc/curve.jssrc/keyhelper.jssrc/numeric_fingerprint.jssrc/queue_job.jssrc/session_builder.jssrc/session_cipher.jssrc/session_record.js
Comment on lines
+85
to
+96
| const calculatedMac = (await calculateMAC(key, data)).subarray(0, length); | ||
| if (mac.length !== length || calculatedMac.length !== length) { | ||
| throw new Error("Bad MAC length"); | ||
| throw new Error("Bad MAC length Expected: " + length + | ||
| " Got: " + mac.length + " and " + calculatedMac.length); | ||
| } | ||
| let diff = 0; | ||
| for (let i = 0; i < length; i++) { | ||
| diff |= mac[i] ^ calculatedMac[i]; | ||
| } | ||
| if (!mac.equals(calculatedMac)) { | ||
| throw new Error("Bad MAC"); | ||
| if (diff !== 0) { | ||
| throw new Error("Bad MAC Expected: " + calculatedMac.toString('hex') + | ||
| " Got: " + mac.toString('hex')); |
There was a problem hiding this comment.
Do not leak MAC material in error strings.
src/session_cipher.js:157-160 logs decryption errors, so these messages now write the expected MAC and received MAC to logs on every verification failure. Keep MAC failures generic; the derived MAC is sensitive key-derived material.
Safer error handling
- throw new Error("Bad MAC length Expected: " + length +
- " Got: " + mac.length + " and " + calculatedMac.length);
+ throw new Error('Bad MAC length');
...
- throw new Error("Bad MAC Expected: " + calculatedMac.toString('hex') +
- " Got: " + mac.toString('hex'));
+ throw new Error('Bad MAC');📝 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 calculatedMac = (await calculateMAC(key, data)).subarray(0, length); | |
| if (mac.length !== length || calculatedMac.length !== length) { | |
| throw new Error("Bad MAC length"); | |
| throw new Error("Bad MAC length Expected: " + length + | |
| " Got: " + mac.length + " and " + calculatedMac.length); | |
| } | |
| let diff = 0; | |
| for (let i = 0; i < length; i++) { | |
| diff |= mac[i] ^ calculatedMac[i]; | |
| } | |
| if (!mac.equals(calculatedMac)) { | |
| throw new Error("Bad MAC"); | |
| if (diff !== 0) { | |
| throw new Error("Bad MAC Expected: " + calculatedMac.toString('hex') + | |
| " Got: " + mac.toString('hex')); | |
| const calculatedMac = (await calculateMAC(key, data)).subarray(0, length); | |
| if (mac.length !== length || calculatedMac.length !== length) { | |
| throw new Error('Bad MAC length'); | |
| } | |
| let diff = 0; | |
| for (let i = 0; i < length; i++) { | |
| diff |= mac[i] ^ calculatedMac[i]; | |
| } | |
| if (diff !== 0) { | |
| throw new Error('Bad MAC'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/crypto.js` around lines 85 - 96, The code in src/crypto.js exposes
sensitive MAC material in error strings when verifying MACs; update the checks
around calculateMAC, mac, and calculatedMac to throw generic errors without
including MAC hex values or key-derived data—e.g. replace the detailed messages
in the length check and verification failure with non-sensitive messages like
"Bad MAC length" and "Bad MAC" (or a single generic "Bad MAC") and ensure no
code path logs mac or calculatedMac; keep the validation logic
(calculateMAC(...).subarray(...), the length comparison, and the constant-time
XOR loop) unchanged but remove any inclusion of mac or calculatedMac in thrown
errors or logs.
Comment on lines
10
to
12
| const PUBLIC_KEY_DER_PREFIX = Buffer.from([ | ||
| 48, 42, 48, 5, 6, 3, 43, 101, 110, 3, 33, 0 | ||
| ]); |
There was a problem hiding this comment.
Unused constant PUBLIC_KEY_DER_PREFIX.
This constant is defined but never used in the file. It may be leftover from the migration to WebCrypto. Consider removing it to avoid confusion.
🧹 Proposed fix
-// DER prefixes for X25519 keys (used for WebCrypto import/export)
+// DER prefix for X25519 private keys (used for WebCrypto import/export)
// from: https://github.com/digitalbazaar/x25519-key-agreement-key-2019/blob/master/lib/crypto.js
-const PUBLIC_KEY_DER_PREFIX = Buffer.from([
- 48, 42, 48, 5, 6, 3, 43, 101, 110, 3, 33, 0
-]);
-
const PRIVATE_KEY_DER_PREFIX = Buffer.from([📝 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 PUBLIC_KEY_DER_PREFIX = Buffer.from([ | |
| 48, 42, 48, 5, 6, 3, 43, 101, 110, 3, 33, 0 | |
| ]); | |
| // DER prefix for X25519 private keys (used for WebCrypto import/export) | |
| // from: https://github.com/digitalbazaar/x25519-key-agreement-key-2019/blob/master/lib/crypto.js | |
| const PRIVATE_KEY_DER_PREFIX = Buffer.from([ |
🧰 Tools
🪛 ESLint
[error] 10-10: 'PUBLIC_KEY_DER_PREFIX' is assigned a value but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/curve.js` around lines 10 - 12, Remove the unused constant
PUBLIC_KEY_DER_PREFIX from src/curve.js: locate the declaration
PUBLIC_KEY_DER_PREFIX and delete it (and any related comments or references if
present) so the file no longer contains an unused Buffer constant leftover from
the WebCrypto migration.
| }; | ||
|
|
||
| exports.generateSignedPreKey = function(identityKeyPair, signedKeyId) { | ||
| exports.generateSignedPreKey = async function(identityKeyPair, signedKeyId) { |
There was a problem hiding this comment.
This is a breaking public API change.
generateSignedPreKey() and generatePreKey() now return Promises instead of plain objects. Existing consumers that read .keyPair / .signature synchronously will break at runtime, so this needs either separate async exports or a semver-major release with docs/examples updated.
Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/keyhelper.js` at line 17, The change made generateSignedPreKey and
generatePreKey to be async (returning Promises) which breaks the public API;
restore backward compatibility by reverting them to synchronous exports: remove
the async keyword and ensure both generateSignedPreKey and generatePreKey return
plain objects with the same properties (e.g., .keyPair and .signature) instead
of a Promise, or alternatively add new async-named variants (e.g.,
generateSignedPreKeyAsync / generatePreKeyAsync) and keep the original function
names synchronous; update the module exports so callers of generateSignedPreKey
and generatePreKey continue to receive plain objects (also apply the same fix to
the other occurrence at the second function on line 36).
Comment on lines
+21
to
+37
| function withTimeout(fn, ms = 15000) { | ||
| if (typeof fn !== 'function') { | ||
| throw new TypeError('fn must be a function to wrap received ' + typeof fn); | ||
| } | ||
| if (typeof ms !== 'number') { | ||
| throw new TypeError('ms must be a number to wrap received ' + typeof ms); | ||
| } | ||
| return new Promise((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error('Job timed out')), ms); | ||
| fn().then((res) => { | ||
| clearTimeout(timer); | ||
| resolve(res); | ||
| }).catch((err) => { | ||
| clearTimeout(timer); | ||
| reject(err); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Preserve queue ordering even after a timeout.
Once withTimeout() rejects, _asyncQueueExecutor() immediately advances to the next item even though the original job.awaitable keeps running. That breaks the serialization guarantee this module is supposed to provide: jobs from src/session_builder.js:22-52 and src/session_cipher.js:68-186 can now overlap and mutate the same session/storage state concurrently.
Minimal direction
- job.resolve(await withTimeout(job.awaitable)); // if the job takes longer than 15 seconds, it will be rejected
+ const work = Promise.resolve().then(job.awaitable);
+ try {
+ job.resolve(await withTimeout(() => work));
+ } catch (e) {
+ job.reject(e);
+ } finally {
+ try { await work; } catch (_) {}
+ }Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/queue_job.js` around lines 21 - 37, withTimeout currently rejects
immediately on timeout which lets _asyncQueueExecutor advance while the original
job.awaitable (the underlying fn) is still running; change withTimeout so that
when the timeout fires it records that the call has timed out (e.g., create a
TimeoutError or set a timedOut flag) but does not resolve/reject the returned
Promise until the underlying fn() settles, then reject with the TimeoutError;
ensure you still clear the timer and clean up on fn() resolution/rejection and
preserve the original behavior when fn() finishes before ms expires—this keeps
_asyncQueueExecutor and job.awaitable serialization intact while providing a
clear timeout signal to callers.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/queue_job.js">
<violation number="1" location="src/queue_job.js:19">
P2: Documentation says "The default timeout is 30 seconds" but the actual default is 15 seconds (`ms = 15000`). This will mislead callers about the timeout behavior.</violation>
<violation number="2" location="src/queue_job.js:47">
P1: When `withTimeout` rejects on a timeout, `fn()` keeps running in the background while the executor advances to the next queued job. This breaks the serialization guarantee that the queue is supposed to provide—two jobs can now mutate session/storage state concurrently. After a timeout rejection, the executor should still `await` the original work promise (ignoring its result) before proceeding to the next job.</violation>
</file>
<file name="src/crypto.js">
<violation number="1" location="src/crypto.js:87">
P2: Similarly, the MAC length error now includes the expected and actual lengths. Keep MAC verification errors generic to avoid leaking any information about the verification process.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| const job = queue[i]; | ||
| try { | ||
| job.resolve(await job.awaitable()); | ||
| job.resolve(await withTimeout(job.awaitable)); // if the job takes longer than 15 seconds, it will be rejected |
There was a problem hiding this comment.
P1: When withTimeout rejects on a timeout, fn() keeps running in the background while the executor advances to the next queued job. This breaks the serialization guarantee that the queue is supposed to provide—two jobs can now mutate session/storage state concurrently. After a timeout rejection, the executor should still await the original work promise (ignoring its result) before proceeding to the next job.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/queue_job.js, line 47:
<comment>When `withTimeout` rejects on a timeout, `fn()` keeps running in the background while the executor advances to the next queued job. This breaks the serialization guarantee that the queue is supposed to provide—two jobs can now mutate session/storage state concurrently. After a timeout rejection, the executor should still `await` the original work promise (ignoring its result) before proceeding to the next job.</comment>
<file context>
@@ -10,14 +10,41 @@
const job = queue[i];
try {
- job.resolve(await job.awaitable());
+ job.resolve(await withTimeout(job.awaitable)); // if the job takes longer than 15 seconds, it will be rejected
} catch(e) {
job.reject(e);
</file context>
| * This is a wrapper around the async function that will reject if it | ||
| * takes longer than the specified timeout. This is useful for | ||
| * preventing a job from hanging indefinitely. The default timeout | ||
| * is 30 seconds. |
There was a problem hiding this comment.
P2: Documentation says "The default timeout is 30 seconds" but the actual default is 15 seconds (ms = 15000). This will mislead callers about the timeout behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/queue_job.js, line 19:
<comment>Documentation says "The default timeout is 30 seconds" but the actual default is 15 seconds (`ms = 15000`). This will mislead callers about the timeout behavior.</comment>
<file context>
@@ -10,14 +10,41 @@
+* This is a wrapper around the async function that will reject if it
+* takes longer than the specified timeout. This is useful for
+* preventing a job from hanging indefinitely. The default timeout
+* is 30 seconds.
+*/
+function withTimeout(fn, ms = 15000) {
</file context>
Suggested change
| * is 30 seconds. | |
| * is 15 seconds. |
| const calculatedMac = (await calculateMAC(key, data)).subarray(0, length); | ||
| if (mac.length !== length || calculatedMac.length !== length) { | ||
| throw new Error("Bad MAC length"); | ||
| throw new Error("Bad MAC length Expected: " + length + |
There was a problem hiding this comment.
P2: Similarly, the MAC length error now includes the expected and actual lengths. Keep MAC verification errors generic to avoid leaking any information about the verification process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crypto.js, line 87:
<comment>Similarly, the MAC length error now includes the expected and actual lengths. Keep MAC verification errors generic to avoid leaking any information about the verification process.</comment>
<file context>
@@ -60,31 +63,37 @@ function deriveSecrets(input, salt, info, chunks) {
+ const calculatedMac = (await calculateMAC(key, data)).subarray(0, length);
if (mac.length !== length || calculatedMac.length !== length) {
- throw new Error("Bad MAC length");
+ throw new Error("Bad MAC length Expected: " + length +
+ " Got: " + mac.length + " and " + calculatedMac.length);
+ }
</file context>
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.
Summary by CodeRabbit
New Features
Improvements