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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
