From 7e30184f4c915f0fda6ecb9d3c51509ef7b635b4 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 11 Aug 2026 11:59:12 -0700 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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();