gnodet commented on code in PR #2211: URL: https://github.com/apache/maven-resolver/pull/2211#discussion_r4335148738
########## 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: **Port normalization missing.** `URI.getAuthority()` returns the raw authority string including an explicit port. `http://host/` has authority `host` while `http://host:80/` has authority `host:80` — these compare as different even though they are the same server. `isSameOrigin` already handles this correctly via `effectivePort()`. `isSameAuthority` should do the same: ```java public static boolean isSameAuthority(URI origin, URI target) { return origin.getHost() != null && origin.getHost().equalsIgnoreCase(target.getHost()) && effectivePort(origin) == effectivePort(target); } ``` Otherwise `SAME_AUTHORITY` mode will reject redirects between `http://host/a` and `http://host:80/b` even though both resolve to the same server. ########## 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( Review Comment: **Fragile enum bridge.** `UrlTransporter` keeps its own private `RedirectMode` enum and converts from `ConfigurationProperties.RedirectMode` via `.name()` / `valueOf()`. Adding a new value to one enum without the other will throw `IllegalArgumentException` at runtime with no compile-time warning. The other two transporters (`JettyTransporter`, `JdkTransporter`) use `ConfigurationProperties.RedirectMode` directly after this PR. Consider retiring the private `UrlTransporter.RedirectMode` enum entirely and storing `ConfigurationProperties.RedirectMode` as the field type — that would remove this bridge and the `@SuppressWarnings("deprecation")` on the constructor. ########## 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( Review Comment: **Missing `@since` and `@param` Javadoc tags.** This is a new public API method but its Javadoc only has `/** Getter for ... */`. The other new methods in this file (`isOriginScopedHeaders`, `isFollowInsecureRedirect`) all carry `@since 2.0.25` and `@param` tags. Please add the same to all three `getRedirectMode` overloads for consistency. -- 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]
