feat!: decouple public key and data types from node:crypto - #562
Closed
cjbarth wants to merge 1 commit into
Closed
Conversation
Every public signature that touched key material named `node:crypto` directly, which hard-coded Node into the type surface: a caller could not pass a `CryptoKey`, the only key representation Web Crypto produces, so the types blocked a Web Crypto backend before any implementation existed. Introduce `BinaryLike` and `KeyLike` in `src/types.ts` and use them for `SignedXmlOptions.privateKey`/`publicCert`, `GetKeyInfoContentArgs.publicCert` and the `SignatureAlgorithm` interface. `SignatureAlgorithm` takes the accepted key type as a parameter rather than declaring the whole union, so each implementation states what it can really use and nothing casts back out. The bundled algorithms declare `crypto.KeyLike | Uint8Array` (`string | Buffer` for MGF1, which needs a key it can put in a `SignPrivateKeyInput`), and a `Uint8Array` is now viewed as a `Buffer` instead of reaching OpenSSL as `ERR_OSSL_UNSUPPORTED`. An `ArrayBuffer` of data is likewise viewed rather than cast, so the widened data type is true for them. The algorithm is looked up by a URI read from the document, so a JavaScript caller can still pair a `CryptoKey` with a Node algorithm without the compiler seeing it. Node answers that by accepting the key through its DEP0203 shim: it signs, the tests pass, and the signature is attributed to a key the algorithm never supported. Reject it explicitly instead. BREAKING CHANGE: implementers of `SignatureAlgorithm` should declare the key type they accept, e.g. `implements SignatureAlgorithm<crypto.KeyLike>`. The bundled algorithms now throw on key material `node:crypto` cannot use rather than silently accepting a `CryptoKey`. Closes #545 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
Contributor
Author
|
Closed in favour of #570 — the same commits, opened from 🤖 Generated with Claude Code |
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 #545
Every public signature that touched key material named
node:cryptodirectly, which hard-coded Node into the type surface: a caller could not pass aCryptoKey— the only key representation Web Crypto produces — so the types blocked a Web Crypto backend before any implementation existed.BinaryLikeandKeyLikeare now defined insrc/types.tsand used forSignedXmlOptions.privateKey/publicCert,GetKeyInfoContentArgs.publicCertand theSignatureAlgorithminterface.How the bundled algorithms narrow, without blanket casts
The issue offers three options; this takes the first.
SignatureAlgorithmtakes the accepted key type as a parameter, so each implementation states what it can really use:RsaSha1,RsaSha256,RsaSha512,HmacSha1SignatureAlgorithm<crypto.KeyLike | Uint8Array>RsaSha256Mgf1SignatureAlgorithm<string | Buffer>— it needs a key it can put in aSignPrivateKeyInputNothing casts back out. Verified that the compiler now rejects what it should:
The two arms the issue flagged as broken are now genuinely supported rather than narrowed away:
Uint8Arrayis viewed as aBuffer(Buffer.from(key.buffer, key.byteOffset, key.byteLength), no copy) instead of reaching OpenSSL asERR_OSSL_UNSUPPORTED.ArrayBufferdata is viewed the same way, soBinaryLikeis true for these algorithms. Test asserts anArrayBuffersigns to the same bytes as the equivalent string.The residual hole, and why it fails closed
The signature algorithm is looked up by a URI read from the document under inspection, so a JavaScript caller can still pair a
CryptoKeywith a Node algorithm without the compiler seeing it. Confirmed onmasterthat Node does not fail in that case — it signs:The signature verifies and the tests pass, so the algorithm appears to support a key it never really did, and it breaks whenever Node removes the shim. On this branch:
That check is the one piece of defensive code here; there is a comment saying why it exists despite the parameter type.
Notes
CryptoKeyis spelledcrypto.webcrypto.CryptoKeyso it resolves underlib: ["es2020"]on the@types/node@16floor. A DOM or globalCryptoKeyis structurally identical and assignable.getKeyInfoContentstill silently emits an empty<X509Data/>for key material it cannot read as PEM. That predates this change (aKeyObjectdoes it today) and is left alone.Breaking
Source-compatible for callers. A break for anyone who implements
SignatureAlgorithm: declare the key type, e.g.implements SignatureAlgorithm<crypto.KeyLike>. READMEUpgradingand a new "Declaring the key material your algorithm accepts" section cover it.Verification
npm run build && npm test && npm run lintclean; 244 passing (241 + 3). New tests intest/key-material-tests.spec.tswere confirmed to fail onmasterfor the right reasons — two as compile errors, theCryptoKeyone becausemastersigns successfully.🤖 Generated with Claude Code