Copilot commented on code in PR #2211: URL: https://github.com/apache/maven-resolver/pull/2211#discussion_r4329145611
########## maven-resolver-util/src/main/java/org/eclipse/aether/util/connector/transport/TransporterUtils.java: ########## @@ -0,0 +1,74 @@ +/* + * 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 org.eclipse.aether.util.connector.transport; + +import java.net.URI; + +/** + * A utility class for transporters. + * + * @since 2.0.25 + */ +public final class TransporterUtils { + private TransporterUtils() {} + + /** + * Checks whether two URIs have the same origin (scheme, host and effective port). + * + * @param origin the repository origin + * @param target the URI to compare with the origin + * @return {@code true} when both URIs have the same origin + * @since 2.0.25 + */ + public static boolean isSameOrigin(URI origin, URI target) { + if (origin.getScheme() == null + || origin.getHost() == null + || target.getScheme() == null + || target.getHost() == null) { + return false; + } + return origin.getScheme().equalsIgnoreCase(target.getScheme()) + && origin.getHost().equalsIgnoreCase(target.getHost()) + && effectivePort(origin) == effectivePort(target); + } + + /** + * Checks whether two URIs have the same authority (user, host and port). + * + * @param origin the URI to compare + * @param target the URI to compare with the origin + * @return {@code true} when both URIs have the same authority + * @since 2.0.25 + */ + public static boolean isSameAuthority(URI origin, URI target) { + String originAuthority = origin.getAuthority(); + String targetAuthority = target.getAuthority(); + return originAuthority != null && targetAuthority != null && originAuthority.equalsIgnoreCase(targetAuthority); + } Review Comment: Comparing `URI#getAuthority()` strings will incorrectly treat equivalent authorities as different when one URI omits the default port and the other spells it explicitly (e.g., `https://host/` vs `https://host:443/`). It also makes any user-info comparison case-insensitive by lowercasing the entire authority. To avoid false redirect rejections (notably in `RedirectMode.SAME_AUTHORITY`), compare components instead: host case-insensitively, ports using an “effective port” (defaulted by scheme), and user-info case-sensitively (or explicitly decide to disallow user-info). ########## maven-resolver-test-http/src/main/java/org/eclipse/aether/internal/test/util/http/HttpServer.java: ########## @@ -840,10 +840,12 @@ public boolean handle(Request req, Response response, Callback callback) throws return false; } StringBuilder location = new StringBuilder(128); - String scheme = Request.getParameters(req).getValue("scheme"); + var parameters = Request.getParameters(req); + String scheme = parameters.getValue("scheme"); + String host = parameters.getValue("host"); Review Comment: `var` requires Java 10+, which can break compilation if this module is built with Java 8/9 source/target (common for Maven Resolver). Use the explicit type returned by `Request.getParameters(req)` instead of `var` to keep the test utility compatible with the project’s supported Java baseline. ########## maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java: ########## @@ -253,16 +253,90 @@ public final class ConfigurationProperties { * a redirect - including redirects that leave the repository origin. Avoid placing credentials (for example * {@code Authorization}, cookies or private token headers) in this map where repository authentication can be * used instead: repository authentication is negotiated per host. All shipped HTTP transports scope configured - * headers to the repository origin by default, see the per-transport {@code originScopedHeaders} configuration - * keys ({@code aether.transport.apache.originScopedHeaders}, {@code aether.transport.jdk.originScopedHeaders}, - * {@code aether.transport.jetty.originScopedHeaders}). + * headers to the repository origin by default, controlled by {@link #HTTP_ORIGIN_SCOPED_HEADERS}. * * @configurationSource {@link RepositorySystemSession#getConfigProperties()} * @configurationType {@link java.util.Map} * @configurationRepoIdSuffix Yes */ public static final String HTTP_HEADERS = PREFIX_TRANSPORT_HTTP + "headers"; + public enum RedirectMode { + /** + * No redirects allowed. + */ + NONE, + /** + * Redirects only within same authority. + */ + SAME_AUTHORITY, + /** + * Any redirect is followed. + */ + ANY + } + + /** + * Optionally restricts HTTP redirects to either the same authority or not allowing any redirects at all. + * By default, any redirect is followed. + * Blocked redirects lead to exceptions being thrown by the transport. + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link ConfigurationProperties.RedirectMode} + * @configurationDefaultValue {@link #HTTP_REDIRECT_MODE} + * @configurationRepoIdSuffix Yes + * @since 2.0.25 + * @see {@link #HTTP_ORIGIN_SCOPED_HEADERS} for restricing headers passed to redirect targets + * @see {@link #HTTP_FOLLOW_INSECURE_REDIRECT} for controlling whether HTTPS-to-HTTP redirects are followed + */ + public static final String HTTP_REDIRECT_MODE = PREFIX_TRANSPORT_HTTP + "redirectMode"; Review Comment: The `@configurationDefaultValue` tag appears to reference `HTTP_REDIRECT_MODE` (the property key) rather than the actual default constant `DEFAULT_REDIRECT_MODE`. Also, the `@see` tags would be clearer as `@see #HTTP_ORIGIN_SCOPED_HEADERS` / `@see #HTTP_FOLLOW_INSECURE_REDIRECT` (without wrapping in `{@link ...}` + trailing prose) so Javadoc renders consistently. ########## maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ResolverRedirectStrategy.java: ########## @@ -29,41 +29,61 @@ import org.apache.http.client.protocol.HttpClientContext; import org.apache.http.impl.client.LaxRedirectStrategy; import org.apache.http.protocol.HttpContext; +import org.eclipse.aether.ConfigurationProperties; +import org.eclipse.aether.ConfigurationProperties.RedirectMode; +import org.eclipse.aether.util.connector.transport.TransporterUtils; /** * A redirect strategy that refuses protocol downgrades: a redirect from an {@code https} URL to an {@code http} * URL strips transport encryption from artifact and checksum bytes and makes repository credentials eligible for * transmission over plaintext, so it fails the transfer instead of being followed silently, unless explicitly - * allowed via {@link ApacheTransporterConfigurationKeys#CONFIG_PROP_FOLLOW_INSECURE_REDIRECTS}. All other - * redirects behave exactly as {@link LaxRedirectStrategy} handled them before. + * allowed via {@link ConfigurationProperties#HTTP_FOLLOW_INSECURE_REDIRECT}. Redirects are also restricted by the + * configured {@link RedirectMode}. * * @since 2.0.23 */ final class ResolverRedirectStrategy extends LaxRedirectStrategy { + private final RedirectMode redirectMode; + + private final URI repositoryUri; + private final boolean followInsecureRedirects; - ResolverRedirectStrategy(boolean followInsecureRedirects) { + ResolverRedirectStrategy(RedirectMode redirectMode, URI repositoryUri, boolean followInsecureRedirects) { + this.redirectMode = redirectMode; + this.repositoryUri = repositoryUri; this.followInsecureRedirects = followInsecureRedirects; } @Override public boolean isRedirected(HttpRequest request, HttpResponse response, HttpContext context) throws ProtocolException { boolean redirected = super.isRedirected(request, response, context); - if (redirected && !followInsecureRedirects) { + if (redirected) { Header locationHeader = response.getFirstHeader("location"); if (locationHeader != null) { URI location = createLocationURI(locationHeader.getValue()); - String locationScheme = location.getScheme(); - String currentScheme = currentScheme(request, context); - if (locationScheme != null - && "https".equalsIgnoreCase(currentScheme) - && !"https".equalsIgnoreCase(locationScheme)) { - throw new ProtocolException("Insecure redirect to '" + location - + "' not followed: protocol downgrade from https to " + locationScheme - + " would expose credentials and artifact content; set the configuration property " - + ApacheTransporterConfigurationKeys.CONFIG_PROP_FOLLOW_INSECURE_REDIRECTS - + "=true to allow it"); + URI target = repositoryUri.resolve(location); + if (redirectMode == RedirectMode.NONE) { + throw new ProtocolException( + "Refusing to follow redirect to '" + target + "': redirect mode is " + redirectMode); + } Review Comment: The error explains *what* happened but not *how* to fix it. Including the controlling property key (`ConfigurationProperties.HTTP_REDIRECT_MODE`, optionally with `.<repoId>`) would make this actionable for users diagnosing failures from logs. ########## maven-resolver-transport-url/src/main/java/org/eclipse/aether/transport/url/UrlTransporter.java: ########## @@ -159,17 +163,11 @@ public UrlTransporter( } } this.proxyAuth = proxyAuthString; - - this.redirectMode = RedirectMode.valueOf(ConfigUtils.getString( - session, - UrlTransporterConfigurationKeys.DEFAULT_REDIRECT_MODE, - UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE + "." + repository.getId(), - UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE)); - this.redirectAllowDowngrade = ConfigUtils.getBoolean( - session, - UrlTransporterConfigurationKeys.DEFAULT_REDIRECT_ALLOW_DOWNGRADE, - UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_ALLOW_DOWNGRADE + "." + repository.getId(), - UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_ALLOW_DOWNGRADE); + this.redirectMode = RedirectMode.valueOf(HttpTransporterUtils.getRedirectMode( + session, repository, UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE) + .name()); + this.redirectAllowDowngrade = HttpTransporterUtils.isFollowInsecureRedirect( + session, repository, UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_ALLOW_DOWNGRADE); Review Comment: Mapping between two different `RedirectMode` enums via `.name()` + `valueOf(...)` is brittle: it will throw at runtime if either enum grows a new value or renames one. Prefer using `ConfigurationProperties.RedirectMode` directly in `UrlTransporter`, or map explicitly via a `switch` with a well-defined default/exception message. ########## maven-resolver-util/src/main/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtils.java: ########## @@ -115,6 +115,134 @@ public static Map<String, String> getHttpHeaders(RepositorySystemSession session ConfigurationProperties.HTTP_HEADERS); } + /** + * Getter for {@link ConfigurationProperties#HTTP_REDIRECT_MODE}. + */ + public static ConfigurationProperties.RedirectMode getRedirectMode( + RepositorySystemSession session, RemoteRepository repository) { + return getRedirectMode(session, repository, null); + } Review Comment: These new public methods are part of the util API surface, but unlike the other newly added getters below, they don’t document `@since 2.0.25`. Adding `@since` improves generated API docs and helps downstream consumers track when the API became available. ########## maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/SameAuthorityRedirectGuard.java: ########## @@ -0,0 +1,41 @@ +/* + * 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 org.eclipse.aether.transport.jetty; + +import java.io.IOException; +import java.net.URI; + +import org.eclipse.aether.util.connector.transport.TransporterUtils; +import org.eclipse.jetty.client.Request; + +final class SameAuthorityRedirectGuard implements Request.QueuedListener { + private final URI repositoryUri; + + SameAuthorityRedirectGuard(URI repositoryUri) { + this.repositoryUri = repositoryUri; + } + + @Override + public void onQueued(Request request) { + URI target = request.getURI(); + if (target == null || !TransporterUtils.isSameAuthority(repositoryUri, target)) { + request.abort(new IOException("Refusing to follow redirect to different authority: " + target)); + } Review Comment: The message says “different authority” but prints the full `target` URI (or `null`) rather than showing the specific authority mismatch (e.g., expected vs actual). Consider including `repositoryUri.getAuthority()` and `target.getAuthority()` (when available) to make debugging configuration/redirect issues easier. ########## maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java: ########## @@ -253,16 +253,90 @@ public final class ConfigurationProperties { * a redirect - including redirects that leave the repository origin. Avoid placing credentials (for example * {@code Authorization}, cookies or private token headers) in this map where repository authentication can be * used instead: repository authentication is negotiated per host. All shipped HTTP transports scope configured - * headers to the repository origin by default, see the per-transport {@code originScopedHeaders} configuration - * keys ({@code aether.transport.apache.originScopedHeaders}, {@code aether.transport.jdk.originScopedHeaders}, - * {@code aether.transport.jetty.originScopedHeaders}). + * headers to the repository origin by default, controlled by {@link #HTTP_ORIGIN_SCOPED_HEADERS}. * * @configurationSource {@link RepositorySystemSession#getConfigProperties()} * @configurationType {@link java.util.Map} * @configurationRepoIdSuffix Yes */ public static final String HTTP_HEADERS = PREFIX_TRANSPORT_HTTP + "headers"; + public enum RedirectMode { + /** + * No redirects allowed. + */ + NONE, + /** + * Redirects only within same authority. + */ + SAME_AUTHORITY, + /** + * Any redirect is followed. + */ + ANY + } + + /** + * Optionally restricts HTTP redirects to either the same authority or not allowing any redirects at all. + * By default, any redirect is followed. + * Blocked redirects lead to exceptions being thrown by the transport. + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link ConfigurationProperties.RedirectMode} + * @configurationDefaultValue {@link #HTTP_REDIRECT_MODE} + * @configurationRepoIdSuffix Yes + * @since 2.0.25 + * @see {@link #HTTP_ORIGIN_SCOPED_HEADERS} for restricing headers passed to redirect targets Review Comment: Correct spelling of 'restricing' to 'restricting'. -- 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]
