jdaugherty commented on code in PR #15809:
URL: https://github.com/apache/grails-core/pull/15809#discussion_r3596873825
##########
grails-wrapper/src/main/java/grails/init/GrailsUpdater.java:
##########
@@ -329,17 +335,100 @@ private String fetchSnapshotForVersion(GrailsWrapperRepo
repo, GrailsVersion bas
}
}
- private static HttpURLConnection createHttpURLConnection(String
mavenMetadataFileUrl) throws IOException {
- final URL url;
+ static HttpURLConnection createHttpURLConnection(String
mavenMetadataFileUrl) throws IOException {
+ return createHttpURLConnection(mavenMetadataFileUrl, url ->
(HttpURLConnection) url.openConnection());
+ }
+
+ @FunctionalInterface
+ interface HttpURLConnectionFactory {
+ HttpURLConnection open(URL url) throws IOException;
+ }
+
+ static HttpURLConnection createHttpURLConnection(String
mavenMetadataFileUrl, HttpURLConnectionFactory openConnection) throws
IOException {
+ URI uri = createSecureRemoteUri(mavenMetadataFileUrl);
+ for (int redirectCount = 0; ; redirectCount++) {
+ URL url = uri.toURL();
+ HttpURLConnection conn = openConnection.open(url);
+ String location;
+ try {
+ conn.setRequestProperty("User-Agent", "Apache-Maven/3.9.6");
+ conn.setInstanceFollowRedirects(false);
+
+ int responseCode = conn.getResponseCode();
+ if (!isRedirect(responseCode)) {
+ // Ownership of the open connection transfers to the
caller.
+ return conn;
Review Comment:
Ownership transfers here, but neither caller honors it on failure: in
`updateJar()` a 404 makes `conn.getInputStream()` throw and the connection is
never disconnected, and `retrieveMavenMetadata()` has the same gap before
wrapping the exception. Given how careful this method is about not leaking
connections, the callers' failure paths should close/disconnect too.
##########
grails-wrapper/src/main/java/grails/init/GrailsWrapperRepo.java:
##########
@@ -113,6 +120,82 @@ private static GrailsWrapperRepo
createGrailsWrapperRepo(String urlOrFile) {
return repo;
}
+ private static void validateRemoteRepositoryUrl(String url) {
+ try {
Review Comment:
To be concrete about what I would like to see here: `hasLeadingUrlScheme()`
re-implements what a one-line scheme regex (`[A-Za-z][A-Za-z0-9+.-]*://`)
already expresses, and the drive-letter handling in
`isFileRepository()`/`normalizeBaseUrl()` overlaps with what `URI` parsing
already tells us (a single-letter scheme with no authority). On top of that,
the wrapper and forge now carry two slightly different hand-rolled classifiers
for the same GRAILS_REPO_URL semantics - they should at least converge on one
shape. Note the new ersatz/system-stubs dependencies are test-only, so the
shipped wrapper jar only grows by these helper methods - the concern is the
expanding main-source surface, not the dependencies.
##########
INSTALL:
##########
@@ -95,6 +95,8 @@ This variable also supports multiple repositories by using
the separator ';'. F
export
GRAILS_REPO_URL=$HOME/.m2/repository;https://repo1.maven.org/maven2/
+The Grails Wrapper and Grails Forge require remote repositories configured
with `GRAILS_REPO_URL` to use HTTPS. Local file system repositories, such as
`$HOME/.m2/repository`, remain supported. The legacy Grails Shell CLI preserves
its existing repository URL behavior.
Review Comment:
This sentence documents the Shell CLI as exempt, but `ProfileRepoConfig` and
`CreateAppCommand` in `grails-shell-cli` still read
`grails.repo.url`/`GRAILS_REPO_URL` with no HTTPS enforcement. My earlier
comment asked for `GRAILS_REPO_URL` handling to be consistent everywhere it is
used - forge is now covered, but the shell CLI only received a doc carve-out.
Either apply the same validation there or explain why it needs to keep
accepting non-HTTPS repositories.
--
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]
