feat: add Serpent256 and XChaCha20 encryption#271
Merged
Conversation
- Use Uint8Array instead of ArrayBuffer - Avoid memory allocations - Avoid copying - Use synchronous API - Use efficient XOR with no allocations
📝 WalkthroughWalkthroughThis PR significantly expands the encryption infrastructure by introducing multiple new cipher implementations (Twofish via WASM, Serpent via leviathan-crypto, XChaCha20 via libsodium), refactoring existing AES from GCM to CTR mode, restructuring key derivation to use HKDF, replacing CRC32 integrity checks with HMAC-SHA256, and shifting from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/core/encryption/ciphers/Twofish/wasm/twofish.test.ts (2)
52-53: Clarify the duplicateencryptcall.
cipher.encrypt(originalData)is called twice; the first result is discarded. If this is an intentional warmup (e.g., to trigger JIT compilation or IV increment), add a comment. Otherwise, remove the redundant call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/ciphers/Twofish/wasm/twofish.test.ts` around lines 52 - 53, The test contains a redundant call to cipher.encrypt(originalData) whose result is discarded; either remove the first call or explicitly document it as a deliberate warm-up. Update the test around cipher.encrypt and the ct assignment so only the meaningful encryption is performed (keep const ct = await cipher.encrypt(originalData)), or if the first call is necessary for JIT/initialization or to advance an IV/nonce, add a clear comment above that first call explaining the reason and ensure test determinism (e.g., reset IV/nonce or explain side effects in the comment).
20-30: Module-level benchmark code pollutes test output.Lines 20-30 execute unconditionally at import time, logging timing data to console and running 65K encrypt iterations. This adds noise to test runs and slows down every test file that imports from this module indirectly.
Consider moving the benchmark into a skipped or dedicated performance test.
♻️ Move benchmark into a dedicated test
-console.time('The loop'); -const plaintext = new Uint8Array([ - 0xd4, 0x91, 0xdb, 0x16, 0xe7, 0xb1, 0xc3, 0x9e, 0x86, 0xcb, 0x08, 0x6b, 0x78, 0x9f, - 0x54, 0x19, -]); -for (let i = 0; i < (1024 * 1024) / 16; i++) { - tf.encrypt(session, plaintext); -} -console.timeEnd('The loop'); - -tf.destroySession(session); +test.skip('Benchmark: encrypt 1 MiB', () => { + const plaintext = new Uint8Array([ + 0xd4, 0x91, 0xdb, 0x16, 0xe7, 0xb1, 0xc3, 0x9e, 0x86, 0xcb, 0x08, 0x6b, 0x78, 0x9f, + 0x54, 0x19, + ]); + console.time('The loop'); + for (let i = 0; i < (1024 * 1024) / 16; i++) { + tf.encrypt(session, plaintext); + } + console.timeEnd('The loop'); + tf.destroySession(session); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/ciphers/Twofish/wasm/twofish.test.ts` around lines 20 - 30, Remove the module-level benchmark that runs tf.encrypt(session, plaintext) in a loop and logs with console.time/console.timeEnd; this code is currently executed on import and should be relocated into a dedicated performance test (or a skipped test) so it doesn't run during normal unit tests. Specifically, delete the import-time loop and timing calls around the plaintext/for-loop/encrypt/destroySession sequence and recreate them inside a test harness (e.g., a test named "twofish performance" or a file for benchmarks) that explicitly sets up session, runs the loop calling tf.encrypt(session, plaintext), logs or asserts results, and calls tf.destroySession(session); alternatively mark that test as skipped so CI unit runs are not affected.src/core/encryption/ciphers/Twofish/wasm/string.h (1)
8-8: Consider using__SIZE_TYPE__for portability.On wasm32,
unsigned longhappens to be 32-bit, but using the compiler-provided__SIZE_TYPE__would be more explicit for a freestanding environment.♻️ Optional: use compiler intrinsic
-typedef unsigned long size_t; +typedef __SIZE_TYPE__ size_t;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/ciphers/Twofish/wasm/string.h` at line 8, Replace the hard-coded typedef for size_t in string.h (the line typedef unsigned long size_t;) with the compiler intrinsic type to ensure portability in freestanding/wasm32 builds: use the compiler-provided __SIZE_TYPE__ for the typedef (i.e., change the typedef to use __SIZE_TYPE__), so code that relies on size_t (in this header) matches the platform's real size type.src/core/encryption/ciphers/Twofish/wasm/README.md (1)
3-4: Consider tracking the TODO as an issue.The PR objectives mention benchmarking and testing against reference vectors. This TODO aligns with that goal but may be forgotten if left only as a comment.
Would you like me to open an issue to track adding tests for the WASM module against reference vectors?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/ciphers/Twofish/wasm/README.md` around lines 3 - 4, The README TODO about adding tests for the Twofish WASM module should be tracked as an issue: create a tracker issue titled e.g. "Add WASM Twofish tests vs reference vectors", include scope (WASM module in src/core/encryption/ciphers/Twofish/wasm), acceptance criteria (test harness, reference vector sets, CI run), a short test plan and benchmark steps, link the PR, assign or request review from maintainers, and add labels like "tests" and "benchmark" so the work isn't lost.src/core/encryption/cipherModes/CTRMode.test.ts (1)
66-68: Remove dead commented code.This duplicate definition of
makeXorEncryptis already defined at lines 7-10 and should be removed.🧹 Proposed fix
-// const makeXorEncrypt = (key: number) => -// async (buffer: Uint8Array): Promise<Uint8Array> => -// new Uint8Array(buffer.map((b) => b ^ key)); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/cipherModes/CTRMode.test.ts` around lines 66 - 68, Remove the dead commented duplicate definition of makeXorEncrypt in the test file: delete the commented block starting with "// const makeXorEncrypt = (key: number) =>" (the duplicate at lines 66-68) because the function is already defined earlier (makeXorEncrypt at lines 7-10); simply eliminate the commented lines so only the single active definition remains and run the tests to confirm no references break.src/core/encryption/ciphers/Twofish/wasm/twofish.ts (1)
328-332: Minor:URL.pathnamemay not work correctly on Windows.For
file://URLs on Windows,URL.pathnameincludes a leading slash (e.g.,/C:/path/to/file), which may cause issues withfs.readFile. Consider usingfileURLToPathfromnode:urlfor proper cross-platform handling.🔧 Proposed fix for cross-platform URL handling
+ const { fileURLToPath } = await import('node:url'); const fs = await import('fs/promises'); - const buf = await fs.readFile(source instanceof URL ? source.pathname : source); + const path = source instanceof URL ? fileURLToPath(source) : source; + const buf = await fs.readFile(path); return buf.buffer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/ciphers/Twofish/wasm/twofish.ts` around lines 328 - 332, The file reader uses URL.pathname which breaks on Windows; update the Node.js fallback in the wasm loader to convert file URLs correctly by importing fileURLToPath from 'node:url' and using fileURLToPath(source) when source is a URL before passing to fs.readFile (keep the existing fs/promises import and buf.buffer return). Target the block that currently checks "source instanceof URL" (the Node.js fallback in twofish wasm loader) and replace the pathname usage with fileURLToPath conversion for cross-platform correctness.
🤖 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/core/encryption/cipherModes/CTRCipherMode.ts`:
- Line 33: Remove the debug timing statements from the CTRCipherMode
implementation: locate the console.time/console.timeEnd calls inside the
CTRCipherMode class (inside the encrypt loop and its counterpart) and delete
them so no console timing is emitted in production; ensure no other stray
console.time or console.timeEnd calls remain in methods like encrypt (and
related loop helpers) before merging.
In `@src/core/encryption/ciphers/Twofish/index.ts`:
- Around line 137-138: The code returns a view into WASM memory
(decryptedBufferWithPaddings.subarray(0, endOfDataOffset).buffer), which can be
overwritten by later WASM operations; update the Twofish decryption return to
produce an independent copy of the decrypted bytes (use a slice/copy of the
subarray into a new ArrayBuffer) so callers like EncryptionController.decrypt
receive a buffer not backed by WASM linear memory and cannot be corrupted by
subsequent operations.
- Line 73: The class was renamed to WasmTwofishCTRCipher which breaks imports
expecting TwofishCTRCipher; add a backward-compatible alias export by exporting
WasmTwofishCTRCipher under the old name TwofishCTRCipher (i.e., append an export
that re-exports WasmTwofishCTRCipher as TwofishCTRCipher) so existing imports in
tests and Encryption.worker continue to work.
In `@src/core/encryption/ciphers/Twofish/wasm/twofish.test.ts`:
- Line 10: The test imports the WASM binary at module load (TwofishModule.load
reading twofish.wasm) but CI doesn't build the .wasm, causing failures; update
the CI test workflow to produce twofish.wasm before running tests by invoking
the existing build script (build.sh) or running the Docker builder (docker
compose run --rm builder using the compose.yml) so the binary exists for
TwofishModule.load, or alternatively pre-build and commit twofish.wasm to the
repo.
---
Nitpick comments:
In `@src/core/encryption/cipherModes/CTRMode.test.ts`:
- Around line 66-68: Remove the dead commented duplicate definition of
makeXorEncrypt in the test file: delete the commented block starting with "//
const makeXorEncrypt = (key: number) =>" (the duplicate at lines 66-68) because
the function is already defined earlier (makeXorEncrypt at lines 7-10); simply
eliminate the commented lines so only the single active definition remains and
run the tests to confirm no references break.
In `@src/core/encryption/ciphers/Twofish/wasm/README.md`:
- Around line 3-4: The README TODO about adding tests for the Twofish WASM
module should be tracked as an issue: create a tracker issue titled e.g. "Add
WASM Twofish tests vs reference vectors", include scope (WASM module in
src/core/encryption/ciphers/Twofish/wasm), acceptance criteria (test harness,
reference vector sets, CI run), a short test plan and benchmark steps, link the
PR, assign or request review from maintainers, and add labels like "tests" and
"benchmark" so the work isn't lost.
In `@src/core/encryption/ciphers/Twofish/wasm/string.h`:
- Line 8: Replace the hard-coded typedef for size_t in string.h (the line
typedef unsigned long size_t;) with the compiler intrinsic type to ensure
portability in freestanding/wasm32 builds: use the compiler-provided
__SIZE_TYPE__ for the typedef (i.e., change the typedef to use __SIZE_TYPE__),
so code that relies on size_t (in this header) matches the platform's real size
type.
In `@src/core/encryption/ciphers/Twofish/wasm/twofish.test.ts`:
- Around line 52-53: The test contains a redundant call to
cipher.encrypt(originalData) whose result is discarded; either remove the first
call or explicitly document it as a deliberate warm-up. Update the test around
cipher.encrypt and the ct assignment so only the meaningful encryption is
performed (keep const ct = await cipher.encrypt(originalData)), or if the first
call is necessary for JIT/initialization or to advance an IV/nonce, add a clear
comment above that first call explaining the reason and ensure test determinism
(e.g., reset IV/nonce or explain side effects in the comment).
- Around line 20-30: Remove the module-level benchmark that runs
tf.encrypt(session, plaintext) in a loop and logs with
console.time/console.timeEnd; this code is currently executed on import and
should be relocated into a dedicated performance test (or a skipped test) so it
doesn't run during normal unit tests. Specifically, delete the import-time loop
and timing calls around the plaintext/for-loop/encrypt/destroySession sequence
and recreate them inside a test harness (e.g., a test named "twofish
performance" or a file for benchmarks) that explicitly sets up session, runs the
loop calling tf.encrypt(session, plaintext), logs or asserts results, and calls
tf.destroySession(session); alternatively mark that test as skipped so CI unit
runs are not affected.
In `@src/core/encryption/ciphers/Twofish/wasm/twofish.ts`:
- Around line 328-332: The file reader uses URL.pathname which breaks on
Windows; update the Node.js fallback in the wasm loader to convert file URLs
correctly by importing fileURLToPath from 'node:url' and using
fileURLToPath(source) when source is a URL before passing to fs.readFile (keep
the existing fs/promises import and buf.buffer return). Target the block that
currently checks "source instanceof URL" (the Node.js fallback in twofish wasm
loader) and replace the pathname usage with fileURLToPath conversion for
cross-platform correctness.
🪄 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: 8db592ac-e368-47fb-8419-256c54f64f24
📒 Files selected for processing (16)
src/core/encryption/cipherModes/CTRCipherMode.tssrc/core/encryption/cipherModes/CTRMode.test.tssrc/core/encryption/ciphers/Twofish/index.tssrc/core/encryption/ciphers/Twofish/wasm/Dockerfilesrc/core/encryption/ciphers/Twofish/wasm/README.mdsrc/core/encryption/ciphers/Twofish/wasm/build.shsrc/core/encryption/ciphers/Twofish/wasm/compose.ymlsrc/core/encryption/ciphers/Twofish/wasm/main.csrc/core/encryption/ciphers/Twofish/wasm/string.hsrc/core/encryption/ciphers/Twofish/wasm/twofish.csrc/core/encryption/ciphers/Twofish/wasm/twofish.hsrc/core/encryption/ciphers/Twofish/wasm/twofish.test.tssrc/core/encryption/ciphers/Twofish/wasm/twofish.tssrc/core/encryption/ciphers/Twofish/wasm/twofish_wasm.csrc/core/encryption/utils/xor.tswords.txt
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (8)
src/features/App/useProfilesList.ts (1)
60-60: Remove debug logging before merge.This
console.logappears to be development debugging. Consider removing it or converting to a proper logging/telemetry call if the information is needed in production.-console.log('Create profile with encryption', profile.algorithm);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useProfilesList.ts` at line 60, Remove the development console.log statement that prints "Create profile with encryption" and profile.algorithm in useProfilesList.ts; either delete the line entirely or replace it with a proper logger/telemetry call (e.g., processLogger.debug or emitTelemetryEvent) if this information is required in production, ensuring you reference the existing message string "Create profile with encryption" and the profile.algorithm usage when making the change.src/features/App/WorkspaceManager/ProfileCreator/index.tsx (1)
26-30: Consider strengthening type safety for algorithm selection.The type cast
as ENCRYPTION_ALGORITHMworks because options come from a controlled source, but theNewProfiletype still declaresalgorithm: string. For consistency and type safety:+import { ENCRYPTION_ALGORITHM } from '@core/features/encryption'; + export type NewProfile = { name: string; password: string | null; - algorithm: string; + algorithm: ENCRYPTION_ALGORITHM; };This would propagate type safety to consumers like
onCreateProfile.Also applies to: 236-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/WorkspaceManager/ProfileCreator/index.tsx` around lines 26 - 30, The NewProfile type currently declares algorithm as string; change it to algorithm: ENCRYPTION_ALGORITHM and update any related signatures (e.g., the onCreateProfile prop and any handlers in ProfileCreator) so they accept ENCRYPTION_ALGORITHM instead of string; replace casts like `as ENCRYPTION_ALGORITHM` by relying on the typed value, import ENCRYPTION_ALGORITHM where needed, and update any consumers or tests that construct NewProfile to use the enum/union type to ensure end-to-end type safety.vitest.config.mts (1)
19-22: Simplify WASM URL export by avoiding URL object construction.
JSON.stringify(new URL(...))serializes to the URL'shrefstring. SinceTwofishModule.load()acceptsstring | URL | BufferSource, you can simplify this:// Return `file://` urls on `.wasm` files if (id.endsWith('.wasm')) { - return `export default ${JSON.stringify(new URL(resolve(id), import.meta.url))};`; + return `export default ${JSON.stringify('file://' + resolve(id))};`; }This makes the intent clearer and avoids the indirect URL-to-string serialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.mts` around lines 19 - 22, The current wasm loader returns a URL by serializing a URL object (JSON.stringify(new URL(resolve(id), import.meta.url))); change it to return a plain string URL so intent is clearer and avoids indirect URL-to-string serialization: inside the id.endsWith('.wasm') branch produce an export default whose value is the string URL (e.g. the .href) rather than JSON.stringify(new URL(...)) so TwofishModule.load() can accept the string; update the return in that branch (referencing id and resolve(id)) accordingly.src/core/encryption/ciphers/Twofish/twofish.wasm.test.ts (1)
13-18: Exercise the explicitload()path in this test.
Encryption.worker.tsnow awaitscipher.load()before the cipher is used. This test skips that path, so an eager-init regression can slip through while the round-trip still passes.♻️ Suggested change
const key = getRandomBytes(32); const cipher1 = new WasmTwofishCTRCipher(key, getRandomBytes); +await cipher1.load(); const ct = await cipher1.encrypt(pt.slice().buffer); const cipher2 = new WasmTwofishCTRCipher(key, getRandomBytes); +await cipher2.load(); await expect(cipher2.decrypt(ct)).resolves.toStrictEqual(pt.buffer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/ciphers/Twofish/twofish.wasm.test.ts` around lines 13 - 18, The test currently constructs two WasmTwofishCTRCipher instances and uses encrypt/decrypt without calling their explicit load() path; update the test to call and await cipher1.load() before calling cipher1.encrypt(...) and await cipher2.load() before calling cipher2.decrypt(...), ensuring the explicit WasmTwofishCTRCipher.load() initialization path is exercised for both cipher1 and cipher2 while keeping the same key, encrypt, and decrypt assertions.src/core/encryption/utils/random.ts (1)
18-22: Fill the destination slice directly in the large-buffer path.The intermediate
blockBufferjust adds an allocation and a copy per chunk. Sincebufferis already aUint8Array, you can fill the target view in place.♻️ Suggested simplification
- const blockBuffer = new Uint8Array(bytesToAdd); - self.crypto.getRandomValues(blockBuffer); - - // Fill with offset - buffer.set(blockBuffer, offset); + self.crypto.getRandomValues( + buffer.subarray(offset, offset + bytesToAdd), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/utils/random.ts` around lines 18 - 22, The code currently allocates an intermediate blockBuffer and copies it into buffer; instead, call self.crypto.getRandomValues directly on a view of the destination to avoid the allocation and copy. Replace creation/usage of blockBuffer with: const target = buffer.subarray(offset, offset + bytesToAdd); self.crypto.getRandomValues(target); (referencing buffer, offset, bytesToAdd and removing blockBuffer and buffer.set).packages/twofish/src/twofish.ts (3)
71-74: Remove debug logging before merging.This
console.logappears to be leftover debug output and should be removed from production code.🧹 Remove debug logging
- console.log( - 'Path to WASM is', - new URL('./twofish.wasm', import.meta.url).toString(), - );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/twofish/src/twofish.ts` around lines 71 - 74, Remove the leftover debug console.log in packages/twofish/src/twofish.ts that prints the WASM path (the console.log call that logs new URL('./twofish.wasm', import.meta.url).toString()); delete that statement so the module no longer emits debug output in production.
289-297: Storing WASM view inprevis fragile—consider.slice().Line 296 assigns the WASM memory view directly to
prev. While this is currently safe (the XOR loop at line 292 completes before the nextencrypt()call overwrites the buffer), it creates a subtle dependency on execution order that could break during refactoring.For consistency with line 287 and line 291 (both use
.slice()), consider copyingctas well.🛡️ Defensive copy for maintainability
const ct = this.encrypt(handle, block); out.set(ct, i); - prev = ct as Uint8Array<ArrayBuffer>; + prev = ct.slice();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/twofish/src/twofish.ts` around lines 289 - 297, The code stores the WASM-backed view returned from this.encrypt into prev (in the decrypt/CTR loop) which is fragile; change the assignment so prev receives a defensive copy (e.g., use .slice()) of ct instead of the raw view to avoid future overwrites—update the block inside the loop where const ct = this.encrypt(handle, block); out.set(ct, i); prev = ct as Uint8Array<ArrayBuffer>; to copy ct into prev (mirroring the .slice() usage used for block earlier).
46-52: CachedmemViewcan become detached if WASM memory grows.If the WASM module ever grows its linear memory (via
memory.grow()), the underlyingArrayBufferis detached andthis.memViewbecomes invalid. The code is also inconsistent—encrypt()usesthis.memView(line 186) whilecreateSession()anddecrypt()create fresh views.Consider either:
- Always create a fresh view when accessing memory, or
- Document that this module never grows memory and ensure the WASM side doesn't call
memory.grow()♻️ Suggested approach: always use fresh view
private readonly mem: WebAssembly.Memory; private readonly ioOffset: number; // byte offset of io_buffer in WASM memory - private readonly memView; private constructor(exports: TwofishWasmExports) { this.exports = exports; this.mem = exports.memory; this.ioOffset = exports.twofish_get_io_buffer(); - this.memView = new Uint8Array(this.mem.buffer); } + + /** Get a fresh view of WASM memory (safe across memory.grow). */ + private get memView(): Uint8Array { + return new Uint8Array(this.mem.buffer); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/twofish/src/twofish.ts` around lines 46 - 52, The cached Uint8Array this.memView (set in the constructor) can become detached if the WASM linear memory grows; update code to stop using a long-lived memView and instead create a fresh view from this.mem.buffer wherever memory is accessed (e.g., inside encrypt(), decrypt(), and createSession())—replace uses of this.memView with new Uint8Array(this.mem.buffer) or a helper getMemView() that returns a fresh view; ensure ioOffset (from exports.twofish_get_io_buffer()) and this.mem are still used consistently and remove the persistent this.memView field from the constructor to avoid stale/detached views.
🤖 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/twofish/build.sh`:
- Around line 28-31: The echo completion prints the size of the wrong artifact;
update the post-build message in build.sh so it references the actual output
path used by the linker (dist/twofish.wasm) instead of twofish.wasm. Locate the
echo that uses wc -c < twofish.wasm and change it to read the same artifact
written by the build command (dist/twofish.wasm) so the byte count and message
reflect the produced file.
In `@packages/twofish/src/c/twofish.c`:
- Around line 173-174: In packages/twofish/src/c/twofish.c remove the `#include`
<string.h> and instead add local prototypes for the three required functions
used in this file: declare void *memset(void *dst, int c, unsigned long n); void
*memcpy(void *dst, const void *src, unsigned long n); and int memcmp(const void
*a, const void *b, unsigned long n); so the file uses the local implementations
provided by the wasm wrapper (matches twofish_wasm.c build without relying on
standard headers).
- Line 217: The Twofish_fatal macro in twofish.c is hardcoded to an infinite
loop at compile time so overrides in twofish_wasm.c don't apply; change the
setup so both translation units see the same definition before preprocessing by
moving the Twofish_fatal definition into a shared header (e.g., add a
declaration/definition in a common header included by twofish.c and
twofish_wasm.c) or expose it as a build-time macro (-DTwofish_fatal=...) and
remove the local `#undef/`#define in twofish_wasm.c; ensure calls in
Twofish_initialise and Twofish_prepare_key will expand to the new trap-based
handler (or the chosen global handler) by including that shared header or using
the build flag in both compilations.
In `@packages/twofish/src/twofish.ts`:
- Around line 352-363: The Node.js fallback in fetchBytes returns buf.buffer
which may contain pooled extra bytes; change it to return only the file's bytes
by slicing the underlying ArrayBuffer using the Buffer's byteOffset/byteLength
(e.g. return buf.buffer.slice(buf.byteOffset, buf.byteOffset + buf.byteLength))
so WebAssembly.instantiate receives exactly the WASM bytes; update the
fetchBytes function accordingly.
- Around line 168-199: encrypt() currently returns a direct view into WASM
memory while decrypt() returns a copied Uint8Array, causing inconsistent API and
contradicting the JSDoc; change encrypt (function encrypt) to return an
independent copy of the ciphertext (call .slice() on the Uint8Array created from
this.mem.buffer at this.ioOffset + 16) so it matches decrypt(), and update the
JSDoc/WARNING text to reflect that encrypt returns a new independent 16-byte
Uint8Array rather than a WASM view.
In `@src/core/encryption/__tests__/ciphers.test.ts`:
- Around line 124-133: The test uses Set<ArrayBuffer> (cipherTexts) which
compares references not byte equality, so replace the identity-based dedupe with
a stable byte-derived key: for each ciphertext from codec.encrypt(originalData)
compute a consistent string/digest (e.g., hex/base64 or crypto.subtle.digest of
new Uint8Array(ct)) and use that string in the Set to assert uniqueness; if you
still need raw buffers for later decryption, push each ArrayBuffer into a
separate array (e.g., rawCiphertexts) while tracking uniqueness via the digest
string Set.
In `@src/core/encryption/cipherModes/CTRCipherMode.ts`:
- Around line 11-12: The CTRCipherMode constructor currently accepts a blockSize
but later code (the scratch buffer and XOR operations in methods like
encrypt/decrypt using xor16) assumes 16-byte blocks; fix by either validating
and rejecting non-16 block sizes in the constructor (throw if blockSize !== 16)
or by making the XOR/scratch logic generic: allocate scratch = new
Uint8Array(blockSize) and replace xor16 calls with a generic xor(bufferA,
bufferB, length=blockSize) used at all XOR sites referenced in CTRCipherMode
(constructor, encrypt, and any decrypt/ctr-step helpers); pick one approach and
apply it consistently to all the XOR sites noted (lines around 29-31, 37-40,
48-51) so blockSize is honored.
In `@src/core/encryption/ciphers/Serpent/SeprentCipher.test.ts`:
- Around line 27-35: The test's uniqueness check uses Set<ArrayBuffer> which
compares references not bytes; change the test around
cipher.encrypt(originalData) so you compute a byte-stable representation (e.g.,
hex or base64 string) of the returned ArrayBuffer/Uint8Array and store that
string in the Set to assert uniqueness, while still keeping the raw
ArrayBuffer/Uint8Array values separately (e.g., in an array) for later decrypt()
assertions; update references to cipherTexts to use the string key (and keep a
separate collection for raw buffers) so identical ciphertext bytes produced in
new buffers are detected as duplicates.
In `@src/core/encryption/ciphers/Twofish/index.ts`:
- Around line 153-158: The Twofish CTR cipher currently creates a WASM session
via tf.createSession(this.key) in getCipher()/constructor but never frees it,
causing memory leaks; add a dispose() (or close()/destroy()) method on
WasmTwofishCTRCipher that calls the TwofishModule session cleanup (e.g.,
tf.destroySession(session) or session.destroy) and also clears
this.cipher/this.session reference; ensure getCipher()/constructor stores the
created session on the instance (e.g., this.session) and that CTRCipherMode
continues to use tf.encrypt(session, ...), and mirror the Serpent cipher's
dispose pattern to remove the session when the cipher is no longer needed.
- Around line 1-2: The imports TwofishModule and twofishModule in
src/core/encryption/ciphers/Twofish/index.ts fail because packages/twofish
hasn't been built and its package.json exports point to ./dist/* which doesn't
exist; run npm run build inside packages/twofish to generate dist/index.mjs,
dist/index.cjs and dist/twofish.wasm, then update the CI pipeline (or root
build/lint script) to execute that build step for packages/twofish before
running lint/type-check so import/no-unresolved errors are resolved.
In `@src/core/encryption/index.ts`:
- Line 23: The test mock in FilesStorage.test.ts returns an ArrayBuffer (uses
.buffer) but the RandomBytesGenerator type requires a Uint8Array-like return
(Uint8Array<ArrayBuffer>); update the mock used in
src/core/features/files/FilesStorage.test.ts (around the mock at line ~58) to
return a Uint8Array instance instead of .buffer so it matches the
RandomBytesGenerator signature (keep the same byte contents but remove the
.buffer suffix and return the Uint8Array).
In `@src/core/features/encryption/algorithms.ts`:
- Around line 3-7: The removal of combination options from
ENCRYPTION_ALGORITHM_OPTIONS is intentional but breaking for profile creation;
update code and docs to explicitly state this: add a clear comment near
ENCRYPTION_ALGORITHM_OPTIONS and update any user-facing docs/README to say the
UI now supports only single-algorithm selection (while parseAlgorithms() and
PipelineProcessor still support decrypting existing combination-encrypted
profiles), and include migration guidance for existing users so it's clear why
combination algorithms were removed and that decryption remains supported.
In `@src/core/features/encryption/worker/encryption.bench.ts`:
- Line 17: Remove the invalid "throws: true" benchmark option from the benchmark
configurations (the occurrences used for the 1k and 1m benchmark suites) so all
bench configs only use valid tinybench/Vitest options like
time/iterations/warmupTime; locate the "throws: true" entries in the benchmark
definitions and delete them so the 1k and 1m suites match the 10k/100k suites.
In `@src/core/features/encryption/worker/Encryption.worker.ts`:
- Around line 56-60: The inline comment is incorrect:
derivedKeys.getDerivedBytes('serpent-cbc-cipher', 32 * 8) returns 256 bits (a
single 32-byte key) but the comment claims "32 bytes encryption + 32 bytes MAC";
update the comment near derivedKeys.getDerivedBytes to state it derives a single
32-byte (256-bit) encryption key for the SeprentCipher (or, if you intended to
derive both key+MAC, change the length to 64 * 8 and adjust usage accordingly).
Reference: derivedKeys.getDerivedBytes and the 'serpent-cbc-cipher' call and the
SeprentCipher consumer.
---
Nitpick comments:
In `@packages/twofish/src/twofish.ts`:
- Around line 71-74: Remove the leftover debug console.log in
packages/twofish/src/twofish.ts that prints the WASM path (the console.log call
that logs new URL('./twofish.wasm', import.meta.url).toString()); delete that
statement so the module no longer emits debug output in production.
- Around line 289-297: The code stores the WASM-backed view returned from
this.encrypt into prev (in the decrypt/CTR loop) which is fragile; change the
assignment so prev receives a defensive copy (e.g., use .slice()) of ct instead
of the raw view to avoid future overwrites—update the block inside the loop
where const ct = this.encrypt(handle, block); out.set(ct, i); prev = ct as
Uint8Array<ArrayBuffer>; to copy ct into prev (mirroring the .slice() usage used
for block earlier).
- Around line 46-52: The cached Uint8Array this.memView (set in the constructor)
can become detached if the WASM linear memory grows; update code to stop using a
long-lived memView and instead create a fresh view from this.mem.buffer wherever
memory is accessed (e.g., inside encrypt(), decrypt(), and
createSession())—replace uses of this.memView with new
Uint8Array(this.mem.buffer) or a helper getMemView() that returns a fresh view;
ensure ioOffset (from exports.twofish_get_io_buffer()) and this.mem are still
used consistently and remove the persistent this.memView field from the
constructor to avoid stale/detached views.
In `@src/core/encryption/ciphers/Twofish/twofish.wasm.test.ts`:
- Around line 13-18: The test currently constructs two WasmTwofishCTRCipher
instances and uses encrypt/decrypt without calling their explicit load() path;
update the test to call and await cipher1.load() before calling
cipher1.encrypt(...) and await cipher2.load() before calling
cipher2.decrypt(...), ensuring the explicit WasmTwofishCTRCipher.load()
initialization path is exercised for both cipher1 and cipher2 while keeping the
same key, encrypt, and decrypt assertions.
In `@src/core/encryption/utils/random.ts`:
- Around line 18-22: The code currently allocates an intermediate blockBuffer
and copies it into buffer; instead, call self.crypto.getRandomValues directly on
a view of the destination to avoid the allocation and copy. Replace
creation/usage of blockBuffer with: const target = buffer.subarray(offset,
offset + bytesToAdd); self.crypto.getRandomValues(target); (referencing buffer,
offset, bytesToAdd and removing blockBuffer and buffer.set).
In `@src/features/App/useProfilesList.ts`:
- Line 60: Remove the development console.log statement that prints "Create
profile with encryption" and profile.algorithm in useProfilesList.ts; either
delete the line entirely or replace it with a proper logger/telemetry call
(e.g., processLogger.debug or emitTelemetryEvent) if this information is
required in production, ensuring you reference the existing message string
"Create profile with encryption" and the profile.algorithm usage when making the
change.
In `@src/features/App/WorkspaceManager/ProfileCreator/index.tsx`:
- Around line 26-30: The NewProfile type currently declares algorithm as string;
change it to algorithm: ENCRYPTION_ALGORITHM and update any related signatures
(e.g., the onCreateProfile prop and any handlers in ProfileCreator) so they
accept ENCRYPTION_ALGORITHM instead of string; replace casts like `as
ENCRYPTION_ALGORITHM` by relying on the typed value, import ENCRYPTION_ALGORITHM
where needed, and update any consumers or tests that construct NewProfile to use
the enum/union type to ensure end-to-end type safety.
In `@vitest.config.mts`:
- Around line 19-22: The current wasm loader returns a URL by serializing a URL
object (JSON.stringify(new URL(resolve(id), import.meta.url))); change it to
return a plain string URL so intent is clearer and avoids indirect URL-to-string
serialization: inside the id.endsWith('.wasm') branch produce an export default
whose value is the string URL (e.g. the .href) rather than JSON.stringify(new
URL(...)) so TwofishModule.load() can accept the string; update the return in
that branch (referencing id and resolve(id)) accordingly.
🪄 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: 9303867a-676a-4938-89cc-2bd911744330
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/core/encryption/__tests__/__snapshots__/ciphers.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (35)
package.jsonpackages/twofish/Dockerfilepackages/twofish/README.mdpackages/twofish/build.shpackages/twofish/compose.ymlpackages/twofish/package.jsonpackages/twofish/src/c/string.hpackages/twofish/src/c/twofish.cpackages/twofish/src/c/twofish.hpackages/twofish/src/c/twofish_wasm.cpackages/twofish/src/index.tspackages/twofish/src/twofish.tspackages/twofish/tests/testvectors.tspackages/twofish/tests/twofish.test.tspackages/twofish/tsconfig.jsonpackages/twofish/tsdown.config.tssrc/core/encryption/__tests__/ciphers.test.tssrc/core/encryption/__tests__/encryption.consistency.test.tssrc/core/encryption/cipherModes/CTRCipherMode.tssrc/core/encryption/ciphers/Serpent/SeprentCipher.test.tssrc/core/encryption/ciphers/Serpent/index.tssrc/core/encryption/ciphers/Twofish/index.tssrc/core/encryption/ciphers/Twofish/twofish.wasm.test.tssrc/core/encryption/index.tssrc/core/encryption/processors/BufferSizeObfuscationProcessor.tssrc/core/encryption/utils/random.tssrc/core/features/encryption/algorithms.tssrc/core/features/encryption/index.tssrc/core/features/encryption/worker/Encryption.worker.tssrc/core/features/encryption/worker/encryption.bench.tssrc/features/App/WorkspaceManager/ProfileCreator/index.tsxsrc/features/App/useProfilesList.tstsconfig.jsonvitest.config.mtswords.txt
✅ Files skipped from review due to trivial changes (9)
- packages/twofish/src/index.ts
- src/core/features/encryption/index.ts
- packages/twofish/README.md
- packages/twofish/compose.yml
- src/core/encryption/tests/encryption.consistency.test.ts
- package.json
- packages/twofish/tsconfig.json
- packages/twofish/tests/testvectors.ts
- packages/twofish/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- words.txt
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/encryption/processors/BufferIntegrityProcessor.ts (2)
15-27: Consider caching the imported CryptoKey to avoid repeatedimportKeycalls.Both
encryptanddecryptcallcrypto.subtle.importKeyon every invocation. Since the key never changes, you could lazily cache the importedCryptoKeyto reduce async overhead:♻️ Optional refactor to cache imported key
export class BufferIntegrityProcessor implements IEncryptionProcessor { + private cachedKey: CryptoKey | null = null; + constructor(private readonly key: Uint8Array<ArrayBuffer>) {} + private async getKey(): Promise<CryptoKey> { + if (!this.cachedKey) { + this.cachedKey = await crypto.subtle.importKey( + 'raw', + this.key, + { name: 'HMAC', hash: 'SHA-256', length: 256 }, + false, + ['sign', 'verify'], + ); + } + return this.cachedKey; + } + public encrypt = async (buffer: ArrayBuffer) => { - const key = await crypto.subtle.importKey( - 'raw', - this.key, - { name: 'HMAC', hash: 'SHA-256', length: 256 }, - false, - ['sign', 'verify'], - ); + const key = await this.getKey(); const signature = await crypto.subtle.sign('HMAC', key, buffer);Also applies to: 36-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/processors/BufferIntegrityProcessor.ts` around lines 15 - 27, BufferIntegrityProcessor currently re-imports the raw key on every call in encrypt and decrypt via crypto.subtle.importKey; add a lazily-initialized cached CryptoKey property (e.g., this.cachedKey) and a helper (or inline check) that imports the key only if this.cachedKey is undefined, then reuse it in both encrypt and decrypt to avoid repeated async importKey calls while keeping the same import parameters ('HMAC', SHA-256, ['sign','verify']) and access to this.key.
8-11: Update docstring to reflect HMAC-based implementation.The docstring still describes the previous CRC32 checksum behavior. Consider updating it to reflect the new HMAC-SHA256 signing/verification mechanism.
/** - * Processor prepend a header with source buffer checksum during encryption, - * and check the sum with actual buffer checksum during decryption. + * Processor that signs buffer with HMAC-SHA256 during encryption (prepending 32-byte signature), + * and verifies signature during decryption, throwing IntegrityError on mismatch. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/processors/BufferIntegrityProcessor.ts` around lines 8 - 11, Update the top-level docstring for BufferIntegrityProcessor to describe the current HMAC-based implementation: state that the processor prepends an HMAC-SHA256 signature (not CRC32) to the encrypted buffer using the configured secret key, and on decryption it verifies the HMAC-SHA256 signature against the buffer and rejects or throws on mismatch; reference BufferIntegrityProcessor and its sign/verify behavior so readers know the header format, algorithm (HMAC-SHA256), key usage, and failure semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/encryption/processors/BufferIntegrityProcessor.ts`:
- Around line 15-27: BufferIntegrityProcessor currently re-imports the raw key
on every call in encrypt and decrypt via crypto.subtle.importKey; add a
lazily-initialized cached CryptoKey property (e.g., this.cachedKey) and a helper
(or inline check) that imports the key only if this.cachedKey is undefined, then
reuse it in both encrypt and decrypt to avoid repeated async importKey calls
while keeping the same import parameters ('HMAC', SHA-256, ['sign','verify'])
and access to this.key.
- Around line 8-11: Update the top-level docstring for BufferIntegrityProcessor
to describe the current HMAC-based implementation: state that the processor
prepends an HMAC-SHA256 signature (not CRC32) to the encrypted buffer using the
configured secret key, and on decryption it verifies the HMAC-SHA256 signature
against the buffer and rejects or throws on mismatch; reference
BufferIntegrityProcessor and its sign/verify behavior so readers know the header
format, algorithm (HMAC-SHA256), key usage, and failure semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdd54b8b-eea1-486f-a338-828544fbed5b
📒 Files selected for processing (3)
src/core/encryption/ciphers/XChaCha20/xchacha.spec.test.tssrc/core/encryption/processors/BufferIntegrityProcessor.test.tssrc/core/encryption/processors/BufferIntegrityProcessor.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/encryption/ciphers/XChaCha20/xchacha.spec.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/encryption/processors/BufferIntegrityProcessor.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/encryption/utils/bytes/binstruct.ts (1)
54-58: Consider adding decode-side length validation for defense-in-depth.The encode path now validates exact length (good), but
slice()in decode will silently return a shorter array if the source buffer is truncated. Since callers should pass valid encoded data, this is low-risk, but explicit validation would catch malformed input earlier.🛡️ Optional: Add decode validation
- decode: (view, off) => - new Uint8Array(view.buffer).slice( - view.byteOffset + off, - view.byteOffset + off + length, - ), + decode: (view, off) => { + const available = view.buffer.byteLength - view.byteOffset - off; + if (available < length) { + throw new RangeError(`Expected ${length} bytes, only ${available} available`); + } + return new Uint8Array(view.buffer).slice( + view.byteOffset + off, + view.byteOffset + off + length, + ); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/utils/bytes/binstruct.ts` around lines 54 - 58, The decode arrow function for the fixed-length byte field (decode: (view, off) => ...) should validate that the slice range is fully available before creating the Uint8Array slice; check that view.byteOffset + off + length <= view.buffer.byteLength (or view.byteOffset + off + length <= view.byteLength depending on view type) and throw a clear error if truncated, then return the slice—this adds defensive validation mirroring the encode path’s exact-length check.src/core/encryption/processors/BufferIntegrityProcessor.ts (1)
8-11: Documentation refers to "checksum" but implementation now uses HMAC.The class comment still mentions "checksum" from the old CRC32-based implementation. Consider updating to reflect the HMAC-based approach.
📝 Suggested documentation update
/** - * Processor prepend a header with source buffer checksum during encryption, - * and check the sum with actual buffer checksum during decryption. + * Processor prepends an HMAC-SHA256 signature during encryption, + * and verifies the signature during decryption. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/processors/BufferIntegrityProcessor.ts` around lines 8 - 11, Update the class comment for BufferIntegrityProcessor to reflect that it uses an HMAC-based integrity header (e.g., HMAC-SHA256 with a secret key) rather than a CRC32 checksum: describe that during encryption the processor prepends an HMAC header computed over the payload, and during decryption it verifies the HMAC and rejects/flags tampered data; mention the relevant methods (BufferIntegrityProcessor, and its encrypt/decrypt or processEncrypt/processDecrypt routines) so readers know where the behavior is implemented.packages/twofish/src/twofish.ts (1)
46-51: CachedmemViewin constructor creates inconsistency withdecrypt().
encrypt()(line 181) usesthis.memViewcached here, whiledecrypt()(line 212) creates a freshnew Uint8Array(this.mem.buffer). If WASM memory ever grows, the cached view becomes detached andencrypt()will throw, butdecrypt()would still work.For consistency and defensive coding, either:
- Remove the cached
memViewand create fresh views in both methods (likedecrypt()does), or- Add a getter that re-creates the view when the buffer changes.
♻️ Option 1: Remove cached memView, create fresh views
- private readonly memView; private constructor(exports: TwofishWasmExports) { this.exports = exports; this.mem = exports.memory; this.ioOffset = exports.twofish_get_io_buffer(); - this.memView = new Uint8Array(this.mem.buffer); }Then update
encrypt():encrypt(handle: SessionHandle, plaintext: Uint8Array): Uint8Array { this.validateBlock(plaintext, 'plaintext'); /* Write plaintext into io_buffer[0..15]. */ - this.memView.set(plaintext, this.ioOffset); + const memView = new Uint8Array(this.mem.buffer); + memView.set(plaintext, this.ioOffset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/twofish/src/twofish.ts` around lines 46 - 51, The constructor currently caches memView (this.memView) which can detach if WASM memory grows, causing encrypt() (which uses this.memView) to fail while decrypt() creates a fresh view; fix by removing the cached memView and update encrypt() to mirror decrypt(): create a new Uint8Array(this.mem.buffer) each time before reading/writing to WASM memory (while keeping ioOffset from twofish_get_io_buffer()), or alternatively implement a memView getter that checks if this.memView.buffer !== this.mem.buffer and recreates it when different; update the constructor to stop assigning this.memView and modify the encrypt() method (and any other uses) to use the fresh view/getter to ensure consistency with decrypt().
🤖 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/core/encryption/utils/keys.ts`:
- Around line 13-18: The local password byte buffer created by new
TextEncoder().encode(password) in deriveBitsFromPassword is not being zeroed, so
allocate it to a variable (e.g., passwordBytes = new
TextEncoder().encode(password)), pass that to
CryptographyUtils.prototype.deriveBits, and in the outer finally block
explicitly overwrite passwordBytes (passwordBytes.fill(0) or equivalent) before
calling utils.dispose(); ensure you still await/return the deriveBits result and
then dispose the utils instance via utils.dispose().
---
Nitpick comments:
In `@packages/twofish/src/twofish.ts`:
- Around line 46-51: The constructor currently caches memView (this.memView)
which can detach if WASM memory grows, causing encrypt() (which uses
this.memView) to fail while decrypt() creates a fresh view; fix by removing the
cached memView and update encrypt() to mirror decrypt(): create a new
Uint8Array(this.mem.buffer) each time before reading/writing to WASM memory
(while keeping ioOffset from twofish_get_io_buffer()), or alternatively
implement a memView getter that checks if this.memView.buffer !==
this.mem.buffer and recreates it when different; update the constructor to stop
assigning this.memView and modify the encrypt() method (and any other uses) to
use the fresh view/getter to ensure consistency with decrypt().
In `@src/core/encryption/processors/BufferIntegrityProcessor.ts`:
- Around line 8-11: Update the class comment for BufferIntegrityProcessor to
reflect that it uses an HMAC-based integrity header (e.g., HMAC-SHA256 with a
secret key) rather than a CRC32 checksum: describe that during encryption the
processor prepends an HMAC header computed over the payload, and during
decryption it verifies the HMAC and rejects/flags tampered data; mention the
relevant methods (BufferIntegrityProcessor, and its encrypt/decrypt or
processEncrypt/processDecrypt routines) so readers know where the behavior is
implemented.
In `@src/core/encryption/utils/bytes/binstruct.ts`:
- Around line 54-58: The decode arrow function for the fixed-length byte field
(decode: (view, off) => ...) should validate that the slice range is fully
available before creating the Uint8Array slice; check that view.byteOffset + off
+ length <= view.buffer.byteLength (or view.byteOffset + off + length <=
view.byteLength depending on view type) and throw a clear error if truncated,
then return the slice—this adds defensive validation mirroring the encode path’s
exact-length check.
🪄 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: f4bc15ce-ed81-4e27-83bd-f802733a1c91
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/core/features/encryption/worker/__snapshots__/CryptographyUtils.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
package.jsonpackages/twofish/src/twofish.tssrc/core/encryption/ciphers/XChaCha20/xchacha.spec.test.tssrc/core/encryption/processors/BufferIntegrityProcessor.test.tssrc/core/encryption/processors/BufferIntegrityProcessor.tssrc/core/encryption/utils/bytes/binstruct.test.tssrc/core/encryption/utils/bytes/binstruct.tssrc/core/encryption/utils/keys.tssrc/core/features/encryption/worker/CryptographyUtils.test.tssrc/core/features/encryption/worker/CryptographyUtils.tssrc/core/features/encryption/worker/CryptographyUtils.worker.tssrc/core/features/encryption/worker/encryption.bench.tssrc/features/App/Profiles/hooks/useProfileContainers.tssrc/features/App/useProfilesList.tstsconfig.jsonvitest.config.mtswords.txt
✅ Files skipped from review due to trivial changes (2)
- tsconfig.json
- src/core/encryption/ciphers/XChaCha20/xchacha.spec.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- package.json
- src/core/features/encryption/worker/CryptographyUtils.test.ts
- src/features/App/Profiles/hooks/useProfileContainers.ts
- words.txt
- src/features/App/useProfilesList.ts
- vitest.config.mts
- src/core/features/encryption/worker/CryptographyUtils.ts
- src/core/encryption/processors/BufferIntegrityProcessor.test.ts
- src/core/encryption/utils/bytes/binstruct.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/encryption/cipherModes/CTRMode.test.ts (2)
73-75: Remove stale commented-out helper.Lines 73-75 are dead commented code and add noise during test maintenance.
🧹 Proposed cleanup
-// const makeXorEncrypt = (key: number) => -// async (buffer: Uint8Array): Promise<Uint8Array> => -// new Uint8Array(buffer.map((b) => b ^ key));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/cipherModes/CTRMode.test.ts` around lines 73 - 75, Remove the stale commented-out helper makeXorEncrypt in CTRMode.test.ts (the three commented lines defining makeXorEncrypt and its inner function), as it is dead code that adds noise; simply delete those commented lines so tests only contain active, relevant helpers and assertions (look for the identifier makeXorEncrypt in the test file to locate the commented block).
121-127: Align test name with asserted behavior.Line 121 says partial blocks are processed, but Lines 124-126 assert rejection. Rename the test (and optionally relax exact error text matching) so intent is unambiguous.
✏️ Proposed rename + less brittle assertion
-it('partial last block uses only the required keystream bytes', async () => { +it('rejects non-block-multiple input without padding', async () => { const ctr = new CTRCipherMode(identityEncrypt, xor); const iv = new Uint8Array(16).fill(0x00); - await expect(ctr.encrypt(new Uint8Array(19).fill(0x00), iv)).rejects.toThrow( - 'Did you mean to add padding?', - ); + await expect(ctr.encrypt(new Uint8Array(19).fill(0x00), iv)).rejects.toThrow(/padding/i); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/encryption/cipherModes/CTRMode.test.ts` around lines 121 - 127, The test name misstates behavior: CTRCipherMode.encrypt rejects when input length is not block-aligned, but the test name implies partial blocks are processed; rename the test to something like "throws when input has a partial final block" and update the assertion to be less brittle (e.g., use await expect(ctr.encrypt(...)).rejects.toThrow(/add padding/); or .rejects.toThrowError() instead of matching the exact string). Update references to CTRCipherMode and encrypt in the test so intent and assertion align.
🤖 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/core/encryption/ciphers/Twofish/index.ts`:
- Around line 31-39: The constructor currently allows arbitrary chunkSize which
breaks encryption/decryption when chunkSize % 16 !== 0; update the
TwofishCTRCipher constructor (the chunkSize property set block) to validate that
any provided chunkSize is a positive integer and is a multiple of 16, throwing a
RangeError with a clear message if not, rather than accepting it, and apply the
same validation to the equivalent chunkSize handling in the other cipher
constructors/blocks referenced (lines ~67-78, ~90-97, ~131-151) so all CTR-like
ciphers reject non-16-byte-aligned chunk sizes before load/encrypt is called.
- Around line 15-18: TWOFISH_HEADER currently stores chunkSize as u32 but
callers (e.g., using CTRCipherMode.getEncryptionLimits(16).maxBytes) can supply
values > u32 max; update the code that constructs/encodes the header to clamp
chunkSize to 0xFFFFFFFF (Number.MAX_SAFE_INTEGER cap not needed here) before
passing into TWOFISH_HEADER so the wire format matches what can be represented,
and add a unit test that round-trips the default
CTRCipherMode.getEncryptionLimits(16).maxBytes through the header to assert the
clamped value; apply the same clamping fix for the other occurrences referenced
(lines ~33-37, 73-75, 113-116) where chunkSize is written to the header.
---
Nitpick comments:
In `@src/core/encryption/cipherModes/CTRMode.test.ts`:
- Around line 73-75: Remove the stale commented-out helper makeXorEncrypt in
CTRMode.test.ts (the three commented lines defining makeXorEncrypt and its inner
function), as it is dead code that adds noise; simply delete those commented
lines so tests only contain active, relevant helpers and assertions (look for
the identifier makeXorEncrypt in the test file to locate the commented block).
- Around line 121-127: The test name misstates behavior: CTRCipherMode.encrypt
rejects when input length is not block-aligned, but the test name implies
partial blocks are processed; rename the test to something like "throws when
input has a partial final block" and update the assertion to be less brittle
(e.g., use await expect(ctr.encrypt(...)).rejects.toThrow(/add padding/); or
.rejects.toThrowError() instead of matching the exact string). Update references
to CTRCipherMode and encrypt in the test so intent and assertion align.
🪄 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: c18d8bd9-c302-435b-b149-762192bae0f4
📒 Files selected for processing (4)
packages/twofish/LICENSEsrc/core/encryption/cipherModes/CTRMode.test.tssrc/core/encryption/ciphers/Twofish/index.tsvitest.config.mts
✅ Files skipped from review due to trivial changes (1)
- packages/twofish/LICENSE
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/twofish/tests/twofish.vectors.test.ts (1)
8-15: Consider usingsubstringinstead of deprecatedsubstr.
String.prototype.substr()is deprecated in modern JavaScript. Usesubstring()for better compatibility.♻️ Suggested fix
function fromHex(str: string) { const l = str.length / 2; const out = new Uint8Array(l); for (let i = 0; i < l; i++) { - out[i] = parseInt(str.substr(2 * i, 2), 16); + out[i] = parseInt(str.substring(2 * i, 2 * i + 2), 16); } return out; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/twofish/tests/twofish.vectors.test.ts` around lines 8 - 15, The fromHex function uses the deprecated String.prototype.substr; update it to use a non-deprecated method (e.g., String.prototype.substring or String.prototype.slice) when extracting the two-character hex pairs inside fromHex so parseInt gets the same substrings; replace str.substr(2 * i, 2) with str.substring(2 * i, 2 * i + 2) (or str.slice(2 * i, 2 * i + 2)) to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/twofish/tests/twofish.vectors.test.ts`:
- Around line 8-15: The fromHex function uses the deprecated
String.prototype.substr; update it to use a non-deprecated method (e.g.,
String.prototype.substring or String.prototype.slice) when extracting the
two-character hex pairs inside fromHex so parseInt gets the same substrings;
replace str.substr(2 * i, 2) with str.substring(2 * i, 2 * i + 2) (or
str.slice(2 * i, 2 * i + 2)) to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2cb39c4-09a8-48cd-9a54-3d747e63027d
📒 Files selected for processing (5)
packages/twofish/src/c/twofish.cpackages/twofish/src/c/twofish_wasm.cpackages/twofish/tests/twofish.errors.test.tspackages/twofish/tests/twofish.vectors.test.tspackages/twofish/tsdown.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/twofish/tsdown.config.ts
vitonsky
changed the title
This was referenced Apr 27, 2026
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.
Closes #265, closes #103, closes #62
Key changes
structurecodec for binary structuresBreaking changes
Benchmark results
Summary by CodeRabbit
Release Notes
New Features
Improvements