chore: housekeeping sweep across the cryptography package - #8
Merged
Merged
Conversation
Combines a security fix, several correctness fixes, public-API tightening, test-framework migration, and structural cleanup so that the package no longer ships with broken / dishonest infrastructure. Security: - Remove debug `console.log(privateKey)` from `rawSignature` (file later deleted along with the rest of the unused signature module) Correctness: - Stop mutating the caller's input array in `poseidonFunc` - Fix `initPoseidon`'s always-false `typeof === 'function'` check - Drop `autoInitializeEddsa` (had inverted poseidon dispatch and depended on a `circomlibjs` patch that was never applied because `patch-package` was not in devDependencies) - Make AES helpers return real `Uint8Array`s instead of `Buffer` instances typed as Uint8Array - Add proper init guards on `eddsa.*` operations with descriptive errors - Type the circomlibjs eddsa build object instead of relying on `any` Test framework: - Migrate from a `npx mocha` script that never worked (mocha not installed, files use `node:test`) to `brittle`, the workspace standard - Drop nested-`it()` and empty-`catch` test patterns that always passed - Drop in-suite micro-benchmarks - Add the missing AES coverage (encrypt/decrypt roundtrip, key-length validation, GCM tag tampering) API + structure: - Rename misspelled `primatives/` folder to `primitives/` (no consumer imports sub-paths, so the package boundary is unchanged) - Drop the `poseidonLib` namespace re-export that leaked the entire `poseidon-lite` library surface - Eliminate the `primitives/index <-> poseidon-lite` circular import - Stop running `initializePoseidonFuncs` as a side effect at module load (the populated array was dead code) - Remove unused public exports: `signature.ts`, `hash.ts`, `poseidon-module.ts`, `poseidonHex`, `getPoseidonFunc`, plus the dead `@noble/secp256k1` dependency - Delete `patches/` (`patch-package` was never wired up) Docs: - Replace the placeholder README with one that documents the actual primitives, init order, and usage examples
- verifyEDDSA was calling .reverse() in place on the caller's signature.R8 and pubkey arrays, silently corrupting them across consecutive verifies. Wrap each input in `new Uint8Array(...)` before reversing, matching signPoseidon. New test asserts that two consecutive verifies on the same inputs both succeed and that no input array is mutated. - decryptGCM length-validation errors were wrapped inside the generic "Unable to decrypt ciphertext" cause and lost in caller logs. Move the guards above the try/catch so the specific message surfaces. Tests updated and two new negative tests cover wrong-length iv and tag. - Annotate the 16-byte AES-GCM iv as a deliberate wire-format choice (NIST recommends 12; existing at-rest ciphertexts assume 16).
- Add a negative test for decryptCTR's iv-length validation. Mirrors the GCM iv/tag tests added earlier; closes the corresponding gap in CTR coverage. - Remove eddsa.genRandomPoint. It was a one-line `poseidon([randomBytes(32)])` wrapper with no consumer in the workspace; deleting raises the bar over testing dead code. Coverage: 100% functions, 98.56% lines/statements, 95.77% branches. The remaining uncovered lines are defensive guards (assertEddsaReady throw branch, poseidon-lite shape-mismatch checks) that require child-process isolation or library mocking to exercise honestly.
- sha256 was being used by wallet-sdk/services/wallet/wallet-id.ts to generate deterministic wallet IDs. Removing it would have changed every consumer's wallet IDs after upgrade — a critical regression. Restore hash.ts and re-add the export to primitives/index. Verified by running wallet-sdk's wallet-id vector tests against local cryptography (7/7 match the canonical IDs). - Drop the _injectPoseidon parameter from initializeEddsa now that the wallet-node call site has been updated in railgun-reloaded/wallet-node#19. The transition shim is no longer needed. - Add a sha256 test file (NIST FIPS-180-4 test vectors).
- Add a single CryptographyError class extending Error with a `code`
discriminator (matching the BytesError pattern). Codes cover every
failure mode in the package. All `throw new Error(...)` sites now use
this class so consumers can branch on `err.code` rather than parsing
message strings.
- Convert internal `interface` declarations to `type` aliases for
consistency with the rest of the package.
- Drop unnecessary `as Uint8Array` casts in eddsa/index.ts: the typed
EddsaBuild interface already declares the right return types.
- Migrate the test suite from brittle to node:test (assertions via
node:assert/strict). Drops `brittle` and `@types/brittle` from
devDependencies. The npm test script now invokes `node --test`.
- Tests now assert error codes via the {name, code} matcher form,
giving a typed contract instead of regex-matching messages.
A deeper read of the engine source showed two of the previously-removed exports are not actually dead code — they have no current consumer in reloaded yet but engine relies on them, and reloaded packages will need them as they catch up to engine functionality: - poseidonHex: engine uses it in merkletree.ts (sibling-node hashing), abstract-wallet.ts, and keys-utils.ts (getRandomScalar). - initPoseidonPromise: engine awaits it twice in railgun-engine.ts startup as the canonical "poseidon is ready" gate. Restore both with engine-compatible semantics: - poseidonHex(inputs: string[]): string accepts hex strings with or without the 0x prefix and any nibble length, returning a 64-character lowercase hex digest. Verified against engine's reference vectors (poseidon(0, 1) === 1bd20834...e65e). - initPoseidon prefers WASM and falls back to pure-JS, matching the one-shot init shape engine consumers expect. The other engine-checked items (rawSignature/formatEthMessage/secp256k1, autoInitializeEddsa, getPoseidonFunc, initializePoseidonFuncs, poseidonLib namespace) all confirmed absent from engine and stay removed.
initPoseidon was the only remaining throw site still using the generic Error class, leftover from before the CryptographyError migration. The defensive `if (!(cause instanceof Error))` guard around the wasm fallback was also paranoia — circomlibjs throws Error instances, and forcing the caller to distinguish "wasm threw an Error" from "wasm threw something else" served no real purpose. Drop the guard, simplify the control flow, and surface the final pure-JS failure as CryptographyError(PoseidonNotLoaded) with the underlying cause attached.
mattgle
marked this pull request as ready for review
May 8, 2026 13:36
Collaborator
|
I had to do this in order to make the install and test work: Adding |
Contributor
Author
Yes! We're aware, we just merged a PR that handles that a few hours ago. This is just until we have packages released to NPM. |
simonmasson
force-pushed
the
chore/cryptography-housekeeping
branch
from
May 12, 2026 10:30
5e5eabc to
0154164
Compare
|
While engine doesn't handle secp256k1, it is used by the likes of TokenShielder for the shield message. That feature is better served by ethersjs/web3js so it's fine to remove here but noting the reasons. |
mesquka
approved these changes
May 12, 2026
Lockfile previously pinned @railgun-reloaded/bytes to a commit that predates its prepare script, so CI installs of cryptography ended up with an unbuilt bytes (missing src/index.js). Refreshing the lock to current bytes#dev (4980c5c) brings in the prepare hook and the build runs on git+https consumers.
Drop inline `bytesToBig`, `bigToBytesBE`, and `fromHex` helpers in `test/eddsa.test.ts` and `test/aes.test.ts` in favour of the canonical `bytesToBigInt`, `bigIntToBytes`, and `hexToBytes` exports from `@railgun-reloaded/bytes`. The bytes-package versions throw on negative values, overflow, and malformed hex rather than silently producing malformed output.
simonmasson
force-pushed
the
chore/cryptography-housekeeping
branch
from
May 12, 2026 16:13
044c219 to
a94ff14
Compare
… test Drop the inline bigToBytes32 helper from test/poseidon.test.ts in favour of bigIntToBytes(value, 32) from @railgun-reloaded/bytes. Continuation of the earlier refactor that did the same for the eddsa and aes test files. The bytes-package version throws NegativeValue on negative input and BigIntOverflow on values that do not fit in the given byte length, instead of silently returning a zero-padded or truncated array.
bhflm
approved these changes
May 12, 2026
mattgle
added a commit
to railgun-reloaded/wallet-node
that referenced
this pull request
May 12, 2026
The argument to initializeEddsa(poseidonBuild.pure) was already a no-op: the cryptography package shipped a circomlibjs patch that would have made it effective, but patch-package was never wired up so circomlibjs's upstream buildEddsa was always the unpatched version that ignores its argument. The injection has been removed from cryptography (see railgun-reloaded/cryptography#8); drop the call-site argument and the now-unused poseidonBuild import. No behavior change: eddsa already builds its own internal poseidon regardless of what was passed in.
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
5 participants
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.
Context
@railgun-reloaded/cryptographywas shipping a stack of broken infrastructure: aconsole.log(privateKey)debug line, anpx mochatest script with no mocha installed, tests that always passed via nested-it()and empty-catch, an EDDSA dispatch that depended on acircomlibjspatch that was never applied, dead code, an in-place mutation of caller arrays inposeidonFunc, AES helpers returningBuffertyped asUint8Array, a misspelledprimatives/folder, and a placeholder README.This PR is the housekeeping pass. The diff has the full list — this description focuses on the load-bearing decisions and the cross-package verification.
Why the
circomlibjspatch is removedThe patch made
circomlibjs.buildEddsa()accept aninjectPoseidonargument so eddsa could sharewallet-node's already-loaded poseidon. It was a no-op in production:patch-packagewas never in cryptography's deps and there was nopostinstall, so on a clean install the.patchfile was never applied. circomlibjs always built its own poseidon and ignored the argument.The behavior was also asymmetric:
merkletree-manager's postinstall did walk into its own copy of cryptography and apply the patch, so the patch was effective when cryptography was reached via merkletree-manager but not whenwallet-nodeinstalled it directly. Same package, different behavior depending on install path.Three options: wire
patch-packagein properly (adds a dep + postinstall hook + maintenance burden), upstream the change (out of scope), or drop the patch. Picked drop. No correctness change — both poseidons are the same circomlibjs poseidon over the same BabyJubJub field; signatures are bit-identical.wallet-nodedrops the now-unused argument in railgun-reloaded/wallet-node#19.Why
@noble/secp256k1is removedUsed by exactly one file:
signature.ts, which exportedrawSignature(Ethereum signed-message signing),formatEthMessage, andpointConversion. No consumer in the workspace imports any of them, and engine has no equivalent code path either — verified by grepping engine forEthereum Signed Message/secp256k1, zero matches. The exports were dead code with no engine basis.Verification across consumers
Ran every consumer's relevant test suite against this branch via
file:..substitution. All passed:poseidonHexvectors145 tests, all green. Existing wallet IDs and ciphertexts continue to decrypt and verify identically.
Follow-up
initializeEddsa(poseidonBuild.pure)arg.merkletree-manager's postinstall still runsnpx patch-packageagainst an empty patches/ folder — harmless no-op, could be simplified later.