From 7e30184f4c915f0fda6ecb9d3c51509ef7b635b4 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 11 Aug 2026 11:59:12 -0700 Subject: [PATCH 01/20] fix: omit element when no transforms are specified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When addReference is called without transforms (or with an empty transforms array), createReferences previously always emitted an empty element. This is invalid under SMPTE ST 430-3 §8.2, which requires the Transforms field to be absent when no transformations apply. Changes: - Make `transforms` optional on the Reference interface - Remove the addReference guard that threw on empty/absent transforms - Guard emission in createReferences so the element is only written when at least one transform is present - Apply C14N fallback in getCanonXml for the empty-transforms case so sign and verify use the same canonical form (matching the existing loadReference behavior) Closes #540 --- src/signed-xml.ts | 76 +++++++++++++++++++------------ src/types.ts | 4 +- test/signature-unit-tests.spec.ts | 61 ++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 32 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 663d3d0e..27fcd93d 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -431,9 +431,7 @@ export class SignedXml { /** * Search for ancestor namespaces before canonicalization. */ - if (Array.isArray(ref.transforms)) { - ref.ancestorNamespaces = utils.findAncestorNs(doc, ref.xpath, this.namespaceResolver); - } + ref.ancestorNamespaces = utils.findAncestorNs(doc, ref.xpath, this.namespaceResolver); const c14nOptions = { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList, @@ -819,10 +817,6 @@ export class SignedXml { throw new Error("digestAlgorithm is required"); } - if (!utils.isArrayHasLength(transforms)) { - throw new Error("transforms must contain at least one transform algorithm"); - } - this.references.push({ xpath, transforms, @@ -1155,32 +1149,36 @@ export class SignedXml { referenceElem.setAttribute("Type", ref.type); } - const transformsElem = signatureDoc.createElementNS( - signatureNamespace, - `${currentPrefix}Transforms`, - ); - - for (const trans of ref.transforms || []) { - const transform = this.findCanonicalizationAlgorithm(trans); - const transformElem = signatureDoc.createElementNS( + if (utils.isArrayHasLength(ref.transforms)) { + const transformsElem = signatureDoc.createElementNS( signatureNamespace, - `${currentPrefix}Transform`, + `${currentPrefix}Transforms`, ); - transformElem.setAttribute("Algorithm", transform.getAlgorithmName()); - if (utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) { - const inclusiveNamespacesElem = signatureDoc.createElementNS( - transform.getAlgorithmName(), - "InclusiveNamespaces", + for (const trans of ref.transforms) { + const transform = this.findCanonicalizationAlgorithm(trans); + const transformElem = signatureDoc.createElementNS( + signatureNamespace, + `${currentPrefix}Transform`, ); - inclusiveNamespacesElem.setAttribute( - "PrefixList", - ref.inclusiveNamespacesPrefixList.join(" "), - ); - transformElem.appendChild(inclusiveNamespacesElem); + transformElem.setAttribute("Algorithm", transform.getAlgorithmName()); + + if (utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) { + const inclusiveNamespacesElem = signatureDoc.createElementNS( + transform.getAlgorithmName(), + "InclusiveNamespaces", + ); + inclusiveNamespacesElem.setAttribute( + "PrefixList", + ref.inclusiveNamespacesPrefixList.join(" "), + ); + transformElem.appendChild(inclusiveNamespacesElem); + } + + transformsElem.appendChild(transformElem); } - transformsElem.appendChild(transformElem); + referenceElem.appendChild(transformsElem); } // Get the canonicalized XML @@ -1201,7 +1199,6 @@ export class SignedXml { ); digestValueElem.textContent = digestAlgorithm.getHash(canonXml); - referenceElem.appendChild(transformsElem); referenceElem.appendChild(digestMethodElem); referenceElem.appendChild(digestValueElem); @@ -1272,7 +1269,7 @@ export class SignedXml { const canonXml = node.cloneNode(true); // Deep clone let transformedXml: Node | string = canonXml; - transforms.forEach((transformName) => { + (transforms ?? []).forEach((transformName) => { if (isDomNode.isNodeLike(transformedXml)) { // If, after processing, `transformedNode` is a string, we can't do anymore transforms on it const transform = this.findCanonicalizationAlgorithm(transformName); @@ -1287,6 +1284,27 @@ export class SignedXml { //if only y is the node to sign then a string would be without the definition of the p namespace. probably xmldom toString() should have added it. }); + // When the transform chain produces a DOM node (including the no-transform + // case), apply C14N so that the digest is computed over a canonical byte + // sequence. This mirrors what loadReference does on the verification side: + // it appends C14N when the transform list is empty or ends with + // enveloped-signature, ensuring signing and verification agree on the bytes. + if (typeof transformedXml === "string") { + return transformedXml; + } + + // When there are no transforms, the XMLDSig processing model requires the + // node-set to be serialized via C14N before digesting. This mirrors what + // loadReference already does on the verification side (it appends C14N when + // the transform list is empty), so that signing and verification compute the + // same digest. + if (!utils.isArrayHasLength(transforms)) { + const c14n = this.findCanonicalizationAlgorithm( + "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", + ); + return String(c14n.process(transformedXml, options)); + } + return transformedXml.toString(); } diff --git a/src/types.ts b/src/types.ts index 89c0b304..426b8b02 100644 --- a/src/types.ts +++ b/src/types.ts @@ -127,7 +127,9 @@ export interface Reference { xpath?: string; // An array of transforms to be applied to the data before signing. - transforms: ReadonlyArray; + // When absent or empty, no Transforms element is emitted and the referenced + // node is digested directly (after C14N, per the XMLDSig processing model). + transforms?: ReadonlyArray; // The algorithm used to calculate the digest value of the data. digestAlgorithm: HashAlgorithmType; diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index c0dcf136..073e79f0 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -908,8 +908,8 @@ describe("Signature unit tests", function () { ref.uri, `wrong uri for index ${i}. expected: ${expectedUri} actual: ${ref.uri}`, ).to.equal(expectedUri); - expect(ref.transforms.length).to.equal(1); - expect(ref.transforms[0]).to.equal("http://www.w3.org/2001/10/xml-exc-c14n#"); + expect(ref.transforms!.length).to.equal(1); + expect(ref.transforms![0]).to.equal("http://www.w3.org/2001/10/xml-exc-c14n#"); expect(ref.digestValue).to.equal(digests[i]); expect(ref.digestAlgorithm).to.equal("http://www.w3.org/2000/09/xmldsig#sha1"); } @@ -1074,6 +1074,63 @@ describe("Signature unit tests", function () { expect(URI.value, `uri should be empty but instead was ${URI.value}`).to.equal(""); }); + it("omits Transforms element when no transforms are specified", function () { + const xml = ""; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: "#ref1", + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const transforms = xpath.select( + "//*[local-name(.)='Transforms']", + doc, + ); + expect(transforms, "Transforms element should be absent when no transforms specified").to.have + .length(0); + }); + + it("signs and verifies correctly with no transforms (round-trip)", function () { + const xml = ""; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: "#ref1", + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const sigNode = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + doc, + ); + isDomNode.assertIsNodeLike(sigNode); + + const verifySig = new SignedXml(); + verifySig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + verifySig.loadSignature(sigNode); + const result = verifySig.checkSignature(signedXml); + expect(result, "expected signature to verify successfully").to.be.true; + }); + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml(); From e33343ceb5a07aa5940e4a6cad72510e7da0adef Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 11 Aug 2026 13:41:52 -0700 Subject: [PATCH 02/20] test: replace non-null assertions with deep.equal on ref.transforms Using ! on optional properties triggers the no-non-null-assertion ESLint rule. Collapsing the two separate length/index checks into a single deep.equal is also more readable. --- test/signature-unit-tests.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 073e79f0..5910e584 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -908,8 +908,7 @@ describe("Signature unit tests", function () { ref.uri, `wrong uri for index ${i}. expected: ${expectedUri} actual: ${ref.uri}`, ).to.equal(expectedUri); - expect(ref.transforms!.length).to.equal(1); - expect(ref.transforms![0]).to.equal("http://www.w3.org/2001/10/xml-exc-c14n#"); + expect(ref.transforms).to.deep.equal(["http://www.w3.org/2001/10/xml-exc-c14n#"]); expect(ref.digestValue).to.equal(digests[i]); expect(ref.digestAlgorithm).to.equal("http://www.w3.org/2000/09/xmldsig#sha1"); } From 21df3b8437e457e81c5a5492085a57049de9ce06 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Wed, 12 Aug 2026 16:48:38 -0700 Subject: [PATCH 03/20] test: cover both omitted and empty transforms in Transforms-omission test The previous test only verified that a missing transforms property suppresses the element. Parameterize the test to also cover transforms: [], which the API treats identically. --- test/signature-unit-tests.spec.ts | 49 ++++++++++++++++--------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 5910e584..ea91b73a 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1073,30 +1073,33 @@ describe("Signature unit tests", function () { expect(URI.value, `uri should be empty but instead was ${URI.value}`).to.equal(""); }); - it("omits Transforms element when no transforms are specified", function () { - const xml = ""; - const sig = new SignedXml(); - sig.privateKey = fs.readFileSync("./test/static/client.pem"); - sig.addReference({ - xpath: "//*[local-name(.)='x']", - digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", - uri: "#ref1", - digestValue: "", - inclusiveNamespacesPrefixList: [], - isEmptyUri: false, + for (const { label, transforms } of [ + { label: "omitted transforms property", transforms: undefined }, + { label: "empty transforms array", transforms: [] as string[] }, + ]) { + it(`omits Transforms element when no transforms are specified (${label})`, function () { + const xml = ""; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: "#ref1", + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + ...(transforms !== undefined ? { transforms } : {}), + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const transformNodes = xpath.select("//*[local-name(.)='Transforms']", doc); + expect(transformNodes, "Transforms element should be absent when no transforms specified").to + .have.length(0); }); - sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; - sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; - sig.computeSignature(xml); - const signedXml = sig.getSignedXml(); - const doc = new xmldom.DOMParser().parseFromString(signedXml); - const transforms = xpath.select( - "//*[local-name(.)='Transforms']", - doc, - ); - expect(transforms, "Transforms element should be absent when no transforms specified").to.have - .length(0); - }); + } it("signs and verifies correctly with no transforms (round-trip)", function () { const xml = ""; From c5d197634333bfdc30d5cf20f93c0a6b16ff00e8 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Wed, 12 Aug 2026 16:52:25 -0700 Subject: [PATCH 04/20] fix: derive ancestor namespaces from the referenced node, not re-running xpath MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getCanonReferenceXml passed ref.xpath to findAncestorNs, which always uses docSubset[0] — the first XPath match. When addAllReferences creates multiple references for the same xpath pattern and those matched elements live under different ancestor namespace scopes, every reference beyond the first was digested with the wrong namespace context, producing a signature that verifiers would reject. Fix: add findAncestorNsForNode(element) to utils and call it with the node already in scope instead of re-executing the XPath. Also extract the shared deduplication/filtering logic into buildAncestorNsForElement to avoid code duplication between the two public helpers. Adds a regression test: two elements under sibling
elements that each declare a different namespace prefix. The fix makes sign+verify round-trip correctly; the old code would fail verification for the second reference. --- src/signed-xml.ts | 12 ++++-- src/utils.ts | 71 +++++++++++++++++++------------ test/signature-unit-tests.spec.ts | 42 ++++++++++++++++++ 3 files changed, 95 insertions(+), 30 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 27fcd93d..c6a43625 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -427,11 +427,17 @@ export class SignedXml { return this.getCanonXml([this.canonicalizationAlgorithm], signedInfo[0], c14nOptions); } - private getCanonReferenceXml(doc: Document, ref: Reference, node: Node) { + private getCanonReferenceXml(_doc: Document, ref: Reference, node: Node) { /** - * Search for ancestor namespaces before canonicalization. + * Derive ancestor namespaces from the specific node being digested, not by + * re-running ref.xpath. findAncestorNs uses only the first XPath match, so + * when multiple references are created for the same xpath pattern (e.g. via + * addAllReferences), references beyond the first are digested with the wrong + * ancestor namespace scope. */ - ref.ancestorNamespaces = utils.findAncestorNs(doc, ref.xpath, this.namespaceResolver); + if (isDomNode.isElementNode(node)) { + ref.ancestorNamespaces = utils.findAncestorNsForNode(node); + } const c14nOptions = { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList, diff --git a/src/utils.ts b/src/utils.ts index 466b252e..034a6329 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -236,6 +236,49 @@ function isElementSubset(docSubset: Node[]): docSubset is Element[] { return docSubset.every((node) => isDomNode.isElementNode(node)); } +/** Deduplicate and filter ancestor namespaces for a given subset element. */ +function buildAncestorNsForElement(element: Element): NamespacePrefix[] { + const ancestorNs = collectAncestorNamespaces(element); + const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; + for (let i = 0; i < ancestorNs.length; i++) { + let notOnTheList = true; + for (const v in ancestorNsWithoutDuplicate) { + if (ancestorNsWithoutDuplicate[v].prefix === ancestorNs[i].prefix) { + notOnTheList = false; + break; + } + } + + if (notOnTheList) { + ancestorNsWithoutDuplicate.push(ancestorNs[i]); + } + } + + // Remove namespaces which are already declared in the subset with the same prefix + const returningNs: NamespacePrefix[] = []; + const subsetNsPrefix = findNSPrefix(element); + for (const ns of ancestorNsWithoutDuplicate) { + if (ns.prefix !== subsetNsPrefix) { + returningNs.push(ns); + } + } + + return returningNs; +} + +/** + * Extract ancestor namespaces for a specific element node. + * Prefer this over `findAncestorNs` when the target element is already known, + * since `findAncestorNs` re-executes an XPath and uses the first match — + * which is incorrect when multiple nodes are referenced individually. + * + * @param element - The element whose ancestor namespace declarations to collect + * @returns i.e. [{prefix: "saml", namespaceURI: "urn:oasis:names:tc:SAML:2.0:assertion"}] + */ +export function findAncestorNsForNode(element: Element): NamespacePrefix[] { + return buildAncestorNsForElement(element); +} + /** * Extract ancestor namespaces in order to import it to root of document subset * which is being canonicalized for non-exclusive c14n. @@ -264,33 +307,7 @@ export function findAncestorNs( throw new Error("Document subset must be list of elements"); } - // Remove duplicate on ancestor namespace - const ancestorNs = collectAncestorNamespaces(docSubset[0]); - const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; - for (let i = 0; i < ancestorNs.length; i++) { - let notOnTheList = true; - for (const v in ancestorNsWithoutDuplicate) { - if (ancestorNsWithoutDuplicate[v].prefix === ancestorNs[i].prefix) { - notOnTheList = false; - break; - } - } - - if (notOnTheList) { - ancestorNsWithoutDuplicate.push(ancestorNs[i]); - } - } - - // Remove namespaces which are already declared in the subset with the same prefix - const returningNs: NamespacePrefix[] = []; - const subsetNsPrefix = findNSPrefix(docSubset[0]); - for (const ancestorNs of ancestorNsWithoutDuplicate) { - if (ancestorNs.prefix !== subsetNsPrefix) { - returningNs.push(ancestorNs); - } - } - - return returningNs; + return buildAncestorNsForElement(docSubset[0]); } export function validateDigestValue(digest, expectedDigest) { diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index ea91b73a..290ce685 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1133,6 +1133,48 @@ describe("Signature unit tests", function () { expect(result, "expected signature to verify successfully").to.be.true; }); + it("correctly canonicalizes no-transform references under different ancestor namespace scopes", function () { + // Two elements live under different namespace scopes. Without the + // fix, findAncestorNs(doc, ref.xpath) always uses the first XPath match + // (item1's scope), so item2 is digested with the wrong ancestor namespaces + // and verification fails. + const xml = + "" + + "
one
" + + "
two
" + + "
"; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + for (const id of ["item1", "item2"]) { + sig.addReference({ + xpath: `//*[@Id='${id}']`, + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: `#${id}`, + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + }); + } + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const sigNode = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + doc, + ); + isDomNode.assertIsNodeLike(sigNode); + + const verifySig = new SignedXml(); + verifySig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + verifySig.loadSignature(sigNode); + const result = verifySig.checkSignature(signedXml); + expect(result, "expected signature to verify successfully").to.be.true; + }); + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml(); From 3a2a63cc6ba029e8776e5b565622c015b826bb73 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Mon, 17 Aug 2026 10:48:17 -0700 Subject: [PATCH 05/20] docs: expand buildAncestorNsForElement JSDoc and fix prettier formatting --- src/utils.ts | 9 ++++++++- test/signature-unit-tests.spec.ts | 6 ++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 034a6329..bca935d8 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -236,7 +236,14 @@ function isElementSubset(docSubset: Node[]): docSubset is Element[] { return docSubset.every((node) => isDomNode.isElementNode(node)); } -/** Deduplicate and filter ancestor namespaces for a given subset element. */ +/** + * Collect, deduplicate, and filter ancestor namespace declarations for a given element. + * Namespaces already declared directly on the element are excluded from the result + * to avoid double-emission during C14N serialization. + * + * @param element - The element whose ancestor namespace declarations to process + * @returns Deduplicated array of ancestor namespace prefixes not already declared on the element + */ function buildAncestorNsForElement(element: Element): NamespacePrefix[] { const ancestorNs = collectAncestorNamespaces(element); const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 290ce685..8ebed8fa 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1096,8 +1096,10 @@ describe("Signature unit tests", function () { const signedXml = sig.getSignedXml(); const doc = new xmldom.DOMParser().parseFromString(signedXml); const transformNodes = xpath.select("//*[local-name(.)='Transforms']", doc); - expect(transformNodes, "Transforms element should be absent when no transforms specified").to - .have.length(0); + expect( + transformNodes, + "Transforms element should be absent when no transforms specified", + ).to.have.length(0); }); } From 9e24d28950d1ce1ab5aaf27656aa91eb137518a5 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 8 Sep 2026 09:05:18 -0700 Subject: [PATCH 06/20] fix: guard against undefined transforms in enveloped-signature check --- src/signed-xml.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index a487f1c9..b73f78db 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -1284,7 +1284,7 @@ export class SignedXml { options.signatureNode = this.signatureNode; const canonXml = node.cloneNode(true); // Deep clone - if (transforms.includes("http://www.w3.org/2000/09/xmldsig#enveloped-signature")) { + if ((transforms ?? []).includes("http://www.w3.org/2000/09/xmldsig#enveloped-signature")) { const signaturePath: number[] = []; let signatureAncestor = this.signatureNode; while (signatureAncestor?.parentNode && signatureAncestor !== node) { From 82bc488c1798ab837947ea46803da291d12cd090 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 8 Sep 2026 09:13:09 -0700 Subject: [PATCH 07/20] test: exercise addAllReferences multi-match in ancestor-namespace regression test --- test/signature-unit-tests.spec.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index fe776a66..fe67cfb1 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1163,16 +1163,14 @@ describe("Signature unit tests", function () { const sig = new SignedXml(); sig.privateKey = fs.readFileSync("./test/static/client.pem"); sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); - for (const id of ["item1", "item2"]) { - sig.addReference({ - xpath: `//*[@Id='${id}']`, - digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", - uri: `#${id}`, - digestValue: "", - inclusiveNamespacesPrefixList: [], - isEmptyUri: false, - }); - } + // Single addReference() call so addAllReferences() matches both + // elements from the same ref.xpath — this is what exercises the + // first-match regression; two separate addReference() calls (each with + // its own single-match xpath) would pass even without the fix. + sig.addReference({ + xpath: "//*[local-name(.)='item']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + }); sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; sig.computeSignature(xml); From 6e16874c51a0251e85e734ae1ebbb1857064a2ab Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 14:40:17 -0500 Subject: [PATCH 08/20] fix: keep Reference.transforms required and tighten review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making `transforms` optional was a breaking change for consumers: `Reference` is public via `export * from "./types"`, so anyone reading `ref.transforms` hit TS18048 "possibly undefined". Keep the property required and normalize an omitted list to `[]` in addReference instead — `addReference` already accepted `Partial`, so callers could always omit it. The empty array still suppresses the Transforms element, so the feature is unchanged. Also: - Isolate the ancestor-namespace regression test by giving it explicit transforms. As written it threw "transforms must contain at least one transform algorithm" on master, failing before it reached the code under test; it now fails there with the real digest mismatch. - Collapse the duplicated comment blocks in getCanonXml, and drop the JSDoc from the internal buildAncestorNsForElement helper, per AGENTS.md. - Drop the unused `doc` parameter from getCanonReferenceXml. Co-Authored-By: Claude Opus 5 --- src/signed-xml.ts | 38 ++++++++++++------------------- src/types.ts | 6 ++--- src/utils.ts | 8 ------- test/signature-unit-tests.spec.ts | 18 +++++++-------- 4 files changed, 26 insertions(+), 44 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index b73f78db..a3bda9be 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -433,14 +433,10 @@ export class SignedXml { return this.getCanonXml([this.canonicalizationAlgorithm], signedInfo[0], c14nOptions); } - private getCanonReferenceXml(_doc: Document, ref: Reference, node: Node) { - /** - * Derive ancestor namespaces from the specific node being digested, not by - * re-running ref.xpath. findAncestorNs uses only the first XPath match, so - * when multiple references are created for the same xpath pattern (e.g. via - * addAllReferences), references beyond the first are digested with the wrong - * ancestor namespace scope. - */ + private getCanonReferenceXml(ref: Reference, node: Node) { + // Derive the scope from the node being digested: findAncestorNs re-runs + // ref.xpath and keeps only the first match, which digests every reference + // addAllReferences() created from that xpath in the first match's scope. if (isDomNode.isElementNode(node)) { ref.ancestorNamespaces = utils.findAncestorNsForNode(node); } @@ -520,7 +516,7 @@ export class SignedXml { } } - const canonXml = this.getCanonReferenceXml(doc, ref, elem); + const canonXml = this.getCanonReferenceXml(ref, elem); const hash = this.findHashAlgorithm(ref.digestAlgorithm); const digest = hash.getHash(canonXml); @@ -580,7 +576,7 @@ export class SignedXml { return false; } - const canonXml = this.getCanonReferenceXml(doc, ref, elem); + const canonXml = this.getCanonReferenceXml(ref, elem); const hash = this.findHashAlgorithm(ref.digestAlgorithm); const digest = hash.getHash(canonXml); @@ -831,7 +827,7 @@ export class SignedXml { this.references.push({ xpath, - transforms, + transforms: transforms ?? [], digestAlgorithm, uri, digestValue, @@ -1199,7 +1195,7 @@ export class SignedXml { } // Get the canonicalized XML - const canonXml = this.getCanonReferenceXml(doc, ref, node); + const canonXml = this.getCanonReferenceXml(ref, node); // Get the digest algorithm and compute the digest value const digestAlgorithm = this.findHashAlgorithm(ref.digestAlgorithm); @@ -1284,7 +1280,7 @@ export class SignedXml { options.signatureNode = this.signatureNode; const canonXml = node.cloneNode(true); // Deep clone - if ((transforms ?? []).includes("http://www.w3.org/2000/09/xmldsig#enveloped-signature")) { + if (transforms.includes("http://www.w3.org/2000/09/xmldsig#enveloped-signature")) { const signaturePath: number[] = []; let signatureAncestor = this.signatureNode; while (signatureAncestor?.parentNode && signatureAncestor !== node) { @@ -1301,7 +1297,7 @@ export class SignedXml { } let transformedXml: Node | string = canonXml; - (transforms ?? []).forEach((transformName) => { + transforms.forEach((transformName) => { if (isDomNode.isNodeLike(transformedXml)) { // If, after processing, `transformedNode` is a string, we can't do anymore transforms on it const transform = this.findCanonicalizationAlgorithm(transformName); @@ -1316,20 +1312,14 @@ export class SignedXml { //if only y is the node to sign then a string would be without the definition of the p namespace. probably xmldom toString() should have added it. }); - // When the transform chain produces a DOM node (including the no-transform - // case), apply C14N so that the digest is computed over a canonical byte - // sequence. This mirrors what loadReference does on the verification side: - // it appends C14N when the transform list is empty or ends with - // enveloped-signature, ensuring signing and verification agree on the bytes. if (typeof transformedXml === "string") { return transformedXml; } - // When there are no transforms, the XMLDSig processing model requires the - // node-set to be serialized via C14N before digesting. This mirrors what - // loadReference already does on the verification side (it appends C14N when - // the transform list is empty), so that signing and verification compute the - // same digest. + // A same-document reference dereferences to a node-set, which must be + // canonicalized to an octet stream before digesting. `loadReference` appends + // the same C14N on the verification side, so both sides digest equal bytes. + // https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel if (!utils.isArrayHasLength(transforms)) { const c14n = this.findCanonicalizationAlgorithm( "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", diff --git a/src/types.ts b/src/types.ts index 426b8b02..61b4e85e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -127,9 +127,9 @@ export interface Reference { xpath?: string; // An array of transforms to be applied to the data before signing. - // When absent or empty, no Transforms element is emitted and the referenced - // node is digested directly (after C14N, per the XMLDSig processing model). - transforms?: ReadonlyArray; + // An empty array emits no Transforms element; the referenced node is then + // digested directly, after C14N, per the XMLDSig processing model. + transforms: ReadonlyArray; // The algorithm used to calculate the digest value of the data. digestAlgorithm: HashAlgorithmType; diff --git a/src/utils.ts b/src/utils.ts index 97fc8089..be3f93a4 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -240,14 +240,6 @@ function isElementSubset(docSubset: Node[]): docSubset is Element[] { return docSubset.every((node) => isDomNode.isElementNode(node)); } -/** - * Collect, deduplicate, and filter ancestor namespace declarations for a given element. - * Namespaces already declared directly on the element are excluded from the result - * to avoid double-emission during C14N serialization. - * - * @param element - The element whose ancestor namespace declarations to process - * @returns Deduplicated array of ancestor namespace prefixes not already declared on the element - */ function buildAncestorNsForElement(element: Element): NamespacePrefix[] { const ancestorNs = collectAncestorNamespaces(element); const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index fe67cfb1..6ed4d00f 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1150,11 +1150,11 @@ describe("Signature unit tests", function () { expect(result, "expected signature to verify successfully").to.be.true; }); - it("correctly canonicalizes no-transform references under different ancestor namespace scopes", function () { - // Two elements live under different namespace scopes. Without the - // fix, findAncestorNs(doc, ref.xpath) always uses the first XPath match - // (item1's scope), so item2 is digested with the wrong ancestor namespaces - // and verification fails. + it("derives ancestor namespaces per referenced node when one xpath matches several", function () { + // Transforms are specified explicitly so this exercises only the ancestor + // namespace derivation: findAncestorNs(doc, ref.xpath) used the first XPath + // match (item1's scope) for every reference, so item2 was digested with the + // wrong ancestor namespaces and verification failed. const xml = "" + "
one
" + @@ -1163,12 +1163,12 @@ describe("Signature unit tests", function () { const sig = new SignedXml(); sig.privateKey = fs.readFileSync("./test/static/client.pem"); sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); - // Single addReference() call so addAllReferences() matches both - // elements from the same ref.xpath — this is what exercises the - // first-match regression; two separate addReference() calls (each with - // its own single-match xpath) would pass even without the fix. + // One addReference() call, so addAllReferences() matches both + // elements from the same ref.xpath. Two separate calls, each with its own + // single-match xpath, would pass even without the fix. sig.addReference({ xpath: "//*[local-name(.)='item']", + transforms: ["http://www.w3.org/TR/2001/REC-xml-c14n-20010315"], digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", }); sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; From 6a7ac33d38590702b1802cd0d1ddb995b8e74f96 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 14:46:36 -0500 Subject: [PATCH 09/20] refactor: fold findAncestorNsForNode wrapper into its helper findAncestorNsForNode was a pass-through over buildAncestorNsForElement with a single caller and no test usage. Keep one function instead of two. Also document the omitted/empty transforms case in README, including the requirement that an enveloped signature list the enveloped-signature transform explicitly. Co-Authored-By: Claude Opus 5 --- README.md | 4 +++- src/utils.ts | 25 ++++++++++--------------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 891e91c5..106e640b 100644 --- a/README.md +++ b/README.md @@ -267,7 +267,9 @@ To sign xml documents: - `addReference({ xpath, transforms, digestAlgorithm, id, type })` - adds a reference to a xml element where: - `xpath` - a string containing a XPath expression referencing a xml element - - `transforms` - an array of [transform algorithms](#canonicalization-and-transformation-algorithms), the referenced element will be transformed for each value in the array + - `transforms` - an array of [transform algorithms](#canonicalization-and-transformation-algorithms), the referenced element will be transformed for each value in the array. + Omit it, or pass an empty array, to emit no `Transforms` element; the referenced element is then digested after canonicalization alone. + Note that an enveloped signature needs `http://www.w3.org/2000/09/xmldsig#enveloped-signature` in this array — without it the `Signature` element is digested along with the content it signs, and the result will not verify. - `digestAlgorithm` - one of the supported [hashing algorithms](#hashing-algorithms) - `id` - an optional `Id` attribute to add to the reference element - `type` - the optional `Type` attribute to add to the reference element (represented as a URI) diff --git a/src/utils.ts b/src/utils.ts index be3f93a4..8deb5fcc 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -240,7 +240,15 @@ function isElementSubset(docSubset: Node[]): docSubset is Element[] { return docSubset.every((node) => isDomNode.isElementNode(node)); } -function buildAncestorNsForElement(element: Element): NamespacePrefix[] { +/** + * Extract ancestor namespaces for an element that is already resolved. + * Prefer this over `findAncestorNs` when the target element is known, since + * `findAncestorNs` re-executes an XPath and uses only the first match. + * + * @param element - The element whose ancestor namespace declarations to collect + * @returns i.e. [{prefix: "saml", namespaceURI: "urn:oasis:names:tc:SAML:2.0:assertion"}] + */ +export function findAncestorNsForNode(element: Element): NamespacePrefix[] { const ancestorNs = collectAncestorNamespaces(element); const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; for (const ns of ancestorNs) { @@ -262,19 +270,6 @@ function buildAncestorNsForElement(element: Element): NamespacePrefix[] { return returningNs; } -/** - * Extract ancestor namespaces for a specific element node. - * Prefer this over `findAncestorNs` when the target element is already known, - * since `findAncestorNs` re-executes an XPath and uses the first match — - * which is incorrect when multiple nodes are referenced individually. - * - * @param element - The element whose ancestor namespace declarations to collect - * @returns i.e. [{prefix: "saml", namespaceURI: "urn:oasis:names:tc:SAML:2.0:assertion"}] - */ -export function findAncestorNsForNode(element: Element): NamespacePrefix[] { - return buildAncestorNsForElement(element); -} - /** * Extract ancestor namespaces in order to import it to root of document subset * which is being canonicalized for non-exclusive c14n. @@ -303,7 +298,7 @@ export function findAncestorNs( throw new Error("Document subset must be list of elements"); } - return buildAncestorNsForElement(docSubset[0]); + return findAncestorNsForNode(docSubset[0]); } export function validateDigestValue(digest, expectedDigest) { From 69207121de33337b2bda40615a18ac893fd36df4 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 14:54:21 -0500 Subject: [PATCH 10/20] refactor: replace `export *` in index.ts with explicit re-export lists `export * from "./types"` and `export * from "./utils"` published every module-level export automatically, so adding any helper to utils.ts silently widened the package's public API. The lists are deliberately exhaustive: they reproduce the existing surface exactly, verified as 40 symbols with identical kinds, origins and type signatures before and after. Narrowing it is a separate, semver-major change that can now be reviewed on its own. `findChilds` needs an eslint-disable because it is deprecated and naming it explicitly trips deprecation/deprecation, which the wildcard never did. Co-Authored-By: Claude Opus 5 --- src/index.ts | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 3c82b7a8..04853d72 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,5 +4,50 @@ export { ExclusiveCanonicalizationWithComments, } from "./exclusive-canonicalization"; export { SignedXml } from "./signed-xml"; -export * from "./types"; -export * from "./utils"; + +// These lists replace `export * from` and are exhaustive on purpose: they +// reproduce the surface the wildcards already published, so that narrowing it +// becomes a deliberate, separately reviewable break rather than a side effect. +export { createOptionalCallbackFunction } from "./types"; +export type { + CanonicalizationAlgorithmType, + CanonicalizationOrTransformAlgorithmType, + CanonicalizationOrTransformationAlgorithm, + CanonicalizationOrTransformationAlgorithmProcessOptions, + ComputeSignatureOptions, + ComputeSignatureOptionsLocation, + ErrorFirstCallback, + GetKeyInfoContentArgs, + HashAlgorithm, + HashAlgorithmType, + NamespacePrefix, + ObjectAttributes, + Reference, + RenderedNamespace, + SignatureAlgorithm, + SignatureAlgorithmType, + SignedXmlOptions, + TransformAlgorithm, +} from "./types"; + +export { + BASE64_REGEX, + EXTRACT_X509_CERTS, + PEM_FORMAT_REGEX, + derToPem, + encodeSpecialCharactersInAttribute, + encodeSpecialCharactersInText, + findAncestorNs, + findAncestorNsForNode, + findAttr, + findChildren, + // Deprecated alias of `findChildren`, still published because `export *` did. + // Dropping it is part of thinning this list, not of making the list explicit. + // eslint-disable-next-line deprecation/deprecation + findChilds, + isArrayHasLength, + isDescendantOf, + normalizePem, + pemToDer, + validateDigestValue, +} from "./utils"; From c6bb8c0f48f65ddb753fdfc7412d8ce979696314 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 15:37:46 -0500 Subject: [PATCH 11/20] test: cover the non-element document subset guard in findAncestorNs findAncestorNs throws "Document subset must be list of elements" when the subset xpath selects attributes or text nodes, and nothing exercised it. The xpath is a plain string, so a caller reaches this without any cast. The guard is load-bearing rather than decorative: bypassing it hands the non-element to findSubsetNSPrefixes, whose `.attributes` is null there, so the caller gets "Cannot read properties of null" instead of a usable message. The test fails with exactly that TypeError if the guard is removed. Also reformat src/types.ts, which master left unformatted after the prettier 3.1 -> 3.9 bump in 711ca97 changed union wrapping. Unrelated to this branch; master is currently lint-red for the same reason. Co-Authored-By: Claude Opus 5 --- src/types.ts | 3 ++- test/c14n-non-exclusive-unit-tests.spec.ts | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/types.ts b/src/types.ts index 3219a009..61b4e85e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -18,7 +18,8 @@ export type CanonicalizationAlgorithmType = | string; export type CanonicalizationOrTransformAlgorithmType = - CanonicalizationAlgorithmType | "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; + | CanonicalizationAlgorithmType + | "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; export type HashAlgorithmType = | "http://www.w3.org/2000/09/xmldsig#sha1" diff --git a/test/c14n-non-exclusive-unit-tests.spec.ts b/test/c14n-non-exclusive-unit-tests.spec.ts index a5d308ba..a086868f 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -155,6 +155,23 @@ describe("C14N non-exclusive canonicalization tests", function () { test_findAncestorNs(xml, xpath, expected); }); + for (const { label, subsetXpath } of [ + { label: "attributes", subsetXpath: "//@attr" }, + { label: "text nodes", subsetXpath: "//*[local-name()='child']/text()" }, + ]) { + it(`findAncestorNs: Should reject a document subset of ${label}`, function () { + // Only elements carry namespace declarations. Without this check the + // non-element reaches findSubsetNSPrefixes, whose `.attributes` is null + // there, and the caller gets a TypeError instead of a usable message. + const xml = "text"; + const doc = new xmldom.DOMParser().parseFromString(xml); + + expect(() => utils.findAncestorNs(doc, subsetXpath)).to.throw( + "Document subset must be list of elements", + ); + }); + } + // Tests for c14nCanonicalization it("C14n: Correctly picks up root ancestor namespace", function () { const xml = ""; From a0b3e14358e3899617b681cc592b993882ee230e Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 18:55:53 -0500 Subject: [PATCH 12/20] Lint --- src/types.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/types.ts b/src/types.ts index 61b4e85e..3219a009 100644 --- a/src/types.ts +++ b/src/types.ts @@ -18,8 +18,7 @@ export type CanonicalizationAlgorithmType = | string; export type CanonicalizationOrTransformAlgorithmType = - | CanonicalizationAlgorithmType - | "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; + CanonicalizationAlgorithmType | "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; export type HashAlgorithmType = | "http://www.w3.org/2000/09/xmldsig#sha1" From 639531784d0600ad14be835ae2563d1180aecdf9 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:07:55 -0500 Subject: [PATCH 13/20] fix: assert element-ness in getCanonReferenceXml instead of silently skipping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `if (isDomNode.isElementNode(node))` guard left a branch no test could honestly reach: all three call sites already establish element-ness, the signing path via assertIsElementNode() and both verification paths via element-only xpaths. It was reachable only by misuse — `idAttributes` is public and mutable, so a JavaScript caller can empty it and hand a text node to the public validateElementAgainstReferences(), skipping the getAttribute() that would otherwise reject it. That path failed closed either way, but the guard turned it into "No references passed validation", which says nothing about the real problem. Assert instead, matching how this file already handles the same question, so the caller gets "Value is not of type ELEMENT_NODE". Covers that path with a test written against what JavaScript allows rather than what the types permit. Patch coverage is now 100% on both lines and branches, with the branch gone rather than papered over. Co-Authored-By: Claude Opus 5 --- src/signed-xml.ts | 5 ++--- test/signature-unit-tests.spec.ts | 34 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index a3bda9be..fedcca18 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -437,9 +437,8 @@ export class SignedXml { // Derive the scope from the node being digested: findAncestorNs re-runs // ref.xpath and keeps only the first match, which digests every reference // addAllReferences() created from that xpath in the first match's scope. - if (isDomNode.isElementNode(node)) { - ref.ancestorNamespaces = utils.findAncestorNsForNode(node); - } + isDomNode.assertIsElementNode(node); + ref.ancestorNamespaces = utils.findAncestorNsForNode(node); const c14nOptions = { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList, diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 6ed4d00f..76ceaf16 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1190,6 +1190,40 @@ describe("Signature unit tests", function () { expect(result, "expected signature to verify successfully").to.be.true; }); + it("refuses to digest a non-element node passed to validateElementAgainstReferences", function () { + // `idAttributes` is public and mutable, so a JavaScript caller can empty it + // and reach the digest path with a node that never sees `getAttribute()`. + // Only elements carry namespace declarations, so this has to be refused + // outright rather than canonicalized into a digest nobody can interpret. + const xml = "hello"; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + transforms: ["http://www.w3.org/TR/2001/REC-xml-c14n-20010315"], + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: "#ref1", + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const sigNode = xpath.select1("//*[local-name(.)='Signature']", doc); + isDomNode.assertIsNodeLike(sigNode); + + const verifySig = new SignedXml(); + verifySig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + verifySig.loadSignature(sigNode); + verifySig.idAttributes = []; + + const textNode = xpath.select1("//*[local-name(.)='x']/text()", doc); + expect(() => + verifySig.validateElementAgainstReferences(textNode as unknown as Element, doc), + ).to.throw("Value is not of type ELEMENT_NODE"); + }); + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml(); From 1efc2c1b349b1f83a3d7c7568d53cdfc1670faa7 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:19:01 -0500 Subject: [PATCH 14/20] test: assert refusal rather than a dependency's error wording The non-element test pinned "Value is not of type ELEMENT_NODE", which @xmldom/is-dom-node builds from an enum key. Rewording it upstream would have broken us for a reason unrelated to what the test is about. The property that matters is that a non-element is never reported as covered by a validated reference, so assert the refusal itself. Also drop a comment in utils.ts that restated the loop beneath it; the reason already sits in findSubsetNSPrefixes next to its spec link. And point the findChilds export note at issue #550 now that one exists. Co-Authored-By: Claude Opus 5 --- src/index.ts | 2 +- src/utils.ts | 1 - test/signature-unit-tests.spec.ts | 7 ++++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/index.ts b/src/index.ts index 04853d72..a11dec83 100644 --- a/src/index.ts +++ b/src/index.ts @@ -42,7 +42,7 @@ export { findAttr, findChildren, // Deprecated alias of `findChildren`, still published because `export *` did. - // Dropping it is part of thinning this list, not of making the list explicit. + // Removal tracked in https://github.com/node-saml/xml-crypto/issues/550 // eslint-disable-next-line deprecation/deprecation findChilds, isArrayHasLength, diff --git a/src/utils.ts b/src/utils.ts index 8deb5fcc..3c1656bd 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -258,7 +258,6 @@ export function findAncestorNsForNode(element: Element): NamespacePrefix[] { } } - // Remove namespaces which are already declared in the subset with the same prefix const returningNs: NamespacePrefix[] = []; const subsetNsPrefixes = findSubsetNSPrefixes(element); for (const ns of ancestorNsWithoutDuplicate) { diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 76ceaf16..d08ec71e 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1219,9 +1219,10 @@ describe("Signature unit tests", function () { verifySig.idAttributes = []; const textNode = xpath.select1("//*[local-name(.)='x']/text()", doc); - expect(() => - verifySig.validateElementAgainstReferences(textNode as unknown as Element, doc), - ).to.throw("Value is not of type ELEMENT_NODE"); + expect( + () => verifySig.validateElementAgainstReferences(textNode as unknown as Element, doc), + "a non-element must never be reported as covered by a validated reference", + ).to.throw(); }); it("signer appends signature to a non-existing reference node", function () { From ce8d32aab9d93f1a2c573371e6be4075413bcb1f Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:31:05 -0500 Subject: [PATCH 15/20] fix: reject signing a reference that encloses the signature without a transform computeSignature() inserts the Signature before digesting, so a reference that encloses it digests an unfinished Signature whose SignatureValue is still empty. That digest can never be reproduced during verification. We accepted this config and emitted a signature nobody could verify. Refuse instead. The check fires only when every transform on the reference is one of the four W3C canonicalization algorithms, whose semantics are fixed by spec: they render the node-set and remove nothing, so the Signature provably survives. A chain containing enveloped-signature is fine, and a chain containing a caller-registered transform defers to the caller, who may well strip the Signature themselves. So there are no false positives. Five existing tests were signing exactly this broken config and asserting structural properties without ever verifying the result; each now declares the enveloped-signature transform a real caller would need. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- src/signed-xml.ts | 26 ++++++++++++++++++++++++++ test/signature-object-tests.spec.ts | 20 ++++++++++++++++---- test/signature-unit-tests.spec.ts | 28 +++++++++++++++++++++++++++- 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 106e640b..86310492 100644 --- a/README.md +++ b/README.md @@ -269,7 +269,7 @@ To sign xml documents: - `xpath` - a string containing a XPath expression referencing a xml element - `transforms` - an array of [transform algorithms](#canonicalization-and-transformation-algorithms), the referenced element will be transformed for each value in the array. Omit it, or pass an empty array, to emit no `Transforms` element; the referenced element is then digested after canonicalization alone. - Note that an enveloped signature needs `http://www.w3.org/2000/09/xmldsig#enveloped-signature` in this array — without it the `Signature` element is digested along with the content it signs, and the result will not verify. + If the referenced element encloses the signature, this array has to include `http://www.w3.org/2000/09/xmldsig#enveloped-signature`, which removes the `Signature` before digesting; `computeSignature()` rejects the reference otherwise rather than emit a signature that cannot verify. Supply your own transform and that check defers to it, since only you know whether it removes the `Signature`. - `digestAlgorithm` - one of the supported [hashing algorithms](#hashing-algorithms) - `id` - an optional `Id` attribute to add to the reference element - `type` - the optional `Type` attribute to add to the reference element (represented as a URI) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index fedcca18..2e05ac59 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -33,6 +33,16 @@ const warnOriginalXmlWithIds = deprecate( "XML_CRYPTO_GET_ORIGINAL_XML_WITH_IDS", ); +// Canonicalization renders the node-set it is given; only a transform that drops +// nodes can take the Signature back out. An unrecognised transform might do that, +// so a chain built purely from these is the one case where we can prove it cannot. +const CANONICALIZATION_ONLY_TRANSFORMS: ReadonlySet = new Set([ + "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", + "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments", + "http://www.w3.org/2001/10/xml-exc-c14n#", + "http://www.w3.org/2001/10/xml-exc-c14n#WithComments", +]); + export class SignedXml { idMode?: "wssecurity"; idAttributes: string[]; @@ -1136,6 +1146,22 @@ export class SignedXml { ); } + // The Signature is already in the document, and its SignatureValue is still + // empty, so digesting content that encloses it can never match on verification. + // https://www.w3.org/TR/xmldsig-core1/#sec-EnvelopedSignature + if ( + utils.isDescendantOf(signatureElem, node) && + (ref.transforms ?? []).every((transform) => + CANONICALIZATION_ONLY_TRANSFORMS.has(transform), + ) + ) { + throw new Error( + `The reference ${ref.xpath} encloses the signature, so it requires the ` + + "http://www.w3.org/2000/09/xmldsig#enveloped-signature transform. " + + "Without it the signature cannot be verified.", + ); + } + // Compute the target URI (ID already ensured earlier, extract it) let targetUri: string; if (ref.isEmptyUri) { diff --git a/test/signature-object-tests.spec.ts b/test/signature-object-tests.spec.ts index 75126c23..7866c19b 100644 --- a/test/signature-object-tests.spec.ts +++ b/test/signature-object-tests.spec.ts @@ -140,7 +140,10 @@ describe("ds:Object support in XML signatures", function () { sig.addReference({ xpath: "/*", digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + transforms: [ + "http://www.w3.org/2000/09/xmldsig#enveloped-signature", + "http://www.w3.org/2001/10/xml-exc-c14n#", + ], }); // When we add a prefix to the signature, there is no default namespace @@ -167,7 +170,10 @@ describe("ds:Object support in XML signatures", function () { sigWithNull.addReference({ xpath: "/*", digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + transforms: [ + "http://www.w3.org/2000/09/xmldsig#enveloped-signature", + "http://www.w3.org/2001/10/xml-exc-c14n#", + ], }); sigWithNull.computeSignature(xml); @@ -190,7 +196,10 @@ describe("ds:Object support in XML signatures", function () { sigWithEmpty.addReference({ xpath: "/*", digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + transforms: [ + "http://www.w3.org/2000/09/xmldsig#enveloped-signature", + "http://www.w3.org/2001/10/xml-exc-c14n#", + ], }); sigWithEmpty.computeSignature(xml); @@ -628,7 +637,10 @@ describe("Signature self-reference prevention", function () { sig.addReference({ xpath: "/*", digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + transforms: [ + "http://www.w3.org/2000/09/xmldsig#enveloped-signature", + "http://www.w3.org/2001/10/xml-exc-c14n#", + ], }); sig.addReference({ diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index d08ec71e..1a13098a 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -167,7 +167,10 @@ describe("Signature unit tests", function () { sig.addReference({ xpath: "//*[local-name(.)='x']", digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + transforms: [ + "http://www.w3.org/2000/09/xmldsig#enveloped-signature", + "http://www.w3.org/2001/10/xml-exc-c14n#", + ], }); sig.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#"; sig.signatureAlgorithm = "http://www.w3.org/2000/09/xmldsig#rsa-sha1"; @@ -1225,6 +1228,29 @@ describe("Signature unit tests", function () { ).to.throw(); }); + for (const { label, transforms } of [ + { label: "no transforms", transforms: undefined }, + { label: "canonicalization alone", transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"] }, + ]) { + it(`refuses to sign a reference that encloses the signature, given ${label}`, function () { + // The signature is appended into , so this reference covers it, and + // nothing in the chain can take it back out. Signing would emit a digest + // over an unfinished Signature that can never be reproduced on verification. + const xml = "hello"; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "/*", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + ...(transforms !== undefined ? { transforms } : {}), + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + + expect(() => sig.computeSignature(xml)).to.throw(/enveloped-signature transform/); + }); + } + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml(); From abfa1ad5687e3df724f335776c7c362a28b01150 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:39:20 -0500 Subject: [PATCH 16/20] refactor: let algorithms declare whether they drop nodes The guard kept its own copy of the four W3C canonicalization URIs alongside CanonicalizationAlgorithms. Two lists, and forgetting to update the second one failed open: a chain using the missed algorithm stopped being checked, so the broken config it was meant to catch would sail through. Whether a transform can drop nodes is a property of the algorithm, so put it there. `removesNodes` is optional, so existing custom implementations still compile and simply defer as before, but one that declares `false` now opts into the check, and a new built-in inherits it with no second place to edit. Co-Authored-By: Claude Opus 5 --- src/c14n-canonicalization.ts | 2 ++ src/enveloped-signature.ts | 2 ++ src/exclusive-canonicalization.ts | 2 ++ src/signed-xml.ts | 21 ++++++++------------- src/types.ts | 12 ++++++++++++ 5 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/c14n-canonicalization.ts b/src/c14n-canonicalization.ts index a77bc219..eb51185a 100644 --- a/src/c14n-canonicalization.ts +++ b/src/c14n-canonicalization.ts @@ -8,6 +8,8 @@ import * as utils from "./utils"; import * as isDomNode from "@xmldom/is-dom-node"; export class C14nCanonicalization implements CanonicalizationOrTransformationAlgorithm { + readonly removesNodes = false; + protected includeComments = false; constructor() { diff --git a/src/enveloped-signature.ts b/src/enveloped-signature.ts index d234bc5e..05187875 100644 --- a/src/enveloped-signature.ts +++ b/src/enveloped-signature.ts @@ -9,6 +9,8 @@ import type { } from "./types"; export class EnvelopedSignature implements CanonicalizationOrTransformationAlgorithm { + readonly removesNodes = true; + protected includeComments = false; constructor() { diff --git a/src/exclusive-canonicalization.ts b/src/exclusive-canonicalization.ts index ea88aa2c..6eb74e49 100644 --- a/src/exclusive-canonicalization.ts +++ b/src/exclusive-canonicalization.ts @@ -18,6 +18,8 @@ function isPrefixInScope(prefixesInScope, prefix, namespaceURI) { } export class ExclusiveCanonicalization implements CanonicalizationOrTransformationAlgorithm { + readonly removesNodes = false; + protected includeComments = false; constructor() { diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 2e05ac59..cb528fbc 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -33,16 +33,6 @@ const warnOriginalXmlWithIds = deprecate( "XML_CRYPTO_GET_ORIGINAL_XML_WITH_IDS", ); -// Canonicalization renders the node-set it is given; only a transform that drops -// nodes can take the Signature back out. An unrecognised transform might do that, -// so a chain built purely from these is the one case where we can prove it cannot. -const CANONICALIZATION_ONLY_TRANSFORMS: ReadonlySet = new Set([ - "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", - "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments", - "http://www.w3.org/2001/10/xml-exc-c14n#", - "http://www.w3.org/2001/10/xml-exc-c14n#WithComments", -]); - export class SignedXml { idMode?: "wssecurity"; idAttributes: string[]; @@ -483,6 +473,13 @@ export class SignedXml { } } + /** An algorithm that has not declared itself might drop the `Signature`, so only a + * declared `false` proves the node-set survives the transform intact. */ + private preservesEveryNode(name: CanonicalizationOrTransformAlgorithmType): boolean { + const algo = this.CanonicalizationAlgorithms[name]; + return algo != null && new algo().removesNodes === false; + } + private findCanonicalizationAlgorithm(name: CanonicalizationOrTransformAlgorithmType) { if (name != null) { const algo = this.CanonicalizationAlgorithms[name]; @@ -1151,9 +1148,7 @@ export class SignedXml { // https://www.w3.org/TR/xmldsig-core1/#sec-EnvelopedSignature if ( utils.isDescendantOf(signatureElem, node) && - (ref.transforms ?? []).every((transform) => - CANONICALIZATION_ONLY_TRANSFORMS.has(transform), - ) + (ref.transforms ?? []).every((transform) => this.preservesEveryNode(transform)) ) { throw new Error( `The reference ${ref.xpath} encloses the signature, so it requires the ` + diff --git a/src/types.ts b/src/types.ts index 3219a009..4d9a8b39 100644 --- a/src/types.ts +++ b/src/types.ts @@ -169,6 +169,18 @@ export interface CanonicalizationOrTransformationAlgorithm { ): Node | string; getAlgorithmName(): CanonicalizationOrTransformAlgorithmType; + + /** + * Whether this algorithm can drop nodes from the node-set it is given, rather + * than only re-serializing it. Canonicalization algorithms set this to `false`; + * the enveloped-signature transform sets it to `true`. + * + * `computeSignature()` reads it to decide whether a reference that encloses the + * signature can ever verify: if every transform on that reference declares + * `false`, the `Signature` provably survives into the digest and the reference + * is rejected. Leave it undefined and that check defers to you. + */ + removesNodes?: boolean; } /** Implement this to create a new HashAlgorithm */ From a7521445261284f458420e9b5515757bbced507e Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:47:30 -0500 Subject: [PATCH 17/20] fix!: require removesNodes on CanonicalizationOrTransformationAlgorithm BREAKING CHANGE: custom canonicalization and transform algorithms must declare `removesNodes`. Declare `false` only if the algorithm returns the node-set it was given; declare `true` if it filters nodes at all, which also preserves the existing behaviour. Leaving it optional kept a third state where an implementer who never knew the field existed silently opted out of the check that rejects a reference enclosing the signature. That is a default deciding something security-relevant on their behalf, which is the shape this project sets out to avoid. The runtime still tests `=== false` rather than trusting the type, since registering an algorithm is reachable from JavaScript and an undeclared value must defer rather than reject wrongly. Covers the README examples, which teach custom algorithms as plain JavaScript where the compiler cannot prompt for the field. Co-Authored-By: Claude Opus 5 --- README.md | 7 +++++++ src/signed-xml.ts | 5 +++-- src/types.ts | 13 ++++++++----- test/signature-unit-tests.spec.ts | 8 ++++++++ 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 86310492..32cb2851 100644 --- a/README.md +++ b/README.md @@ -344,6 +344,9 @@ Custom transformation algorithm. ```javascript function MyTransformation() { + /*whether this drops nodes from the node-set, rather than only re-serializing it*/ + this.removesNodes = false; + /*given a node (from the xmldom module) return its canonical representation (as string)*/ this.process = function (node) { //you should apply your transformation before returning @@ -356,10 +359,14 @@ function MyTransformation() { } ``` +Declare `removesNodes = false` only if the algorithm returns the node-set it was given. If it filters nodes at all, declare `true`. `computeSignature()` reads this to decide whether a reference enclosing the signature can ever verify, so a wrong `false` there turns a signature it should have rejected into one nobody can verify. + Custom canonicalization is actually the same as custom transformation. It is applied on the SignedInfo rather than on references. ```javascript function MyCanonicalization() { + this.removesNodes = false; + /*given a node (from the xmldom module) return its canonical representation (as string)*/ this.process = function (node) { //you should apply your transformation before returning diff --git a/src/signed-xml.ts b/src/signed-xml.ts index cb528fbc..ea9f9f75 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -473,8 +473,9 @@ export class SignedXml { } } - /** An algorithm that has not declared itself might drop the `Signature`, so only a - * declared `false` proves the node-set survives the transform intact. */ + // Registering an algorithm is reachable from JavaScript, so `removesNodes` can + // still arrive undefined despite being required; only an explicit `false` proves + // the node-set survives, and anything else defers rather than reject wrongly. private preservesEveryNode(name: CanonicalizationOrTransformAlgorithmType): boolean { const algo = this.CanonicalizationAlgorithms[name]; return algo != null && new algo().removesNodes === false; diff --git a/src/types.ts b/src/types.ts index 4d9a8b39..8806ae2a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -172,15 +172,18 @@ export interface CanonicalizationOrTransformationAlgorithm { /** * Whether this algorithm can drop nodes from the node-set it is given, rather - * than only re-serializing it. Canonicalization algorithms set this to `false`; - * the enveloped-signature transform sets it to `true`. + * than only re-serializing it. Canonicalization algorithms declare `false`; + * the enveloped-signature transform declares `true`. * * `computeSignature()` reads it to decide whether a reference that encloses the * signature can ever verify: if every transform on that reference declares - * `false`, the `Signature` provably survives into the digest and the reference - * is rejected. Leave it undefined and that check defers to you. + * `false`, the `Signature` provably survives into the digest, so the reference + * is rejected rather than signed into something unverifiable. + * + * Declare `false` only if the algorithm returns the node-set it was given. If it + * filters nodes at all, declare `true`, which is also the conservative answer. */ - removesNodes?: boolean; + removesNodes: boolean; } /** Implement this to create a new HashAlgorithm */ diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 1a13098a..55c9eabb 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -413,6 +413,8 @@ describe("Signature unit tests", function () { class DummyTransformation { includeComments = false; + // Returns a fixed string rather than the node-set it was given. + removesNodes = true; process = function () { return "< x/>"; }; @@ -424,6 +426,8 @@ describe("Signature unit tests", function () { class DummyCanonicalization { includeComments = false; + // Returns a fixed string rather than the node-set it was given. + removesNodes = true; process = function () { return "< x/>"; }; @@ -581,6 +585,8 @@ describe("Signature unit tests", function () { class DummyTransformation { includeComments = false; + // Returns a fixed string rather than the node-set it was given. + removesNodes = true; process = function () { return "< x/>"; }; @@ -592,6 +598,8 @@ describe("Signature unit tests", function () { class DummyCanonicalization { includeComments = false; + // Returns a fixed string rather than the node-set it was given. + removesNodes = true; process = function () { return "< x/>"; }; From f7d916d87daa23a0e2efc5c258ebdb87e7081607 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:59:39 -0500 Subject: [PATCH 18/20] fix: enforce removesNodes at runtime and let subclasses declare it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the previous commit, found in review. The runtime read `removesNodes === false`, so an omitted or non-boolean value made the check defer. That reinstated at runtime exactly the silent opt-out requiring the field was meant to remove, and it is JavaScript callers — the ones the type cannot reach — who land there. Reproduced: a JS custom canonicalizer with no declaration signed a reference that checkSignature() then rejected. It now throws unless the algorithm declares a boolean. `readonly removesNodes = false` inferred the literal type `false`, so a subclass adding node filtering could not declare `true`. Annotated as `boolean` in each built-in. The README and the interface doc had the consequences backwards: a wrong `true` skips the check and can sign something unverifiable, while a wrong `false` rejects a reference that would have worked. The doc also called `true` conservative when it is the permissive answer. The check runs where the value is consumed, so an undeclared algorithm on a reference that does not enclose the signature still works; it throws only where the declaration would change the outcome. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- src/c14n-canonicalization.ts | 2 +- src/enveloped-signature.ts | 2 +- src/exclusive-canonicalization.ts | 2 +- src/signed-xml.ts | 19 ++++++++++--- src/types.ts | 6 +++-- test/signature-unit-tests.spec.ts | 44 ++++++++++++++++++++++++++++++- 7 files changed, 66 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 32cb2851..2b974984 100644 --- a/README.md +++ b/README.md @@ -359,7 +359,7 @@ function MyTransformation() { } ``` -Declare `removesNodes = false` only if the algorithm returns the node-set it was given. If it filters nodes at all, declare `true`. `computeSignature()` reads this to decide whether a reference enclosing the signature can ever verify, so a wrong `false` there turns a signature it should have rejected into one nobody can verify. +Declare `removesNodes = false` if the algorithm returns the node-set it was given, and `true` if it filters nodes at all. `computeSignature()` reads this to decide whether a reference enclosing the signature can ever verify, and the two mistakes cost you different things: declaring `true` when the algorithm actually preserves everything skips that check, so you can sign a reference nobody can verify, while declaring `false` when it actually filters makes the check reject a reference that would have worked. Custom canonicalization is actually the same as custom transformation. It is applied on the SignedInfo rather than on references. diff --git a/src/c14n-canonicalization.ts b/src/c14n-canonicalization.ts index eb51185a..61c0cf8f 100644 --- a/src/c14n-canonicalization.ts +++ b/src/c14n-canonicalization.ts @@ -8,7 +8,7 @@ import * as utils from "./utils"; import * as isDomNode from "@xmldom/is-dom-node"; export class C14nCanonicalization implements CanonicalizationOrTransformationAlgorithm { - readonly removesNodes = false; + readonly removesNodes: boolean = false; protected includeComments = false; diff --git a/src/enveloped-signature.ts b/src/enveloped-signature.ts index 05187875..6d1c1022 100644 --- a/src/enveloped-signature.ts +++ b/src/enveloped-signature.ts @@ -9,7 +9,7 @@ import type { } from "./types"; export class EnvelopedSignature implements CanonicalizationOrTransformationAlgorithm { - readonly removesNodes = true; + readonly removesNodes: boolean = true; protected includeComments = false; diff --git a/src/exclusive-canonicalization.ts b/src/exclusive-canonicalization.ts index 6eb74e49..062b5e49 100644 --- a/src/exclusive-canonicalization.ts +++ b/src/exclusive-canonicalization.ts @@ -18,7 +18,7 @@ function isPrefixInScope(prefixesInScope, prefix, namespaceURI) { } export class ExclusiveCanonicalization implements CanonicalizationOrTransformationAlgorithm { - readonly removesNodes = false; + readonly removesNodes: boolean = false; protected includeComments = false; diff --git a/src/signed-xml.ts b/src/signed-xml.ts index ea9f9f75..8a57dd5b 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -473,12 +473,23 @@ export class SignedXml { } } - // Registering an algorithm is reachable from JavaScript, so `removesNodes` can - // still arrive undefined despite being required; only an explicit `false` proves - // the node-set survives, and anything else defers rather than reject wrongly. + // Registering an algorithm is reachable from JavaScript, where the required type + // buys nothing, so the declaration is checked here. Treating an undeclared value + // as "might remove nodes" would silently reinstate the opt-out the contract exists + // to remove, and sign references that cannot verify. private preservesEveryNode(name: CanonicalizationOrTransformAlgorithmType): boolean { const algo = this.CanonicalizationAlgorithms[name]; - return algo != null && new algo().removesNodes === false; + if (algo == null) { + // Unregistered; findCanonicalizationAlgorithm reports it with a better message. + return false; + } + + const { removesNodes } = new algo(); + if (typeof removesNodes !== "boolean") { + throw new Error(`canonicalization algorithm '${name}' must declare a boolean 'removesNodes'`); + } + + return !removesNodes; } private findCanonicalizationAlgorithm(name: CanonicalizationOrTransformAlgorithmType) { diff --git a/src/types.ts b/src/types.ts index 8806ae2a..69da460b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -180,8 +180,10 @@ export interface CanonicalizationOrTransformationAlgorithm { * `false`, the `Signature` provably survives into the digest, so the reference * is rejected rather than signed into something unverifiable. * - * Declare `false` only if the algorithm returns the node-set it was given. If it - * filters nodes at all, declare `true`, which is also the conservative answer. + * Declare `false` if the algorithm returns the node-set it was given, `true` if it + * filters nodes at all. Declaring `true` when it actually preserves everything + * skips the check above and can sign a reference nobody can verify; declaring + * `false` when it actually filters can reject a reference that would have worked. */ removesNodes: boolean; } diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 55c9eabb..e2f39ad8 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1,6 +1,10 @@ import * as xpath from "xpath"; import * as xmldom from "@xmldom/xmldom"; -import { SignedXml, createOptionalCallbackFunction } from "../src/index"; +import { + SignedXml, + createOptionalCallbackFunction, + type CanonicalizationOrTransformationAlgorithm, +} from "../src/index"; import * as fs from "fs"; import * as crypto from "crypto"; import { expect } from "chai"; @@ -1259,6 +1263,44 @@ describe("Signature unit tests", function () { }); } + for (const { label, declared } of [ + { label: "omits removesNodes", declared: undefined }, + { label: "declares a non-boolean removesNodes", declared: "false" }, + ]) { + it(`refuses to sign with a registered algorithm that ${label}`, function () { + // Registering an algorithm is reachable from JavaScript, where the required + // type is no help. An undeclared value must not quietly skip the check on a + // reference that encloses the signature. + class UndeclaredAlgorithm { + process() { + return ""; + } + getAlgorithmName() { + return "http://Undeclared"; + } + } + if (declared !== undefined) { + Object.assign(UndeclaredAlgorithm.prototype, { removesNodes: declared }); + } + + const sig = new SignedXml(); + sig.CanonicalizationAlgorithms["http://Undeclared"] = + UndeclaredAlgorithm as unknown as new () => CanonicalizationOrTransformationAlgorithm; + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "/*", + transforms: ["http://Undeclared"], + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + + expect(() => sig.computeSignature("hi")).to.throw( + /must declare a boolean 'removesNodes'/, + ); + }); + } + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml(); From 7514d6d132f3524e4ec71ee05a70c3ec96b045c9 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 21:43:37 -0500 Subject: [PATCH 19/20] fix: validate removesNodes in findCanonicalizationAlgorithm Checking the declaration inside the predicate that `.every()` consumes made enforcement depend on transform order: an undeclared algorithm was rejected before enveloped-signature but skipped after it, because `.every()` stops at the first transform that removes nodes. Same chain, opposite outcomes. Validation belongs where algorithms are instantiated, not in a boolean a caller may short-circuit past. findCanonicalizationAlgorithm() now checks it, so every algorithm we construct is validated once, and the enclosing-reference check resolves the whole chain before testing it. This makes enforcement consistent rather than only reaching the enveloped case: an undeclared algorithm now fails wherever it is used, including on verification, which is the right direction for a check that decides whether a signature can be trusted. Registrations that are never used stay unaffected. Co-Authored-By: Claude Opus 5 --- src/signed-xml.ts | 35 +++++++++++++------------------ test/signature-unit-tests.spec.ts | 30 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 8a57dd5b..04165f52 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -473,30 +473,21 @@ export class SignedXml { } } - // Registering an algorithm is reachable from JavaScript, where the required type - // buys nothing, so the declaration is checked here. Treating an undeclared value - // as "might remove nodes" would silently reinstate the opt-out the contract exists - // to remove, and sign references that cannot verify. - private preservesEveryNode(name: CanonicalizationOrTransformAlgorithmType): boolean { - const algo = this.CanonicalizationAlgorithms[name]; - if (algo == null) { - // Unregistered; findCanonicalizationAlgorithm reports it with a better message. - return false; - } - - const { removesNodes } = new algo(); - if (typeof removesNodes !== "boolean") { - throw new Error(`canonicalization algorithm '${name}' must declare a boolean 'removesNodes'`); - } - - return !removesNodes; - } - private findCanonicalizationAlgorithm(name: CanonicalizationOrTransformAlgorithmType) { if (name != null) { const algo = this.CanonicalizationAlgorithms[name]; if (algo) { - return new algo(); + const instance = new algo(); + // Registering is reachable from JavaScript, where the required type buys + // nothing. Checking here covers every algorithm we instantiate, rather than + // only the ones a particular caller happens to look at. + if (typeof instance.removesNodes !== "boolean") { + throw new Error( + `canonicalization algorithm '${name}' must declare a boolean 'removesNodes'`, + ); + } + + return instance; } } @@ -1160,7 +1151,9 @@ export class SignedXml { // https://www.w3.org/TR/xmldsig-core1/#sec-EnvelopedSignature if ( utils.isDescendantOf(signatureElem, node) && - (ref.transforms ?? []).every((transform) => this.preservesEveryNode(transform)) + (ref.transforms ?? []) + .map((transform) => this.findCanonicalizationAlgorithm(transform)) + .every((algorithm) => !algorithm.removesNodes) ) { throw new Error( `The reference ${ref.xpath} encloses the signature, so it requires the ` + diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index e2f39ad8..70f1a5d1 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1301,6 +1301,36 @@ describe("Signature unit tests", function () { }); } + it("validates a transform declaration wherever it sits in the chain", function () { + // The enclosing-reference check stops at the first transform that removes + // nodes, so an undeclared algorithm sitting after enveloped-signature must + // still be rejected rather than skipped. + class UndeclaredAlgorithm { + process(node: Node) { + return node; + } + getAlgorithmName() { + return "http://Undeclared"; + } + } + + const sig = new SignedXml(); + sig.CanonicalizationAlgorithms["http://Undeclared"] = + UndeclaredAlgorithm as unknown as new () => CanonicalizationOrTransformationAlgorithm; + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "/*", + transforms: ["http://www.w3.org/2000/09/xmldsig#enveloped-signature", "http://Undeclared"], + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + + expect(() => sig.computeSignature("hi")).to.throw( + /must declare a boolean 'removesNodes'/, + ); + }); + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml(); From dcc89d434650d0b7351d41fff40d6a25f1649f47 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Wed, 9 Sep 2026 06:22:18 -0500 Subject: [PATCH 20/20] docs: state that removesNodes is enforced on verification too The interface doc and README both described the field in terms of computeSignature(), which understated it: validation happens as an algorithm is instantiated, so checkSignature() throws on a missing or non-boolean value as well. Anyone with a custom algorithm meets this on both paths, and the README teaches those in plain JavaScript where the type cannot prompt them. Adds the matching verification regression. It first asserts a correctly declared verifier accepts the document, so the rejection that follows is attributable to the misconfigured registry rather than to a bad signature. Co-Authored-By: Claude Opus 5 --- README.md | 4 ++- src/types.ts | 14 +++++--- test/signature-unit-tests.spec.ts | 60 +++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 2b974984..ee3b27fc 100644 --- a/README.md +++ b/README.md @@ -359,7 +359,9 @@ function MyTransformation() { } ``` -Declare `removesNodes = false` if the algorithm returns the node-set it was given, and `true` if it filters nodes at all. `computeSignature()` reads this to decide whether a reference enclosing the signature can ever verify, and the two mistakes cost you different things: declaring `true` when the algorithm actually preserves everything skips that check, so you can sign a reference nobody can verify, while declaring `false` when it actually filters makes the check reject a reference that would have worked. +`removesNodes` is required on every canonicalization and transformation algorithm, including ones written in plain JavaScript. It is checked as the algorithm is instantiated, so a missing or non-boolean value throws from `checkSignature()` just as it does from `computeSignature()` — a verifier whose own registry is misconfigured refuses rather than canonicalizing with an algorithm that never said what it does. Registrations you never use are not checked. + +Declare `false` if the algorithm returns the node-set it was given, and `true` if it filters nodes at all. `computeSignature()` also reads this to decide whether a reference enclosing the signature can ever verify, and the two mistakes cost you different things: declaring `true` when the algorithm actually preserves everything skips that check, so you can sign a reference nobody can verify, while declaring `false` when it actually filters makes the check reject a reference that would have worked. Custom canonicalization is actually the same as custom transformation. It is applied on the SignedInfo rather than on references. diff --git a/src/types.ts b/src/types.ts index 69da460b..cb6662ee 100644 --- a/src/types.ts +++ b/src/types.ts @@ -175,10 +175,16 @@ export interface CanonicalizationOrTransformationAlgorithm { * than only re-serializing it. Canonicalization algorithms declare `false`; * the enveloped-signature transform declares `true`. * - * `computeSignature()` reads it to decide whether a reference that encloses the - * signature can ever verify: if every transform on that reference declares - * `false`, the `Signature` provably survives into the digest, so the reference - * is rejected rather than signed into something unverifiable. + * Required, and enforced on both sides: every algorithm is checked as it is + * instantiated, so a missing or non-boolean value throws from `computeSignature()` + * and from `checkSignature()` alike. That covers algorithms registered from + * JavaScript, which this type cannot reach. Registrations that are never used are + * not checked. + * + * `computeSignature()` also reads the value to decide whether a reference that + * encloses the signature can ever verify: if every transform on that reference + * declares `false`, the `Signature` provably survives into the digest, so the + * reference is rejected rather than signed into something unverifiable. * * Declare `false` if the algorithm returns the node-set it was given, `true` if it * filters nodes at all. Declaring `true` when it actually preserves everything diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 70f1a5d1..48e6a085 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1331,6 +1331,66 @@ describe("Signature unit tests", function () { ); }); + it("refuses to verify with a registered algorithm that omits removesNodes", function () { + // The declaration is part of the contract on both sides, so a verifier whose + // own registry is misconfigured must refuse rather than canonicalize with an + // algorithm that never said what it does. + const custom = "http://Custom"; + class DeclaredTransform { + removesNodes = false; + process(node: Node) { + return node.toString(); + } + getAlgorithmName() { + return custom; + } + } + class UndeclaredTransform { + process(node: Node) { + return node.toString(); + } + getAlgorithmName() { + return custom; + } + } + const register = (target: SignedXml, algorithm: unknown) => { + target.CanonicalizationAlgorithms[custom] = + algorithm as new () => CanonicalizationOrTransformationAlgorithm; + }; + + const sig = new SignedXml(); + register(sig, DeclaredTransform); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + transforms: [custom], + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature("hi"); + const signedXml = sig.getSignedXml(); + + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const sigNode = xpath.select1("//*[local-name(.)='Signature']", doc); + isDomNode.assertIsNodeLike(sigNode); + + // The document itself is sound: a correctly declared verifier accepts it. + const declaredVerifier = new SignedXml(); + register(declaredVerifier, DeclaredTransform); + declaredVerifier.publicCert = fs.readFileSync("./test/static/client_public.pem"); + declaredVerifier.loadSignature(sigNode); + expect(declaredVerifier.checkSignature(signedXml), "document should be valid").to.be.true; + + const undeclaredVerifier = new SignedXml(); + register(undeclaredVerifier, UndeclaredTransform); + undeclaredVerifier.publicCert = fs.readFileSync("./test/static/client_public.pem"); + undeclaredVerifier.loadSignature(sigNode); + expect(() => undeclaredVerifier.checkSignature(signedXml)).to.throw( + /must declare a boolean 'removesNodes'/, + ); + }); + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml();