From d37e6bf49566cdb1d6fe1535d48a59641b129d83 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Tue, 11 Aug 2026 11:26:45 -0700 Subject: [PATCH 1/5] fix: collect all subset namespace prefixes when filtering ancestor namespaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findNSPrefix returned only the first xmlns:* attribute on a subset element, so findAncestorNs filtered only that one prefix when deciding which ancestor namespace declarations to hoist. When a subset element in the default namespace also declared a prefixed namespace (e.g. ), findNSPrefix returned "enc" and left the inherited default namespace in the ancestor list. The C14N serializer then rendered the default namespace twice — once from the element itself and once from the hoisted ancestor entry — producing a digest that no other implementation would ever match. Replace findNSPrefix with findSubsetNSPrefixes, which collects every xmlns:* attribute on the subset element into a Set and always includes the element's own namespace prefix (empty string for the default namespace). findAncestorNs now uses Set.has() to filter, so all already-declared prefixes are suppressed regardless of how many xmlns:* attributes appear on the subset. The change is backward-compatible: it filters more ancestor entries than before, so no previously-hoisted namespace starts being suppressed. The concrete trigger is SMPTE ST 430-3 ETMs, where the XML signature covers as a subset reference. Fixes #538 --- src/utils.ts | 32 +++++++++++++---- test/c14n-non-exclusive-unit-tests.spec.ts | 40 ++++++++++++++++++++++ 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 466b252e..6872a3eb 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -221,15 +221,35 @@ function collectAncestorNamespaces( return collectAncestorNamespaces(parent, nsArray); } -function findNSPrefix(subset) { +/** + * Collect all namespace prefixes declared directly on a subset element. + * + * This includes every `xmlns:*` attribute on the element as well as the + * element's own namespace prefix (or `""` for the default namespace). The + * result is used by {@link findAncestorNs} to decide which ancestor namespace + * declarations are already in scope and therefore must not be hoisted onto the + * subset root during non-exclusive C14N. + * + * The previous single-return implementation (`findNSPrefix`) stopped at the + * *first* `xmlns:*` attribute, so elements that declare more than one + * namespace (e.g. `` in the default namespace) caused the + * inherited default namespace to be hoisted even though the C14N serializer + * already renders it, producing a duplicate `xmlns="…"` declaration. + */ +function findSubsetNSPrefixes(subset: Element): Set { + const prefixes = new Set(); const subsetAttributes = subset.attributes; for (let k = 0; k < subsetAttributes.length; k++) { const nodeName = subsetAttributes[k].nodeName; - if (nodeName.search(/^xmlns:?/) !== -1) { - return nodeName.replace(/^xmlns:?/, ""); + if (/^xmlns:?/.test(nodeName)) { + prefixes.add(nodeName.replace(/^xmlns:?/, "")); } } - return subset.prefix || ""; + // Always include the element's own prefix (empty string for the default + // namespace) so that the C14N serializer's own rendering of that namespace + // is not duplicated by hoisting. + prefixes.add(subset.prefix || ""); + return prefixes; } function isElementSubset(docSubset: Node[]): docSubset is Element[] { @@ -283,9 +303,9 @@ export function findAncestorNs( // Remove namespaces which are already declared in the subset with the same prefix const returningNs: NamespacePrefix[] = []; - const subsetNsPrefix = findNSPrefix(docSubset[0]); + const subsetNsPrefixes = findSubsetNSPrefixes(docSubset[0]); for (const ancestorNs of ancestorNsWithoutDuplicate) { - if (ancestorNs.prefix !== subsetNsPrefix) { + if (!subsetNsPrefixes.has(ancestorNs.prefix)) { returningNs.push(ancestorNs); } } diff --git a/test/c14n-non-exclusive-unit-tests.spec.ts b/test/c14n-non-exclusive-unit-tests.spec.ts index ee7f2ba4..4955d326 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -117,6 +117,31 @@ describe("C14N non-exclusive canonicalization tests", function () { test_findAncestorNs(xml, xpath, expected); }); + it("findAncestorNs: Should not hoist default namespace when subset also declares a prefixed namespace", function () { + // child2 is in the default namespace (inherited from root) and also + // declares xmlns:enc. The default namespace must not be hoisted because + // the C14N serializer already renders it; previously findNSPrefix stopped + // at the first xmlns:* attribute ("enc") and missed the inherited "". + const xml = + ""; + const xpath = "//*[local-name()='child2']"; + const expected = []; + + test_findAncestorNs(xml, xpath, expected); + }); + + it("findAncestorNs: Should hoist non-default ancestor namespaces when subset is in default namespace", function () { + // child2 is in the default namespace and declares xmlns:enc, but its + // ancestor also declares xmlns:aaa which child2 does not redeclare. + // xmlns:aaa must still be hoisted; only the default namespace is suppressed. + const xml = + ""; + const xpath = "//*[local-name()='child2']"; + const expected = [{ prefix: "aaa", namespaceURI: "zzz" }]; + + test_findAncestorNs(xml, xpath, expected); + }); + // Tests for c14nCanonicalization it("C14n: Correctly picks up root ancestor namespace", function () { const xml = ""; @@ -210,4 +235,19 @@ describe("C14N non-exclusive canonicalization tests", function () { test_C14nCanonicalization(xml, xpath, expected); }); + + it("C14n: Should not produce duplicate default namespace when subset declares a prefixed namespace", function () { + // child2 is in the default namespace and also declares xmlns:enc. + // The C14N output must contain exactly one xmlns="bbb" declaration. + // Previously findNSPrefix returned "enc" (the first xmlns:* attribute), + // leaving the default namespace in the ancestor list; the C14N serializer + // then rendered it twice — once from the element itself and once from the + // hoisted ancestor entry. + const xml = + ""; + const xpath = "//*[local-name()='child2']"; + const expected = ''; + + test_C14nCanonicalization(xml, xpath, expected); + }); }); From 4d04fd6fbc4424ef07741f730f33fd0d8dedd111 Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Wed, 12 Aug 2026 16:45:39 -0700 Subject: [PATCH 2/5] fix: use exact xmlns attribute match instead of regex in findSubsetNSPrefixes The /^xmlns:?/ regex also matched ordinary attributes whose names start with "xmlns" but have no colon (e.g. xmlnsfoo). Such attributes are not namespace declarations, but the old code added "foo" to the suppression set, causing findAncestorNs to incorrectly drop an inherited xmlns:foo declaration that must be hoisted to the subset root. Replace the regex test with an exact equality check: nodeName === "xmlns" || nodeName.startsWith("xmlns:") Adds a regression test to cover the xmlnsfoo case. --- src/utils.ts | 2 +- test/c14n-non-exclusive-unit-tests.spec.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/utils.ts b/src/utils.ts index 6872a3eb..b85bd805 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -241,7 +241,7 @@ function findSubsetNSPrefixes(subset: Element): Set { const subsetAttributes = subset.attributes; for (let k = 0; k < subsetAttributes.length; k++) { const nodeName = subsetAttributes[k].nodeName; - if (/^xmlns:?/.test(nodeName)) { + if (nodeName === "xmlns" || nodeName.startsWith("xmlns:")) { prefixes.add(nodeName.replace(/^xmlns:?/, "")); } } diff --git a/test/c14n-non-exclusive-unit-tests.spec.ts b/test/c14n-non-exclusive-unit-tests.spec.ts index 4955d326..c2843c40 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -142,6 +142,18 @@ describe("C14N non-exclusive canonicalization tests", function () { test_findAncestorNs(xml, xpath, expected); }); + it("findAncestorNs: Should not suppress ancestor namespace for non-namespace attribute starting with 'xmlns'", function () { + // xmlnsfoo is an ordinary attribute, not a namespace declaration. + // findSubsetNSPrefixes must not add "foo" to the suppression set, so + // an inherited xmlns:foo declaration on an ancestor is still hoisted. + const xml = + ""; + const xpath = "//*[local-name()='child2']"; + const expected = [{ prefix: "foo", namespaceURI: "zzz" }]; + + test_findAncestorNs(xml, xpath, expected); + }); + // Tests for c14nCanonicalization it("C14n: Correctly picks up root ancestor namespace", function () { const xml = ""; From fce45a23d318dde7aeee54ae5bcc53d768a8282f Mon Sep 17 00:00:00 2001 From: Matthew Sheby Date: Thu, 13 Aug 2026 11:59:58 -0700 Subject: [PATCH 3/5] refactor: replace for-in over array with for-of and Array#some in findAncestorNs --- src/utils.ts | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index b85bd805..3d019040 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -287,17 +287,10 @@ export function findAncestorNs( // 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]); + for (const ns of ancestorNs) { + const isDuplicate = ancestorNsWithoutDuplicate.some((seen) => seen.prefix === ns.prefix); + if (!isDuplicate) { + ancestorNsWithoutDuplicate.push(ns); } } From c27a5d229fa262d95a92e93053c9ca1bf021567c Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 4 Sep 2026 18:55:19 -0500 Subject: [PATCH 4/5] Lint --- test/c14n-non-exclusive-unit-tests.spec.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/c14n-non-exclusive-unit-tests.spec.ts b/test/c14n-non-exclusive-unit-tests.spec.ts index c2843c40..2a2866cf 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -122,8 +122,7 @@ describe("C14N non-exclusive canonicalization tests", function () { // declares xmlns:enc. The default namespace must not be hoisted because // the C14N serializer already renders it; previously findNSPrefix stopped // at the first xmlns:* attribute ("enc") and missed the inherited "". - const xml = - ""; + const xml = ""; const xpath = "//*[local-name()='child2']"; const expected = []; @@ -146,8 +145,7 @@ describe("C14N non-exclusive canonicalization tests", function () { // xmlnsfoo is an ordinary attribute, not a namespace declaration. // findSubsetNSPrefixes must not add "foo" to the suppression set, so // an inherited xmlns:foo declaration on an ancestor is still hoisted. - const xml = - ""; + const xml = ""; const xpath = "//*[local-name()='child2']"; const expected = [{ prefix: "foo", namespaceURI: "zzz" }]; @@ -255,8 +253,7 @@ describe("C14N non-exclusive canonicalization tests", function () { // leaving the default namespace in the ancestor list; the C14N serializer // then rendered it twice — once from the element itself and once from the // hoisted ancestor entry. - const xml = - ""; + const xml = ""; const xpath = "//*[local-name()='child2']"; const expected = ''; From 05aa0b38351b64788c96d9e1e345a60f2f98d014 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 4 Sep 2026 22:50:35 -0500 Subject: [PATCH 5/5] ``` test: add comprehensive namespace canonicalization tests ``` Expands unit test coverage for namespace handling in both C14n (inclusive) and Exclusive C14n algorithms. These new tests address various edge cases and complex interactions to ensure correct namespace propagation, suppression, and declaration rendering according to W3C specifications. Specific scenarios covered include: * Preventing duplicate default namespace declarations for subset roots (regression for #538). * Correctly handling inherited, locally declared, and cleared default namespaces. * Validating inclusive C14N's behavior with unused ancestor prefixes. * Verifying Exclusive C14N's `PrefixList` logic for retaining specified inclusive namespaces. Existing namespace-related tests are also refactored to run against `WithComments` variants, and their descriptions are enhanced with W3C specification references. ``` --- test/c14n-non-exclusive-unit-tests.spec.ts | 133 ++++++++++++++++----- test/canonicalization-unit-tests.spec.ts | 51 +++++++- 2 files changed, 151 insertions(+), 33 deletions(-) diff --git a/test/c14n-non-exclusive-unit-tests.spec.ts b/test/c14n-non-exclusive-unit-tests.spec.ts index 2a2866cf..bcdbe069 100644 --- a/test/c14n-non-exclusive-unit-tests.spec.ts +++ b/test/c14n-non-exclusive-unit-tests.spec.ts @@ -1,15 +1,22 @@ import { expect } from "chai"; -import { C14nCanonicalization } from "../src/c14n-canonicalization"; +import { + C14nCanonicalization, + C14nCanonicalizationWithComments, +} from "../src/c14n-canonicalization"; import * as xmldom from "@xmldom/xmldom"; import * as xpath from "xpath"; import * as utils from "../src/utils"; import * as isDomNode from "@xmldom/is-dom-node"; -const test_C14nCanonicalization = function (xml, xpathArg, expected) { +const test_C14nCanonicalization = function ( + xml: string, + xpathArg: string, + expected: string, + can = new C14nCanonicalization(), +) { const doc = new xmldom.DOMParser().parseFromString(xml); const node = xpath.select1(xpathArg, doc); - const can = new C14nCanonicalization(); isDomNode.assertIsNodeLike(node); const result = can @@ -118,11 +125,9 @@ describe("C14N non-exclusive canonicalization tests", function () { }); it("findAncestorNs: Should not hoist default namespace when subset also declares a prefixed namespace", function () { - // child2 is in the default namespace (inherited from root) and also - // declares xmlns:enc. The default namespace must not be hoisted because - // the C14N serializer already renders it; previously findNSPrefix stopped - // at the first xmlns:* attribute ("enc") and missed the inherited "". - const xml = ""; + // The element's own default namespace must be rendered only once. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#ProcessingModel + const xml = ''; const xpath = "//*[local-name()='child2']"; const expected = []; @@ -130,24 +135,22 @@ describe("C14N non-exclusive canonicalization tests", function () { }); it("findAncestorNs: Should hoist non-default ancestor namespaces when subset is in default namespace", function () { - // child2 is in the default namespace and declares xmlns:enc, but its - // ancestor also declares xmlns:aaa which child2 does not redeclare. - // xmlns:aaa must still be hoisted; only the default namespace is suppressed. + // Inclusive C14N retains ancestor bindings even when the subset does not visibly use them. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#DataModel const xml = - ""; + ''; const xpath = "//*[local-name()='child2']"; - const expected = [{ prefix: "aaa", namespaceURI: "zzz" }]; + const expected = [{ prefix: "aaa", namespaceURI: "urn:aaa" }]; test_findAncestorNs(xml, xpath, expected); }); it("findAncestorNs: Should not suppress ancestor namespace for non-namespace attribute starting with 'xmlns'", function () { - // xmlnsfoo is an ordinary attribute, not a namespace declaration. - // findSubsetNSPrefixes must not add "foo" to the suppression set, so - // an inherited xmlns:foo declaration on an ancestor is still hoisted. - const xml = ""; + // Only xmlns and xmlns:* declare namespaces; xmlnsfoo must not hide an inherited binding. + // https://www.w3.org/TR/REC-xml-names/#ns-decl + const xml = ''; const xpath = "//*[local-name()='child2']"; - const expected = [{ prefix: "foo", namespaceURI: "zzz" }]; + const expected = [{ prefix: "foo", namespaceURI: "urn:foo" }]; test_findAncestorNs(xml, xpath, expected); }); @@ -246,17 +249,85 @@ describe("C14N non-exclusive canonicalization tests", function () { test_C14nCanonicalization(xml, xpath, expected); }); - it("C14n: Should not produce duplicate default namespace when subset declares a prefixed namespace", function () { - // child2 is in the default namespace and also declares xmlns:enc. - // The C14N output must contain exactly one xmlns="bbb" declaration. - // Previously findNSPrefix returned "enc" (the first xmlns:* attribute), - // leaving the default namespace in the ancestor list; the C14N serializer - // then rendered it twice — once from the element itself and once from the - // hoisted ancestor entry. - const xml = ""; - const xpath = "//*[local-name()='child2']"; - const expected = ''; - - test_C14nCanonicalization(xml, xpath, expected); - }); + for (const Canonicalization of [C14nCanonicalization, C14nCanonicalizationWithComments]) { + describe(`${Canonicalization.name}: subset namespace declarations`, function () { + it("does not duplicate the default namespace when the subset declares a prefixed namespace", function () { + // Render the inherited default namespace exactly once on the subset root. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#ProcessingModel + test_C14nCanonicalization( + '', + "//*[local-name()='child2']", + '', + new Canonicalization(), + ); + }); + + it("does not duplicate the default namespace for the reported #538 document", function () { + // Literal reproduction from https://github.com/node-saml/xml-crypto/issues/538: + // a subset root inheriting a default namespace while declaring a prefixed one, + // with a prefixed child that must resolve against the local declaration. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#ProcessingModel + test_C14nCanonicalization( + '' + + '' + + "x", + "//*[local-name()='Body']", + '' + + "x", + new Canonicalization(), + ); + }); + + it("retains unused ancestor prefixes alongside the default and local namespaces", function () { + // Inclusive C14N preserves every in-scope namespace, including unused ancestor bindings. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#DataModel + test_C14nCanonicalization( + '', + "//*[local-name()='child2']", + '', + new Canonicalization(), + ); + }); + + for (const declarations of [ + 'xmlns:a="urn:local-a" xmlns:b="urn:local-b"', + 'xmlns:b="urn:local-b" xmlns:a="urn:local-a"', + ]) { + it(`uses both local prefix bindings with ${declarations}`, function () { + // Local declarations override ancestor bindings regardless of declaration order. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#DataModel + test_C14nCanonicalization( + '' + + ``, + "//*[local-name()='child2']", + '', + new Canonicalization(), + ); + }); + } + + it("renders the default namespace on a prefixed apex that redeclares it", function () { + // Every namespace in scope at the apex is rendered there, including the default one. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#ProcessingModel + test_C14nCanonicalization( + '', + "//*[local-name()='child2']", + '', + new Canonicalization(), + ); + }); + + it("does not restore a default namespace explicitly cleared on a prefixed root", function () { + // An empty default declaration removes the inherited binding; no reset is needed at the apex. + // https://www.w3.org/TR/REC-xml-names/#defaulting + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#ProcessingModel + test_C14nCanonicalization( + '', + "//*[local-name()='child2']", + '', + new Canonicalization(), + ); + }); + }); + } }); diff --git a/test/canonicalization-unit-tests.spec.ts b/test/canonicalization-unit-tests.spec.ts index 7a39f168..4d37d3bc 100644 --- a/test/canonicalization-unit-tests.spec.ts +++ b/test/canonicalization-unit-tests.spec.ts @@ -1,9 +1,12 @@ import { expect } from "chai"; -import { ExclusiveCanonicalization } from "../src/exclusive-canonicalization"; +import { + ExclusiveCanonicalization, + ExclusiveCanonicalizationWithComments, +} from "../src/exclusive-canonicalization"; import * as xmldom from "@xmldom/xmldom"; import * as xpath from "xpath"; -import { SignedXml } from "../src/index"; +import { findAncestorNs, SignedXml } from "../src/index"; import * as isDomNode from "@xmldom/is-dom-node"; const compare = function ( @@ -28,6 +31,50 @@ const compare = function ( }; describe("Canonicalization unit tests", function () { + for (const Canonicalization of [ + ExclusiveCanonicalization, + ExclusiveCanonicalizationWithComments, + ]) { + describe(`${Canonicalization.name}: configured inclusive namespaces`, function () { + const xml = + '' + + ''; + const selector = "//*[local-name()='target']"; + + it("retains local and inherited bindings requested by the caller", function () { + // PrefixList includes QName-value bindings without replacing local declarations with ancestors. + // https://www.w3.org/TR/xml-exc-c14n/#sec-Specification + const doc = new xmldom.DOMParser().parseFromString(xml); + const node = xpath.select1(selector, doc); + isDomNode.assertIsElementNode(node); + + const result = new Canonicalization().process(node, { + ancestorNamespaces: findAncestorNs(doc, selector), + inclusiveNamespacesPrefixList: ["b", "c"], + }); + + expect(result).to.equal( + '', + ); + }); + + it("omits non-visible bindings when the caller supplies an empty PrefixList", function () { + // Prefixes used only in attribute values are not visibly utilized in exclusive C14N. + // https://www.w3.org/TR/xml-exc-c14n/#def-visibly-utilizes + const doc = new xmldom.DOMParser().parseFromString(xml); + const node = xpath.select1(selector, doc); + isDomNode.assertIsElementNode(node); + + const result = new Canonicalization().process(node, { + ancestorNamespaces: findAncestorNs(doc, selector), + inclusiveNamespacesPrefixList: [], + }); + + expect(result).to.equal(''); + }); + }); + } + it("Exclusive canonicalization works on xml with no namespaces", function () { compare("123", "//*", "123"); });