From 86dc651bffacf60c3ab605f49374ac4c29ac81d4 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 11 Aug 2026 11:59:12 -0700 Subject: [PATCH 01/18] 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 | 79 +++++++++++++++------------- src/types.ts | 4 +- test/signature-unit-tests.spec.ts | 85 ++++++++++++++++++++++++++++++- 3 files changed, 130 insertions(+), 38 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index a6735089..e7ea1576 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -615,9 +615,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 = { ancestorNamespaces: ref.ancestorNamespaces, @@ -636,7 +634,7 @@ export class SignedXml { // Signing gives the reference's current list to the exclusive canonicalization transforms, the // ones createReferences writes it under, so the digest matches the Transforms it serializes. private getSigningPrefixLists(ref: Reference): string[][] { - return ref.transforms.map((transform) => + return (ref.transforms ?? []).map((transform) => isExclusiveC14n(this.findCanonicalizationAlgorithm(transform).getAlgorithmName()) ? ref.inclusiveNamespacesPrefixList : [], @@ -1043,10 +1041,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, @@ -1500,35 +1494,39 @@ 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) && - isExclusiveC14n(transform.getAlgorithmName()) - ) { - const inclusiveNamespacesElem = signatureDoc.createElementNS( - exclusiveC14nNamespace, - "InclusiveNamespaces", - ); - inclusiveNamespacesElem.setAttribute( - "PrefixList", - ref.inclusiveNamespacesPrefixList.join(" "), + + for (const trans of ref.transforms) { + const transform = this.findCanonicalizationAlgorithm(trans); + const transformElem = signatureDoc.createElementNS( + signatureNamespace, + `${currentPrefix}Transform`, ); - transformElem.appendChild(inclusiveNamespacesElem); + transformElem.setAttribute("Algorithm", transform.getAlgorithmName()); + + if ( + utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList) && + isExclusiveC14n(transform.getAlgorithmName()) + ) { + const inclusiveNamespacesElem = signatureDoc.createElementNS( + exclusiveC14nNamespace, + "InclusiveNamespaces", + ); + inclusiveNamespacesElem.setAttribute( + "PrefixList", + ref.inclusiveNamespacesPrefixList.join(" "), + ); + transformElem.appendChild(inclusiveNamespacesElem); + } + + transformsElem.appendChild(transformElem); } - transformsElem.appendChild(transformElem); + referenceElem.appendChild(transformsElem); } const digestAlgorithm = this.findHashAlgorithm(ref.digestAlgorithm); @@ -1545,7 +1543,6 @@ export class SignedXml { ); digestValueElem.textContent = digestValue ?? this.calculateReferenceDigest(ref, node); - referenceElem.appendChild(transformsElem); referenceElem.appendChild(digestMethodElem); referenceElem.appendChild(digestValueElem); @@ -1628,9 +1625,13 @@ export class SignedXml { options.ancestorXmlAttributes ?? utils.findAncestorXmlAttributes(node); options.signatureNode = this.signatureNode; + // transforms is now optional on Reference (see addReference) — empty/no transforms is a + // valid, SMPTE-required state, not an error. Treat it as an empty list everywhere below. + const effectiveTransforms = transforms ?? []; + const canonXml = node.cloneNode(true); // Deep clone const preserveDocument = isDomNode.isDocumentNode(node); - if (transforms.includes("http://www.w3.org/2000/09/xmldsig#enveloped-signature")) { + if (effectiveTransforms.includes("http://www.w3.org/2000/09/xmldsig#enveloped-signature")) { const signaturePath: number[] = []; let signatureAncestor = this.findLoadedSignature(node); while (signatureAncestor?.parentNode && signatureAncestor !== node) { @@ -1656,7 +1657,10 @@ export class SignedXml { // Octets are parsed into a node-set for the next transform, and a node-set left at the end is // converted to octets with C14N: https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel - for (const [index, transformName] of transforms.entries()) { + // (When effectiveTransforms is empty this loop simply doesn't run, and the isNodeLike check + // below applies plain C14N to the untransformed node-set — exactly the SMPTE-required + // no-transforms case, so no separate empty-transforms branch is needed here.) + for (const [index, transformName] of effectiveTransforms.entries()) { if (!isDomNode.isNodeLike(transformedXml)) { transformedXml = this.parseTransformInput(transformedXml, transformName, preserveDocument); // The parsed octets are a new document, so the referenced node's ancestors are gone. @@ -1693,6 +1697,11 @@ export class SignedXml { ).process(transformedXml, transformOptions); } + // No separate empty-transforms case needed here: the isNodeLike check above already + // applies C14N to any remaining node-set, which is exactly what happens when + // effectiveTransforms is empty (the for loop above never ran) — the SMPTE-required + // no-transforms case is just the simplest instance of the same general rule upstream + // already enforces for "a node-set left at the end of the transforms" (see #581). return transformedXml.toString(); } diff --git a/src/types.ts b/src/types.ts index 87e622f2..547288b2 100644 --- a/src/types.ts +++ b/src/types.ts @@ -145,7 +145,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 80465d97..dce0d389 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1001,8 +1001,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"); } @@ -1231,6 +1231,87 @@ 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(); + + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='repository']", + digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", + transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + }); + + try { + sig.computeSignature(xml, { + location: { + reference: "/root/foobar", + action: "append", + }, + }); + expect.fail("Expected an error to be thrown"); + } catch (err) { + expect(err).not.to.be.an.instanceof(TypeError); + } + }); + it("signer adds existing prefixes", function () { function getKeyInfoContentWithAssertionId({ assertionId }) { return ( From ac379fe4977ea9f48841ba1425949615caa5647c Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 11 Aug 2026 13:41:52 -0700 Subject: [PATCH 02/18] 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 dce0d389..8d68d703 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1001,8 +1001,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 935a29deb38f2f767ad60ca2b91aedcb06462a2e Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Wed, 12 Aug 2026 16:48:38 -0700 Subject: [PATCH 03/18] 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 8d68d703..5676c697 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1230,30 +1230,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 22f4d8448f9ab1167f881301196097e129be5562 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Wed, 12 Aug 2026 16:52:25 -0700 Subject: [PATCH 04/18] 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 | 14 ++++++++--- test/signature-unit-tests.spec.ts | 42 +++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index e7ea1576..d604ef2f 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -611,11 +611,19 @@ export class SignedXml { }; } - 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. findAncestorNsForElement (added upstream for + * findSignedInfoAncestorContext, above) already solves exactly this for a + * known element, so reuse it here instead of re-running the xpath. */ - ref.ancestorNamespaces = utils.findAncestorNs(doc, ref.xpath, this.namespaceResolver); + if (isDomNode.isElementNode(node)) { + ref.ancestorNamespaces = utils.findAncestorNsForElement(node); + } const c14nOptions = { ancestorNamespaces: ref.ancestorNamespaces, diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 5676c697..2d2893c3 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1290,6 +1290,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 4d5db12cf2904fa827e596aa21a01ffc404f05da Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Mon, 17 Aug 2026 10:48:17 -0700 Subject: [PATCH 05/18] docs: expand buildAncestorNsForElement JSDoc and fix prettier formatting --- src/utils.ts | 8 ++++++++ test/signature-unit-tests.spec.ts | 6 ++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index b8bfeb67..da1b320b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -579,6 +579,14 @@ 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 node - The element whose ancestor namespace declarations to process + * @returns Deduplicated array of ancestor namespace prefixes not already declared on the element + */ export function findAncestorNsForElement(node: Element): NamespacePrefix[] { const ancestorNs = collectAncestorNamespaces(node); const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 2d2893c3..8d974380 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1253,8 +1253,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 cb9a7f46984e8373c46c28b5e3b7bc3c8b378abd Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 8 Sep 2026 09:13:09 -0700 Subject: [PATCH 06/18] 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 8d974380..21acc5f4 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1305,16 +1305,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 6b692bef8992f59f3fcf6c3a27fb045904e61db2 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 14:40:17 -0500 Subject: [PATCH 07/18] 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 | 37 +++++++++++++------------------ src/types.ts | 6 ++--- test/signature-unit-tests.spec.ts | 18 +++++++-------- 3 files changed, 28 insertions(+), 33 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index d604ef2f..c0f69dd7 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -611,16 +611,11 @@ export class SignedXml { }; } - 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. findAncestorNsForElement (added upstream for - * findSignedInfoAncestorContext, above) already solves exactly this for a - * known element, so reuse it here instead of re-running the xpath. - */ + 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. findAncestorNsForElement (above) already + // takes a known element instead, so reuse it here. if (isDomNode.isElementNode(node)) { ref.ancestorNamespaces = utils.findAncestorNsForElement(node); } @@ -642,7 +637,7 @@ export class SignedXml { // Signing gives the reference's current list to the exclusive canonicalization transforms, the // ones createReferences writes it under, so the digest matches the Transforms it serializes. private getSigningPrefixLists(ref: Reference): string[][] { - return (ref.transforms ?? []).map((transform) => + return ref.transforms.map((transform) => isExclusiveC14n(this.findCanonicalizationAlgorithm(transform).getAlgorithmName()) ? ref.inclusiveNamespacesPrefixList : [], @@ -742,7 +737,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); @@ -808,7 +803,7 @@ export class SignedXml { return undefined; } - const canonXml = this.getCanonReferenceXml(doc, ref, elem); + const canonXml = this.getCanonReferenceXml(ref, elem); const hash = this.findHashAlgorithm(ref.digestAlgorithm); const digest = hash.getHash(canonXml); @@ -1051,7 +1046,7 @@ export class SignedXml { this.references.push({ xpath, - transforms, + transforms: transforms ?? [], digestAlgorithm, uri, digestValue, @@ -1633,13 +1628,9 @@ export class SignedXml { options.ancestorXmlAttributes ?? utils.findAncestorXmlAttributes(node); options.signatureNode = this.signatureNode; - // transforms is now optional on Reference (see addReference) — empty/no transforms is a - // valid, SMPTE-required state, not an error. Treat it as an empty list everywhere below. - const effectiveTransforms = transforms ?? []; - const canonXml = node.cloneNode(true); // Deep clone const preserveDocument = isDomNode.isDocumentNode(node); - if (effectiveTransforms.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.findLoadedSignature(node); while (signatureAncestor?.parentNode && signatureAncestor !== node) { @@ -1665,10 +1656,10 @@ export class SignedXml { // Octets are parsed into a node-set for the next transform, and a node-set left at the end is // converted to octets with C14N: https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel - // (When effectiveTransforms is empty this loop simply doesn't run, and the isNodeLike check + // (When transforms is empty this loop simply doesn't run, and the isNodeLike check // below applies plain C14N to the untransformed node-set — exactly the SMPTE-required // no-transforms case, so no separate empty-transforms branch is needed here.) - for (const [index, transformName] of effectiveTransforms.entries()) { + for (const [index, transformName] of transforms.entries()) { if (!isDomNode.isNodeLike(transformedXml)) { transformedXml = this.parseTransformInput(transformedXml, transformName, preserveDocument); // The parsed octets are a new document, so the referenced node's ancestors are gone. @@ -1699,6 +1690,10 @@ export class SignedXml { ); } + if (typeof transformedXml === "string") { + return transformedXml; + } + if (isDomNode.isNodeLike(transformedXml)) { transformedXml = this.findCanonicalizationAlgorithm( "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", diff --git a/src/types.ts b/src/types.ts index 547288b2..b31b119f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -145,9 +145,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/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 21acc5f4..e0822894 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1292,11 +1292,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
" + @@ -1305,12 +1305,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 23450f539fb85adf94bf63a8dde0902ded22db50 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 14:46:36 -0500 Subject: [PATCH 08/18] 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 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 497943c3..2c4aec25 100644 --- a/README.md +++ b/README.md @@ -417,7 +417,9 @@ To sign xml documents: - `addReference({ xpath, transforms, digestAlgorithm, id, type, isEmptyUri, inclusiveNamespacesPrefixList })` - 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) From 667a514d636950f7059bffaaa230c89f1b50d194 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 15:37:46 -0500 Subject: [PATCH 09/18] 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 b31b119f..6dea1f0e 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 447a69ae..2d376327 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -172,6 +172,23 @@ describe("C14N non-exclusive canonicalization tests", function () { test_findAncestorNs(xml, xpath, []); }); + 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 0f7aca6e1060c9eb7d10f062823f1cebfc59902f Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 18:55:53 -0500 Subject: [PATCH 10/18] Lint --- src/types.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/types.ts b/src/types.ts index 6dea1f0e..b31b119f 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 22ced8ada00e4ef4fcf0efc52f98330784285afa Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:07:55 -0500 Subject: [PATCH 11/18] 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 | 8 ++--- test/c14n-non-exclusive-unit-tests.spec.ts | 2 +- test/signature-unit-tests.spec.ts | 34 ++++++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index c0f69dd7..620ef74d 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -615,10 +615,10 @@ 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. findAncestorNsForElement (above) already - // takes a known element instead, so reuse it here. - if (isDomNode.isElementNode(node)) { - ref.ancestorNamespaces = utils.findAncestorNsForElement(node); - } + // takes a known element instead, so reuse it here. All call sites already establish + // element-ness, so assert it rather than silently skipping. + isDomNode.assertIsElementNode(node); + ref.ancestorNamespaces = utils.findAncestorNsForElement(node); const c14nOptions = { ancestorNamespaces: ref.ancestorNamespaces, diff --git a/test/c14n-non-exclusive-unit-tests.spec.ts b/test/c14n-non-exclusive-unit-tests.spec.ts index 2d376327..0e5ba10b 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -183,7 +183,7 @@ describe("C14N non-exclusive canonicalization tests", function () { const xml = "text"; const doc = new xmldom.DOMParser().parseFromString(xml); - expect(() => utils.findAncestorNs(doc, subsetXpath)).to.throw( + expect(() => findAncestorNs(doc, subsetXpath)).to.throw( "Document subset must be list of elements", ); }); diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index e0822894..1cc207f1 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1332,6 +1332,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 98bca3c40fbf00a90afaee2be7e07a1a766eaf73 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:19:01 -0500 Subject: [PATCH 12/18] 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 --- test/signature-unit-tests.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 1cc207f1..7b9ee0da 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1361,9 +1361,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 b1fbcd4d8e575ecb7d032790250b8e22368d1e59 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:31:05 -0500 Subject: [PATCH 13/18] 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 | 36 +++++++++++++++++++++--- test/signature-integration-tests.spec.ts | 6 +++- test/signature-object-tests.spec.ts | 15 ++++++++-- test/signature-unit-tests.spec.ts | 29 ++++++++++++++++++- test/types-tests.spec.ts | 5 +++- 6 files changed, 82 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 2c4aec25..e35774af 100644 --- a/README.md +++ b/README.md @@ -419,7 +419,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 620ef74d..4761636e 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -133,6 +133,16 @@ const warnEnvelopedSignatureAfterCanonicalization = deprecate( "XML_CRYPTO_ENVELOPED_SIGNATURE_AFTER_CANONICALIZATION", ); +// 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[]; @@ -615,10 +625,14 @@ 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. findAncestorNsForElement (above) already - // takes a known element instead, so reuse it here. All call sites already establish - // element-ness, so assert it rather than silently skipping. - isDomNode.assertIsElementNode(node); - ref.ancestorNamespaces = utils.findAncestorNsForElement(node); + // takes a known element instead, so reuse it here. Every call site establishes + // element-ness except validateReference's document-level-processing-instruction case, + // which passes the whole Document — one with no ancestor namespace scope of its own. + if (isDomNode.isElementNode(node)) { + ref.ancestorNamespaces = utils.findAncestorNsForElement(node); + } else { + isDomNode.assertIsDocumentNode(node); + } const c14nOptions = { ancestorNamespaces: ref.ancestorNamespaces, @@ -1469,6 +1483,20 @@ 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-integration-tests.spec.ts b/test/signature-integration-tests.spec.ts index 6b1fddaf..1153781c 100644 --- a/test/signature-integration-tests.spec.ts +++ b/test/signature-integration-tests.spec.ts @@ -997,7 +997,11 @@ describe("Signature integration tests", function () { }); signer.addReference({ xpath: reference, - transforms: detached ? [canonicalization] : [enveloped, canonicalization], + // A detached reference's digest is still computed before the signature is + // inserted, and the verified document never has one embedded either, so this + // transform is a no-op either way — included so a whole-document reference + // never has to special-case whether its signature ends up detached. + transforms: [enveloped, canonicalization], digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", }); diff --git a/test/signature-object-tests.spec.ts b/test/signature-object-tests.spec.ts index 23101dca..0cb9ad8f 100644 --- a/test/signature-object-tests.spec.ts +++ b/test/signature-object-tests.spec.ts @@ -132,7 +132,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 @@ -159,7 +162,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); @@ -182,7 +188,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); diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 7b9ee0da..2b657dbd 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -216,7 +216,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"; @@ -1362,11 +1365,35 @@ describe("Signature unit tests", function () { const textNode = xpath.select1("//*[local-name(.)='x']/text()", doc); expect( + /* eslint-disable-next-line deprecation/deprecation */ () => verifySig.validateElementAgainstReferences(textNode as unknown as Element, doc), "a non-element must never be reported as covered by a validated reference", ).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(); diff --git a/test/types-tests.spec.ts b/test/types-tests.spec.ts index 15a2d4e6..22133e3f 100644 --- a/test/types-tests.spec.ts +++ b/test/types-tests.spec.ts @@ -11,7 +11,10 @@ describe("Callback invocation", 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"; From 37d1120d51ffe51b90830c01b6261651e82dcc45 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:39:20 -0500 Subject: [PATCH 14/18] 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 | 19 ++++++++----------- src/types.ts | 12 ++++++++++++ 5 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/c14n-canonicalization.ts b/src/c14n-canonicalization.ts index abbe283f..3761986f 100644 --- a/src/c14n-canonicalization.ts +++ b/src/c14n-canonicalization.ts @@ -12,6 +12,8 @@ type C14nOptionsWithXmlAttributes = CanonicalizationOrTransformationAlgorithmPro }; 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 58ac6544..03cbf6fd 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 251bff97..8eec487c 100644 --- a/src/exclusive-canonicalization.ts +++ b/src/exclusive-canonicalization.ts @@ -21,6 +21,8 @@ export function warnUnlessExclusiveC14nNamespace(inclusiveNamespaces: Element): } 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 4761636e..bf6be1df 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -133,16 +133,6 @@ const warnEnvelopedSignatureAfterCanonicalization = deprecate( "XML_CRYPTO_ENVELOPED_SIGNATURE_AFTER_CANONICALIZATION", ); -// 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[]; @@ -704,6 +694,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]; @@ -1488,7 +1485,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 b31b119f..6b43e624 100644 --- a/src/types.ts +++ b/src/types.ts @@ -188,6 +188,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 49a1cdfd26da98fa0f4218481598026efe301445 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:47:30 -0500 Subject: [PATCH 15/18] 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/canonicalization-unit-tests.spec.ts | 3 +++ test/signature-integration-tests.spec.ts | 11 +++++++++++ test/signature-unit-tests.spec.ts | 8 ++++++++ 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index e35774af..804d9cb3 100644 --- a/README.md +++ b/README.md @@ -518,6 +518,9 @@ Custom transformation algorithm. ```javascript class MyTransformation { + // Whether this drops nodes from the node-set, rather than only re-serializing it. + removesNodes = false; + // Given a node (from the xmldom module), return its canonical representation as a string. process(node) { // You should apply your transformation before returning. @@ -530,10 +533,14 @@ class 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 class MyCanonicalization { + removesNodes = false; + // Given a node (from the xmldom module), return its canonical representation as a string. process(node) { // You should apply your canonicalization before returning. diff --git a/src/signed-xml.ts b/src/signed-xml.ts index bf6be1df..66342173 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -694,8 +694,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 6b43e624..6558d202 100644 --- a/src/types.ts +++ b/src/types.ts @@ -191,15 +191,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/canonicalization-unit-tests.spec.ts b/test/canonicalization-unit-tests.spec.ts index 81535257..5e810584 100644 --- a/test/canonicalization-unit-tests.spec.ts +++ b/test/canonicalization-unit-tests.spec.ts @@ -505,6 +505,9 @@ describe("Canonicalization unit tests", function () { const sig = new SignedXml(); sig.CanonicalizationAlgorithms["urn:test:malformed"] = class { + // Returns a fixed string rather than the node-set it was given. + removesNodes = true; + process() { return ""; } diff --git a/test/signature-integration-tests.spec.ts b/test/signature-integration-tests.spec.ts index 1153781c..09864453 100644 --- a/test/signature-integration-tests.spec.ts +++ b/test/signature-integration-tests.spec.ts @@ -324,6 +324,8 @@ describe("Signature integration tests", function () { describe("transforms that follow a canonicalization", function () { class DropNotes { + removesNodes = true; + process(node: Node) { const notes = xpath.select(".//note", node); isDomNode.assertIsArrayOfNodes(notes); @@ -337,6 +339,10 @@ describe("Signature integration tests", function () { } class ReverseSignaturesToString { + // Reorders existing signatures without dropping any node; enveloped-signature, + // applied after this in these tests, is what actually removes one. + removesNodes = false; + process(node: Node) { const signatures = xpath.select("./*[local-name(.)='Signature']", node); isDomNode.assertIsArrayOfNodes(signatures); @@ -510,6 +516,8 @@ describe("Signature integration tests", function () { // again, and the last one's PrefixList keeps it: // https://www.w3.org/TR/xmldsig-core1/#sec-Transforms class DeclareB { + removesNodes = false; + process(node: Node) { (node as Element).setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns:b", "urn:b"); return node; @@ -920,6 +928,9 @@ describe("Signature integration tests", function () { it("should remove comments before the first transform", function () { const transformInputs: string[] = []; class RecordInput { + // Returns the node-set it was given, unchanged. + removesNodes = false; + process(node: Node) { transformInputs.push(node.toString()); return node; diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 2b657dbd..c574edf4 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -484,6 +484,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/>"; }; @@ -495,6 +497,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/>"; }; @@ -652,6 +656,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/>"; }; @@ -663,6 +669,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 2ed6506cbe6b56a991524032bf292783c16a5972 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 19:59:39 -0500 Subject: [PATCH 16/18] 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 | 46 ++++++++++++++++++++++++++++++- 7 files changed, 68 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 804d9cb3..7259d5e1 100644 --- a/README.md +++ b/README.md @@ -533,7 +533,7 @@ class 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 3761986f..7671c8f4 100644 --- a/src/c14n-canonicalization.ts +++ b/src/c14n-canonicalization.ts @@ -12,7 +12,7 @@ type C14nOptionsWithXmlAttributes = CanonicalizationOrTransformationAlgorithmPro }; 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 03cbf6fd..c96bd759 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 8eec487c..c8ef3418 100644 --- a/src/exclusive-canonicalization.ts +++ b/src/exclusive-canonicalization.ts @@ -21,7 +21,7 @@ export function warnUnlessExclusiveC14nNamespace(inclusiveNamespaces: Element): } 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 66342173..88986c49 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -694,12 +694,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 6558d202..3d563e8e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -199,8 +199,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 c574edf4..48ce90e6 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1,6 +1,12 @@ import * as xpath from "xpath"; import * as xmldom from "@xmldom/xmldom"; -import { SignedXml, createOptionalCallbackFunction, pemCertificates, toPem } from "../src/index"; +import { + SignedXml, + createOptionalCallbackFunction, + pemCertificates, + toPem, + type CanonicalizationOrTransformationAlgorithm, +} from "../src/index"; import * as fs from "fs"; import * as crypto from "crypto"; import { expect } from "chai"; @@ -1402,6 +1408,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 fcf0f34568e21533b3de2c0a5a45cd86302f092a Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 21:43:37 -0500 Subject: [PATCH 17/18] 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 88986c49..40b5c75d 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -694,30 +694,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; } } @@ -1497,7 +1488,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 48ce90e6..87d325f1 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1446,6 +1446,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 0642e9a7d10cdb8830af93f3023a45e43be1399d Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Wed, 9 Sep 2026 06:22:18 -0500 Subject: [PATCH 18/18] 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 7259d5e1..285106b2 100644 --- a/README.md +++ b/README.md @@ -533,7 +533,9 @@ class 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 3d563e8e..c65a5b2d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -194,10 +194,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 87d325f1..c8567aa2 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1476,6 +1476,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();