Extend scheme validation to inner schemes of jar: URLs

src/main/java/org/apache/commons/configuration2/ex/ConfigurationDeniedException.java

@@ -36,4 +36,16 @@ public class ConfigurationDeniedException extends ConfigurationRuntimeException
     public ConfigurationDeniedException(final String message, final Object... args) {
         super(message, args);
     }
+
+    /**
+     * Constructs a new {@code ConfigurationDeniedException} with specified detail message using {@link String#format(String,Object...)} and cause.
+     *
+     * @param cause the cause.
+     * @param message the error message.
+     * @param args    arguments to the error message.
+     * @see String#format(String,Object...)
+     */
+    public ConfigurationDeniedException(final Throwable cause, final String message, final Object... args) {
+        super(cause, message, args);
+    }
 }

src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java

@@ -17,6 +17,7 @@
 
 package org.apache.commons.configuration2.io;
 
+import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.Collections;
 import java.util.LinkedHashSet;
@@ -204,6 +205,55 @@ public T get() {
      */
     private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
 
+    private static void checkHost(final String value, final Set<Pattern> validSet) {
+        final String lowerCase = StringUtils.toRootLowerCase(value);
+        if (!validSet.isEmpty() && StringUtils.isNotEmpty(lowerCase) && validSet.stream().noneMatch(p -> p.matcher(lowerCase).matches())) {
+            throw new ConfigurationDeniedException("URL host is not enabled: %s; must be one of %s", value, validSet);
+        }
+    }
+
+    /**
+     * Checks if the scheme is allowed.
+     *
+     * @param value A URL scheme, never empty or {@code null}.
+     * @param validSet the scheme valid-set.
+     */
+    private static void checkScheme(final String value, final Set<String> validSet) {
+        if (!validSet.isEmpty() && !validSet.contains(StringUtils.toRootLowerCase(value))) {
+            throw new ConfigurationDeniedException("URL scheme \"%s\" is not enabled, must be one of %s, override defaults with the system property \"%s\", "
+                    + "complete set: \"file,http,https,jar\"", value, validSet, KEY_SCHEMES);
+        }
+    }
+
+    /**
+     * Validates {@code url} against the scheme and host allow-lists.
+     *
+     * @param url           the URL to check.
+     * @param validSchemes  the scheme valid-set.
+     * @param validHosts    the host valid-set.
+     * @throws ConfigurationDeniedException if the URL or any embedded URL fails the check, or a {@code jar:} URL is malformed.
+     */
+    static void checkUrl(final URL url, final Set<String> validSchemes, final Set<Pattern> validHosts) {
+        String scheme = url.getProtocol();
+        checkScheme(scheme, validSchemes);
+        if ("jar".equalsIgnoreCase(scheme)) {
+            try {
+                // Follows the logic of JarURLConnection#parseSpecs without the cost of opening a connection.
+                final String spec = url.getFile();
+                final int sep = spec.lastIndexOf("!/");
+                if (sep < 0) {
+                    throw new MalformedURLException("no !/ found in url spec:" + spec);
+                }
+                final URL inner = new URL(spec.substring(0, sep));
+                checkUrl(inner, validSchemes, validHosts);
+            } catch (final MalformedURLException e) {
+                throw new ConfigurationDeniedException(e, "Malformed 'jar:' URL: %s", url);
+            }
+        } else {
+            checkHost(url.getHost(), validHosts);
+        }
+    }
+
     private static Set<String> getSchemesProperty() {
         final Set<String> set = new LinkedHashSet<>();
         final String[] split = System.getProperty(KEY_SCHEMES, DEFAULT_SCHEMES).split(",");
@@ -247,27 +297,11 @@ private static Set<String> getSchemesProperty() {
 
     URL check(final URL url) {
         if (url != null) {
-            checkScheme("scheme", url, url.getProtocol(), schemes);
-            checkHost("host", url, url.getHost(), hosts);
+            checkUrl(url, schemes, hosts);
         }
         return url;
     }
 
-    void checkHost(final String type, final URL url, final String value, final Set<Pattern> validSet) {
-        if (!validSet.isEmpty() && StringUtils.isNotEmpty(value)) {
-            hosts.stream().filter(p -> p.matcher(StringUtils.toRootLowerCase(value)).matches()).findFirst()
-                    .orElseThrow(() -> new ConfigurationDeniedException(String.format("URL %s is not enabled: %s; must be one of %s", type, value, validSet)));
-        }
-    }
-
-    void checkScheme(final String type, final URL url, final String value, final Set<String> validSet) {
-        if (!validSet.isEmpty() && value != null && !validSet.contains(StringUtils.toRootLowerCase(value))) {
-            throw new ConfigurationDeniedException(String.format(
-                    "URL %s \"%s\" is not enabled, must be one of %s, override defaults with the system property \"%s\", complete set: \"file,http,https,jar\"",
-                    type, value, validSet, KEY_SCHEMES));
-        }
-    }
-
     /**
      * Gets the enabled hosts.
      *

src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java

@@ -17,17 +17,101 @@
 
 package org.apache.commons.configuration2.io;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import java.net.URL;
+import java.util.Arrays;
+import java.util.LinkedHashSet;
+import java.util.Set;
+import java.util.regex.Pattern;
+import java.util.stream.Stream;
+
+import org.apache.commons.configuration2.ex.ConfigurationDeniedException;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 /**
  * Tests {@link AbstractFileLocationStrategy}.
  */
 public class TestAbstractFileLocationStrategy {
 
+    private static URL url(final String spec) throws Exception {
+        return new URL(spec);
+    }
+
+    // Bypasses the validation of the single-arg constructor
+    private static URL jarUrl(final String spec) throws Exception {
+        return new URL("jar", null, spec);
+    }
+
+    private static Set<String> schemes(final String... values) {
+        return new LinkedHashSet<>(Arrays.asList(values));
+    }
+
+    private static Set<Pattern> hosts(final String... regexes) {
+        final LinkedHashSet<Pattern> set = new LinkedHashSet<>();
+        for (final String r : regexes) {
+            set.add(Pattern.compile(r, Pattern.CASE_INSENSITIVE));
+        }
+        return set;
+    }
+
+    static Stream<Arguments> testCheckUrlAccepts() throws Exception {
+        return Stream.of(
+                // Empty scheme allows all.
+                Arguments.of(url("file:/tmp/x.properties"), schemes(), hosts()),
+                Arguments.of(url("https://example.com/x.properties"), schemes(), hosts()),
+                // Bare schemes that match the allow-set.
+                Arguments.of(url("file:/tmp/x.properties"), schemes("file"), hosts()),
+                Arguments.of(url("https://example.com/x.properties"), schemes("https"), hosts()),
+                // jar: unwraps to the inner scheme, which is in the allow-set.
+                Arguments.of(url("jar:file:/tmp/x.jar!/y.properties"), schemes("file", "jar"), hosts()),
+                Arguments.of(url("jar:https://example.com/x.jar!/y.properties"), schemes("https", "jar"), hosts()),
+                // Empty host allow-set means "any host".
+                Arguments.of(url("file:///tmp/x.properties"), schemes("file"), hosts()),
+                Arguments.of(url("http://anything.example/x.properties"), schemes("http"), hosts()),
+                Arguments.of(url("jar:https://anything.example/x.jar!/y.properties"), schemes("https", "jar"), hosts()),
+                // Host satisfies allow-set
+                Arguments.of(url("file:///tmp/x.properties"), schemes("file"), hosts("trusted\\.example")),
+                Arguments.of(url("https://trusted.example/x.properties"), schemes("https", "jar"), hosts("trusted\\.example")),
+                Arguments.of(url("jar:https://trusted.example/x.jar!/y.properties"), schemes("https", "jar"), hosts("trusted\\.example"))
+        );
+    }
+
+    static Stream<Arguments> testCheckUrlRejects() throws Exception {
+        return Stream.of(
+                // Plain scheme not in the allow-set.
+                Arguments.of(url("http://example.com/x.properties"), schemes("file", "jar"), hosts()),
+                // jar: is allowed but the inner scheme is not.
+                Arguments.of(url("jar:file:/tmp/x.jar!/y.properties"), schemes("jar"), hosts()),
+                Arguments.of(url("jar:https://example.com/x.jar!/y.properties"), schemes("jar"), hosts()),
+                // Invalid jar URL
+                Arguments.of(jarUrl("file:/tmp/x.properties"), schemes("file", "jar"), hosts()),
+                Arguments.of(jarUrl("invalid url!/y.properties"), schemes("file", "jar"), hosts()),
+                // Host is not allowed
+                Arguments.of(url("https://evilhost/x.properties"), schemes(), hosts("trusted\\.example")),
+                Arguments.of(url("jar:https://evilhost/x.jar!/y.properties"), schemes(), hosts("trusted\\.example"))
+        );
+    }
+
     @Test
     void testBuilder() {
         assertThrows(NullPointerException.class, () -> new AbstractFileLocationStrategy.StrategyBuilder<>(null));
     }
+
+    @ParameterizedTest
+    @MethodSource
+    void testCheckUrlAccepts(final URL url, final Set<String> validSchemes, final Set<Pattern> validHosts) {
+        assertDoesNotThrow(() -> AbstractFileLocationStrategy.checkUrl(url, validSchemes, validHosts));
+    }
+
+    @ParameterizedTest
+    @MethodSource
+    void testCheckUrlRejects(final URL url, final Set<String> validSchemes, final Set<Pattern> validHosts) {
+        assertThrows(ConfigurationDeniedException.class, () -> AbstractFileLocationStrategy.checkUrl(url, validSchemes, validHosts));
+    }
+
 }