feat(security): Phase 3 audit — TS-boundary validation + WebCrypto hardening#985
Merged
feat(security): Phase 3 audit — TS-boundary validation + WebCrypto hardening#985
Conversation
Resolves the 6 annotations surfaced on PR #984 runs: * Node.js 20 deprecations — add `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true'` to every workflow so actions still pinned to Node 20 (setup-java, upload/download-artifact, setup-android, peter-evans/*, McCzarny/*, reviewdog/*) run on the runner's Node 24 instead. Silences the deprecation warning until each action ships a v5. * `actions/checkout@v4` → `@v5` and `actions/setup-node@v4` → `@v5` across all workflows (these have native Node 24 builds). * Cache save failures ("Unable to reserve cache with key … another job may be creating this cache") — replace `\${{ github.run_id }}` keys with `\${{ hashFiles(...) }}` keys for Gradle and node_modules caches, and a stable `avd-pixel7pro-34-x86_64-v1` key for the AVD cache. Re-runs and cross-job races no longer collide on the same key. * Obsolete AGP 8.7.3 — bump library `classpath` to 8.12.2 to match react-native-nitro-modules. AGP 9.x requires Gradle 9 + JDK 21, which RN 0.81 + the example app's Gradle 8.14.3 + JDK 17 toolchain cannot support yet, so disable the `AndroidGradlePluginVersion` and `GradleDependency` lint checks on the library to stop the yutailang0119/action-android-lint annotation from re-firing on the newer-but-still-not-9 baseline. Bumping `actions/setup-node` `node-version` from 20 to 24 in the Android e2e workflow keeps it consistent with the runner default.
Pre-validate cipher algorithm, key, and IV byte-lengths at the JS↔C++ boundary before reaching the native factory. Previously, wrong sizes fell through to OpenSSL which produced opaque error strings and (for algorithm typos) only failed inside the C++ factory. `validateCipherParams()` in `cipher.ts`: * Rejects empty / non-string `cipherType` with `TypeError`. * Rejects empty key (`byteLength === 0`) with `RangeError` upfront. * For OpenSSL ciphers, splits the existing `getCipherInfo(name, k, iv)` probe into three calls — name-only, name+keyLen, name+ivLen — so the thrown error names exactly which parameter is wrong (key vs iv) and reports the offending byte-length. * Rejects empty IV when the cipher requires one, and rejects non-empty IV for ciphers that take none (covers WRAP-style ciphers if a caller ever invokes one directly even though `getCiphers()` filters them). * Hard-codes (key=32, iv=24) for the libsodium ciphers `xsalsa20`, `xsalsa20-poly1305`, and `xchacha20-poly1305`, which OpenSSL's `EVP_CIPHER_fetch` does not see. Wired into both `Cipheriv` / `Decipheriv` constructors and the `xsalsa20()` `@noble/ciphers` shim, all of which now compute the ArrayBuffer once, validate, and reuse the exact buffer for the native call. Tests: 11 new regressions in `example/src/tests/cipher/cipher_tests.ts`: empty-name, unknown-name, too-short / too-long / empty key for AES-CBC, wrong IV for CBC and CCM, accepted variable IV for GCM, createDecipheriv mirror, and wrong-key + wrong-nonce for xsalsa20.
Pre-validate scrypt, HKDF, and Argon2 parameters at the JS↔C++ boundary
so invalid inputs surface as clear RangeErrors before the native call,
not as opaque OpenSSL/libsodium errors after.
* **Scrypt** (`scrypt.ts`) — RFC 7914 §6: N must be a power of 2 > 1, r
and p must be positive integers, r * p < 2^30, and the working set
128 * r * N must fit in maxmem. Both async (`scrypt`) and sync
(`scryptSync`) paths share `validateScryptParams()`. Async errors
flow through the callback as RangeError.
* **HKDF** (`hkdf.ts`) — RFC 5869 §2.3: L (output keylen in bytes) must
be ≤ 255 * HashLen. The new `validateHkdfKeylen()` looks the digest
up in a static `HKDF_HASH_BYTES` table covering sha1/224/256/384/512,
sha3-256/384/512, ripemd160. Wired into `hkdf`, `hkdfSync`, and the
WebCrypto `hkdfDeriveBits` path.
* **Argon2** (`argon2.ts`) — RFC 9106 §3.1 minimums: 1 ≤ p ≤ 2^24-1,
T ≥ 4, m ≥ 8 * p (in KiB), t ≥ 1, salt 8..2^32-1 bytes,
version ∈ {0x10, 0x13}. Async surfaces param errors via callback.
Existing argon2 tests that asserted the C++ `validateUInt`-style
messages ("non-negative", "out of range", "integer") are updated to
match the new RFC 9106 wording, since the JS-side check now fires
first. Adds 7 new RFC 9106 minimum-bound tests (parallelism = 0,
tagLength < 4, passes = 0, memory < 8*p, nonce < 8 bytes,
unsupported version, plus an existing-message refresh).
Tests: 8 new scrypt regressions (N=1, N not power of 2, fractional N,
negative r, p=0, working-set > maxmem, negative keylen, async
callback path), 5 new HKDF regressions (negative keylen, two
ceiling-violation cases for sha256 and sha1, ceiling-equal accepted,
async callback path), and 7 new Argon2 RFC 9106 minimum-bound checks
on top of the message refresh.
The TS layer previously rejected only `modulusLength < 256`. NIST SP 800-131A Rev. 2 has deprecated all RSA keys < 2048 bits for new cryptographic uses, and academic / hardware factoring of 768- and 1024-bit moduli has been demonstrated. Lift the minimum to 2048. `rsa.ts` — share `RSA_MIN_MODULUS_LENGTH = 2048` between the WebCrypto (`rsa_generateKeyPair`) and Node-API (`rsa_prepareKeyGenParams`) entry points. Both throw with a clear "RSA modulusLength must be at least 2048 bits (got N)" message; WebCrypto path remains a DOMException so existing JOSE / spec-conformant callers see the same exception type. Tests: bump 1024 → 2048 in `subtle/generateKey.ts` (3 RSA matrix entries) and `subtle/import_export.ts` (9 generateKey call-sites used to seed export round-trips). Refresh the existing modulusLength=0 rejection assertion to match the new error message, plus an explicit modulusLength=1024 case both at the WebCrypto boundary (`subtle.generateKey`) and the Node boundary (`generateKeyPairSync`).
The TS layer previously accepted any modulusLength > 0. FIPS 186-4 § 4.2 sanctions only L = 1024, 2048, 3072 bits for DSA, and DSA below 1024 bits is well below modern security minimums. We retain 1024 as the floor (rather than 2048) so legacy interop callers have a fallback path; new applications should still use 2048 or 3072. `dsa.ts` — pull the existing `modulusLength <= 0` check up to a shared `DSA_MIN_MODULUS_LENGTH = 1024` constant, swap the generic Error for RangeError, and emit a "DSA modulusLength must be at least 1024 bits (got N)" message that names the offending value. Tests: 2 new regressions in `keys/generate_keypair.ts`: modulusLength = 512 (between 0 and 1024) and the existing modulusLength = 0 path now both throw the new message.
Bring `subtle.ts` closer to the WebCrypto spec on four under-enforced
edges flagged in the audit:
* **Case-insensitive `normalizeAlgorithm`** (§18.4.4). The previous
implementation was a no-op, so `'aes-gcm'` reached the
`SUPPORTED_ALGORITHMS` set comparisons (which only know the canonical
mixed-case form) and failed downstream with confusing not-supported
errors. We now build a lazy lowercase→canonical map from
`SUPPORTED_ALGORITHMS` on first call and rewrite the algorithm name
to its canonical form. Lookup is lazy because `SUPPORTED_ALGORITHMS`
is declared after `normalizeAlgorithm` in the file.
* **JWK `ext` and `key_ops` validation** (§25.7.6). Added a shared
`validateJwkExtAndKeyOps()` helper that throws a `DataError` when
`jwk.ext === false` and the requested `extractable` is `true`, and
when `jwk.key_ops` is present but does not cover every requested
usage. Wired into KMAC, RSA, HMAC, AES, and Ed/CFRG JWK import
branches.
* **`subtle.deriveBits` strict usage gate** (§SubtleCrypto-method
-deriveBits step 11). The previous `deriveBits || deriveKey`
accept-either branch silently promoted `deriveKey`-only keys into
`deriveBits` use, contradicting the spec. Now requires `deriveBits`
on `baseKey.[[usages]]`. `subtle.deriveKey` still accepts either, in
line with Node-compat.
* **HKDF `extractable: false`** (§28.7.6). `hkdfImportKey` now throws
`SyntaxError` if the caller requests `extractable: true`, and forces
`extractable: false` on the resulting `CryptoKey` regardless of input
— matching the spec invariant that prevents input keying material
from round-tripping via `exportKey`.
Tests:
* `subtle/digest.ts` — `digest('sha-256')` (lowercase) round-trips.
* `subtle/deriveBits.ts` — PBKDF2 baseKey with only `deriveKey` usage
must reject the `deriveBits` operation.
* `hkdf/hkdf_tests.ts` — `extractable=true` rejection plus an
invariant check that `extractable=false` propagates.
* `subtle/import_export.ts` — three new AES JWK import paths:
`ext=false + extractable=true` rejection, `key_ops` missing the
requested usage rejection, and a positive-path `key_ops` superset.
…agation Synchronous failures inside `Hash`, `Hmac`, and `Cipher` `_transform` / `_flush` previously threw out of the Transform plumbing because the callback was only invoked on the success path. Throws are eventually converted to 'error' events by readable-stream, but the stream is left in a half-written state — the callback contract requires exactly one invocation, and a missing call can hang downstream consumers. Wrap each `_transform` and `_flush` body in a try/catch that forwards the thrown error through the callback so it emits as a regular stream 'error' event and the Transform sees the callback once on every code path. Also widen the callback parameter type from `() => void` to `(err?: Error | null) => void` so the new error-forwarding usage type- checks. Tests: 6 new regressions — Hash and Hmac `_transform` after digest() (invokes update() on a finalized native context) and `_flush` after digest() (double-finalize), plus Cipher `_transform` after final() and Decipher `_flush` with a tampered AES-GCM auth tag.
Tick the Phase 3 boxes (cipher params, KDFs, RSA/DSA modulus mins, subtle hardening, stream error propagation) and append progress-log entries for the CI warning sweep (PR #984 follow-up) and each of 3.1–3.6 with branch reference.
…getCipherInfo for IV length
Three follow-on tweaks to Phase 3.1 cipher param validation that landed
together but post-dated the audit commit:
* libsodium error messages — change "Invalid iv length" → "Invalid
nonce length" and "(expected N)" → "(nonce must be N bytes)" /
"(key must be N bytes)" so the wording matches the libsodium
parlance that callers expect for xsalsa20 / xsalsa20-poly1305 /
xchacha20-poly1305. Test regex is updated in lockstep.
* allCiphers loop key-length detection — add an explicit `DES-EDE3`
branch ahead of `DES-EDE`, since `'DES-EDE3-CBC'.includes('DES-EDE')`
is also true and the loop was silently picking the 16-byte 2-key
3DES path for 24-byte 3-key 3DES ciphers.
* allCiphers loop IV-length detection — replace the hard-coded
iv12 / iv16 split (only correct for AEAD vs everything else) with a
per-cipher `getCipherInfo(name)?.ivLength` query, so the loop covers
ECB (0), DES-family (8), AES (16), etc. without further hard-coding.
Also fix the `'strings'` test, which was passing `key16.toString('hex')`
(32 UTF-8 bytes) into `aes-128-cbc` — fine before validation, but now
rejected by the new key-length check. Use 16-char ASCII so the byte
length matches the cipher's (16, 16) sizes.
…KDF unknown-digest throw, type tightening, idiomatic stream tests Six follow-ups from the Phase 3 self-review: * **(2) cipher fast-path.** `validateCipherParams` now caches the `getCipherInfo(name)` result and short-circuits the common case where `(key, iv)` match the cipher's default lengths exactly (AES-CBC, ECB, AES-GCM with the canonical 12-byte IV, etc.). Native round-trip count drops from 3 → 1 on the happy path while still producing the per-parameter error message on failure. * **(3) drop unreachable Number.isFinite check.** `ArrayBuffer.byteLength` is always a non-negative integer; the only meaningful guard is `keyByteLength === 0`. * **(4) HKDF unsupported-digest throw.** `validateHkdfKeylen` previously silently skipped the RFC 5869 ceiling check for digests not in the `HKDF_HASH_BYTES` table — including XOFs like SHAKE128/256, which are invalid HKDF inputs (HKDF builds on HMAC, which requires a fixed-length hash). Throw `TypeError: Unsupported HKDF digest: …` instead, with a regression test for `shake128`. * **(5) Argon2 nonce single-resolution.** `validateArgon2Params` now returns the resolved `nonceAB: ArrayBuffer` and the caller passes it straight to native, dropping the second `binaryLikeToArrayBuffer` round-trip on `params.nonce`. * **(6) canonical-name map type.** Type the lazy `_canonicalAlgorithmNames` map as `Map<string, AnyAlgorithm>` and do the cast once at insertion (where `SUPPORTED_ALGORITHMS` is the contract source) rather than on every `normalizeAlgorithm` lookup. All `as AnyAlgorithm` casts in `normalizeAlgorithm` go away. * **(7) idiomatic stream tests.** Re-express the Phase 3.6 Hash / Hmac / Cipher / Decipher stream-error regressions through the public stream API: drive `_transform` via `stream.write()` and `_flush` via `stream.end()`, then `await` an `'error'` event. Removes every `(stream as any)._transform / _flush(...)` cast and the matching `eslint-disable @typescript-eslint/no-explicit-any` annotations.
Contributor
🤖 End-to-End Test Results - iOSStatus: ✅ Passed 📸 Final Test ScreenshotScreenshot automatically captured from End-to-End tests and will expire in 30 days This comment is automatically updated on each test run. |
Contributor
🤖 End-to-End Test Results - AndroidStatus: ✅ Passed 📸 Final Test ScreenshotScreenshot automatically captured from End-to-End tests and will expire in 30 days This comment is automatically updated on each test run. |
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
Phase 3 of the security audit: pre-validate cryptographic parameters at the TypeScript ↔ C++ boundary so invalid inputs surface as clear
RangeError/TypeError/DOMExceptionbefore reaching native, instead of as opaque OpenSSL / libsodium error strings (or, in a few cases, undefined behavior). Also brings WebCryptosubtlecloser to spec on four under-enforced edges, and ensures stream_transform/_flusherrors propagate as'error'events instead of throwing through the Transform plumbing.Plus: a CI sweep that fixes the 6 GitHub Actions warnings that surfaced on PR #984's runs.
Sub-tasks
subtlehardening_transform/_flusherror propagationBreaking changes
These are intentional and aligned with current cryptographic best practice — call them out in release notes:
modulusLength < 2048will now get an explicit rejection (was 256). Aligns with NIST SP 800-131A.modulusLength < 1024will now be rejected (previously only≤ 0was rejected). 1024 retained over the stricter 2048 for legacy interop. Aligns with FIPS 186-4 §4.2.extractable: truenow throwsSyntaxError. The resultingCryptoKeyis forced toextractable: falseregardless of input. WebCrypto §28.7.6.subtle.deriveBits: now strictly requires the literalderiveBitsusage on the base key (wasderiveBits || deriveKey).subtle.deriveKeyis unaffected and still accepts either, since it decomposes toderiveBits + importKey.jwk.ext === falsewithextractable: trueis now rejected;jwk.key_opsmissing any requested usage is now rejected.'aes-gcm'→'AES-GCM'). Behavior fix only — existing canonical-form callers are unaffected.Changes
TS sources
src/cipher.ts—validateCipherParams()with single-call fast-path; libsodium nonce wording; wired intoCipheriv/Decipherivconstructors and thexsalsa20()shim.src/scrypt.ts—validateScryptParams()(RFC 7914 §6) andvalidateScryptKeylen(); covers async + sync paths.src/hkdf.ts—validateHkdfKeylen()(RFC 5869 §2.3); rejects unsupported digests (e.g. SHAKE128, since HKDF requires HMAC); covers sync, async, and WebCryptohkdfDeriveBits.src/argon2.ts—validateArgon2Params()(RFC 9106 §3.1); single-resolution ofparams.nonceshared with the native call.src/rsa.ts—RSA_MIN_MODULUS_LENGTH = 2048shared between WebCrypto and Node-API entry points; WebCrypto path remains aDOMExceptionfor JOSE callers.src/dsa.ts—DSA_MIN_MODULUS_LENGTH = 1024; genericError→RangeErrorwith offending value in the message.src/subtle.ts— case-insensitivenormalizeAlgorithmwith lazy lower → canonicalMap<string, AnyAlgorithm>;validateJwkExtAndKeyOps()helper wired into KMAC / RSA / HMAC / AES / Ed/CFRG JWK import; strictderiveBitsusage gate; HKDFextractable: falseinvariant.src/hash.ts,src/hmac.ts,src/cipher.ts—_transform/_flushwrap in try/catch and forward errors via the stream callback so they emit as regular'error'events.CI / tooling
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true'(silences Node 20 deprecation forsetup-java@v4,upload/download-artifact@v4,setup-android@v3,peter-evans/*,McCzarny/*,reviewdog/*).actions/checkout@v4→@v5andactions/setup-node@v4→@v5everywhere.${{ github.run_id }}→${{ hashFiles(...) }}for Gradle /node_modules; stableavd-pixel7pro-34-x86_64-v1for the AVD cache. Fixes the "another job may be creating this cache" save failures.classpath8.7.3 → 8.12.2 (matches Nitro).AndroidGradlePluginVersionandGradleDependencylint checks disabled, since AGP 9.x requires Gradle 9 + JDK 21 which RN 0.81's toolchain can't supply.Tests
~50 new regressions across:
tests/cipher/cipher_tests.ts— 11 param-validation tests + 2 stream-error event tests;'strings'test fixed to use 16-char ASCII; DES-EDE3 / DES-EDE detection order fix in theallCiphers.forEachloop;getCipherInfo-driven IV-length lookup.tests/scrypt/scrypt_tests.ts— 8 RFC 7914 minimum-bound + async callback tests.tests/hkdf/hkdf_tests.ts— 5 RFC 5869 ceiling tests + HKDF non-extractable / force-false invariant + unsupported-digest test.tests/argon2/argon2_tests.ts— 7 RFC 9106 minimum-bound tests; existingvalidateUInt-style assertions refreshed to match the new wording (the JS-side check now fires first).tests/keys/generate_keypair.ts— 1 RSA + 2 DSA modulus-rejection tests.tests/subtle/{generateKey,import_export,deriveBits,digest}.ts— 12 RSA fixtures bumped 1024 → 2048; explicitmodulusLength=1024rejection at both boundaries; lowercase-digest test; deriveBits-usage gate test; AES JWK ext / key_ops triad.tests/hash/hash_tests.ts,tests/hmac/hmac_tests.ts— 2 stream-error event tests each.bun tscpasses clean across both packages.Commits
Test plan
bun tsc(clean across both packages) ✓ verified locally