From 4b13c5ce9106a8514a9ad8f2d65cf8a67407cb84 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 12:25:14 +0200 Subject: [PATCH 1/7] Extend scheme validation to inner schemes of jar: URLs Builds on #633 by recursively validating the inner URL of a jar: URL against the same scheme and host allow-lists. This deliberately changes the previous semantics: for `jar:http://host/...` to be accepted, both `jar` and `http` must appear in the allow-list, and the inner host must satisfy the host allow-list. An alternative considered was the grammar documented by [`XMLConstants`](https://docs.oracle.com/en/java/javase/25/docs/api/java.xml/javax/xml/XMLConstants.html), where tokens like `jar:file` or `jar:http` would explicitly allow specific inner schemes. That grammar is documented but not honored by the JDK reference implementation: `jdk.xml.internal.SecuritySupport.checkAccess` (verified on JDK 8, 17 and 25) strips the `jar:` prefix and matches only the inner scheme as a bare token, so a `jar:http` entry in the allow-list never matches anything. Aligning with the documented spec would have added marginal expressiveness at the cost of diverging from what JDKs actually do. --- .../ex/ConfigurationDeniedException.java | 12 +++ .../io/AbstractFileLocationStrategy.java | 68 +++++++++++---- .../io/TestAbstractFileLocationStrategy.java | 83 +++++++++++++++++++ 3 files changed, 146 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/apache/commons/configuration2/ex/ConfigurationDeniedException.java b/src/main/java/org/apache/commons/configuration2/ex/ConfigurationDeniedException.java index 69ef3ca25d..c31c27007f 100644 --- a/src/main/java/org/apache/commons/configuration2/ex/ConfigurationDeniedException.java +++ b/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); + } } diff --git a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java index 1eb5716a56..d6d9391252 100644 --- a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java +++ b/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 validSet) { + if (!validSet.isEmpty() && StringUtils.isNotEmpty(value)) { + validSet.stream().filter(p -> p.matcher(StringUtils.toRootLowerCase(value)).matches()).findFirst() + .orElseThrow(() -> new ConfigurationDeniedException(String.format("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 allow-set. + */ + private static void checkScheme(final String value, final Set 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 allow-set. + * @param validHosts the host allow-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 validSchemes, final Set 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 getSchemesProperty() { final Set set = new LinkedHashSet<>(); final String[] split = System.getProperty(KEY_SCHEMES, DEFAULT_SCHEMES).split(","); @@ -247,27 +297,11 @@ private static Set 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 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 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. * diff --git a/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java b/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java index 5ceb2e7997..00c8aa8877 100644 --- a/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java +++ b/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java @@ -17,17 +17,100 @@ 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.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 schemes(final String... values) { + return new LinkedHashSet<>(java.util.Arrays.asList(values)); + } + + private static Set hosts(final String... regexes) { + final LinkedHashSet set = new LinkedHashSet<>(); + for (final String r : regexes) { + set.add(Pattern.compile(r, Pattern.CASE_INSENSITIVE)); + } + return set; + } + + static Stream 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 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(name = "[{index}] {0}") + @MethodSource + void testCheckUrlAccepts(final URL url, final Set validSchemes, final Set validHosts) { + assertDoesNotThrow(() -> AbstractFileLocationStrategy.checkUrl(url, validSchemes, validHosts)); + } + + @ParameterizedTest(name = "[{index}] {0}") + @MethodSource + void testCheckUrlRejects(final URL url, final Set validSchemes, final Set validHosts) { + assertThrows(ConfigurationDeniedException.class, () -> AbstractFileLocationStrategy.checkUrl(url, validSchemes, validHosts)); + } + } From 8e9461b8696f3c1d06a8fc08ea7b0d892598d9d1 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 12:40:11 +0200 Subject: [PATCH 2/7] fix: use `anyMatch` --- .../configuration2/io/AbstractFileLocationStrategy.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java index d6d9391252..7262fc5126 100644 --- a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java +++ b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java @@ -205,10 +205,10 @@ 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 validSet) { - if (!validSet.isEmpty() && StringUtils.isNotEmpty(value)) { - validSet.stream().filter(p -> p.matcher(StringUtils.toRootLowerCase(value)).matches()).findFirst() - .orElseThrow(() -> new ConfigurationDeniedException(String.format("URL host is not enabled: %s; must be one of %s", value, validSet))); + private static void checkHost(String value, final Set validSet) { + final String lowerCase = StringUtils.toRootLowerCase(value); + if (!validSet.isEmpty() && StringUtils.isNotEmpty(lowerCase) && validSet.stream().anyMatch(p -> p.matcher(lowerCase).matches())) { + throw new ConfigurationDeniedException(String.format("URL host is not enabled: %s; must be one of %s", value, validSet)); } } From 5f3c9b56265b10f174fa2c39e026aae696e7237a Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 12:41:42 +0200 Subject: [PATCH 3/7] fix: useless `String.format` --- .../commons/configuration2/io/AbstractFileLocationStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java index 7262fc5126..511e4af59e 100644 --- a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java +++ b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java @@ -208,7 +208,7 @@ public T get() { private static void checkHost(String value, final Set validSet) { final String lowerCase = StringUtils.toRootLowerCase(value); if (!validSet.isEmpty() && StringUtils.isNotEmpty(lowerCase) && validSet.stream().anyMatch(p -> p.matcher(lowerCase).matches())) { - throw new ConfigurationDeniedException(String.format("URL host is not enabled: %s; must be one of %s", value, validSet)); + throw new ConfigurationDeniedException("URL host is not enabled: %s; must be one of %s", value, validSet); } } From 6671834bb1b474ceca0a979b672401d037e40f7c Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 12:45:05 +0200 Subject: [PATCH 4/7] fix: anyMatch -> noneMatch The classical inversion bug. :wink: --- .../commons/configuration2/io/AbstractFileLocationStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java index 511e4af59e..c53e0f0dbb 100644 --- a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java +++ b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java @@ -207,7 +207,7 @@ public T get() { private static void checkHost(String value, final Set validSet) { final String lowerCase = StringUtils.toRootLowerCase(value); - if (!validSet.isEmpty() && StringUtils.isNotEmpty(lowerCase) && validSet.stream().anyMatch(p -> p.matcher(lowerCase).matches())) { + 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); } } From e9fd94f27000536ee9edbef575788a78f06bdb9c Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 13:09:02 +0200 Subject: [PATCH 5/7] fix: import and formatting errors --- .../configuration2/io/TestAbstractFileLocationStrategy.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java b/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java index 00c8aa8877..2a7798a38a 100644 --- a/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java +++ b/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java @@ -21,6 +21,7 @@ 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; @@ -43,11 +44,11 @@ private static URL url(final String spec) throws Exception { // Bypasses the validation of the single-arg constructor private static URL jarUrl(final String spec) throws Exception { - return new URL("jar", null, spec); + return new URL("jar", null, spec); } private static Set schemes(final String... values) { - return new LinkedHashSet<>(java.util.Arrays.asList(values)); + return new LinkedHashSet<>(Arrays.asList(values)); } private static Set hosts(final String... regexes) { From 8c0ea9acf654f4687dd1872866f02434f81e4396 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 13:09:40 +0200 Subject: [PATCH 6/7] fix: no parameterized test `name` --- .../configuration2/io/TestAbstractFileLocationStrategy.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java b/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java index 2a7798a38a..ac915278a7 100644 --- a/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java +++ b/src/test/java/org/apache/commons/configuration2/io/TestAbstractFileLocationStrategy.java @@ -102,13 +102,13 @@ void testBuilder() { assertThrows(NullPointerException.class, () -> new AbstractFileLocationStrategy.StrategyBuilder<>(null)); } - @ParameterizedTest(name = "[{index}] {0}") + @ParameterizedTest @MethodSource void testCheckUrlAccepts(final URL url, final Set validSchemes, final Set validHosts) { assertDoesNotThrow(() -> AbstractFileLocationStrategy.checkUrl(url, validSchemes, validHosts)); } - @ParameterizedTest(name = "[{index}] {0}") + @ParameterizedTest @MethodSource void testCheckUrlRejects(final URL url, final Set validSchemes, final Set validHosts) { assertThrows(ConfigurationDeniedException.class, () -> AbstractFileLocationStrategy.checkUrl(url, validSchemes, validHosts)); From 27a7746c4aa7e1a3a15b1b9629c4e245f2be83a1 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 13:13:37 +0200 Subject: [PATCH 7/7] fix: apply review comments --- .../io/AbstractFileLocationStrategy.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java index c53e0f0dbb..5173e424ff 100644 --- a/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java +++ b/src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java @@ -205,7 +205,7 @@ public T get() { */ private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes"; - private static void checkHost(String value, final Set validSet) { + private static void checkHost(final String value, final Set 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); @@ -216,7 +216,7 @@ private static void checkHost(String value, final Set validSet) { * Checks if the scheme is allowed. * * @param value A URL scheme, never empty or {@code null}. - * @param validSet the scheme allow-set. + * @param validSet the scheme valid-set. */ private static void checkScheme(final String value, final Set validSet) { if (!validSet.isEmpty() && !validSet.contains(StringUtils.toRootLowerCase(value))) { @@ -229,8 +229,8 @@ private static void checkScheme(final String value, final Set validSet) * Validates {@code url} against the scheme and host allow-lists. * * @param url the URL to check. - * @param validSchemes the scheme allow-set. - * @param validHosts the host allow-set. + * @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 validSchemes, final Set validHosts) { @@ -247,7 +247,7 @@ static void checkUrl(final URL url, final Set validSchemes, final Set