You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@santuario.apache.org by co...@apache.org on 2023/05/18 10:46:28 UTC

[santuario-xml-security-java] branch 2.3.x-fixes updated: SANTUARIO-600 - Propagate failures from the NodeFilter via a checked exception (#169)

This is an automated email from the ASF dual-hosted git repository.

coheigea pushed a commit to branch 2.3.x-fixes
in repository https://gitbox.apache.org/repos/asf/santuario-xml-security-java.git


The following commit(s) were added to refs/heads/2.3.x-fixes by this push:
     new 1a08323a SANTUARIO-600 - Propagate failures from the NodeFilter via a checked exception (#169)
1a08323a is described below

commit 1a08323ad76ef0808c17382bd94ce07dd717b308
Author: Colm O hEigeartaigh <co...@users.noreply.github.com>
AuthorDate: Thu May 18 11:01:36 2023 +0100

    SANTUARIO-600 - Propagate failures from the NodeFilter via a checked exception (#169)
---
 .../xml/dsig/internal/dom/ApacheNodeSetData.java   | 13 +++++----
 .../c14n/implementations/CanonicalizerBase.java    | 34 +++++++++++++++-------
 .../apache/xml/security/signature/NodeFilter.java  |  5 ++--
 .../transforms/implementations/TransformXPath.java | 12 +++-----
 .../security/test/dom/interop/BaltimoreTest.java   | 25 +++++-----------
 .../test/dom/interop/BaltimoreXalanTest.java       | 32 ++++++++++++++++++++
 6 files changed, 77 insertions(+), 44 deletions(-)

diff --git a/src/main/java/org/apache/jcp/xml/dsig/internal/dom/ApacheNodeSetData.java b/src/main/java/org/apache/jcp/xml/dsig/internal/dom/ApacheNodeSetData.java
index c43e0295..6c4f8a69 100644
--- a/src/main/java/org/apache/jcp/xml/dsig/internal/dom/ApacheNodeSetData.java
+++ b/src/main/java/org/apache/jcp/xml/dsig/internal/dom/ApacheNodeSetData.java
@@ -27,6 +27,8 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 import javax.xml.crypto.NodeSetData;
+
+import org.apache.xml.security.transforms.TransformationException;
 import org.w3c.dom.Node;
 import org.apache.xml.security.signature.NodeFilter;
 import org.apache.xml.security.signature.XMLSignatureInput;
@@ -42,11 +44,12 @@ public class ApacheNodeSetData implements ApacheData, NodeSetData {
 
     public Iterator<Node> iterator() {
         // If nodefilters are set, must execute them first to create node-set
-        if (xi.getNodeFilters() != null && !xi.getNodeFilters().isEmpty()) {
-            return Collections.unmodifiableSet
-                (getNodeSet(xi.getNodeFilters())).iterator();
-        }
         try {
+            if (xi.getNodeFilters() != null && !xi.getNodeFilters().isEmpty()) {
+                return Collections.unmodifiableSet
+                        (getNodeSet(xi.getNodeFilters())).iterator();
+            }
+
             return Collections.unmodifiableSet(xi.getNodeSet()).iterator();
         } catch (Exception e) {
             // should not occur
@@ -59,7 +62,7 @@ public class ApacheNodeSetData implements ApacheData, NodeSetData {
         return xi;
     }
 
-    private Set<Node> getNodeSet(List<NodeFilter> nodeFilters) {
+    private Set<Node> getNodeSet(List<NodeFilter> nodeFilters) throws TransformationException {
         if (xi.isNeedsToBeExpanded()) {
             XMLUtils.circumventBug2650
                 (XMLUtils.getOwnerDocument(xi.getSubNode()));
diff --git a/src/main/java/org/apache/xml/security/c14n/implementations/CanonicalizerBase.java b/src/main/java/org/apache/xml/security/c14n/implementations/CanonicalizerBase.java
index 473199bc..9360e25d 100644
--- a/src/main/java/org/apache/xml/security/c14n/implementations/CanonicalizerBase.java
+++ b/src/main/java/org/apache/xml/security/c14n/implementations/CanonicalizerBase.java
@@ -454,13 +454,17 @@ public abstract class CanonicalizerBase extends CanonicalizerSpi {
         } while(true);
     }
 
-    protected int isVisibleDO(Node currentNode, int level) {
+    protected int isVisibleDO(Node currentNode, int level) throws CanonicalizationException {
         if (nodeFilter != null) {
             Iterator<NodeFilter> it = nodeFilter.iterator();
             while (it.hasNext()) {
-                int i = it.next().isNodeIncludeDO(currentNode, level);
-                if (i != 1) {
-                    return i;
+                try {
+                    int i = it.next().isNodeIncludeDO(currentNode, level);
+                    if (i != 1) {
+                        return i;
+                    }
+                } catch (Exception e) {
+                    throw new CanonicalizationException(e);
                 }
             }
         }
@@ -470,13 +474,17 @@ public abstract class CanonicalizerBase extends CanonicalizerSpi {
         return 1;
     }
 
-    protected int isVisibleInt(Node currentNode) {
+    protected int isVisibleInt(Node currentNode) throws CanonicalizationException {
         if (nodeFilter != null) {
             Iterator<NodeFilter> it = nodeFilter.iterator();
             while (it.hasNext()) {
-                int i = it.next().isNodeInclude(currentNode);
-                if (i != 1) {
-                    return i;
+                try {
+                    int i = it.next().isNodeInclude(currentNode);
+                    if (i != 1) {
+                        return i;
+                    }
+                } catch (Exception e) {
+                    throw new CanonicalizationException(e);
                 }
             }
         }
@@ -486,12 +494,16 @@ public abstract class CanonicalizerBase extends CanonicalizerSpi {
         return 1;
     }
 
-    protected boolean isVisible(Node currentNode) {
+    protected boolean isVisible(Node currentNode) throws CanonicalizationException {
         if (nodeFilter != null) {
             Iterator<NodeFilter> it = nodeFilter.iterator();
             while (it.hasNext()) {
-                if (it.next().isNodeInclude(currentNode) != 1) {
-                    return false;
+                try {
+                    if (it.next().isNodeInclude(currentNode) != 1) {
+                        return false;
+                    }
+                } catch (Exception e) {
+                    throw new CanonicalizationException(e);
                 }
             }
         }
diff --git a/src/main/java/org/apache/xml/security/signature/NodeFilter.java b/src/main/java/org/apache/xml/security/signature/NodeFilter.java
index 92d05a0a..b2cecb97 100644
--- a/src/main/java/org/apache/xml/security/signature/NodeFilter.java
+++ b/src/main/java/org/apache/xml/security/signature/NodeFilter.java
@@ -18,6 +18,7 @@
  */
 package org.apache.xml.security.signature;
 
+import org.apache.xml.security.transforms.TransformationException;
 import org.w3c.dom.Node;
 
 /**
@@ -33,7 +34,7 @@ public interface NodeFilter {
      * 		  -1 if the node and all it's child must not be output.
      *
      */
-    int isNodeInclude(Node n);
+    int isNodeInclude(Node n) throws TransformationException;
 
     /**
      * Tells if a node must be output in a c14n.
@@ -46,6 +47,6 @@ public interface NodeFilter {
      * 		   0 if node must not be output,
      * 		  -1 if the node and all it's child must not be output.
      */
-    int isNodeIncludeDO(Node n, int level);
+    int isNodeIncludeDO(Node n, int level) throws TransformationException;
 
 }
diff --git a/src/main/java/org/apache/xml/security/transforms/implementations/TransformXPath.java b/src/main/java/org/apache/xml/security/transforms/implementations/TransformXPath.java
index 8b1c140f..5e418e0b 100644
--- a/src/main/java/org/apache/xml/security/transforms/implementations/TransformXPath.java
+++ b/src/main/java/org/apache/xml/security/transforms/implementations/TransformXPath.java
@@ -45,9 +45,6 @@ import org.w3c.dom.Node;
  */
 public class TransformXPath extends TransformSpi {
 
-    private static final org.slf4j.Logger LOG =
-            org.slf4j.LoggerFactory.getLogger(TransformXPath.class);
-
     /**
      * {@inheritDoc}
      */
@@ -133,20 +130,19 @@ public class TransformXPath extends TransformSpi {
         /**
          * @see org.apache.xml.security.signature.NodeFilter#isNodeInclude(org.w3c.dom.Node)
          */
-        public int isNodeInclude(Node currentNode) {
+        public int isNodeInclude(Node currentNode) throws TransformationException {
             try {
                 boolean include = xPathAPI.evaluate(currentNode, xpathnode, str, xpathElement);
                 if (include) {
                     return 1;
                 }
                 return 0;
-            } catch (TransformerException e) {
-                LOG.debug("Error evaluating XPath expression", e);
-                return 0;
+            } catch (TransformerException ex) {
+                throw new TransformationException(ex);
             }
         }
 
-        public int isNodeIncludeDO(Node n, int level) {
+        public int isNodeIncludeDO(Node n, int level) throws TransformationException {
             return isNodeInclude(n);
         }
 
diff --git a/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreTest.java b/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreTest.java
index 54362eb4..06647c70 100644
--- a/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreTest.java
+++ b/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreTest.java
@@ -18,6 +18,7 @@
  */
 package org.apache.xml.security.test.dom.interop;
 
+import org.apache.xml.security.signature.MissingResourceFailureException;
 import org.apache.xml.security.test.dom.utils.resolver.OfflineResolver;
 import org.apache.xml.security.utils.resolver.ResourceResolverSpi;
 
@@ -26,7 +27,6 @@ import java.nio.charset.StandardCharsets;
 
 import org.apache.xml.security.signature.XMLSignatureException;
 
-import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -278,12 +278,8 @@ public class BaltimoreTest extends InteropTestBase {
     }
 
     /**
-     * Method test_sixteen_bad_signature. This tests make sure that an
-     * invalid signature is not valid. This is validating merlin's 16
-     * signature but some of the referenced content has been modified so
-     * some of the references should be invalid.
-     *
-     * @throws Exception
+     * Method test_sixteen_bad_signature. This should fail due to lack of support for the here() function
+     * as we don't have Xalan installed.
      */
     @org.junit.jupiter.api.Test
     public void test_sixteen_bad_signature() throws Exception {
@@ -292,20 +288,13 @@ public class BaltimoreTest extends InteropTestBase {
             merlinsDir16 + "/bad-signature.xml";
         ResourceResolverSpi resolver = new OfflineResolver();
         boolean followManifests = false;
-        boolean verify = false;
 
         try {
-            verify = this.verify(filename, resolver, followManifests);
-        } catch (RuntimeException ex) {
-            LOG.error("Verification crashed for " + filename);
-            throw ex;
+            this.verify(filename, resolver, followManifests);
+            fail("Failure expected due to no support for the here() function");
+        } catch (MissingResourceFailureException ex) {
+            assertTrue(ex.getCause().getMessage().contains("Could not find function: here"));
         }
-
-        if (verify) {
-            LOG.error("Verification passed (should have failed) for " + filename);
-        }
-
-        assertFalse(verify, filename);
     }
 
     /**
diff --git a/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreXalanTest.java b/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreXalanTest.java
index 9853b472..aa49754b 100644
--- a/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreXalanTest.java
+++ b/src/test/java/org/apache/xml/security/test/dom/interop/BaltimoreXalanTest.java
@@ -101,6 +101,38 @@ public class BaltimoreXalanTest extends InteropTestBase {
         assertTrue(verify, filename);
     }
 
+    /**
+     * Method test_sixteen_bad_signature. This tests make sure that an
+     * invalid signature is not valid. This is validating merlin's 16
+     * signature but some of the referenced content has been modified so
+     * some of the references should be invalid.
+     *
+     * @throws Exception
+     */
+    @org.junit.jupiter.api.Test
+    public void test_sixteen_bad_signature() throws Exception {
+
+        String filename =
+                merlinsDir16 + "/bad-signature.xml";
+        ResourceResolverSpi resolver = new OfflineResolver();
+        boolean followManifests = false;
+        boolean verify = false;
+
+        try {
+            verify = this.verify(filename, resolver, followManifests);
+        } catch (RuntimeException ex) {
+            LOG.error("Verification crashed for " + filename);
+            throw ex;
+        }
+
+        if (verify) {
+            LOG.error("Verification passed (should have failed) for " + filename);
+        }
+
+        assertFalse(verify, filename);
+    }
+
+
     /**
      * Method test_twenty_three_external_dsa_2
      *