Skip to content

Commit 110dd7c

Browse files
cjbarthcjbarth
authored
Fix transform processing regression (#379)
1 parent fa2922f commit 110dd7c

9 files changed

+111
-82
lines changed

‎src/c14n-canonicalization.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,11 +167,14 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
167167
return { rendered: res.join(""), newDefaultNs };
168168
}
169169

170-
processInner(node: Node, prefixesInScope, defaultNs, defaultNsForPrefix, ancestorNamespaces) {
170+
/**
171+
* @param node Node
172+
*/
173+
processInner(node, prefixesInScope, defaultNs, defaultNsForPrefix, ancestorNamespaces) {
171174
if (xpath.isComment(node)) {
172175
return this.renderComment(node);
173176
}
174-
if (xpath.isComment(node)) {
177+
if (node.data) {
175178
return utils.encodeSpecialCharactersInText(node.data);
176179
}
177180

@@ -198,7 +201,7 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
198201
return res.join("");
199202
}
200203

201-
return "";
204+
throw new Error(`Unable to canonicalize node type: ${node.nodeType}`);
202205
}
203206

204207
// Thanks to deoxxa/xml-c14n for comment renderer
@@ -247,7 +250,7 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
247250
* @param node
248251
* @api public
249252
*/
250-
process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
253+
process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions): string {
251254
options = options || {};
252255
const defaultNs = options.defaultNs || "";
253256
const defaultNsForPrefix = options.defaultNsForPrefix || {};

‎src/enveloped-signature.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import type {
88

99
export class EnvelopedSignature implements CanonicalizationOrTransformationAlgorithm {
1010
includeComments = false;
11-
process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
11+
process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions): Node {
1212
if (null == options.signatureNode) {
1313
const signature = xpath.select1(
1414
"./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",

‎src/exclusive-canonicalization.ts

Lines changed: 35 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,9 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
167167
return { rendered: res.join(""), newDefaultNs: newDefaultNs };
168168
}
169169

170+
/**
171+
* @param node Node
172+
*/
170173
processInner(
171174
node,
172175
prefixesInScope,
@@ -181,32 +184,36 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
181184
return utils.encodeSpecialCharactersInText(node.data);
182185
}
183186

184-
let i;
185-
let pfxCopy;
186-
const ns = this.renderNs(
187-
node,
188-
prefixesInScope,
189-
defaultNs,
190-
defaultNsForPrefix,
191-
inclusiveNamespacesPrefixList,
192-
);
193-
const res = ["<", node.tagName, ns.rendered, this.renderAttrs(node), ">"];
194-
195-
for (i = 0; i < node.childNodes.length; ++i) {
196-
pfxCopy = prefixesInScope.slice(0);
197-
res.push(
198-
this.processInner(
199-
node.childNodes[i],
200-
pfxCopy,
201-
ns.newDefaultNs,
202-
defaultNsForPrefix,
203-
inclusiveNamespacesPrefixList,
204-
),
187+
if (xpath.isElement(node)) {
188+
let i;
189+
let pfxCopy;
190+
const ns = this.renderNs(
191+
node,
192+
prefixesInScope,
193+
defaultNs,
194+
defaultNsForPrefix,
195+
inclusiveNamespacesPrefixList,
205196
);
197+
const res = ["<", node.tagName, ns.rendered, this.renderAttrs(node), ">"];
198+
199+
for (i = 0; i < node.childNodes.length; ++i) {
200+
pfxCopy = prefixesInScope.slice(0);
201+
res.push(
202+
this.processInner(
203+
node.childNodes[i],
204+
pfxCopy,
205+
ns.newDefaultNs,
206+
defaultNsForPrefix,
207+
inclusiveNamespacesPrefixList,
208+
),
209+
);
210+
}
211+
212+
res.push("</", node.tagName, ">");
213+
return res.join("");
206214
}
207215

208-
res.push("</", node.tagName, ">");
209-
return res.join("");
216+
throw new Error(`Unable to exclusive canonicalize node type: ${node.nodeType}`);
210217
}
211218

212219
// Thanks to deoxxa/xml-c14n for comment renderer
@@ -250,13 +257,11 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
250257
}
251258

252259
/**
253-
* Perform canonicalization of the given node
260+
* Perform canonicalization of the given element node
254261
*
255-
* @param {Node} node
256-
* @return {String}
257262
* @api public
258263
*/
259-
process(node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
264+
process(elem: Element, options: CanonicalizationOrTransformationAlgorithmProcessOptions): string {
260265
options = options || {};
261266
let inclusiveNamespacesPrefixList = options.inclusiveNamespacesPrefixList || [];
262267
const defaultNs = options.defaultNs || "";
@@ -267,7 +272,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
267272
* If the inclusiveNamespacesPrefixList has not been explicitly provided then look it up in CanonicalizationMethod/InclusiveNamespaces
268273
*/
269274
if (!utils.isArrayHasLength(inclusiveNamespacesPrefixList)) {
270-
const CanonicalizationMethod = utils.findChildren(node, "CanonicalizationMethod");
275+
const CanonicalizationMethod = utils.findChildren(elem, "CanonicalizationMethod");
271276
if (CanonicalizationMethod.length !== 0) {
272277
const inclusiveNamespaces = utils.findChildren(
273278
CanonicalizationMethod[0],
@@ -289,7 +294,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
289294
if (ancestorNamespaces) {
290295
ancestorNamespaces.forEach(function (ancestorNamespace) {
291296
if (prefix === ancestorNamespace.prefix) {
292-
node.setAttributeNS(
297+
elem.setAttributeNS(
293298
"http://www.w3.org/2000/xmlns/",
294299
`xmlns:${prefix}`,
295300
ancestorNamespace.namespaceURI,
@@ -301,7 +306,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
301306
}
302307

303308
const res = this.processInner(
304-
node,
309+
elem,
305310
[],
306311
defaultNs,
307312
defaultNsForPrefix,

‎src/signed-xml.ts

Lines changed: 34 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -471,12 +471,11 @@ export class SignedXml {
471471
}
472472

473473
validateReferences(doc: Document) {
474-
for (const ref of this.references) {
475-
if (!this.validateReference(ref, doc)) {
476-
return false;
477-
}
478-
}
479-
return true;
474+
return (
475+
Array.isArray(this.references) &&
476+
this.references.length > 0 &&
477+
this.references.every((ref) => this.validateReference(ref, doc))
478+
);
480479
}
481480

482481
/**
@@ -595,39 +594,38 @@ export class SignedXml {
595594
.flatMap((namespace) => (namespace.getAttribute("PrefixList") ?? "").split(" "))
596595
.filter((value) => value.length > 0);
597596
}
597+
}
598598

599-
if (utils.isArrayHasLength(this.implicitTransforms)) {
600-
this.implicitTransforms.forEach(function (t) {
601-
transforms.push(t);
602-
});
603-
}
604-
605-
/**
606-
* DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
607-
* need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
608-
* transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
609-
* @see:
610-
* https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
611-
* https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
612-
* https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
613-
*/
614-
if (
615-
transforms.length === 0 ||
616-
transforms[transforms.length - 1] ===
617-
"http://www.w3.org/2000/09/xmldsig#enveloped-signature"
618-
) {
619-
transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
620-
}
621-
622-
this.addReference({
623-
transforms,
624-
digestAlgorithm: digestAlgo,
625-
uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
626-
digestValue,
627-
inclusiveNamespacesPrefixList,
628-
isEmptyUri: false,
599+
if (utils.isArrayHasLength(this.implicitTransforms)) {
600+
this.implicitTransforms.forEach(function (t) {
601+
transforms.push(t);
629602
});
630603
}
604+
605+
/**
606+
* DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
607+
* need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
608+
* transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
609+
* @see:
610+
* https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
611+
* https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
612+
* https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
613+
*/
614+
if (
615+
transforms.length === 0 ||
616+
transforms[transforms.length - 1] === "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
617+
) {
618+
transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
619+
}
620+
621+
this.addReference({
622+
transforms,
623+
digestAlgorithm: digestAlgo,
624+
uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
625+
digestValue,
626+
inclusiveNamespacesPrefixList,
627+
isEmptyUri: false,
628+
});
631629
}
632630

633631
/**

‎test/c14nWithComments-unit-tests.spec.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,12 @@ const compare = function (xml, xpathArg, expected, inclusiveNamespacesPrefixList
99
const doc = new xmldom.DOMParser().parseFromString(xml);
1010
const elem = xpath.select1(xpathArg, doc);
1111
const can = new c14nWithComments();
12-
const result = can.process(elem, { inclusiveNamespacesPrefixList }).toString();
13-
14-
expect(result).to.equal(expected);
12+
if (xpath.isElement(elem)) {
13+
const result = can.process(elem, { inclusiveNamespacesPrefixList }).toString();
14+
expect(result).to.equal(expected);
15+
} else {
16+
throw new Error("Element not found.");
17+
}
1518
};
1619

1720
describe("Exclusive canonicalization with comments", function () {

‎test/canonicalization-unit-tests.spec.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,18 @@ const compare = function (
1515
const doc = new xmldom.DOMParser().parseFromString(xml);
1616
const elem = xpath.select1(xpathArg, doc);
1717
const can = new ExclusiveCanonicalization();
18-
const result = can
19-
.process(elem, {
20-
inclusiveNamespacesPrefixList,
21-
defaultNsForPrefix,
22-
})
23-
.toString();
24-
25-
expect(expected).to.equal(result);
18+
if (xpath.isElement(elem)) {
19+
const result = can
20+
.process(elem, {
21+
inclusiveNamespacesPrefixList,
22+
defaultNsForPrefix,
23+
})
24+
.toString();
25+
26+
expect(expected).to.equal(result);
27+
} else {
28+
throw new Error("Invalid element");
29+
}
2630
};
2731

2832
describe("Canonicalization unit tests", function () {

‎test/signature-unit-tests.spec.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -877,6 +877,10 @@ describe("Signature unit tests", function () {
877877
passValidSignature("./test/static/valid_signature_with_unused_prefixes.xml");
878878
});
879879

880+
it("verifies valid signature without transforms element", function () {
881+
passValidSignature("./test/static/valid_signature_without_transforms_element.xml");
882+
});
883+
880884
it("fails invalid signature - signature value", function () {
881885
failInvalidSignature("./test/static/invalid_signature - signature value.xml");
882886
});
@@ -918,6 +922,10 @@ describe("Signature unit tests", function () {
918922
);
919923
});
920924

925+
it("fails invalid signature without transforms element", function () {
926+
failInvalidSignature("./test/static/invalid_signature_without_transforms_element.xml");
927+
});
928+
921929
it("allow empty reference uri when signing", function () {
922930
const xml = "<root><x /></root>";
923931
const sig = new SignedXml();

‎test/static/invalid_signature_without_transforms_element.xml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?xml version="1.0"?>
2+
<library><book ID="bookid"><name>some tampered text</name></book><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/><Reference URI="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue></Reference></SignedInfo><SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
3+
lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
4+
UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>

‎test/static/valid_signature_without_transforms_element.xml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?xml version="1.0"?>
2+
<library><book ID="bookid"><name>some text</name></book><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/><Reference URI="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue></Reference></SignedInfo><SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
3+
lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
4+
UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>

0 commit comments

Comments
 (0)