[
https://issues.apache.org/jira/browse/GROOVY-12266?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105846#comment-18105846
]
ASF GitHub Bot commented on GROOVY-12266:
-----------------------------------------
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
> Grape: warn on plaintext HTTP resolver roots in @GrabResolver
> -------------------------------------------------------------
>
> Key: GROOVY-12266
> URL: https://issues.apache.org/jira/browse/GROOVY-12266
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
