You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2026/05/12 10:35:33 UTC
[PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
ppkarwasz opened a new pull request, #636:
URL: https://github.com/apache/commons-configuration/pull/636
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225921737
##########
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<String> schemes(final String... values) {
+ return new LinkedHashSet<>(java.util.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(name = "[{index}] {0}")
Review Comment:
I never used the option to run selected test cases in IDEA, but I removed it to improve testing under Eclipse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3226011468
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-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 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<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);
Review Comment:
Gross, but OK, no sense in holding up this PR for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225940999
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-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 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<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);
Review Comment:
Fixed in https://github.com/apache/commons-configuration/pull/636/commits/27a7746c4aa7e1a3a15b1b9629c4e245f2be83a1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225905799
##########
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);
Review Comment:
Nice catch! :100:
Shouldn't checkstyle catch this kind of formatting errors?
Fixed in https://github.com/apache/commons-configuration/pull/636/commits/e9fd94f27000536ee9edbef575788a78f06bdb9c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225938844
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-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 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<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);
Review Comment:
The message intentionally matches the one in `JarURLConnection`:
https://github.com/openjdk/jdk/blob/4edfc387f160dfeb8a67408b1bd98ea45f51d36c/src/java.base/share/classes/java/net/JarURLConnection.java#L167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225759026
##########
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<String> schemes(final String... values) {
+ return new LinkedHashSet<>(java.util.Arrays.asList(values));
Review Comment:
Looks like you have AI-slop left over here, don't use a FQCN for `Arrays`.
##########
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);
Review Comment:
You have 2 spaces here instead of 1.
##########
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) {
Review Comment:
Nice :)
##########
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<String> schemes(final String... values) {
+ return new LinkedHashSet<>(java.util.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(name = "[{index}] {0}")
Review Comment:
No need for `name`, here. Less clutter ;) Plus some IDEs like Eclipse let you re-run individual arg tests when you don't mess with the format.
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-set.
+ */
+ private static void checkScheme(final String value, final Set<String> validSet) {
Review Comment:
The parameter name and Javdoc don't match, pick "allow" or "valid" for both,
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(String value, final Set<Pattern> validSet) {
Review Comment:
Use `final` for `value`.
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-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 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<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);
Review Comment:
This reads weird to me, how about:
- `"Malformed 'jar:' URL: %s"`
- `"Malformed JAR URL: %s"`
- ?
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-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 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<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);
Review Comment:
- "no" -> "No", a sentence starts with a capital letter.
- "url" -> "URL", upper case acronyms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #636:
URL: https://github.com/apache/commons-configuration/pull/636
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225899659
##########
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<String> schemes(final String... values) {
+ return new LinkedHashSet<>(java.util.Arrays.asList(values));
Review Comment:
Fixed in https://github.com/apache/commons-configuration/pull/636/commits/e9fd94f27000536ee9edbef575788a78f06bdb9c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225943732
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(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 allow-set.
+ */
+ private static void checkScheme(final String value, final Set<String> validSet) {
Review Comment:
I replaced `allow-set` with `valid-set`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3225948650
##########
src/main/java/org/apache/commons/configuration2/io/AbstractFileLocationStrategy.java:
##########
@@ -204,6 +205,55 @@ public T get() {
*/
private static final String KEY_SCHEMES = "org.apache.commons.configuration2.io.FileLocationStrategy.schemes";
+ private static void checkHost(String value, final Set<Pattern> validSet) {
Review Comment:
Shouldn't this also be a checkstyle or PMD rule?
Fixed in https://github.com/apache/commons-configuration/pull/636/commits/27a7746c4aa7e1a3a15b1b9629c4e245f2be83a1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend scheme validation to inner schemes of jar: URLs [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #636:
URL: https://github.com/apache/commons-configuration/pull/636#discussion_r3226003597
##########
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);
Review Comment:
I think you can enforce "at least one space" but not "at most one space": https://checkstyle.sourceforge.io/checks/whitespace/index.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org