Handle validation for Composite URIs without parens (#2004)

Parentheses are optional when creating composite and nested URIs so this
updates the validation to handle missing parens as well.

Follow on to #1847

Author: cshannon

activemq-broker/src/main/java/org/apache/activemq/broker/jmx/BrokerView.java

@@ -614,26 +614,39 @@ private static void validateAllowedUrl(String uriString) throws URISyntaxExcepti
     // Validate the URI does not contain a denied transport scheme
     private static void validateAllowedUri(URI uri, int depth) throws URISyntaxException {
         // Don't allow more than 5 nested URIs to prevent blowing the stack
-        // If we are greater than 4 then this is the 5th level of composite
-        if (depth > 4) {
+        if (depth > 5) {
             throw new IllegalArgumentException("URI can't contain more than 5 nested composite URIs");
         }
 
         // First check the main URI scheme
         validateAllowedScheme(uri.getScheme());
 
-        // If composite, iterate and check each of the composite URIs
-        if (URISupport.isCompositeURI(uri)) {
-            URISupport.CompositeData data = URISupport.parseComposite(uri);
+        // We need to check if the URI is composite and/or contains nested URIs
+        // The utility method URISupport#isCompositeURI is not good enough here
+        // because it misses if there are no parentheses and also is primarily meant
+        // for checking comma separated URIs and not nested URIs.
+        //
+        // The best way to handle all cases is to use the same logic that the transports
+        // use to process the URIs and that is to simply attempt to parse it and check each
+        // of the parsed components. This wll correctly handle the case when there
+        // are parentheses and also when the parentheses are skipped.
+        final URISupport.CompositeData data;
+        try {
+            data = URISupport.parseComposite(uri);
+        } catch (URISyntaxException e) {
+            // If this is not a valid URI then we can stop checking
+            // This can happen when parsing a nested URI and at the last portion
+            return;
+        }
+
+        if (data.getComponents() != null) {
             depth++;
             for (URI component : data.getComponents()) {
-                // Each URI could be a nested composite URI so call validateAllowedUri()
-                // to validate it. This check if composite first so we don't add to
-                // the recursive stack depth if there's a lot of URIs that are not composite
-                if (URISupport.isCompositeURI(component)) {
+                // Each URI could be a nested and/or composite URI so call validateAllowedUri()
+                // to validate it. If the scheme is null then the original URI is not composite
+                // or nested so we can skip the check, and we are finished.
+                if (component.getScheme() != null) {
                     validateAllowedUri(component, depth);
-                } else {
-                    validateAllowedScheme(component.getScheme());
                 }
             }
         }

activemq-unit-tests/src/test/java/org/apache/activemq/broker/jmx/MBeanTest.java

@@ -2084,6 +2084,14 @@ protected void testAddTransportConnectorBlockedBrokerView(String scheme) throws
             assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage());
         }
 
+        try {
+            // verify any composite URI is blocked as well without parens
+            brokerView.addConnector("static:tcp://0.0.0.0:0," + scheme + "://" + brokerName);
+            fail("Should have failed trying to add connector with scheme: " + scheme);
+        } catch (IllegalArgumentException e) {
+            assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage());
+        }
+
         try {
             // verify nested composite URI is blocked
             brokerView.addConnector("static:(static:(static:(" + scheme + "://localhost)))");
@@ -2106,6 +2114,15 @@ public void testNestedAddTransportConnector() throws Exception {
         } catch (IllegalArgumentException e) {
             assertEquals("URI can't contain more than 5 nested composite URIs", e.getMessage());
         }
+
+        try {
+            // verify nested composite URI with more than 5 levels is blocked without parens
+            brokerView.addConnector(
+                    "static:static:static:static:static:static:tcp://localhost:0");
+            fail("Should have failed trying to add vm connector bridge");
+        } catch (IllegalArgumentException e) {
+            assertEquals("URI can't contain more than 5 nested composite URIs", e.getMessage());
+        }
     }
 
 }

activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxCreateNCTest.java

@@ -82,6 +82,31 @@ public void testBridgeRegistration() throws Exception {
         assertEquals("NC", nc.getName());
     }
 
+    @Test
+    public void testTransportSchemeBridgeAllowed() throws Exception {
+        // Test composite network connector uri
+        String name = proxy.addNetworkConnector("static:(tcp://localhost,amqp://localhost)");
+        proxy.removeNetworkConnector(name);
+
+        // Test composite with missing parens
+        name = proxy.addNetworkConnector("static:amqp://localhost,tcp://127.0.0.1:0");
+        proxy.removeNetworkConnector(name);
+
+        // verify direct connector as well
+        name = proxy.addNetworkConnector("static:stomp://localhost");
+        proxy.removeNetworkConnector(name);
+
+        // verify nested composite URI
+        name = proxy.addNetworkConnector(
+                "static:(static:(static:(tcp+ssl://localhost:0,auto+nio+ssl://localhost)))");
+        proxy.removeNetworkConnector(name);
+
+        // verify nested composite URI is not blocked when not using parens
+        name = proxy.addNetworkConnector(
+                "static:static:static:123://localhost:0,auto+nio+ssl://localhost");
+        proxy.removeNetworkConnector(name);
+    }
+
     @Test
     public void testTransportSchemeBridgeBlocked() throws Exception {
         for (String deniedScheme : DENIED_TRANSPORT_SCHEMES) {
@@ -99,6 +124,14 @@ protected void testTransportSchemeBridgeBlocked(String scheme) throws Exception
             assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage());
         }
 
+        // Test composite with missing parens
+        try {
+            proxy.addNetworkConnector("static:" + scheme + "://localhost,tcp://127.0.0.1:0");
+            fail("Should have failed trying to add connector bridge with scheme: " + scheme);
+        } catch (IllegalArgumentException e) {
+            assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage());
+        }
+
         // verify direct connector as well
         try {
             proxy.addNetworkConnector(scheme + "://localhost");
@@ -114,6 +147,14 @@ protected void testTransportSchemeBridgeBlocked(String scheme) throws Exception
         } catch (IllegalArgumentException e) {
             assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage());
         }
+
+        try {
+            // verify nested composite URI is blocked when not using parens
+            proxy.addNetworkConnector("static:static:static:tcp://localhost:0," + scheme + "://localhost");
+            fail("Should have failed trying to add connector bridge with scheme: " + scheme);
+        } catch (IllegalArgumentException e) {
+            assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage());
+        }
     }
 
     @Test
@@ -124,12 +165,21 @@ public void testAddNetworkConnectorMaxComposite() throws Exception {
 
         try {
             // verify nested composite URI with more than 5 levels is blocked. This has 6 nested
-            // (not including first wrapper url
+            // (not including first wrapper url)
             proxy.addNetworkConnector(
                     "static:(static:(static:(static:(static:(static:(tcp://localhost:0))))))");
             fail("Should have failed trying to add more than 5 connector bridges");
         } catch (IllegalArgumentException e) {
             assertEquals("URI can't contain more than 5 nested composite URIs", e.getMessage());
         }
+
+        try {
+            // verify nested composite URI with more than 5 levels is blocked without parens
+            proxy.addNetworkConnector(
+                    "static:static:static:static:static:static:tcp://localhost:0");
+            fail("Should have failed trying to add more than 5 connector bridges");
+        } catch (IllegalArgumentException e) {
+            assertEquals("URI can't contain more than 5 nested composite URIs", e.getMessage());
+        }
     }
 }