Added web-crypto support (#1) #513
Conversation
* Added web-crypto support * fix: review comments * fix: types * fix: review comments * fix: review comments * fix: review comments * fix: review comments * fix: review comments * fix: review comments * fix: review comments * fix: update the types
WalkthroughThis pull request adds comprehensive Web Crypto API support to xml-crypto, enabling use in browser and WinterCG environments (e.g., Cloudflare Workers). Introduces async-first hash and signature algorithm implementations alongside type abstractions decoupling from Node.js crypto, maintaining backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SignedXml
participant SigAlgo as Signature Algorithm
participant WebCrypto
rect rgb(200, 230, 255)
Note over App,WebCrypto: Async Callback-based Signing
App->>SignedXml: sign(callback)
SignedXml->>SignedXml: Prepare references & SignedInfo
SignedXml->>SigAlgo: getSignature(data, key, callback)
SigAlgo->>SigAlgo: Normalize key (PEM/KeyObject/CryptoKey)
SigAlgo->>WebCrypto: crypto.subtle.sign()
WebCrypto-->>SigAlgo: signature (ArrayBuffer)
SigAlgo->>SigAlgo: Base64 encode
SigAlgo-->>SignedXml: callback(null, signature)
SignedXml-->>App: callback(null, signed XML)
end
rect rgb(220, 240, 220)
Note over App,WebCrypto: Async Callback-based Verification
App->>SignedXml: checkSignature(callback)
SignedXml->>SignedXml: Extract signature & references
SignedXml->>SigAlgo: verifySignature(material, key, sig, callback)
SigAlgo->>SigAlgo: Normalize key
SigAlgo->>SigAlgo: Decode base64 signature
SigAlgo->>WebCrypto: crypto.subtle.verify()
WebCrypto-->>SigAlgo: boolean result
SigAlgo-->>SignedXml: callback(null, isValid)
SignedXml-->>App: callback(null, isValid)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: The changes span 11 files with mixed complexity. Key drivers include: (1) intricate Web Crypto key normalization and import logic in Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/signed-xml.ts (2)
170-178: Restore default KeyInfo certificate extractionAssigning
getCertFromKeyInfotoSignedXml.noopwipes out the built-in X.509 extraction when no override is supplied, so verification stops finding the embedded cert and fails even with valid signatures. Keep the original helper as the default.- this.getKeyInfoContent = getKeyInfoContent ?? this.getKeyInfoContent; - this.getCertFromKeyInfo = getCertFromKeyInfo ?? SignedXml.noop; + this.getKeyInfoContent = getKeyInfoContent ?? this.getKeyInfoContent; + this.getCertFromKeyInfo = getCertFromKeyInfo ?? SignedXml.getCertFromKeyInfo;
467-488: Prevent async verify from being reported as “invalid signature”WebCrypto-enabled
verifySignatureimplementations return a Promise. In the sync path that Promise lands here, falls through the=== truecheck, and we throw an “invalid signature” error even though verification would succeed once awaited. Detect a Promise/thenable and direct callers tocheckSignatureAsync()instead of giving a false negative.- const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); - if (sigRes === true) { + const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); + if (sigRes && typeof (sigRes as PromiseLike<boolean>).then === "function") { + const err = new Error( + "Async signature algorithms cannot be used with sync methods. Use checkSignatureAsync() instead.", + ); + if (callback) { + callback(err, false); + return; + } + throw err; + } + if (sigRes === true) {
🧹 Nitpick comments (1)
WEBCRYPTO.md (1)
156-163: Fix markdownlint MD031 around migration code blockmarkdownlint is flagging this fenced block because it lacks the required blank line before the indented fence in list item 2. Add a blank line before (and keep one after) the fence to satisfy MD031.
-2. Update method calls to async: - ```javascript +2. Update method calls to async: + + ```javascript // Before sig.computeSignature(xml); // After await sig.computeSignatureAsync(xml); ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
WEBCRYPTO.md(1 hunks)example/webcrypto-example.js(1 hunks)src/hash-algorithms-webcrypto.ts(1 hunks)src/index.ts(1 hunks)src/signature-algorithms-webcrypto.ts(1 hunks)src/signature-algorithms.ts(4 hunks)src/signed-xml.ts(14 hunks)src/types.ts(5 hunks)src/webcrypto-utils.ts(1 hunks)test/document-tests.spec.ts(1 hunks)test/signature-unit-tests.spec.ts(5 hunks)test/webcrypto-tests.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
example/webcrypto-example.js (2)
src/signed-xml.ts (1)
SignedXml(39-1771)src/signature-algorithms-webcrypto.ts (1)
WebCryptoRsaSha256(207-256)
src/signature-algorithms-webcrypto.ts (2)
src/types.ts (2)
SignatureAlgorithm(170-198)createAsyncOptionalCallbackFunction(271-290)src/webcrypto-utils.ts (5)
importRsaPrivateKey(63-79)arrayBufferToBase64(34-41)importRsaPublicKey(87-146)base64ToArrayBuffer(48-55)importHmacKey(154-170)
test/webcrypto-tests.spec.ts (3)
src/webcrypto-utils.ts (2)
importRsaPrivateKey(63-79)importRsaPublicKey(87-146)src/signature-algorithms-webcrypto.ts (4)
WebCryptoRsaSha256(207-256)WebCryptoRsaSha1(152-201)WebCryptoRsaSha512(262-311)WebCryptoHmacSha1(317-363)src/signed-xml.ts (1)
SignedXml(39-1771)
test/signature-unit-tests.spec.ts (1)
src/types.ts (2)
BinaryLike(17-17)KeyLike(23-23)
src/signed-xml.ts (1)
src/types.ts (4)
KeyLike(23-23)Reference(120-150)ComputeSignatureOptions(110-115)ErrorFirstCallback(11-11)
src/signature-algorithms.ts (1)
src/types.ts (4)
SignatureAlgorithm(170-198)createOptionalCallbackFunction(241-263)BinaryLike(17-17)KeyLike(23-23)
🪛 markdownlint-cli2 (0.18.1)
WEBCRYPTO.md
165-165: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
* fix coderabbit review comments * fix: formatting
src/signed-xml.ts
Outdated
| this.keyInfoAttributes = keyInfoAttributes ?? this.keyInfoAttributes; | ||
| this.getKeyInfoContent = getKeyInfoContent ?? this.getKeyInfoContent; | ||
| this.getCertFromKeyInfo = getCertFromKeyInfo ?? SignedXml.noop; | ||
| this.getCertFromKeyInfo = getCertFromKeyInfo ?? this.getCertFromKeyInfo; |
There was a problem hiding this comment.
@markusahlstrand that function and how it was used by default was at the heart of GHSA-2xp3-57p7-qf4v ( CVE-2024-32962 ). It is proposed to be removed completely see #470 (comment)
Unless your PR introduce safe way to use that it must be result of some vibe coding or similar session(*). I did not read through your PR because because it is not organized to coherent commits. I just spotted @cjbarth 's comment which was related to reintroduction of that particular implementation and felt that history attached to it must be highlighted.
(*) Referring to your comment at
It looks like it could be done without to many changes by using the callback and implementing a new hash-algorithm and signature-algorihm. Maybe this is good grunt work for Claude :)
and
Tried it out and did a PR: #513 . It is AI generated but I also reviewed it and checked it with code rabbit. It's a pretty big change so I'm not sure it really fits into the scope of this project? If not just let me know and I'll use the fork instead and try to keep it up to date.
sidenote: I do not remember whether test case to spot reintroduction of CVE-2024-32962 was added.
There was a problem hiding this comment.
This change is reverted. I changed the approach now and focused on getting the web crypto support in using the callback functionality with the minimal changes needed.
src/signed-xml.ts
Outdated
|
|
||
| const doc = new xmldom.DOMParser().parseFromString(xml); | ||
|
|
||
| // Security: Prevent cross-document signature reuse attacks while supporting |
There was a problem hiding this comment.
If this is strictly security, can we get it in a separate PR? There are some security-oriented folks that watch this repo closely and would be very interested in checking over any changes calling out security issues.
There was a problem hiding this comment.
This was something the coderabbit highlighted, but it's totally out of scope for this PR.
I'll revert most changes in this file and only keep the minimum needed to get the web crypto support in. I'll remove the acync functions as well and we can use the callbacks for now which will reduce the amount of changes.
| * @returns void | ||
| * @throws TypeError If the xml can not be parsed. | ||
| */ | ||
| computeSignature(xml: string): void; |
There was a problem hiding this comment.
There is something off with this diff, maybe whitespace? Can you see what might be going on as this is very difficult to review when the diff is so misaligned and unchanged lines appear changed.
There was a problem hiding this comment.
Yes, there was a lot of indentation and unnecessary restructuring. I hope it's better now, even though it's still larger than I hoped. The basic changes are:
- KeyLike, support for node.js crypto and web crypto
- Added callback support to functions
- Updated algorithm interfaces
- Updated eror handling to support callbacks
| // Check if this is a certificate | ||
| if (pem.includes("BEGIN CERTIFICATE")) { | ||
| // For certificates, we need to extract the public key | ||
| // This is a basic implementation - for production use, consider using a proper ASN.1 parser |
There was a problem hiding this comment.
Why would we be adding support for WebCrypto if not for use in production?
There was a problem hiding this comment.
Yes, this comment is an artifact from the AI and should be clarified.
My understanding is that web crypto doesn't support x.509 certificates like node does. I have used the @peculiar/x509 library before to handle this, but I'm not sure if it's worth adding an external dependency or if it's better to limit the support to SPKI with web crypto?
* feat: add minimal WebCrypto support via Promise detection - Add KeyLike type import from types module - Remove Node.js crypto module dependency for type definitions - Detect async signature algorithms in checkSignature() and wrap Promise in callback - Detect async hash algorithms in validateReference() and throw helpful error - Detect async signature generation in calculateSignatureValue() and throw helpful error - Maintain full backward compatibility with synchronous algorithms - Users must use callback form when using async algorithms * test: convert WebCrypto tests from async/await to callbacks - Convert all integration tests to use callback-style API - Skip security-related tests that were not in original code - Tests now work with minimal WebCrypto changes (no async methods) * feat: add async hash algorithm support for WebCrypto - Modified createReferences() to detect and handle async hash algorithms * Uses placeholder pattern to batch Promise.all() for async digests * Returns Promise<string> when async hashes detected - Modified createSignedInfo() to propagate async handling from createReferences() - Modified computeSignature() to handle async SignedInfo creation * Extracts signature creation logic to helper function * Detects Promise from createSignedInfo and waits for completion * Requires callback when async hash algorithms used - Modified checkSignature() to handle async reference validation * Created validateReferenceInternal() to return boolean | Promise<boolean> * Uses Promise.all() to batch async reference validations * Added verifySignatureValue() helper for signature verification - Updated WebCrypto tests with manual signature loading * Added xmldom import for DOM parsing * Load signature before checkSignature() calls * Updated error message expectations for async detection - Skipped non-WebCrypto security tests that require removed features All WebCrypto tests now passing with minimal changes to core library * refactor: reduce code duplication in signed-xml.ts - Consolidated signature verification logic by using verifySignatureValue() helper in both sync and async paths of checkSignature() - Removed duplicate Promise handling for signature algorithms - Consolidated DigestValue XML generation in createReferences() - Net reduction of ~34 lines of duplicated code Tests: All 221 tests still passing * minimize the changes to signed-xml.ts * first path a minmizing the changes * Remove generics wrapper and update web crypto to use callbacks * Deduplicate computeSignature * Update the webcrypto-example * Remove async helper and update webcrypto docs * Removed deprecated function call * Reverted processSignedInfo helper * Rename variables * Refactor compute signature * Restructure to minimize visual diff * Refactor createReferences
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/signed-xml.ts (1)
702-706: Normalize string input to the Signature element (prevents detached flow regressions)When a string is passed, you store the Document, not the
<Signature>element. Normalize todocumentElementto maintain consistent node shape.- if (typeof signatureNode === "string") { - this.signatureNode = signatureNode = new xmldom.DOMParser().parseFromString(signatureNode); - } else { + if (typeof signatureNode === "string") { + const parsedDoc = new xmldom.DOMParser().parseFromString(signatureNode, "text/xml"); + if (!parsedDoc.documentElement) { + throw new Error("Parsed signature document has no root element"); + } + this.signatureNode = parsedDoc.documentElement; + signatureNode = this.signatureNode; + } else {
🧹 Nitpick comments (7)
src/types.ts (1)
24-25: Clarify KeyLike usage across signing vs. KeyInfo/cert APIs
KeyLike = crypto.KeyLike | CryptoKey | Uint8Arrayis broad. For APIs that emit X509Data (e.g.,GetKeyInfoContentArgs.publicCert), a rawUint8ArrayorCryptoKeywon’t work. Consider:
- Narrowing type at those call sites (e.g.,
string | Buffer) or- Documenting that non-PEM inputs will omit X509Data.
This reduces confusion for consumers.
test/signature-unit-tests.spec.ts (1)
767-846: Add a test for async digest path (createReferences callback)You exercise async
getSignature, but not async digest calculation increateReferences. Add a case whereHashAlgorithms[sha1]is wrapped viacreateOptionalCallbackFunctionandcomputeSignatureis invoked with a callback to ensure the async digest path is correct. This would catch aggregation bugs early.WEBCRYPTO.md (2)
146-149: Node.js WebCrypto availability phrasingNode’s global
crypto.subtleis not guaranteed until newer releases; on many versions it’s exposed underrequire('node:crypto').webcrypto.subtle. Suggest updating:
- “WebCrypto is available in Node.js 15+ via crypto.webcrypto.subtle; newer Node may expose a global ‘crypto’. Use a fallback.”
31-71: Fix markdown fence spacing and add Node fallback snippet
- Add blank lines around fenced code blocks (MD031).
- Provide a small Node fallback example:
In Node.js: ```js import { webcrypto as nodeWebCrypto } from "node:crypto"; globalThis.crypto = globalThis.crypto || (nodeWebCrypto as any);Also applies to: 73-104, 221-281 </blockquote></details> <details> <summary>example/webcrypto-example.js (1)</summary><blockquote> `14-18`: **Ensure `globalThis.crypto` exists in Node before using WebCrypto** Add a tiny fallback so the example works on Node versions where `crypto` isn’t global: ```diff import { SignedXml, WebCryptoRsaSha256, WebCryptoSha256 } from "../lib/index.js"; import { readFileSync } from "fs"; import { createPublicKey } from "crypto"; import { DOMParser } from "@xmldom/xmldom"; +// Node fallback: expose WebCrypto on globalThis if needed +try { + // eslint-disable-next-line no-undef + if (!globalThis.crypto?.subtle) { + const { webcrypto } = await import("node:crypto"); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (globalThis as any).crypto = (globalThis as any).crypto || webcrypto; + } +} catch { /* noop */ }This mirrors the library guidance and prevents
crypto is not definedfailures.src/signature-algorithms-webcrypto.ts (2)
40-42: Optional: Consider documenting the constructor.name limitation.The
constructor.name === "Buffer"check works but is fragile (minification or subclassing could break it). The comment explains the browser-safety rationale, which is good. Consider adding a note that this approach may not detect Buffer subclasses or minified code, though it's acceptable given the browser-safety constraint.
380-380: Align material conversion with other RSA classes.Lines 220, 300, and 462 use
new TextEncoder().encode(material)for consistency, while this line usestoArrayBuffer(material). Both work sincematerialis a string, but for maintainability all four classes should use the same approach.Apply this diff for consistency with the other RSA classes:
- const data = toArrayBuffer(material); + const data = new TextEncoder().encode(material);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
WEBCRYPTO.md(1 hunks)example/webcrypto-example.js(1 hunks)src/hash-algorithms-webcrypto.ts(1 hunks)src/signature-algorithms-webcrypto.ts(1 hunks)src/signed-xml.ts(14 hunks)src/types.ts(4 hunks)test/signature-unit-tests.spec.ts(1 hunks)test/webcrypto-tests.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/webcrypto-tests.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/signed-xml.ts
🧬 Code graph analysis (6)
test/signature-unit-tests.spec.ts (1)
src/signed-xml.ts (2)
SignedXml(29-1517)loadSignature(701-790)
example/webcrypto-example.js (4)
src/signed-xml.ts (1)
SignedXml(29-1517)src/index.ts (3)
SignedXml(6-6)WebCryptoSha256(11-11)WebCryptoRsaSha256(14-14)src/hash-algorithms-webcrypto.ts (1)
WebCryptoSha256(48-83)src/signature-algorithms-webcrypto.ts (1)
WebCryptoRsaSha256(237-311)
src/types.ts (2)
example/webcrypto-example.js (2)
xml(39-39)privateKey(43-43)example/example.js (1)
xml(32-32)
src/signed-xml.ts (1)
src/types.ts (2)
KeyLike(24-24)Reference(121-151)
src/hash-algorithms-webcrypto.ts (1)
src/types.ts (2)
HashAlgorithm(164-169)ErrorFirstCallback(11-11)
src/signature-algorithms-webcrypto.ts (2)
src/types.ts (4)
SignatureAlgorithm(172-196)BinaryLike(17-17)KeyLike(24-24)ErrorFirstCallback(11-11)src/webcrypto-utils.ts (5)
importRsaPrivateKey(63-79)arrayBufferToBase64(34-41)importRsaPublicKey(87-146)base64ToArrayBuffer(48-55)importHmacKey(154-170)
🪛 markdownlint-cli2 (0.18.1)
WEBCRYPTO.md
182-182: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (3)
src/signature-algorithms-webcrypto.ts (3)
173-182: LGTM: Key normalization now handles all formats correctly.The key handling properly supports CryptoKey, PEM strings, DER/SPKI binary data (Buffer/Uint8Array/ArrayBuffer), and KeyObject formats. The past issue requiring string-only keys has been resolved—
normalizeKeynow correctly returnsstring | ArrayBufferand the import helpers accept both.Also applies to: 209-218, 253-262, 289-298, 333-342, 369-378
465-466: LGTM: Good security practice.Using
crypto.subtle.verifyfor HMAC verification provides constant-time comparison, which correctly prevents timing attacks.
228-230: LGTM: Algorithm URIs match XML DSig spec.The algorithm identifiers correctly follow the W3C XML Digital Signature specification, using the
xmldsig#namespace for SHA-1 algorithms andxmldsig-more#for SHA-256/SHA-512 extensions.Also applies to: 308-310, 388-390, 472-474
| crypto.subtle | ||
| .digest("SHA-1", data) | ||
| .then((hashBuffer) => { | ||
| const hash = this.arrayBufferToBase64(hashBuffer); | ||
| callback(null, hash); | ||
| }) | ||
| .catch((err) => { | ||
| callback(err); | ||
| }); |
There was a problem hiding this comment.
WebCrypto globals and base64 encoding break in Node; deduplicate helper
crypto.subtleassumes a browser/global crypto; Node often needsrequire('node:crypto').webcrypto.btoais not defined in Node; digest-to-base64 should use Buffer fallback.arrayBufferToBase64is duplicated in all classes.
Please make subtle/base64 cross-runtime safe and DRY. Example patch (apply pattern to all 3 classes):
+// Top-level small helpers (can be moved to a shared utils module later)
+function getSubtle(): SubtleCrypto {
+ const g = (typeof globalThis !== "undefined" ? globalThis : undefined) as any;
+ if (g?.crypto?.subtle) return g.crypto.subtle as SubtleCrypto;
+ try {
+ // Node.js fallback
+ // eslint-disable-next-line @typescript-eslint/no-var-requires
+ const { webcrypto } = require("node:crypto");
+ return webcrypto.subtle as SubtleCrypto;
+ } catch {
+ throw new Error("WebCrypto SubtleCrypto is not available in this runtime");
+ }
+}
+
+function arrayBufferToBase64(buffer: ArrayBuffer): string {
+ // Node/browser-safe
+ // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+ // @ts-ignore Buffer is available in Node
+ if (typeof Buffer !== "undefined") return Buffer.from(new Uint8Array(buffer)).toString("base64");
+ let binary = "";
+ const bytes = new Uint8Array(buffer);
+ for (let i = 0; i < bytes.byteLength; i++) binary += String.fromCharCode(bytes[i]);
+ // eslint-disable-next-line no-undef
+ return btoa(binary);
+}
@@
- crypto.subtle
+ getSubtle()
.digest("SHA-1", data)
.then((hashBuffer) => {
- const hash = this.arrayBufferToBase64(hashBuffer);
+ const hash = arrayBufferToBase64(hashBuffer);
callback(null, hash);
})Also remove the per-class private arrayBufferToBase64 and reuse the shared helper.
Also applies to: 34-41, 60-69, 75-82, 101-110, 116-123
🤖 Prompt for AI Agents
In src/hash-algorithms-webcrypto.ts around lines 19 to 27 (and also update
ranges 34-41, 60-69, 75-82, 101-110, 116-123), the code assumes browser globals
(crypto.subtle and btoa) and duplicates arrayBufferToBase64 across classes;
replace direct crypto.subtle usage with a cross-runtime resolver (const
runtimeCrypto = globalThis.crypto ?? require('node:crypto').webcrypto) and call
runtimeCrypto.subtle.digest instead, implement a single shared helper (exported
from a new or existing utils file) that converts an ArrayBuffer to base64 using
TextEncoder/Uint8Array -> Buffer.from(...).toString('base64') as a Node-safe
fallback, remove the private arrayBufferToBase64 methods from each class and
import/use the shared helper, and update all afflicted line ranges to use
runtimeCrypto and the shared base64 helper.
| .then((result) => callback(null, result)) | ||
| .catch((err) => callback(err instanceof Error ? err : new Error("Unknown error"))); |
There was a problem hiding this comment.
Pass explicit result value in error callbacks.
All error callbacks currently omit the second parameter. Per the PR objectives stating "ensure error callbacks consistently call callback(error, false)", error callbacks should explicitly pass the failure value as the second argument for consistency and to meet caller expectations.
Apply this pattern across all .catch() blocks. For getSignature methods (which return string):
- .catch((err) => callback(err instanceof Error ? err : new Error("Unknown error")));
+ .catch((err) => callback(err instanceof Error ? err : new Error("Unknown error"), undefined));For verifySignature methods (which return boolean):
- .catch((err) => callback(err instanceof Error ? err : new Error("Unknown error")));
+ .catch((err) => callback(err instanceof Error ? err : new Error("Unknown error"), false));Also applies to: 224-225, 268-269, 304-305, 348-349, 384-385, 429-430, 468-469
🤖 Prompt for AI Agents
In src/signature-algorithms-webcrypto.ts around lines 188-189 (and similarly at
224-225, 268-269, 304-305, 348-349, 384-385, 429-430, 468-469), the .catch()
error handlers only call callback(err) and omit the explicit failure result;
update each .catch() to call callback(err instanceof Error ? err : new
Error("Unknown error"), <failureValue>) where <failureValue> is an empty string
("") for getSignature methods that must return a string, and false for
verifySignature methods that must return a boolean, keeping the same error
construction logic as currently used.
| if (key == null) { | ||
| throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); | ||
| } |
There was a problem hiding this comment.
Don’t throw in async path; callback the error
verifySignatureValue throws when key is missing, breaking async flow. Use the provided callback.
- if (key == null) {
- throw new Error("KeyInfo or publicCert or privateKey is required to validate signature");
- }
+ if (key == null) {
+ callback(new Error("KeyInfo or publicCert or privateKey is required to validate signature"), false);
+ return;
+ }Same principle for other early errors in async-only code paths.
📝 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.
| if (key == null) { | |
| throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); | |
| } | |
| if (key == null) { | |
| callback(new Error("KeyInfo or publicCert or privateKey is required to validate signature"), false); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/signed-xml.ts around lines 400 to 402, the function currently throws an
Error when key is null which breaks the async/callback flow; change this to call
the provided callback with the Error (e.g., callback(new Error(...))) and return
immediately so the async caller receives the error, and apply the same pattern
for other early validation failures in async-only code paths (use
callback(error) rather than throwing).
| if (!callback) { | ||
| const digestValue = digestAlgorithm.getHash(canonXml); | ||
| res += `<${prefix}DigestMethod Algorithm="${digestAlgorithm.getAlgorithmName()}" />`; | ||
| res += `<${prefix}DigestValue>${digestValue}</${prefix}DigestValue>`; | ||
| res += `</${prefix}Reference>`; | ||
| } else { | ||
| // Capture the current reference XML prefix before the async callback | ||
| const refXmlPrefix = res; | ||
| res = ""; // Reset for next iteration | ||
|
|
||
| digestAlgorithm.getHash(canonXml, (err, digest) => { | ||
| if (err) { | ||
| callback(err); | ||
| return; | ||
| } | ||
|
|
||
| let refXml = refXmlPrefix; | ||
| refXml += `<${prefix}DigestMethod Algorithm="${digestAlgorithm.getAlgorithmName()}" />`; | ||
| refXml += `<${prefix}DigestValue>${digest}</${prefix}DigestValue>`; | ||
| refXml += `</${prefix}Reference>`; | ||
| referenceXmls.push(refXml); | ||
|
|
||
| if (referenceXmls.length === nodes.length * refs.length) { | ||
| callback(null, referenceXmls.join("")); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Async reference aggregation is incorrect; callback may fire early or multiple times
referenceXmls.length === nodes.length * refs.length uses the current ref’s nodes.length and multiplies by total refs, which is wrong when refs match varying node counts. It also resets res mid-loop, conflating state.
Use a stable counter and per-fragment slots to preserve order and ensure a single final callback. Example fix:
- // Capture the current reference XML prefix before the async callback
- const refXmlPrefix = res;
- res = ""; // Reset for next iteration
-
- digestAlgorithm.getHash(canonXml, (err, digest) => {
+ // Build per-node fragment; track completion
+ if (typeof (this as any)._refPending === "undefined") (this as any)._refPending = 0;
+ if (typeof (this as any)._refFragments === "undefined") (this as any)._refFragments = [] as string[];
+ const fragments: string[] = (this as any)._refFragments;
+ let pending: number = (this as any)._refPending;
+ const slot = fragments.length;
+ (this as any)._refPending = pending + 1;
+
+ const open =
+ (ref.isEmptyUri ? `<${prefix}Reference URI="">` : `<${prefix}Reference URI="#${ref.uri}">`) +
+ `<${prefix}Transforms>` +
+ (ref.transforms || [])
+ .map((t) => {
+ const transform = this.findCanonicalizationAlgorithm(t);
+ if (utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) {
+ return `<${prefix}Transform Algorithm="${transform.getAlgorithmName()}">` +
+ `<InclusiveNamespaces PrefixList="${ref.inclusiveNamespacesPrefixList.join(" ")}" xmlns="${transform.getAlgorithmName()}"/>` +
+ `</${prefix}Transform>`;
+ }
+ return `<${prefix}Transform Algorithm="${transform.getAlgorithmName()}" />`;
+ })
+ .join("") +
+ `</${prefix}Transforms>`;
+
+ digestAlgorithm.getHash(canonXml, (err, digest) => {
if (err) {
callback(err);
return;
}
-
- let refXml = refXmlPrefix;
- refXml += `<${prefix}DigestMethod Algorithm="${digestAlgorithm.getAlgorithmName()}" />`;
- refXml += `<${prefix}DigestValue>${digest}</${prefix}DigestValue>`;
- refXml += `</${prefix}Reference>`;
- referenceXmls.push(refXml);
-
- if (referenceXmls.length === nodes.length * refs.length) {
- callback(null, referenceXmls.join(""));
- }
+ fragments[slot] =
+ open +
+ `<${prefix}DigestMethod Algorithm="${digestAlgorithm.getAlgorithmName()}" />` +
+ `<${prefix}DigestValue>${digest}</${prefix}DigestValue>` +
+ `</${prefix}Reference>`;
+ (this as any)._refPending = ((this as any)._refPending as number) - 1;
+ if ((this as any)._refPending === 0) {
+ const xml = (this as any)._refFragments.join("");
+ // cleanup
+ (this as any)._refFragments = [];
+ callback(null, xml);
+ }
});This preserves order, avoids shared res mutation, and ensures a single final callback.
Committable suggestion skipped: line range outside the PR's diff.
|
I'll have a another go on the web crypto specific files. Now I looked most on what needed to be updated in the common files to have support for async crypto. |
3 participants
Add WebCrypto API Support for Browser Compatibility
Overview
This PR adds comprehensive WebCrypto API support to xml-crypto, enabling the library to work in browser environments and modern JavaScript runtimes that lack Node.js’s
cryptomodule.It maintains full backward compatibility with existing Node.js code while introducing a modern, standards-based cryptographic API.
Motivation
The current implementation relies exclusively on Node.js’s
cryptomodule, preventing use in:Adding WebCrypto support enables xml-crypto to run in all JavaScript environments while maintaining the same API surface.
Key Features
1. Dual Cryptography Backend
cryptoand WebCrypto2. New WebCrypto Signature Algorithms (
signature-algorithms-webcrypto.ts)RsaSha1WebCrypto– RSA-SHA1 signaturesRsaSha256WebCrypto– RSA-SHA256 signaturesRsaSha512WebCrypto– RSA-SHA512 signaturesHmacSha1WebCrypto– HMAC-SHA1 signaturesHmacSha256WebCrypto– HMAC-SHA256 signaturesAll implementations support:
signAsync()andverifySignatureAsync()CryptoKey,KeyObject,Buffer,Uint8Array,ArrayBuffer,string(PEM)3. Enhanced Type System (
types.ts)4. Async API Support (
signed-xml.ts)New async methods for WebCrypto-based signing and verification.
5. Browser-Safe Utilities (
webcrypto-utils.ts)ArrayBuffer/Uint8ArrayutilitiesSecurity Fixes
1. Cross-Document Signature Reuse (CVE-level issue)
Problem: Signatures loaded via
loadSignature()could be reused to validate different XML documents.Fix:
Tests Added:
2. Unsigned Document Validation
Problem: Unsigned documents could validate if a signature was preloaded.
Fix: Reject unsigned documents unless using explicit detached signature pattern.
3. Callback Consistency
Problem: Error callbacks inconsistently passed
isValid.Fix: All error paths now consistently call
callback(error, false).4. Buffer Handling for
KeyObjectProblem:
KeyObject.export()returned pooled Buffers incompatible withUint8Array.set().Fix: Properly extract buffer data using
buffer,byteOffset, andbyteLength.Breaking Changes
None — fully backward compatible.
Migration Guide
Using WebCrypto in Browsers
Instructions for signing and verification with WebCrypto.
Verification with WebCrypto
Examples demonstrating interoperability with Node.js keys and signatures.
Implementation Details
Runtime Detection
Automatic detection of available crypto APIs.
Key Normalization
normalizeKey()supports all key formats:CryptoKey→ use directlyKeyObject→ convert toCryptoKeyBuffer/Uint8Array→ parse PEM and importstring→ parse PEM and importArrayBuffer→ import raw key materialAlgorithm Mapping
Mapping between XMLDSIG and WebCrypto algorithms.
Testing
Test Coverage
New Test Categories
KeyObjectbuffer handling – Node.js interop testsBrowser Compatibility
Works in all modern environments supporting WebCrypto API:
✅ Chrome/Edge 37+
✅ Firefox 34+
✅ Safari 11+
✅ Node.js 15+ (WebCrypto available)
✅ Deno, Bun, Cloudflare Workers
Performance
WebCrypto operations are asynchronous but typically faster than Node.js crypto in browsers due to native implementations.
Node.js code continues using synchronous crypto for backward compatibility.
Documentation
Dependencies
Backward Compatibility
✅ 100% backward compatible
cryptobackend remains default in Node.jscrypto.KeyLikeandCryptoKeyRelated Issues
Closes
#477Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests