Feat: new (limited) transport#1966
Conversation
Many applications outside of Maven integrate Resolver and their use case is almost always "consume" (use resolver to resolve). Also, many of those applications are still Java 8 level, hence, the JDK transport for them is no-go, on the other hand, there is no lightweigth replacement for them, except to use some "heavyweight" transporter. Transitive hull sizes of several existing transports: * jdk - 1.5MB / Java 11+ * wagon - 1.9MB (without any provider; unusable like this) / Java 8 * apache - 2.4MB / Java 8 * jetty - 10.9MB / Java 11 * minio - 23.5MB / Java 8 The new transport has no dependencies and small size: * url - 870 KB
There was a problem hiding this comment.
Pull request overview
This PR introduces a new lightweight, dependency-free “url” transport intended for Java 8 consumption-only Resolver integrations (HTTP(S) GET/HEAD with basic auth, proxy, redirects, checksums), and wires it into the multi-module build.
Changes:
- Adds new
maven-resolver-transport-urlmodule (factory +HttpURLConnection-based transporter) plus documentation and tests. - Updates the root reactor
pom.xmlmodule list to include the new transport module. - Adds module-level site descriptor and README describing limitations and supported features.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Adds maven-resolver-transport-url to the reactor module list (and reorders transports). |
| maven-resolver-transport-url/pom.xml | New module POM for the URL transport and its tests. |
| maven-resolver-transport-url/src/main/java/org/eclipse/aether/transport/url/UrlTransporterFactory.java | Factory registering the new url transporter for http/https repositories. |
| maven-resolver-transport-url/src/main/java/org/eclipse/aether/transport/url/UrlTransporter.java | Core read-only HTTP transporter implementation using HttpURLConnection. |
| maven-resolver-transport-url/src/test/java/org/eclipse/aether/transport/url/UrlTransporterTest.java | Test suite integration via HttpTransporterTest with capability-based disables. |
| maven-resolver-transport-url/src/site/site.xml | New module site descriptor. |
| maven-resolver-transport-url/README.md | New module README documenting purpose, features, and limitations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| con.setRequestMethod(method); | ||
| con.setUseCaches(false); | ||
| con.setInstanceFollowRedirects(false); | ||
| con.setRequestProperty(HttpConstants.ACCEPT_ENCODING, "gzip"); |
| if (currAuth != null) { | ||
| con.setRequestProperty(HEADER_AUTHORIZATION, basicAuthorization(auth)); | ||
| } |
| } else if (responseCode == HttpURLConnection.HTTP_MOVED_PERM | ||
| || responseCode == HttpURLConnection.HTTP_MOVED_TEMP) { | ||
| target.add(0, URI.create(con.getHeaderField(HEADER_LOCATION))); | ||
| return perform(method, target, currAuth, currProxyAuth, task); |
| <site xmlns="http://maven.apache.org/SITE/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/SITE/2.0.0 https://maven.apache.org/xsd/site-2.0.0.xsd" name="Transport Apache"> | ||
| <body> |
| This is special Transport with limited HTTP capabilities. The main use case of this transport is outside of | ||
| Maven, but in case of other apps that are Java 8, and integrate Resolver mostly for **consumption purposes**. | ||
| Before this transport, the only option was to either up Java level to 11 and use JDK transport, or to use | ||
| the heavyweight Apache HttpClient transport, which is not always desirable. |
| This transport is not a fully functional transport, and should be handled a such. It is quite usable in | ||
| artifact consumption scenarios, but it is not suitable for artifact deployment. No newline at end of file |
|
|
||
| /** | ||
| * A special, "read only" and limited capability transport usable for bootstrapping. It provides HTTP with minimal | ||
| * support (only basic auth, only GET/HEAD). |
There was a problem hiding this comment.
Please mention which HTTP library is used here and in the module description
| @@ -0,0 +1,19 @@ | |||
| # Maven Resolver URL Transport | |||
There was a problem hiding this comment.
Shouldn't this rather be added to https://github.com/apache/maven-resolver/blob/master/src/site/markdown/transporter-known-issues.md?
| # Maven Resolver URL Transport | ||
|
|
||
| This is special Transport with limited HTTP capabilities. The main use case of this transport is outside of | ||
| Maven, but in case of other apps that are Java 8, and integrate Resolver mostly for **consumption purposes**. |
There was a problem hiding this comment.
| Maven, but in case of other apps that are Java 8, and integrate Resolver mostly for **consumption purposes**. | |
| Maven, but in case of other apps that are Java 8, and integrate Resolver only for **consumption purposes**. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| private static String basicAuthorization(String credentials) { | ||
| return AUTH_SCHEME_BASIC + " " | ||
| + Base64.getEncoder().encodeToString(credentials.getBytes(StandardCharsets.UTF_8)); | ||
| } |
| } else if (responseCode == HttpURLConnection.HTTP_MOVED_PERM | ||
| || responseCode == HttpURLConnection.HTTP_MOVED_TEMP) { | ||
| String location = con.getHeaderField(HEADER_LOCATION); | ||
| if (location == null) { | ||
| throw new IOException("Redirect response missing Location header"); | ||
| } | ||
| URI redirectUri = URI.create(con.getURL().toString()).resolve(location); | ||
| String scheme = redirectUri.getScheme(); | ||
| if (!"http".equalsIgnoreCase(scheme) && !"https".equalsIgnoreCase(scheme)) { | ||
| throw new IOException("Unsupported redirect protocol: " + scheme); | ||
| } | ||
| target.add(0, redirectUri); | ||
| return perform(method, target, currAuth, currProxyAuth, task); | ||
| } else if (currAuth == null && this.auth != null && responseCode == HttpURLConnection.HTTP_UNAUTHORIZED) { | ||
| return perform(method, target, this.auth, currProxyAuth, task); | ||
| } else if (currProxyAuth == null | ||
| && this.proxyAuth != null | ||
| && responseCode == HttpURLConnection.HTTP_PROXY_AUTH) { | ||
| return perform(method, target, currAuth, this.proxyAuth, task); | ||
| } |
| protected void implPeek(PeekTask task) throws Exception { | ||
| HttpURLConnection con = perform(METHOD_HEAD, baseUri.resolve(task.getLocation()), null); | ||
| if (HttpURLConnection.HTTP_OK != con.getResponseCode()) { | ||
| throw new HttpTransporterException(con.getResponseCode()); | ||
| } | ||
| } |
| HttpURLConnection con = perform(METHOD_GET, baseUri.resolve(task.getLocation()), task); | ||
| if (HttpURLConnection.HTTP_OK != con.getResponseCode()) { | ||
| throw new HttpTransporterException(con.getResponseCode()); | ||
| } |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Initial review of PR #1966 — new URL transport
Clean design for a lightweight transport that fills a real gap for Java 8+ consumers. The negative priority (-1.0f) correctly ensures it won't be auto-selected over more capable transports, and the test class properly disables unsupported scenarios. Good reuse of HttpTransporterUtils and HttpTransporterTest.
However, the manual redirect implementation has several issues compared to what HttpClient.Redirect.NORMAL provides for free in the JDK transport:
Findings
🔴 Connection resource leak in perform() recursion (high)
When a redirect (301/302) or auth retry (401/407) triggers the recursive return perform(...) call, the current HttpURLConnection is abandoned without calling con.disconnect(). Since the response body is not consumed on these status codes, the connection cannot be returned to the JDK connection pool. Up to MAX_REDIRECTS (5) plus 2 auth-retry connections can leak sockets per request. Each recursive branch should call con.disconnect() before the recursive return.
The same issue applies in implPeek which never disconnects.
🟡 Credentials forwarded on cross-origin redirects (medium, security)
When following a redirect to a different host, currAuth and currProxyAuth are passed unchanged to the recursive call, causing the Authorization header to be sent to the redirect target. Per RFC 7235, credentials should be stripped when the redirect target has a different authority (host:port). The JDK transport uses HttpClient.Redirect.NORMAL which handles this automatically.
🟡 HTTPS-to-HTTP redirect downgrade allowed (medium, security)
The scheme validation checks that the redirect uses HTTP or HTTPS, but does not reject a redirect from HTTPS to HTTP. The JDK transport's Redirect.NORMAL explicitly prevents this. Combined with the credential forwarding issue above, this could cause credentials to be transmitted in plaintext.
🟡 Missing 307/308 redirect codes (medium)
Only 301 (HTTP_MOVED_PERM) and 302 (HTTP_MOVED_TEMP) are handled. 307 (Temporary Redirect) and 308 (Permanent Redirect) are commonly used in practice. Both preserve the HTTP method, which is safe for this read-only (GET/HEAD) transport.
Suggested fix for redirect handling
All four redirect/security issues can be addressed together in perform():
} else if (responseCode == HttpURLConnection.HTTP_MOVED_PERM
|| responseCode == HttpURLConnection.HTTP_MOVED_TEMP
|| responseCode == 307
|| responseCode == 308) {
String location = con.getHeaderField(HEADER_LOCATION);
if (location == null) {
throw new IOException("Redirect response missing Location header");
}
URI redirectUri = URI.create(con.getURL().toString()).resolve(location);
String scheme = redirectUri.getScheme();
if (!"http".equalsIgnoreCase(scheme) && !"https".equalsIgnoreCase(scheme)) {
throw new IOException("Unsupported redirect protocol: " + scheme);
}
// Strip credentials on cross-origin redirect or protocol downgrade
String origAuthority = con.getURL().getAuthority();
boolean crossOrigin = !redirectUri.getAuthority().equals(origAuthority);
boolean downgrade = "https".equalsIgnoreCase(con.getURL().getProtocol())
&& "http".equalsIgnoreCase(scheme);
con.disconnect();
target.add(0, redirectUri);
return perform(method, target,
(crossOrigin || downgrade) ? null : currAuth,
currProxyAuth, task);
}Similarly, add con.disconnect() before each auth-retry recursive call.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| final Path dataFile = task.getDataPath(); | ||
| if (dataFile == null) { | ||
| try (InputStream is = inputStreamSupplier.get()) { | ||
| utilGet(task, is, true, con.getContentLengthLong(), false); | ||
| } | ||
| } else { | ||
| try (PathProcessor.CollocatedTempFile tempFile = pathProcessor.newTempFile(dataFile)) { | ||
| task.setDataPath(tempFile.getPath(), false); | ||
| try (InputStream is = inputStreamSupplier.get()) { | ||
| utilGet(task, is, true, con.getContentLengthLong(), false); | ||
| } | ||
| tempFile.move(); | ||
| } finally { | ||
| task.setDataPath(dataFile); | ||
| } | ||
| } | ||
| if (task.getDataPath() != null) { | ||
| long lastModified = con.getLastModified(); | ||
| if (lastModified != 0) { | ||
| pathProcessor.setLastModified(task.getDataPath(), lastModified); | ||
| } | ||
| } | ||
| } |
| if (target.size() > MAX_REDIRECTS) { | ||
| throw new IOException("Too many redirects"); | ||
| } |
| } else if (responseCode == HttpURLConnection.HTTP_MOVED_PERM | ||
| || responseCode == HttpURLConnection.HTTP_MOVED_TEMP | ||
| || responseCode == HTTP_STATUS_TEMPORARY_REDIRECT | ||
| || responseCode == HTTP_STATUS_PERMANENT_REDIRECT) { |
| * HTTP gzip and deflate compression support | ||
| * HTTP Basic authentication (w/ preemptive support) | ||
| * HTTP proxy support (w/ Basic proxy authentication) | ||
| * HTTP auth caching (lowers "known to needed" HTTP round-trips) |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review #2 — commits 0898a9e5, 8e716484, 79cb996a
All four findings from the initial review have been addressed:
✅ Connection resource leak: con.disconnect() is now called before every recursive perform() call (redirect, auth retry, proxy auth retry) and before throwing exceptions on error paths. implPeek uses try/finally for cleanup, and implGet disconnects on non-200. Clean.
✅ Cross-origin credential forwarding: Authority comparison (currentAuthority.equalsIgnoreCase(redirectAuthority)) correctly strips currAuth on cross-origin redirects. Keeping currProxyAuth intact is correct — proxy auth is for the proxy, not the target.
✅ HTTPS→HTTP downgrade prevention: Explicit check before the general scheme validation. Clear error message.
✅ 307/308 redirects: Named constants (HTTP_STATUS_TEMPORARY_REDIRECT, HTTP_STATUS_PERMANENT_REDIRECT) and added to the redirect condition. Good.
The status code reshuffling (response code check first in the else if conditions) improves readability.
Looks good to me. 👍
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review #3 — commits b754bdf9, 744e55af, eabbb7af (configurable redirects & cleanup)
Excellent evolution — you've taken the security feedback and built a proper configurable redirect system. This is the right approach for a library: default to the secure option while giving users control.
What's new
✅ RedirectMode enum (NONE / SAME_AUTHORITY / ANY) — users can choose their security/convenience tradeoff. Default ANY with auth stripping on cross-origin is a pragmatic default.
✅ redirectAllowDowngrade — HTTPS→HTTP downgrade is now configurable (default: false = secure). Users who need it can opt in per repository.
✅ maxRedirects — configurable (default: 5). The target.size() - 1 accounting is correct.
✅ Per-repository configuration via .<repoId> suffix — standard Maven Resolver pattern, nicely documented.
✅ implGet refactored — entire body wrapped in try/finally with con.disconnect(). Much cleaner than the previous per-path disconnect calls. implPeek similarly improved.
✅ 303 See Other added to redirect codes — correct for a read-only (GET/HEAD) transport.
✅ UrlTransporterConfigurationKeys — clean separation of config constants with proper Javadoc annotations.
Minor observation (not blocking)
RedirectMode.valueOf(...) will throw IllegalArgumentException if a user misconfigures the value (e.g. typo like "ANYY"). This is reasonable fail-fast behavior, consistent with how Maven Resolver handles other configs. Could optionally log a warning and fall back to the default, but not necessary.
Looks good to me. 👍
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| URI currentUri = URI.create(con.getURL().toString()); | ||
| URI redirectUri = currentUri.resolve(location); |
|
|
||
| @Override | ||
| protected void implPut(PutTask task) throws Exception { | ||
| throw new IOException("unsupported operation"); |
| This transport is limited functionality transport, usable only in **consumer use cases** (resolving artifacts). | ||
| It is implemented using `java.net.HttpURLConnection` class. |
| this.redirectMode = RedirectMode.valueOf(ConfigUtils.getString( | ||
| session, | ||
| UrlTransporterConfigurationKeys.DEFAULT_REDIRECT_MODE, | ||
| UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE + "." + repository.getId(), | ||
| UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE)); |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review #4 — reviewing commit 4c5d834 ("Applied PR reviews")
Clean polish pass. All changes look correct:
- ✅
UnsupportedOperationExceptioninimplPut— matches theClasspathTransporterprecedent for read-only transports. Removingthrows Exceptionfrom the override is valid (narrowing exception signature). - ✅ Defensive
IllegalArgumentExceptioncatch aroundURI.create()/resolve()in redirect handling — addresses a real gap where a malformedLocationheader from a server could cause an unhandled crash. Connection disconnect and cause chain are properly preserved. - ✅
finaldeclarations on redirect variables — idiomatic and enforce single-assignment. - ✅ Better error message including
redirectAuthority— helpful for debugging, no security concern. - ✅ Documentation expansion accurately reflects the transport's capabilities.
One very minor nit: transporter-known-issues.md is missing a trailing newline (the \ No newline at end of file marker in the diff), while most other markdown files in src/site/markdown/ end with one. Not a blocker at all.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review #5 — commit 1ee10959 ("Add NL to file end")
✅ Trailing newline added to transporter-known-issues.md — addresses the nit from review #4. Thanks for the quick fix!
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review #6 — commits 812ad23f (merge from master) + a771559c ("Adopt to latest test changes")
✅ Test adaptation after merging master. The UrlTransporterTest now overrides supportsHttp3() and supportsHttp2() to return false — correct, since URL transport uses HttpURLConnection which only supports HTTP/1.1. Clean merge, no conflicts.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review #7 — commits 18–19 (test adaptations + merge from upstream/master)
Changes reviewed:
- Commit 18 ("Fixes"): Guards SSL property assertions (
SSL_PROTOCOL,SSL_CIPHER_SUITE) intestPeek_SSL()andtestGet_SSL()behindexposeContentCodingInTransportProperties().UrlTransporterTestoverrides this to returnfalse— correct, sinceHttpURLConnectiondoesn't expose SSL/TLS metadata. - Commit 19 (merge): Cleanly incorporates the merged PR #1967 (HTTP/3 name-based comparison in
JdkTransporter.toHttpVersion()).
Observations:
- The
exposeContentCodingInTransportProperties()method was originally introduced (in PR #1762) for content-coding assertions, so reusing it for SSL property guards is a slight semantic stretch. If a future transporter exposes SSL properties without content-coding, a separate guard would be needed. This is non-blocking — the method lives on master, not in this PR's scope.
Verdict: ✅ Looks good — test-only changes, consistent with the existing override pattern, all 18 CI matrix jobs pass.
Reviewed with AI on behalf of @gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Many applications (outside of Maven) integrate Resolver and their use case is almost always "consume" (use Resolver to resolve artifacts). Also, many of those applications are still Java 8 level, hence, the JDK transport for them is no-go. On the other hand, there is no lightweight replacement for them, except to use some "heavyweight" transporter, but in case of CLI applications this is usually undesirable, as they count every byte.
Transitive hull sizes of several existing transports:
The new transport has no dependencies and small size: