Copilot commented on code in PR #2803:
URL: https://github.com/apache/groovy/pull/2803#discussion_r3811486731
##########
src/main/java/groovy/grape/Grape.java:
##########
@@ -380,16 +402,147 @@ public static Map[] listDependencies(ClassLoader cl) {
/**
* Adds a resolver to the shared grape engine.
+ * <p>
+ * A resolver root using a plaintext protocol is subject to
+ * {@value #INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY}: {@code warn} (the
default) logs a
+ * warning and adds the resolver, {@code fail} rejects it, and {@code
ignore} skips the
+ * check. Roots naming a loopback host are exempt under every policy.
*
* @param args the resolver descriptor
+ * @throws RuntimeException under the {@code fail} policy, if the root is
a plaintext remote root
*/
public static void addResolver(Map<String, Object> args) {
if (enableGrapes) {
+ checkResolverRootProtocol(args);
GrapeEngine instance = getInstance();
if (instance != null) {
instance.addResolver(args);
}
}
}
+ /**
+ * Applies {@value #INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY} to a
resolver descriptor.
+ *
+ * @param args the resolver descriptor
+ * @throws RuntimeException under the {@code fail} policy, if the root is
a plaintext remote root
+ */
+ private static void checkResolverRootProtocol(Map<String, Object> args) {
+ if (args == null) {
+ return;
+ }
+ String policy = insecureProtocolPolicy();
+ if (INSECURE_PROTOCOL_POLICY_IGNORE.equals(policy)) {
+ return;
+ }
+ Object value = args.get("root");
+ if (value == null) value = args.get("value");
+ if (!(value instanceof CharSequence)) {
+ return;
+ }
+ String root = value.toString();
+ if (!isInsecureResolverRoot(root)) {
+ return;
+ }
+ Object name = args.get("name");
+ Object label = name != null ? name : root;
+ if (INSECURE_PROTOCOL_POLICY_FAIL.equals(policy)) {
+ throw new RuntimeException("Grape resolver '" + label + "' uses
the plaintext root '" + root
+ + "' and was rejected because -D" +
INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY + "="
+ + INSECURE_PROTOCOL_POLICY_FAIL + " is set. Use an https
root, or relax the policy to '"
+ + INSECURE_PROTOCOL_POLICY_WARN + "' or '" +
INSECURE_PROTOCOL_POLICY_IGNORE + "'.");
+ }
+ // Warn once per distinct root; a script may add the same resolver
repeatedly.
+ if (WARNED_INSECURE_ROOTS.add(root)) {
+ LOGGER.log(WARNING,
+ "Grape resolver ''{0}'' uses the plaintext root ''{1}'';
artifacts fetched from it can be"
+ + " read or modified in transit. Prefer https, or
set -D{2}={3} to silence this warning.",
+ label, root, INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY,
INSECURE_PROTOCOL_POLICY_IGNORE);
+ }
Review Comment:
`isInsecureResolverRoot` trims and lowercases the scheme for classification,
but the “warn once” de-duplication key uses the raw `root` string. This means
the same logical root can warn multiple times if it differs only by surrounding
whitespace or scheme casing (e.g., `'http://…'` vs `' HTTP://… '`). Consider
normalizing the key used for `WARNED_INSECURE_ROOTS` (at least `trim()`, and
ideally a URI-based normalization consistent with the classifier) to match the
documented intent.
##########
src/main/java/groovy/grape/Grape.java:
##########
@@ -21,12 +21,18 @@
import org.codehaus.groovy.tools.GrapeUtil;
import java.net.URI;
+import java.net.URISyntaxException;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import static java.lang.System.Logger.Level.DEBUG;
import static java.lang.System.Logger.Level.ERROR;
Review Comment:
`WARNING` is used in the newly added `LOGGER.log(WARNING, ...)` calls, but
it is not included in the visible static imports. This will fail compilation
unless `WARNING` is already imported elsewhere in the file. Fix by adding
`import static java.lang.System.Logger.Level.WARNING;` or by qualifying the
usage with `System.Logger.Level.WARNING`.
##########
src/main/java/groovy/grape/Grape.java:
##########
@@ -380,16 +402,147 @@ public static Map[] listDependencies(ClassLoader cl) {
/**
* Adds a resolver to the shared grape engine.
+ * <p>
+ * A resolver root using a plaintext protocol is subject to
+ * {@value #INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY}: {@code warn} (the
default) logs a
+ * warning and adds the resolver, {@code fail} rejects it, and {@code
ignore} skips the
+ * check. Roots naming a loopback host are exempt under every policy.
*
* @param args the resolver descriptor
+ * @throws RuntimeException under the {@code fail} policy, if the root is
a plaintext remote root
*/
public static void addResolver(Map<String, Object> args) {
if (enableGrapes) {
+ checkResolverRootProtocol(args);
GrapeEngine instance = getInstance();
if (instance != null) {
instance.addResolver(args);
}
}
}
+ /**
+ * Applies {@value #INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY} to a
resolver descriptor.
+ *
+ * @param args the resolver descriptor
+ * @throws RuntimeException under the {@code fail} policy, if the root is
a plaintext remote root
+ */
+ private static void checkResolverRootProtocol(Map<String, Object> args) {
+ if (args == null) {
+ return;
+ }
+ String policy = insecureProtocolPolicy();
+ if (INSECURE_PROTOCOL_POLICY_IGNORE.equals(policy)) {
+ return;
+ }
+ Object value = args.get("root");
+ if (value == null) value = args.get("value");
+ if (!(value instanceof CharSequence)) {
+ return;
+ }
+ String root = value.toString();
+ if (!isInsecureResolverRoot(root)) {
+ return;
+ }
+ Object name = args.get("name");
+ Object label = name != null ? name : root;
+ if (INSECURE_PROTOCOL_POLICY_FAIL.equals(policy)) {
+ throw new RuntimeException("Grape resolver '" + label + "' uses
the plaintext root '" + root
+ + "' and was rejected because -D" +
INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY + "="
+ + INSECURE_PROTOCOL_POLICY_FAIL + " is set. Use an https
root, or relax the policy to '"
+ + INSECURE_PROTOCOL_POLICY_WARN + "' or '" +
INSECURE_PROTOCOL_POLICY_IGNORE + "'.");
+ }
+ // Warn once per distinct root; a script may add the same resolver
repeatedly.
+ if (WARNED_INSECURE_ROOTS.add(root)) {
+ LOGGER.log(WARNING,
+ "Grape resolver ''{0}'' uses the plaintext root ''{1}'';
artifacts fetched from it can be"
+ + " read or modified in transit. Prefer https, or
set -D{2}={3} to silence this warning.",
+ label, root, INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY,
INSECURE_PROTOCOL_POLICY_IGNORE);
+ }
+ }
+
+ /**
+ * Returns the configured insecure-protocol policy, defaulting to {@code
warn}. An
+ * unrecognised value falls back to {@code warn} rather than to the laxer
{@code ignore},
+ * so that a typo cannot silently disable the check; the fallback is
reported once per
+ * offending value.
+ *
+ * @return one of {@code fail}, {@code warn} or {@code ignore}
+ */
+ static String insecureProtocolPolicy() {
+ String policy =
System.getProperty(INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY,
INSECURE_PROTOCOL_POLICY_WARN)
+ .trim().toLowerCase(Locale.ROOT);
+ if (INSECURE_PROTOCOL_POLICY_FAIL.equals(policy)
+ || INSECURE_PROTOCOL_POLICY_WARN.equals(policy)
+ || INSECURE_PROTOCOL_POLICY_IGNORE.equals(policy)) {
+ return policy;
+ }
+ if (WARNED_POLICY_VALUES.add(policy)) {
+ LOGGER.log(WARNING, "Unrecognised -D{0} value ''{1}''; using
''{2}''. Expected one of {3}, {4}, {5}.",
+ INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY, policy,
INSECURE_PROTOCOL_POLICY_WARN,
+ INSECURE_PROTOCOL_POLICY_FAIL,
INSECURE_PROTOCOL_POLICY_WARN, INSECURE_PROTOCOL_POLICY_IGNORE);
+ }
+ return INSECURE_PROTOCOL_POLICY_WARN;
+ }
+
+ /**
+ * Returns whether the given resolver root fetches over a plaintext
protocol from a host
+ * other than loopback.
+ * <p>
+ * Only schemes known to be plaintext are classified as insecure,
currently {@code http}
+ * and {@code ftp}. This is deliberately an allow-list of bad schemes
rather than a
+ * deny-list of good ones: transports such as {@code s3} and {@code gs}
are encrypted in
+ * practice and would otherwise be reported falsely. The consequence is
that an exotic
+ * plaintext scheme is not reported, so {@code fail} means "reject
known-plaintext roots",
+ * not "reject anything not proven safe".
+ * <p>
+ * {@code file:} roots are never insecure. They cross no network, and a
{@code file:} root
+ * on a network mount cannot be distinguished from a local one by
inspecting the URI.
+ * Integrity for such repositories is the job of checksum verification,
which applies to
+ * every transport rather than only to remote ones. Roots which are not
valid URIs, or
+ * which name no scheme at all, are likewise left to the engine.
+ *
+ * @param root the resolver root
+ * @return true if the root is a plaintext remote root
+ */
+ static boolean isInsecureResolverRoot(String root) {
+ if (root == null) {
+ return false;
+ }
+ String scheme;
+ String host;
+ try {
+ URI uri = new URI(root.trim());
+ scheme = uri.getScheme();
+ host = uri.getHost();
+ } catch (URISyntaxException e) {
+ return false; // not a URI we can reason about; leave it to the
engine
+ }
+ if (scheme == null) {
+ return false;
+ }
+ scheme = scheme.toLowerCase(Locale.ROOT);
+ if (!"http".equals(scheme) && !"ftp".equals(scheme)) {
+ return false;
+ }
+ return !isLoopbackHost(host);
+ }
+
+ private static boolean isLoopbackHost(String host) {
+ if (host == null) {
+ return false;
+ }
+ String name = host.toLowerCase(Locale.ROOT);
+ if (name.startsWith("[") && name.endsWith("]")) { // IPv6 literal
+ name = name.substring(1, name.length() - 1);
+ }
+ if ("localhost".equals(name) || "::1".equals(name)) {
+ return true;
+ }
+ // 127.0.0.0/8, matched as a dotted quad so that a host merely
beginning with "127."
+ // (such as 127.example.com) is not mistaken for a loopback address.
+ Matcher ipv4 = IPV4_LITERAL.matcher(name);
+ return ipv4.matches() && "127".equals(ipv4.group(1));
+ }
+
Review Comment:
The loopback exemption treats any dotted-quad matching the regex as an IPv4
literal and exempts it when the first octet is `127`, even if the address is
not a valid IPv4 literal (e.g., `127.999.999.999`). This can incorrectly
suppress warnings for invalid-but-dotted hosts. Consider validating all octets
are within `0..255` (e.g., via stricter parsing) before applying the 127/8
exemption.
##########
src/test/groovy/groovy/grape/GrapeInsecureResolverRootTest.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.grape
+
+import groovy.transform.CompileStatic
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Tests the classification behind the warning Grape logs for plaintext
resolver roots.
+ * The classification is shared by both engines because every documented route
to adding a
+ * resolver -- {@code @GrabResolver}, the {@code grape} command line tool and
+ * {@link Grape#addResolver(java.util.Map)} -- passes through the same facade
method.
+ */
+@CompileStatic
+final class GrapeInsecureResolverRootTest {
+
+ @Test
+ void testPlaintextRemoteRootsAreInsecure() {
+ assert Grape.isInsecureResolverRoot('http://repo.corp.example/maven2')
+ assert Grape.isInsecureResolverRoot('ftp://repo.corp.example/maven2')
+ }
+
+ @Test
+ void testSchemeComparisonIgnoresCase() {
+ assert Grape.isInsecureResolverRoot('HTTP://repo.corp.example/maven2')
+ assert
!Grape.isInsecureResolverRoot('HTTPS://repo.corp.example/maven2')
+ }
+
+ @Test
+ void testSurroundingWhitespaceIsIgnored() {
+ assert Grape.isInsecureResolverRoot(' http://repo.corp.example/maven2
')
+ }
+
+ @Test
+ void testEncryptedRootsAreNotInsecure() {
+ assert
!Grape.isInsecureResolverRoot('https://repo.maven.apache.org/maven2/')
+ }
+
+ @Test
+ void testLocalRootsAreNotInsecure() {
+ // file: roots never cross a network, so the warning would be noise.
+ assert !Grape.isInsecureResolverRoot('file:/home/dev/repo')
+ assert !Grape.isInsecureResolverRoot(new
File('build').toURI().toString())
+ }
+
+ @Test
+ void testLoopbackRootsAreExempt() {
+ // A local mirror or proxy over plaintext is not exposed in transit.
+ assert
!Grape.isInsecureResolverRoot('http://localhost:8081/repository/maven-public')
+ assert
!Grape.isInsecureResolverRoot('http://LocalHost:8081/repository/maven-public')
+ assert !Grape.isInsecureResolverRoot('http://127.0.0.1:8081/repo')
+ assert !Grape.isInsecureResolverRoot('http://127.1.2.3/repo')
+ assert !Grape.isInsecureResolverRoot('http://[::1]:8081/repo')
+ }
+
+ @Test
+ void testNonLoopbackLookalikesAreStillInsecure() {
+ // Guard the prefix test against hosts that merely start with the same
text.
+ assert Grape.isInsecureResolverRoot('http://127.evil.example/repo')
+ assert
Grape.isInsecureResolverRoot('http://localhost.evil.example/repo')
+ }
+
+ @Test
+ void testUnusableRootsAreLeftToTheEngine() {
+ assert !Grape.isInsecureResolverRoot(null)
+ assert !Grape.isInsecureResolverRoot('')
+ assert !Grape.isInsecureResolverRoot('not a uri at all')
+ assert !Grape.isInsecureResolverRoot('repo.corp.example/maven2') // no
scheme
+ }
+
+ @Test
+ void testUnknownSchemesAreNotReported() {
+ // Deliberate: an allow-list of known-plaintext schemes, so encrypted
transports such
+ // as s3 and gs are not reported falsely. See the
isInsecureResolverRoot javadoc.
+ assert !Grape.isInsecureResolverRoot('s3://corp-artifacts/maven2')
+ assert !Grape.isInsecureResolverRoot('gs://corp-artifacts/maven2')
+ }
+
+ // --- policy selection ---
+
+ @Test
+ void testPolicyDefaultsToWarn() {
+ withPolicy(null) {
+ assert Grape.insecureProtocolPolicy() == 'warn'
+ }
+ }
+
+ @Test
+ void testPolicyValuesAreRecognised() {
+ withPolicy('fail') { assert Grape.insecureProtocolPolicy() == 'fail' }
+ withPolicy('warn') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ withPolicy('ignore') { assert Grape.insecureProtocolPolicy() ==
'ignore' }
+ }
+
+ @Test
+ void testPolicyIsCaseInsensitiveAndTrimmed() {
+ withPolicy(' FAIL ') { assert Grape.insecureProtocolPolicy() ==
'fail' }
+ }
+
+ @Test
+ void testUnrecognisedPolicyFallsBackToWarnNotIgnore() {
+ // A typo must not silently disable the check, so the fallback is the
stricter of the
+ // two non-failing policies.
+ withPolicy('flase') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ withPolicy('true') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ }
+
+ // --- policy application ---
+
+ @Test
+ void testFailPolicyRejectsPlaintextRoot() {
+ withPolicy('fail') {
+ def ex = shouldFail(RuntimeException) {
+ Grape.addResolver([name: 'corp', root:
'http://repo.corp.example/maven2'] as Map<String, Object>)
+ }
Review Comment:
These tests call `Grape.addResolver`, which mutates a shared/static resolver
configuration for the JVM. This can leak state into other tests in the same run
and cause order-dependent failures. Prefer testing the policy enforcement
without mutating the global engine (e.g., by making `checkResolverRootProtocol`
package-private for test access, or by adding a supported cleanup/reset step in
the test fixture and restoring prior resolver state after each test).
##########
src/test/groovy/groovy/grape/GrapeInsecureResolverRootTest.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.grape
+
+import groovy.transform.CompileStatic
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Tests the classification behind the warning Grape logs for plaintext
resolver roots.
+ * The classification is shared by both engines because every documented route
to adding a
+ * resolver -- {@code @GrabResolver}, the {@code grape} command line tool and
+ * {@link Grape#addResolver(java.util.Map)} -- passes through the same facade
method.
+ */
+@CompileStatic
+final class GrapeInsecureResolverRootTest {
+
+ @Test
+ void testPlaintextRemoteRootsAreInsecure() {
+ assert Grape.isInsecureResolverRoot('http://repo.corp.example/maven2')
+ assert Grape.isInsecureResolverRoot('ftp://repo.corp.example/maven2')
+ }
+
+ @Test
+ void testSchemeComparisonIgnoresCase() {
+ assert Grape.isInsecureResolverRoot('HTTP://repo.corp.example/maven2')
+ assert
!Grape.isInsecureResolverRoot('HTTPS://repo.corp.example/maven2')
+ }
+
+ @Test
+ void testSurroundingWhitespaceIsIgnored() {
+ assert Grape.isInsecureResolverRoot(' http://repo.corp.example/maven2
')
+ }
+
+ @Test
+ void testEncryptedRootsAreNotInsecure() {
+ assert
!Grape.isInsecureResolverRoot('https://repo.maven.apache.org/maven2/')
+ }
+
+ @Test
+ void testLocalRootsAreNotInsecure() {
+ // file: roots never cross a network, so the warning would be noise.
+ assert !Grape.isInsecureResolverRoot('file:/home/dev/repo')
+ assert !Grape.isInsecureResolverRoot(new
File('build').toURI().toString())
+ }
+
+ @Test
+ void testLoopbackRootsAreExempt() {
+ // A local mirror or proxy over plaintext is not exposed in transit.
+ assert
!Grape.isInsecureResolverRoot('http://localhost:8081/repository/maven-public')
+ assert
!Grape.isInsecureResolverRoot('http://LocalHost:8081/repository/maven-public')
+ assert !Grape.isInsecureResolverRoot('http://127.0.0.1:8081/repo')
+ assert !Grape.isInsecureResolverRoot('http://127.1.2.3/repo')
+ assert !Grape.isInsecureResolverRoot('http://[::1]:8081/repo')
+ }
+
+ @Test
+ void testNonLoopbackLookalikesAreStillInsecure() {
+ // Guard the prefix test against hosts that merely start with the same
text.
+ assert Grape.isInsecureResolverRoot('http://127.evil.example/repo')
+ assert
Grape.isInsecureResolverRoot('http://localhost.evil.example/repo')
+ }
+
+ @Test
+ void testUnusableRootsAreLeftToTheEngine() {
+ assert !Grape.isInsecureResolverRoot(null)
+ assert !Grape.isInsecureResolverRoot('')
+ assert !Grape.isInsecureResolverRoot('not a uri at all')
+ assert !Grape.isInsecureResolverRoot('repo.corp.example/maven2') // no
scheme
+ }
+
+ @Test
+ void testUnknownSchemesAreNotReported() {
+ // Deliberate: an allow-list of known-plaintext schemes, so encrypted
transports such
+ // as s3 and gs are not reported falsely. See the
isInsecureResolverRoot javadoc.
+ assert !Grape.isInsecureResolverRoot('s3://corp-artifacts/maven2')
+ assert !Grape.isInsecureResolverRoot('gs://corp-artifacts/maven2')
+ }
+
+ // --- policy selection ---
+
+ @Test
+ void testPolicyDefaultsToWarn() {
+ withPolicy(null) {
+ assert Grape.insecureProtocolPolicy() == 'warn'
+ }
+ }
+
+ @Test
+ void testPolicyValuesAreRecognised() {
+ withPolicy('fail') { assert Grape.insecureProtocolPolicy() == 'fail' }
+ withPolicy('warn') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ withPolicy('ignore') { assert Grape.insecureProtocolPolicy() ==
'ignore' }
+ }
+
+ @Test
+ void testPolicyIsCaseInsensitiveAndTrimmed() {
+ withPolicy(' FAIL ') { assert Grape.insecureProtocolPolicy() ==
'fail' }
+ }
+
+ @Test
+ void testUnrecognisedPolicyFallsBackToWarnNotIgnore() {
+ // A typo must not silently disable the check, so the fallback is the
stricter of the
+ // two non-failing policies.
+ withPolicy('flase') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ withPolicy('true') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ }
+
+ // --- policy application ---
+
+ @Test
+ void testFailPolicyRejectsPlaintextRoot() {
+ withPolicy('fail') {
+ def ex = shouldFail(RuntimeException) {
+ Grape.addResolver([name: 'corp', root:
'http://repo.corp.example/maven2'] as Map<String, Object>)
+ }
+ assert ex.message.contains('plaintext root')
+ assert
ex.message.contains(Grape.INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY)
+ }
+ }
+
+ @Test
+ void testFailPolicyAllowsSecureLoopbackAndFileRoots() {
+ withPolicy('fail') {
+ Grape.addResolver([name: 'secure', root:
'https://repo.corp.example/maven2'] as Map<String, Object>)
+ Grape.addResolver([name: 'local', root:
'http://localhost:8081/repo'] as Map<String, Object>)
+ Grape.addResolver([name: 'onDisk', root: 'file:/home/dev/repo'] as
Map<String, Object>)
+ }
+ }
+
+ @Test
+ void testIgnorePolicyAcceptsPlaintextRoot() {
+ withPolicy('ignore') {
+ Grape.addResolver([name: 'corp', root:
'http://repo.corp.example/maven2'] as Map<String, Object>)
+ }
Review Comment:
These tests call `Grape.addResolver`, which mutates a shared/static resolver
configuration for the JVM. This can leak state into other tests in the same run
and cause order-dependent failures. Prefer testing the policy enforcement
without mutating the global engine (e.g., by making `checkResolverRootProtocol`
package-private for test access, or by adding a supported cleanup/reset step in
the test fixture and restoring prior resolver state after each test).
##########
src/test/groovy/groovy/grape/GrapeInsecureResolverRootTest.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.grape
+
+import groovy.transform.CompileStatic
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Tests the classification behind the warning Grape logs for plaintext
resolver roots.
+ * The classification is shared by both engines because every documented route
to adding a
+ * resolver -- {@code @GrabResolver}, the {@code grape} command line tool and
+ * {@link Grape#addResolver(java.util.Map)} -- passes through the same facade
method.
+ */
+@CompileStatic
+final class GrapeInsecureResolverRootTest {
+
+ @Test
+ void testPlaintextRemoteRootsAreInsecure() {
+ assert Grape.isInsecureResolverRoot('http://repo.corp.example/maven2')
+ assert Grape.isInsecureResolverRoot('ftp://repo.corp.example/maven2')
+ }
+
+ @Test
+ void testSchemeComparisonIgnoresCase() {
+ assert Grape.isInsecureResolverRoot('HTTP://repo.corp.example/maven2')
+ assert
!Grape.isInsecureResolverRoot('HTTPS://repo.corp.example/maven2')
+ }
+
+ @Test
+ void testSurroundingWhitespaceIsIgnored() {
+ assert Grape.isInsecureResolverRoot(' http://repo.corp.example/maven2
')
+ }
+
+ @Test
+ void testEncryptedRootsAreNotInsecure() {
+ assert
!Grape.isInsecureResolverRoot('https://repo.maven.apache.org/maven2/')
+ }
+
+ @Test
+ void testLocalRootsAreNotInsecure() {
+ // file: roots never cross a network, so the warning would be noise.
+ assert !Grape.isInsecureResolverRoot('file:/home/dev/repo')
+ assert !Grape.isInsecureResolverRoot(new
File('build').toURI().toString())
+ }
+
+ @Test
+ void testLoopbackRootsAreExempt() {
+ // A local mirror or proxy over plaintext is not exposed in transit.
+ assert
!Grape.isInsecureResolverRoot('http://localhost:8081/repository/maven-public')
+ assert
!Grape.isInsecureResolverRoot('http://LocalHost:8081/repository/maven-public')
+ assert !Grape.isInsecureResolverRoot('http://127.0.0.1:8081/repo')
+ assert !Grape.isInsecureResolverRoot('http://127.1.2.3/repo')
+ assert !Grape.isInsecureResolverRoot('http://[::1]:8081/repo')
+ }
+
+ @Test
+ void testNonLoopbackLookalikesAreStillInsecure() {
+ // Guard the prefix test against hosts that merely start with the same
text.
+ assert Grape.isInsecureResolverRoot('http://127.evil.example/repo')
+ assert
Grape.isInsecureResolverRoot('http://localhost.evil.example/repo')
+ }
+
+ @Test
+ void testUnusableRootsAreLeftToTheEngine() {
+ assert !Grape.isInsecureResolverRoot(null)
+ assert !Grape.isInsecureResolverRoot('')
+ assert !Grape.isInsecureResolverRoot('not a uri at all')
+ assert !Grape.isInsecureResolverRoot('repo.corp.example/maven2') // no
scheme
+ }
+
+ @Test
+ void testUnknownSchemesAreNotReported() {
+ // Deliberate: an allow-list of known-plaintext schemes, so encrypted
transports such
+ // as s3 and gs are not reported falsely. See the
isInsecureResolverRoot javadoc.
+ assert !Grape.isInsecureResolverRoot('s3://corp-artifacts/maven2')
+ assert !Grape.isInsecureResolverRoot('gs://corp-artifacts/maven2')
+ }
+
+ // --- policy selection ---
+
+ @Test
+ void testPolicyDefaultsToWarn() {
+ withPolicy(null) {
+ assert Grape.insecureProtocolPolicy() == 'warn'
+ }
+ }
+
+ @Test
+ void testPolicyValuesAreRecognised() {
+ withPolicy('fail') { assert Grape.insecureProtocolPolicy() == 'fail' }
+ withPolicy('warn') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ withPolicy('ignore') { assert Grape.insecureProtocolPolicy() ==
'ignore' }
+ }
+
+ @Test
+ void testPolicyIsCaseInsensitiveAndTrimmed() {
+ withPolicy(' FAIL ') { assert Grape.insecureProtocolPolicy() ==
'fail' }
+ }
+
+ @Test
+ void testUnrecognisedPolicyFallsBackToWarnNotIgnore() {
+ // A typo must not silently disable the check, so the fallback is the
stricter of the
+ // two non-failing policies.
+ withPolicy('flase') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ withPolicy('true') { assert Grape.insecureProtocolPolicy() == 'warn' }
+ }
+
+ // --- policy application ---
+
+ @Test
+ void testFailPolicyRejectsPlaintextRoot() {
+ withPolicy('fail') {
+ def ex = shouldFail(RuntimeException) {
+ Grape.addResolver([name: 'corp', root:
'http://repo.corp.example/maven2'] as Map<String, Object>)
+ }
+ assert ex.message.contains('plaintext root')
+ assert
ex.message.contains(Grape.INSECURE_PROTOCOL_POLICY_SYSTEM_PROPERTY)
+ }
+ }
+
+ @Test
+ void testFailPolicyAllowsSecureLoopbackAndFileRoots() {
+ withPolicy('fail') {
+ Grape.addResolver([name: 'secure', root:
'https://repo.corp.example/maven2'] as Map<String, Object>)
+ Grape.addResolver([name: 'local', root:
'http://localhost:8081/repo'] as Map<String, Object>)
+ Grape.addResolver([name: 'onDisk', root: 'file:/home/dev/repo'] as
Map<String, Object>)
+ }
Review Comment:
These tests call `Grape.addResolver`, which mutates a shared/static resolver
configuration for the JVM. This can leak state into other tests in the same run
and cause order-dependent failures. Prefer testing the policy enforcement
without mutating the global engine (e.g., by making `checkResolverRootProtocol`
package-private for test access, or by adding a supported cleanup/reset step in
the test fixture and restoring prior resolver state after each test).
--
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]
