feat(native): introduce @zapo-js/native, Rust NAPI accelerators#128
Draft
vinikjkkj wants to merge 1 commit into
Draft
feat(native): introduce @zapo-js/native, Rust NAPI accelerators#128vinikjkkj wants to merge 1 commit into
vinikjkkj wants to merge 1 commit into
Conversation
New workspace package `@zapo-js/native` with Rust-backed accelerators for the messaging crypto hot path. The library try-requires the binding at module load and falls back to the JS impl when missing, so consumers without the prebuilt binary keep working. Accelerators (`src/crypto/`): - **XEdDSA sign / verify**, sync + async. Avoids the WebCrypto Ed25519 round-trip that dominates SKDM sign in fanout. - **X25519 ECDH scalar mult**. Avoids the `createPrivateKey` / `createPublicKey` DER round-trip in `node:crypto.diffieHellman`. Layout (`src/lib.rs` thin entry + `src/<concern>/`) leaves room for future non-crypto accelerators (hash, codec) without another rename. Force-JS escape hatches: `ZAPO_XEDDSA_FORCE_JS`, `ZAPO_X25519_FORCE_JS`. ## Bench fake-server sqlite messaging, 1000 msgs / scenario, 1000 contacts x 2 devices, 4 groups x 500 members. Medians of 3 runs after warmup, msg/s: | Scenario | JS (master) | Native | Delta | | ------------ | ----------: | -----: | ----------------: | | `send_1to1` | 121 | ~384 | +217% (2.93x) | | `recv_1to1` | 123 | ~502 | +308% (4.08x) | | `send_group` | 168 | ~1083 | +545% (6.45x) | | `recv_group` | 363 | ~1573 | +333% (4.34x) | `feMul` was 10.78% on master, absent post; new top is sqlite I/O. ## Validation Cross-check (50 sign / verify pairs both directions) passes; 722 / 722 unit tests, lint, typecheck all green.
📝 WalkthroughWalkthroughThis PR introduces a complete native Rust cryptographic module ( ChangesNative Cryptographic Module
Sequence DiagramsequenceDiagram
participant App as TypeScript App
participant TS_XEdDSA as xeddsaSign/Verify
participant NativeGateway as `@zapo-js/native` Gateway
participant RustSync as Rust xeddsa_sign/verify
participant RustAsync as Rust async Task
App->>TS_XEdDSA: xeddsaSign(privKey, msg)
TS_XEdDSA->>NativeGateway: nativeBinding.xeddsaSign?
alt Native available
NativeGateway->>RustSync: x27519_scalar_mult(privKey, msg)
RustSync-->>NativeGateway: Buffer signature
NativeGateway-->>TS_XEdDSA: signature Buffer
else Native unavailable
TS_XEdDSA-->>TS_XEdDSA: JS signing (existing logic)
end
TS_XEdDSA-->>App: signature
App->>TS_XEdDSA: xeddsaVerify(pubKey, msg, sig)
TS_XEdDSA->>NativeGateway: nativeBinding.xeddsaVerify?
alt Native available
NativeGateway->>RustSync: xeddsa_verify(pubKey, msg, sig)
RustSync-->>NativeGateway: bool
NativeGateway-->>TS_XEdDSA: bool
else Native unavailable
TS_XEdDSA-->>TS_XEdDSA: JS verification (existing logic)
end
TS_XEdDSA-->>App: boolean result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces a substantial new Rust module with cryptographic primitives, multiple N-API bindings (sync and async variants), dense mathematical operations, comprehensive test coverage, and integration points across the TypeScript codebase. While individual sections are logical and well-separated, the breadth of crypto logic, unfamiliar domain (XEdDSA group operations), and multiple interdependent layers demand careful review of correctness, security assumptions, and integration. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/native/__test__/bench-sync-vs-async.cjs`:
- Around line 132-167: The current "sync" baseline is being measured wrapped in
a Promise because the calls to timeitParallel use async () => xeddsaSign(...)
and async () => xeddsaVerify(...), which adds Promise overhead; update the
benchmark so the true synchronous path is measured without Promise wrapping by
invoking xeddsaSign and xeddsaVerify directly in a non-async harness (or add a
separate non-Promise timing function instead of timeitParallel for the sync
path), and keep the async measurements using xeddsaSignAsync and
xeddsaVerifyAsync; alternatively, if you intentionally want a promise-wrapped
comparison, relabel the sync entries to indicate they are promise-wrapped to
avoid misleading "sync" vs "async" speedups.
In `@packages/native/__test__/cross-check.cjs`:
- Around line 1-49: The file fails the repo's formatting check; run the
repository Prettier formatter (using the project's config, e.g., via the repo's
format script or npx prettier --write) against the test file containing main(),
native.xeddsaSign and native.xeddsaVerify, stage the updated file, and commit
the formatted result so CI `ci / format` passes.
In `@packages/native/package.json`:
- Around line 12-25: Update the package.json for the `@zapo-js/native` optional
package: remove or set "private" to false (so it can be published), add
"publishConfig": { "access": "public" }, and declare "zapo-js" under
"peerDependencies" with a compatible version (e.g., match the workspace/root
zapo-js version or use workspace range). Ensure these keys ("private",
"publishConfig", "peerDependencies") are added/updated in the existing
package.json for `@zapo-js/native`.
In `@packages/native/src/crypto/x25519.rs`:
- Around line 8-30: The PR added the new x25519_scalar_mult binding but the test
suite lacks interop/validation coverage; add native-vs-existing-path tests that
exercise the x25519_scalar_mult function: write at least one test using a known
RFC7748 test vector (fixed 32-byte scalar and public key producing expected
shared secret) and one peer-agreement test that generates a keypair A (private
a, public A) and keypair B (private b, public B) and asserts
x25519_scalar_mult(a, B) == x25519_scalar_mult(b, A); place tests alongside
existing crypto validation tests and call the exported x25519_scalar_mult
function to ensure regressions are caught.
In `@src/crypto/core/xeddsa.ts`:
- Around line 24-41: The try/catch around require('`@zapo-js/native`') currently
swallows all errors; change it to catch the error into a variable and only treat
the "package not installed" case as a benign fallback (e.g., error.code ===
'MODULE_NOT_FOUND' or an explicit check for "Cannot find module
'`@zapo-js/native`'"); for any other error (broken native binary, wrong exports,
init failures) re-throw or log it with context so failures are visible. Locate
the self-invoking initializer that assigns nativeBinding and adjust its catch
handler to inspect the thrown error, permit fallback only when the module is
missing, and otherwise surface the error (including details about
xeddsaSign/xeddsaVerify) instead of silently returning null.
In `@src/crypto/curves/X25519.ts`:
- Around line 22-31: The try/catch that loads '`@zapo-js/native`' currently
swallows all errors; change it to only fall back when the package is truly
missing and rethrow other failures with normalized context: import toError from
'`@util/primitives`', catch the require error as e, if toError(e).code ===
'MODULE_NOT_FOUND' (or e.code === 'MODULE_NOT_FOUND') then continue to fallback
to node:crypto, otherwise throw a new error that includes contextual text (e.g.,
"[X25519] failed to load `@zapo-js/native`") and the normalized error via
toError(e); apply this change around the require block that returns
mod.x25519ScalarMult.
- Around line 169-170: The native branch should normalize and validate the
shared secret before returning: call nativeX25519ScalarMult(privKey, pubKey),
wrap its result with toBytesView(...) to convert any Buffer subclass to a plain
Uint8Array view, assert that the resulting byte length is exactly 32 (throw or
reject if not), and then return that validated Uint8Array; update the code
around the nativeX25519ScalarMult call to use toBytesView(...) and the 32-byte
length check so callers always receive a plain Uint8Array of length 32.
🪄 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: a4e1e083-f11c-4911-8b8a-7bd03a17199e
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/native/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
eslint.config.jspackages/native/.gitignorepackages/native/Cargo.tomlpackages/native/__test__/bench-sync-vs-async.cjspackages/native/__test__/cross-check.cjspackages/native/__test__/smoke.cjspackages/native/build.rspackages/native/package.jsonpackages/native/src/crypto/mod.rspackages/native/src/crypto/x25519.rspackages/native/src/crypto/xeddsa.rspackages/native/src/lib.rssrc/crypto/core/xeddsa.tssrc/crypto/curves/X25519.ts
Comment on lines
+132
to
+167
| for (const conc of [1, 4, 8, 16, 32]) { | ||
| console.log(`SIGN — concurrency ${conc} (total ${PAR_TOTAL} ops x ${RUNS} runs):`) | ||
| const sSync = await timeitParallel( | ||
| `sync (loop)`, | ||
| RUNS, | ||
| PAR_TOTAL, | ||
| conc, | ||
| async () => xeddsaSign(PRIV, MSG) | ||
| ) | ||
| const sAsync = await timeitParallel( | ||
| `async (libuv pool)`, | ||
| RUNS, | ||
| PAR_TOTAL, | ||
| conc, | ||
| () => xeddsaSignAsync(PRIV, MSG) | ||
| ) | ||
| console.log(` → speedup async vs sync: ${(sSync / sAsync).toFixed(2)}x\n`) | ||
| } | ||
|
|
||
| for (const conc of [1, 4, 8, 16, 32]) { | ||
| console.log(`VERIFY — concurrency ${conc} (total ${PAR_TOTAL} ops x ${RUNS} runs):`) | ||
| const vSync = await timeitParallel( | ||
| `sync (loop)`, | ||
| RUNS, | ||
| PAR_TOTAL, | ||
| conc, | ||
| async () => xeddsaVerify(PUB, MSG, SIG) | ||
| ) | ||
| const vAsync = await timeitParallel( | ||
| `async (libuv pool)`, | ||
| RUNS, | ||
| PAR_TOTAL, | ||
| conc, | ||
| () => xeddsaVerifyAsync(PUB, MSG, SIG) | ||
| ) | ||
| console.log(` → speedup async vs sync: ${(vSync / vAsync).toFixed(2)}x\n`) |
There was a problem hiding this comment.
The parallel "sync" baseline is still promise-wrapped.
Here the sync branch is passed as async () => xeddsaSign(...) / async () => xeddsaVerify(...), and timeitParallel() runs everything through Promise.all(...). That means every "sync" sample includes Promise overhead, so the async-vs-sync speedups reported here are not a clean baseline. Please benchmark the synchronous path with a separate non-Promise harness, or relabel this as a promise-wrapped comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/native/__test__/bench-sync-vs-async.cjs` around lines 132 - 167, The
current "sync" baseline is being measured wrapped in a Promise because the calls
to timeitParallel use async () => xeddsaSign(...) and async () =>
xeddsaVerify(...), which adds Promise overhead; update the benchmark so the true
synchronous path is measured without Promise wrapping by invoking xeddsaSign and
xeddsaVerify directly in a non-async harness (or add a separate non-Promise
timing function instead of timeitParallel for the sync path), and keep the async
measurements using xeddsaSignAsync and xeddsaVerifyAsync; alternatively, if you
intentionally want a promise-wrapped comparison, relabel the sync entries to
indicate they are promise-wrapped to avoid misleading "sync" vs "async"
speedups.
Comment on lines
+1
to
+49
| // Cross-check: native sign / JS verify AND JS sign / native verify. | ||
| // Run from repo root via tsx so @crypto path alias resolves. | ||
| const native = require('@zapo-js/native') | ||
| const assert = require('node:assert') | ||
| const { randomBytes, createPrivateKey } = require('node:crypto') | ||
|
|
||
| if (typeof native.xeddsaSign !== 'function' || typeof native.xeddsaVerify !== 'function') { | ||
| console.error('native binding not loaded') | ||
| process.exit(2) | ||
| } | ||
|
|
||
| async function main() { | ||
| const { xeddsaSign: jsSign, xeddsaVerify: jsVerify } = await import('../../../src/crypto/core/xeddsa.ts') | ||
|
|
||
| const X25519_PKCS8_PREFIX = Buffer.from('302e020100300506032b656e04220420', 'hex') | ||
|
|
||
| let okPairs = 0 | ||
| for (let i = 0; i < 50; i += 1) { | ||
| const priv = randomBytes(32) | ||
| const keyObj = createPrivateKey({ | ||
| key: Buffer.concat([X25519_PKCS8_PREFIX, priv]), | ||
| format: 'der', | ||
| type: 'pkcs8' | ||
| }) | ||
| const jwk = keyObj.export({ format: 'jwk' }) | ||
| const pub = Buffer.from(jwk.x, 'base64url') | ||
| const message = randomBytes(80 + (i % 200)) | ||
|
|
||
| // native sign -> JS verify | ||
| const privClone1 = Buffer.from(priv) | ||
| const sigNative = native.xeddsaSign(privClone1, message) | ||
| const okJsVerifiesNative = await jsVerify(pub, message, Buffer.from(sigNative)) | ||
| assert.equal(okJsVerifiesNative, true, `iter ${i}: js verify failed on native sig`) | ||
|
|
||
| // JS sign -> native verify | ||
| const privClone2 = Buffer.from(priv) | ||
| const sigJs = await jsSign(privClone2, message) | ||
| const okNativeVerifiesJs = native.xeddsaVerify(pub, message, sigJs) | ||
| assert.equal(okNativeVerifiesJs, true, `iter ${i}: native verify failed on JS sig`) | ||
|
|
||
| okPairs += 1 | ||
| } | ||
| console.log(`cross-check OK: ${okPairs} pairs both directions`) | ||
| } | ||
|
|
||
| main().catch((e) => { | ||
| console.error(e) | ||
| process.exit(1) | ||
| }) |
There was a problem hiding this comment.
Run Prettier on this file before merge.
ci / format is already failing on this script, so the current revision does not match the repo's enforced formatting.
🧰 Tools
🪛 GitHub Actions: ci / 14_format.txt
[warning] 1-1: Prettier reported formatting/style issues in this file during 'prettier . --check'. Run 'prettier --write' to fix.
🪛 GitHub Actions: ci / format
[warning] 1-1: Prettier --check reported formatting/style issues in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/native/__test__/cross-check.cjs` around lines 1 - 49, The file fails
the repo's formatting check; run the repository Prettier formatter (using the
project's config, e.g., via the repo's format script or npx prettier --write)
against the test file containing main(), native.xeddsaSign and
native.xeddsaVerify, stage the updated file, and commit the formatted result so
CI `ci / format` passes.
Comment on lines
+12
to
+25
| "private": true, | ||
| "engines": { | ||
| "node": ">=20.9.0" | ||
| }, | ||
| "napi": { | ||
| "binaryName": "zapo-native" | ||
| }, | ||
| "scripts": { | ||
| "build": "napi build --platform --release --js binding.js --dts binding.d.ts", | ||
| "build:debug": "napi build --platform --js binding.js --dts binding.d.ts" | ||
| }, | ||
| "devDependencies": { | ||
| "@napi-rs/cli": "^3.0.0" | ||
| } |
There was a problem hiding this comment.
Add required optional-package publish metadata and peer dependency.
@zapo-js/native is missing required peerDependencies.zapo-js and publishConfig.access: 'public', and
private: true conflicts with optional package publishing requirements.
Suggested fix
{
"name": "`@zapo-js/native`",
@@
- "private": true,
+ "publishConfig": {
+ "access": "public"
+ },
@@
+ "peerDependencies": {
+ "zapo-js": "*"
+ },
"engines": {
"node": ">=20.9.0"
},As per coding guidelines, "Declare zapo-js as a peerDependency in optional packages
(packages/<name>/) ... Each optional package must ... publish with
\"publishConfig\": { \"access\": \"public\" } in package.json."
📝 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
| "private": true, | |
| "engines": { | |
| "node": ">=20.9.0" | |
| }, | |
| "napi": { | |
| "binaryName": "zapo-native" | |
| }, | |
| "scripts": { | |
| "build": "napi build --platform --release --js binding.js --dts binding.d.ts", | |
| "build:debug": "napi build --platform --js binding.js --dts binding.d.ts" | |
| }, | |
| "devDependencies": { | |
| "@napi-rs/cli": "^3.0.0" | |
| } | |
| "publishConfig": { | |
| "access": "public" | |
| }, | |
| "peerDependencies": { | |
| "zapo-js": "*" | |
| }, | |
| "engines": { | |
| "node": ">=20.9.0" | |
| }, | |
| "napi": { | |
| "binaryName": "zapo-native" | |
| }, | |
| "scripts": { | |
| "build": "napi build --platform --release --js binding.js --dts binding.d.ts", | |
| "build:debug": "napi build --platform --js binding.js --dts binding.d.ts" | |
| }, | |
| "devDependencies": { | |
| "`@napi-rs/cli`": "^3.0.0" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/native/package.json` around lines 12 - 25, Update the package.json
for the `@zapo-js/native` optional package: remove or set "private" to false (so
it can be published), add "publishConfig": { "access": "public" }, and declare
"zapo-js" under "peerDependencies" with a compatible version (e.g., match the
workspace/root zapo-js version or use workspace range). Ensure these keys
("private", "publishConfig", "peerDependencies") are added/updated in the
existing package.json for `@zapo-js/native`.
Comment on lines
+8
to
+30
| #[napi] | ||
| pub fn x25519_scalar_mult(private_key: Buffer, public_key: Buffer) -> Result<Buffer> { | ||
| if private_key.len() != 32 { | ||
| return Err(Error::new( | ||
| Status::InvalidArg, | ||
| format!("invalid x25519 private key length {}", private_key.len()), | ||
| )); | ||
| } | ||
| if public_key.len() != 32 { | ||
| return Err(Error::new( | ||
| Status::InvalidArg, | ||
| format!("invalid x25519 public key length {}", public_key.len()), | ||
| )); | ||
| } | ||
| let mut sk = [0u8; 32]; | ||
| sk.copy_from_slice(&private_key); | ||
| let mut pk = [0u8; 32]; | ||
| pk.copy_from_slice(&public_key); | ||
|
|
||
| // mul_clamped clamps the scalar per RFC 7748 internally; passing an | ||
| // already-clamped key is a no-op since clamping is idempotent. | ||
| let shared = MontgomeryPoint(pk).mul_clamped(sk).to_bytes(); | ||
| Ok(Buffer::from(shared.to_vec())) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add interop coverage for the new X25519 binding.
This introduces a new public crypto primitive, but the validation files in this PR only exercise XEdDSA paths. Please add at least one native-vs-existing-path test for x25519_scalar_mult - ideally a known vector plus a peer-agreement case - so a scalar-multiplication regression does not land unnoticed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/native/src/crypto/x25519.rs` around lines 8 - 30, The PR added the
new x25519_scalar_mult binding but the test suite lacks interop/validation
coverage; add native-vs-existing-path tests that exercise the x25519_scalar_mult
function: write at least one test using a known RFC7748 test vector (fixed
32-byte scalar and public key producing expected shared secret) and one
peer-agreement test that generates a keypair A (private a, public A) and keypair
B (private b, public B) and asserts x25519_scalar_mult(a, B) ==
x25519_scalar_mult(b, A); place tests alongside existing crypto validation tests
and call the exported x25519_scalar_mult function to ensure regressions are
caught.
Comment on lines
+24
to
+41
| const nativeBinding: NativeBinding | null = (() => { | ||
| if (process.env.ZAPO_XEDDSA_FORCE_JS) return null | ||
| try { | ||
| const mod = require('@zapo-js/native') as { | ||
| xeddsaSign?: NativeBinding['xeddsaSign'] | ||
| xeddsaVerify?: NativeBinding['xeddsaVerify'] | ||
| } | ||
| if ( | ||
| mod && | ||
| typeof mod.xeddsaSign === 'function' && | ||
| typeof mod.xeddsaVerify === 'function' | ||
| ) { | ||
| return { xeddsaSign: mod.xeddsaSign, xeddsaVerify: mod.xeddsaVerify } | ||
| } | ||
| } catch { | ||
| // optional native binding not installed; fall through to JS implementation | ||
| } | ||
| return null |
There was a problem hiding this comment.
Only fall back when the optional package is actually missing.
This catch swallows every load/init failure from @zapo-js/native, including broken binaries and export mismatches, then silently downgrades to JS. That makes native deployment failures invisible. Please limit the fallback to the package-not-installed case and log or re-throw everything else.
As per coding guidelines, "Do not silently swallow exceptions; always log or re-throw caught errors with contextual metadata."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/crypto/core/xeddsa.ts` around lines 24 - 41, The try/catch around
require('`@zapo-js/native`') currently swallows all errors; change it to catch the
error into a variable and only treat the "package not installed" case as a
benign fallback (e.g., error.code === 'MODULE_NOT_FOUND' or an explicit check
for "Cannot find module '`@zapo-js/native`'"); for any other error (broken native
binary, wrong exports, init failures) re-throw or log it with context so
failures are visible. Locate the self-invoking initializer that assigns
nativeBinding and adjust its catch handler to inspect the thrown error, permit
fallback only when the module is missing, and otherwise surface the error
(including details about xeddsaSign/xeddsaVerify) instead of silently returning
null.
Comment on lines
+22
to
+31
| try { | ||
| const mod = require('@zapo-js/native') as { | ||
| x25519ScalarMult?: NativeX25519ScalarMult | ||
| } | ||
| if (mod && typeof mod.x25519ScalarMult === 'function') { | ||
| return mod.x25519ScalarMult | ||
| } | ||
| } catch { | ||
| // optional native binding not installed; fall through to node:crypto | ||
| } |
There was a problem hiding this comment.
Only swallow the missing-native-package case.
This catch {} hides every load failure from @zapo-js/native, including ABI mismatches and bugs inside the binding, then silently degrades to JS. Please narrow the fallback to the expected "module not found" case and rethrow unexpected errors with context instead. As per coding guidelines, "Normalize unknown errors with toError() from @util/primitives in error handling blocks" and "Do not silently swallow exceptions; always log or re-throw caught errors with contextual metadata."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/crypto/curves/X25519.ts` around lines 22 - 31, The try/catch that loads
'`@zapo-js/native`' currently swallows all errors; change it to only fall back
when the package is truly missing and rethrow other failures with normalized
context: import toError from '`@util/primitives`', catch the require error as e,
if toError(e).code === 'MODULE_NOT_FOUND' (or e.code === 'MODULE_NOT_FOUND')
then continue to fallback to node:crypto, otherwise throw a new error that
includes contextual text (e.g., "[X25519] failed to load `@zapo-js/native`") and
the normalized error via toError(e); apply this change around the require block
that returns mod.x25519ScalarMult.
Comment on lines
+169
to
+170
| if (nativeX25519ScalarMult) { | ||
| return nativeX25519ScalarMult(privKey, pubKey) |
There was a problem hiding this comment.
Normalize and validate the native shared secret at the boundary.
The native path returns the binding output directly, so this method can now leak a Buffer subclass instead of a plain Uint8Array, and it skips the 32-byte invariant enforced by the Node path. Normalize with toBytesView(...) here and assert the returned length before handing it to callers.
Suggested fix
if (nativeX25519ScalarMult) {
- return nativeX25519ScalarMult(privKey, pubKey)
+ const sharedSecret = toBytesView(nativeX25519ScalarMult(privKey, pubKey))
+ assertByteLength(sharedSecret, 32, 'x25519 shared secret must be 32 bytes')
+ return sharedSecret
}As per coding guidelines, "Use Uint8Array for all binary data; do not use Buffer in runtime code" and "Use toBytesView() only at system boundaries where input may not be a plain Uint8Array."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/crypto/curves/X25519.ts` around lines 169 - 170, The native branch should
normalize and validate the shared secret before returning: call
nativeX25519ScalarMult(privKey, pubKey), wrap its result with toBytesView(...)
to convert any Buffer subclass to a plain Uint8Array view, assert that the
resulting byte length is exactly 32 (throw or reject if not), and then return
that validated Uint8Array; update the code around the nativeX25519ScalarMult
call to use toBytesView(...) and the 32-byte length check so callers always
receive a plain Uint8Array of length 32.
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
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.
New workspace package
@zapo-js/nativewith Rust-backed accelerators for the messaging crypto hot path. The library try-requires the binding at module load and falls back to the JS impl when missing, so consumers without the prebuilt binary keep working.Accelerators (
src/crypto/):createPrivateKey/createPublicKeyDER round-trip innode:crypto.diffieHellman.Layout (
src/lib.rsthin entry +src/<concern>/) leaves room for future non-crypto accelerators (hash, codec) without another rename. Force-JS escape hatches:ZAPO_XEDDSA_FORCE_JS,ZAPO_X25519_FORCE_JS.Bench
fake-server sqlite messaging, 1000 msgs / scenario, 1000 contacts x 2 devices, 4 groups x 500 members. Medians of 3 runs after warmup, msg/s:
send_1to1recv_1to1send_grouprecv_groupfeMulwas 10.78% on master, absent post; new top is sqlite I/O.Validation
Cross-check (50 sign / verify pairs both directions) passes; 722 / 722 unit tests, lint, typecheck all green.
Summary by CodeRabbit