Skip to content

bump xmldom to 0.9 #501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
elbuo8 wants to merge 2 commits into from
Closed

bump xmldom to 0.9 #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
034bf63
bump xmldom to 0.9
elbuo8 Apr 22, 2025
0b70372
lint
elbuo8 Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
},
"dependencies": {
"@xmldom/is-dom-node": "^1.0.1",
"@xmldom/xmldom": "^0.8.10",
"@xmldom/xmldom": "^0.9.8",
"xpath": "^0.0.33"
},
"devDependencies": {
Expand Down
74 changes: 49 additions & 25 deletions src/signed-xml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,12 @@ export class SignedXml {

this.signedXml = xml;

const doc = new xmldom.DOMParser().parseFromString(xml);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");

// Reset the references as only references from our re-parsed signedInfo node can be trusted
this.references = [];

const unverifiedSignedInfoCanon = this.getCanonSignedInfoXml(doc);
const unverifiedSignedInfoCanon = this.getCanonSignedInfoXml(doc as unknown as Document);
if (!unverifiedSignedInfoCanon) {
if (callback) {
callback(new Error("Canonical signed info cannot be empty"), false);
Expand All @@ -295,7 +295,7 @@ export class SignedXml {
throw new Error("Could not parse unverifiedSignedInfoCanon into a document");
}

const references = utils.findChildren(unverifiedSignedInfoDoc, "Reference");
const references = utils.findChildren(unverifiedSignedInfoDoc as unknown as Node, "Reference");
if (!utils.isArrayHasLength(references)) {
if (callback) {
callback(new Error("could not find any Reference elements"), false);
Expand All @@ -313,7 +313,9 @@ export class SignedXml {
}

/* eslint-disable-next-line deprecation/deprecation */
if (!this.getReferences().every((ref) => this.validateReference(ref, doc))) {
if (
!this.getReferences().every((ref) => this.validateReference(ref, doc as unknown as Document))
) {
/* Trustworthiness can only be determined if SignedInfo's (which holds References' DigestValue(s)
which were validated at this stage) signature is valid. Execution does not proceed to validate
signature phase thus each References' DigestValue must be considered to be untrusted (attacker
Expand Down Expand Up @@ -406,7 +408,10 @@ export class SignedXml {
/**
* Search for ancestor namespaces before canonicalization.
*/
const ancestorNamespaces = utils.findAncestorNs(doc, "//*[local-name()='SignedInfo']");
const ancestorNamespaces = utils.findAncestorNs(
doc as unknown as Document,
"//*[local-name()='SignedInfo']",
);

const c14nOptions = {
ancestorNamespaces: ancestorNamespaces,
Expand Down Expand Up @@ -596,7 +601,10 @@ export class SignedXml {
*/
loadSignature(signatureNode: Node | string): void {
if (typeof signatureNode === "string") {
this.signatureNode = signatureNode = new xmldom.DOMParser().parseFromString(signatureNode);
this.signatureNode = signatureNode = new xmldom.DOMParser().parseFromString(
signatureNode,
"text/xml",
) as unknown as Node;
} else {
this.signatureNode = signatureNode;
}
Expand All @@ -605,7 +613,7 @@ export class SignedXml {

const node = xpath.select1(
".//*[local-name(.)='CanonicalizationMethod']/@Algorithm",
signatureNode,
signatureNode as unknown as Node,
);
if (!isDomNode.isNodeLike(node)) {
throw new Error("could not find CanonicalizationMethod/@Algorithm element");
Expand All @@ -617,14 +625,14 @@ export class SignedXml {

const signatureAlgorithm = xpath.select1(
".//*[local-name(.)='SignatureMethod']/@Algorithm",
signatureNode,
signatureNode as unknown as Node,
);

if (isDomNode.isAttributeNode(signatureAlgorithm)) {
this.signatureAlgorithm = signatureAlgorithm.value as SignatureAlgorithmType;
}

const signedInfoNodes = utils.findChildren(this.signatureNode, "SignedInfo");
const signedInfoNodes = utils.findChildren(this.signatureNode as unknown as Node, "SignedInfo");
if (!utils.isArrayHasLength(signedInfoNodes)) {
throw new Error("no signed info node found");
}
Expand Down Expand Up @@ -659,7 +667,7 @@ export class SignedXml {
const signedInfoDoc = temporaryCanonSignedInfoXml.documentElement;

this.references = [];
const references = utils.findChildren(signedInfoDoc, "Reference");
const references = utils.findChildren(signedInfoDoc as unknown as Node, "Reference");

if (!utils.isArrayHasLength(references)) {
throw new Error("could not find any Reference elements");
Expand All @@ -671,14 +679,17 @@ export class SignedXml {

const signatureValue = xpath.select1(
".//*[local-name(.)='SignatureValue']/text()",
signatureNode,
signatureNode as unknown as Node,
);

if (isDomNode.isTextNode(signatureValue)) {
this.signatureValue = signatureValue.data.replace(/\r?\n/g, "");
}

const keyInfo = xpath.select1(".//*[local-name(.)='KeyInfo']", signatureNode);
const keyInfo = xpath.select1(
".//*[local-name(.)='KeyInfo']",
signatureNode as unknown as Node,
);

if (isDomNode.isNodeLike(keyInfo)) {
this.keyInfo = keyInfo;
Expand Down Expand Up @@ -900,7 +911,7 @@ export class SignedXml {
options = (options ?? {}) as ComputeSignatureOptions;
}

const doc = new xmldom.DOMParser().parseFromString(xml);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
let xmlNsAttr = "xmlns";
const signatureAttrs: string[] = [];
let currentPrefix: string;
Expand Down Expand Up @@ -970,13 +981,15 @@ export class SignedXml {
// A trick to remove the namespaces that already exist in the xml
// This only works if the prefix and namespace match with those in the xml
const dummySignatureWrapper = `<Dummy ${existingPrefixesString}>${signatureXml}</Dummy>`;
const nodeXml = new xmldom.DOMParser().parseFromString(dummySignatureWrapper);
const nodeXml = new xmldom.DOMParser().parseFromString(dummySignatureWrapper, "text/xml");

// Because we are using a dummy wrapper hack described above, we know there will be a `firstChild`
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const signatureDoc = nodeXml.documentElement.firstChild!;

const referenceNode = xpath.select1(location.reference, doc);
const referenceNode = xpath.select1(location.reference, doc as unknown as Document);

if (!isDomNode.isNodeLike(referenceNode)) {
const err2 = new Error(
Expand All @@ -991,26 +1004,29 @@ export class SignedXml {
}

if (location.action === "append") {
referenceNode.appendChild(signatureDoc);
referenceNode.appendChild(signatureDoc as unknown as Node);
} else if (location.action === "prepend") {
referenceNode.insertBefore(signatureDoc, referenceNode.firstChild);
referenceNode.insertBefore(signatureDoc as unknown as Node, referenceNode.firstChild);
} else if (location.action === "before") {
if (referenceNode.parentNode == null) {
throw new Error(
"`location.reference` refers to the root node (by default), so we can't insert `before`",
);
}
referenceNode.parentNode.insertBefore(signatureDoc, referenceNode);
referenceNode.parentNode.insertBefore(signatureDoc as unknown as Node, referenceNode);
} else if (location.action === "after") {
if (referenceNode.parentNode == null) {
throw new Error(
"`location.reference` refers to the root node (by default), so we can't insert `after`",
);
}
referenceNode.parentNode.insertBefore(signatureDoc, referenceNode.nextSibling);
referenceNode.parentNode.insertBefore(
signatureDoc as unknown as Node,
referenceNode.nextSibling,
);
}

this.signatureNode = signatureDoc;
this.signatureNode = signatureDoc as unknown as Node;
const signedInfoNodes = utils.findChildren(this.signatureNode, "SignedInfo");
if (signedInfoNodes.length === 0) {
const err3 = new Error("could not find SignedInfo element in the message");
Expand All @@ -1025,21 +1041,27 @@ export class SignedXml {

if (typeof callback === "function") {
// Asynchronous flow
this.calculateSignatureValue(doc, (err, signature) => {
this.calculateSignatureValue(doc as unknown as Document, (err, signature) => {
if (err) {
callback(err);
} else {
this.signatureValue = signature || "";
signatureDoc.insertBefore(this.createSignature(prefix), signedInfoNode.nextSibling);
signatureDoc.insertBefore(
this.createSignature(prefix),
signedInfoNode.nextSibling as unknown as xmldom.Node,
);
this.signatureXml = signatureDoc.toString();
this.signedXml = doc.toString();
callback(null, this);
}
});
} else {
// Synchronous flow
this.calculateSignatureValue(doc);
signatureDoc.insertBefore(this.createSignature(prefix), signedInfoNode.nextSibling);
this.calculateSignatureValue(doc as unknown as Document);
signatureDoc.insertBefore(
this.createSignature(prefix),
signedInfoNode.nextSibling as unknown as xmldom.Node,
);
this.signatureXml = signatureDoc.toString();
this.signedXml = doc.toString();
}
Expand Down Expand Up @@ -1248,9 +1270,11 @@ export class SignedXml {
//we need to wrap the info in a dummy signature since it contains the default namespace.
const dummySignatureWrapper = `<${prefix}Signature ${xmlNsAttr}="http://www.w3.org/2000/09/xmldsig#">${signatureValueXml}</${prefix}Signature>`;

const doc = new xmldom.DOMParser().parseFromString(dummySignatureWrapper);
const doc = new xmldom.DOMParser().parseFromString(dummySignatureWrapper, "text/xml");

// Because we are using a dummy wrapper hack described above, we know there will be a `firstChild`
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return doc.documentElement.firstChild!;
}
Expand Down
10 changes: 5 additions & 5 deletions test/c14n-non-exclusive-unit-tests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@ import * as utils from "../src/utils";
import * as isDomNode from "@xmldom/is-dom-node";

const test_C14nCanonicalization = function (xml, xpathArg, expected) {
const doc = new xmldom.DOMParser().parseFromString(xml);
const node = xpath.select1(xpathArg, doc);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
const node = xpath.select1(xpathArg, doc as unknown as Node);
const can = new C14nCanonicalization();

isDomNode.assertIsNodeLike(node);
const result = can
.process(node, {
ancestorNamespaces: utils.findAncestorNs(doc, xpathArg),
ancestorNamespaces: utils.findAncestorNs(doc as unknown as Document, xpathArg),
})
.toString();

expect(result).to.equal(expected);
};

const test_findAncestorNs = function (xml, xpath, expected) {
const doc = new xmldom.DOMParser().parseFromString(xml);
const result = utils.findAncestorNs(doc, xpath);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
const result = utils.findAncestorNs(doc as unknown as Document, xpath);

expect(result).to.deep.equal(expected);
};
Expand Down
11 changes: 6 additions & 5 deletions test/c14nWithComments-unit-tests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import { SignedXml } from "../src/index";
import * as isDomNode from "@xmldom/is-dom-node";

const compare = function (xml, xpathArg, expected, inclusiveNamespacesPrefixList?: string[]) {
const doc = new xmldom.DOMParser().parseFromString(xml);
const elem = xpath.select1(xpathArg, doc);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
const elem = xpath.select1(xpathArg, doc as unknown as Node);
const can = new c14nWithComments();
isDomNode.assertIsElementNode(elem);
const result = can.process(elem, { inclusiveNamespacesPrefixList }).toString();
Expand Down Expand Up @@ -350,8 +350,9 @@ describe("Exclusive canonicalization with comments", function () {
it("Multiple Canonicalization with namespace definition outside of signed element", function () {
const doc = new xmldom.DOMParser().parseFromString(
'<x xmlns:p="myns"><p:y><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"></ds:Signature></p:y></x>',
"text/xml",
);
const node = xpath.select1("//*[local-name(.)='y']", doc);
const node = xpath.select1("//*[local-name(.)='y']", doc as unknown as Node);
isDomNode.assertIsNodeLike(node);
const sig = new SignedXml();
const res = sig.getCanonXml(
Expand All @@ -370,8 +371,8 @@ describe("Exclusive canonicalization with comments", function () {
// in a document.
const xml =
'<x><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#" /><y><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#" /></y></x>';
const doc = new xmldom.DOMParser().parseFromString(xml);
const node = xpath.select1("//*[local-name(.)='y']", doc);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
const node = xpath.select1("//*[local-name(.)='y']", doc as unknown as Node);
const sig = new SignedXml();
const transforms = ["http://www.w3.org/2000/09/xmldsig#enveloped-signature"];
isDomNode.assertIsNodeLike(node);
Expand Down
18 changes: 10 additions & 8 deletions test/canonicalization-unit-tests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ const compare = function (
inclusiveNamespacesPrefixList?: string[],
defaultNsForPrefix?: Record<string, string>,
) {
const doc = new xmldom.DOMParser().parseFromString(xml);
const elem = xpath.select1(xpathArg, doc);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
const elem = xpath.select1(xpathArg, doc as unknown as Node);
const can = new ExclusiveCanonicalization();
isDomNode.assertIsElementNode(elem);
const result = can
Expand Down Expand Up @@ -54,7 +54,7 @@ describe("Canonicalization unit tests", function () {

it("Exclusive canonicalization works with default namespace for prefix", function () {
compare(
'<ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:SignedInfo>',
'<ds:SignedInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:SignedInfo>',
Copy link

@srd90 srd90 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why version bump required addition of xmlns definition to input xml?

I guess that new xmldom version refuses to understand that particular input xml anymore when it is parsed at the beginning of compare function...but is this test case ("Exclusive canonicalization works with default namespace for prefix") still testing what it claims to to test. Line 61 configures

{ ds: "http://www.w3.org/2000/09/xmldsig#" }

as parameter for compare functions defaultNsForPrefix which is passed as parameter for canonicalization process. I did not spend time to try to understand how canonicalization function should work but parameter name implicates that canonixalization function was previously instructed to use aforementioned NS for prefix ds (if/when it was missing from input xml).

Disclaimer: I am not xml dsig or canonicalization expert. This change just popped up from the mass when I quickly scrolled through changes out of curiosity.

Copy link
Author

@elbuo8 elbuo8 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 61 configures
as parameter for compare functions defaultNsForPrefix which is passed as parameter for canonicalization process.

These happen after parsing the DOM, so they are unrelated/ignored by xmldom.
It considers it invalid payload since it was never defined. The excerpt itself
<ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:SignedInfo> is invalid in any XML validator I punch it into. (I'm def no expert on XML either 😅 ).

Seems xmldom is becoming more strict overall.

Copy link

@srd90 srd90 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These happen after parsing the DOM, so they are unrelated/ignored by xmldom

Yes...canonicalzation's process implementation is tested at compare after inputxml is parsed with xmldom and after SignedInfo is extracted from DOM produced by xmldom and yes xmldom is more stricter now, but:
Under what conditions canonicalization was invoked with Element which did/might not have namespace uri available / defined for prefix (in this case for ds). Dunno why such test case and functionality (defaultNsForPrefix') would exists without some reason. Should such functionality be preserved with tests that actually test it(?)

Unfortunately there isn't much info available at these related commits:

  1. 7d69a26
  2. 5f2278b

"//*[local-name(.)='SignedInfo']",
'<ds:SignedInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"></ds:CanonicalizationMethod></ds:SignedInfo>',
undefined,
Expand Down Expand Up @@ -400,8 +400,9 @@ describe("Canonicalization unit tests", function () {
it("Multiple Canonicalization with namespace definition outside of signed element", function () {
const doc = new xmldom.DOMParser().parseFromString(
'<x xmlns:p="myns"><p:y><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"></ds:Signature></p:y></x>',
"text/xml",
);
const node = xpath.select1("//*[local-name(.)='y']", doc);
const node = xpath.select1("//*[local-name(.)='y']", doc as unknown as Node);
isDomNode.assertIsNodeLike(node);

const sig = new SignedXml();
Expand All @@ -418,9 +419,10 @@ describe("Canonicalization unit tests", function () {
it("Shouldn't continue processing transforms if we end up with a string as a result of a transform", function () {
const doc = new xmldom.DOMParser().parseFromString(
'<x xmlns:p="myns"><p:y><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"></ds:Signature></p:y></x>',
"text/xml",
);
const node1 = xpath.select1("//*[local-name(.)='y']", doc);
const node2 = xpath.select1("//*[local-name(.)='y']", doc);
const node1 = xpath.select1("//*[local-name(.)='y']", doc as unknown as Node);
const node2 = xpath.select1("//*[local-name(.)='y']", doc as unknown as Node);
isDomNode.assertIsNodeLike(node1);
isDomNode.assertIsNodeLike(node2);
const sig = new SignedXml();
Expand All @@ -445,8 +447,8 @@ describe("Canonicalization unit tests", function () {
// in a document.
const xml =
'<x><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#" /><y><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#" /></y></x>';
const doc = new xmldom.DOMParser().parseFromString(xml);
const node = xpath.select1("//*[local-name(.)='y']", doc);
const doc = new xmldom.DOMParser().parseFromString(xml, "text/xml");
const node = xpath.select1("//*[local-name(.)='y']", doc as unknown as Node);
isDomNode.assertIsNodeLike(node);

const sig = new SignedXml();
Expand Down
Loading