Fix transform processing regression

node-saml · Aug 20, 2023 · 540457c · 540457c
1 parent 2e32d50
commit 540457c
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 39 deletions.
diff --git a/src/c14n-canonicalization.ts b/src/c14n-canonicalization.ts
@@ -171,7 +171,7 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
     if (xpath.isComment(node)) {
       return this.renderComment(node);
     }
-    if (xpath.isComment(node)) {
+    if (xpath.isComment(node) || xpath.isTextNode(node)) {
       return utils.encodeSpecialCharactersInText(node.data);
     }
 
@@ -247,7 +247,7 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
    * @param node
    * @api public
    */
-  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
+  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions): Node {
     options = options || {};
     const defaultNs = options.defaultNs || "";
     const defaultNsForPrefix = options.defaultNsForPrefix || {};

diff --git a/src/enveloped-signature.ts b/src/enveloped-signature.ts
@@ -8,7 +8,7 @@ import type {
 
 export class EnvelopedSignature implements CanonicalizationOrTransformationAlgorithm {
   includeComments = false;
-  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
+  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions): Node {
     if (null == options.signatureNode) {
       const signature = xpath.select1(
         "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",

diff --git a/src/exclusive-canonicalization.ts b/src/exclusive-canonicalization.ts
@@ -250,13 +250,11 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
   }
 
   /**
-   * Perform canonicalization of the given node
+   * Perform canonicalization of the given element node
    *
-   * @param {Node} node
-   * @return {String}
    * @api public
    */
-  process(node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
+  process(elem: Element, options: CanonicalizationOrTransformationAlgorithmProcessOptions): string {
     options = options || {};
     let inclusiveNamespacesPrefixList = options.inclusiveNamespacesPrefixList || [];
     const defaultNs = options.defaultNs || "";
@@ -267,7 +265,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
      * If the inclusiveNamespacesPrefixList has not been explicitly provided then look it up in CanonicalizationMethod/InclusiveNamespaces
      */
     if (!utils.isArrayHasLength(inclusiveNamespacesPrefixList)) {
-      const CanonicalizationMethod = utils.findChildren(node, "CanonicalizationMethod");
+      const CanonicalizationMethod = utils.findChildren(elem, "CanonicalizationMethod");
       if (CanonicalizationMethod.length !== 0) {
         const inclusiveNamespaces = utils.findChildren(
           CanonicalizationMethod[0],
@@ -289,7 +287,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
         if (ancestorNamespaces) {
           ancestorNamespaces.forEach(function (ancestorNamespace) {
             if (prefix === ancestorNamespace.prefix) {
-              node.setAttributeNS(
+              elem.setAttributeNS(
                 "http://www.w3.org/2000/xmlns/",
                 `xmlns:${prefix}`,
                 ancestorNamespace.namespaceURI,
@@ -301,7 +299,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
     }
 
     const res = this.processInner(
-      node,
+      elem,
       [],
       defaultNs,
       defaultNsForPrefix,

diff --git a/src/signed-xml.ts b/src/signed-xml.ts
@@ -595,39 +595,38 @@ export class SignedXml {
           .flatMap((namespace) => (namespace.getAttribute("PrefixList") ?? "").split(" "))
           .filter((value) => value.length > 0);
       }
+    }
 
-      if (utils.isArrayHasLength(this.implicitTransforms)) {
-        this.implicitTransforms.forEach(function (t) {
-          transforms.push(t);
-        });
-      }
+    if (utils.isArrayHasLength(this.implicitTransforms)) {
+      this.implicitTransforms.forEach(function (t) {
+        transforms.push(t);
+      });
+    }
 
-      /**
-       * DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
-       * need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
-       * transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
-       * @see:
-       * https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
-       * https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
-       * https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
-       */
-      if (
-        transforms.length === 0 ||
-        transforms[transforms.length - 1] ===
-          "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
-      ) {
-        transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
-      }
+    /**
+     * DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
+     * need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
+     * transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
+     * @see:
+     * https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
+     * https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
+     * https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
+     */
+    if (
+      transforms.length === 0 ||
+      transforms[transforms.length - 1] === "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
+    ) {
+      transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
+    }
 
       this.addReference({
-        transforms,
-        digestAlgorithm: digestAlgo,
-        uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
-        digestValue,
-        inclusiveNamespacesPrefixList,
-        isEmptyUri: false,
-      });
-    }
+      transforms,
+      digestAlgorithm: digestAlgo,
+      uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
+      digestValue,
+      inclusiveNamespacesPrefixList,
+      isEmptyUri: false,
+    });
   }
 
   /**

diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts
@@ -877,6 +877,10 @@ describe("Signature unit tests", function () {
     passValidSignature("./test/static/valid_signature_with_unused_prefixes.xml");
   });
 
+  it("verifies valid signature without transforms element", function () {
+    passValidSignature("./test/static/valid_signature_without_transforms_element.xml");
+  });
+
   it("fails invalid signature - signature value", function () {
     failInvalidSignature("./test/static/invalid_signature - signature value.xml");
   });
@@ -918,6 +922,10 @@ describe("Signature unit tests", function () {
     );
   });
 
+  it("fails invalid signature without transforms element", function () {
+    failInvalidSignature("./test/static/invalid_signature_without_transforms_element.xml");
+  });
+
   it("allow empty reference uri when signing", function () {
     const xml = "<root><x /></root>";
     const sig = new SignedXml();

diff --git a/test/static/invalid_signature_without_transforms_element.xml b/test/static/invalid_signature_without_transforms_element.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<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
+lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
+UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>
diff --git a/test/static/valid_signature_without_transforms_element.xml b/test/static/valid_signature_without_transforms_element.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<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
+lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
+UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>