Add support for artifacts uploaded by actions/upload-artifact@v4

.github/workflows/maven-build.yml

@@ -1,6 +1,6 @@
 name: CI
 
-on: 
+on:
   push:
     branches:
       - main
@@ -66,6 +66,9 @@ jobs:
         os: [ ubuntu, windows ]
         java: [ 11, 17 ]
     steps:
+    - name: Support longpaths on Windows
+      if: ${{ matrix.os == 'windows' }}
+      run: git config --global core.longpaths true
     - uses: actions/checkout@v4
     - name: Set up JDK
       uses: actions/setup-java@v4
@@ -84,15 +87,15 @@ jobs:
       env:
         MAVEN_OPTS: ${{ env.JAVA_11_PLUS_MAVEN_OPTS }}
       run: mvn -B clean install -D enable-ci --file pom.xml "-Dsurefire.argLine=--add-opens java.base/java.net=ALL-UNNAMED"
-    - name: Codecov Report 
+    - name: Codecov Report
       if: matrix.os == 'ubuntu' && matrix.java == '17'
       uses: codecov/codecov-action@v3.1.4
 
   test-java-8:
     name: test Java 8 (no-build)
     needs: build
     runs-on: ubuntu-latest
-    steps: 
+    steps:
     - uses: actions/checkout@v4
     - uses: actions/download-artifact@v3
       with:
@@ -103,6 +106,6 @@ jobs:
       with:
         java-version: 8
         distribution: 'temurin'
-        cache: 'maven'   
+        cache: 'maven'
     - name: Maven Test (no build) Java 8
       run: mvn -B surefire:test -DfailIfNoTests -Dsurefire.excludesFile=src/test/resources/slow-or-flaky-tests.txt

src/main/java11/org/kohsuke/github/extras/HttpClientGitHubConnector.java

@@ -8,12 +8,16 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InterruptedIOException;
+import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.http.HttpClient;
+import java.net.http.HttpHeaders;
 import java.net.http.HttpRequest;
+import java.net.http.HttpRequest.BodyPublisher;
 import java.net.http.HttpResponse;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
@@ -22,16 +26,30 @@
  * {@link GitHubConnector} for {@link HttpClient}.
  *
  * @author Liam Newman
+ * @author Guillaume Smet
  */
 public class HttpClientGitHubConnector implements GitHubConnector {
 
+    private static final String AUTHORIZATION_HEADER = "Authorization";
+    private static final int MAX_REDIRECTS = 5;
+
     private final HttpClient client;
 
     /**
      * Instantiates a new HttpClientGitHubConnector with a default HttpClient.
      */
     public HttpClientGitHubConnector() {
-        this(HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL).build());
+        // We handle redirects manually as Java is copying all the headers when redirected
+        // even when redirecting to a different host which is problematic as we don't want
+        // to push the Authorization header when redirected to a different host.
+        // This problem was discovered when upload-artifact@v4 was released as the new
+        // service we are redirected to for downloading the artifacts doesn't support
+        // having the Authorization header set.
+        // The new implementation does not push the Authorization header when redirected
+        // to a different host, which is similar to what Okhttp is doing:
+        // https://github.com/square/okhttp/blob/f9dfd4e8cc070ca2875a67d8f7ad939d95e7e296/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L313-L318
+        // See also https://github.com/arduino/report-size-deltas/pull/83 for more context
+        this(HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NEVER).build());
     }
 
     /**
@@ -69,13 +87,50 @@ public GitHubConnectorResponse send(GitHubConnectorRequest connectorRequest) thr
         HttpRequest request = builder.build();
 
         try {
-            HttpResponse<InputStream> httpResponse = client.send(request, HttpResponse.BodyHandlers.ofInputStream());
+            HttpResponse<InputStream> httpResponse = send(request, 0);
             return new HttpClientGitHubConnectorResponse(connectorRequest, httpResponse);
         } catch (InterruptedException e) {
             throw (InterruptedIOException) new InterruptedIOException(e.getMessage()).initCause(e);
         }
     }
 
+    private HttpResponse<InputStream> send(HttpRequest request, int redirects)
+            throws IOException, InterruptedException {
+        HttpResponse<InputStream> response = client.send(request, HttpResponse.BodyHandlers.ofInputStream());
+
+        if (!isRedirecting(response.statusCode()) || redirects > MAX_REDIRECTS) {
+            return response;
+        }
+
+        URI redirectedUri = getRedirectedUri(request.uri(), response.headers());
+        boolean sameHost = redirectedUri.getHost().equalsIgnoreCase(request.uri().getHost());
+
+        // mimicking the behavior of Redirect#NORMAL which was the behavior we used before
+        if (!request.uri().getScheme().equalsIgnoreCase(redirectedUri.getScheme())
+                && !"https".equalsIgnoreCase(redirectedUri.getScheme())) {
+            return response;
+        }
+
+        String redirectedMethod = getRedirectedMethod(response.statusCode(), request.method());
+
+        // let's build the new redirected request
+        HttpRequest.Builder newRequestBuilder = HttpRequest.newBuilder();
+        newRequestBuilder.uri(redirectedUri);
+        newRequestBuilder.method(redirectedMethod,
+                getRedirectedPublisher(request, response.statusCode(), redirectedMethod));
+        for (Entry<String, List<String>> headerEntry : request.headers().map().entrySet()) {
+            // if we redirect to a different host, we don't copy the Authorization header
+            if (!sameHost && headerEntry.getKey().equalsIgnoreCase(AUTHORIZATION_HEADER)) {
+                continue;
+            }
+            for (String value : headerEntry.getValue()) {
+                newRequestBuilder.header(headerEntry.getKey(), value);
+            }
+        }
+
+        return send(newRequestBuilder.build(), redirects + 1);
+    }
+
     /**
      * Initial response information when a response is initially received and before the body is processed.
      *
@@ -104,4 +159,49 @@ public void close() throws IOException {
             IOUtils.closeQuietly(response.body());
         }
     }
+
+    private static URI getRedirectedUri(URI originalUri, HttpHeaders headers) throws IOException {
+        URI redirectedURI;
+        redirectedURI = headers.firstValue("Location")
+                .map(URI::create)
+                .orElseThrow(() -> new IOException("Invalid redirection"));
+
+        // redirect could be relative to original URL, but if not
+        // then redirect is used.
+        redirectedURI = originalUri.resolve(redirectedURI);
+        return redirectedURI;
+    }
+
+    // This implements the exact same rules as the ones applied in RedirectFilter
+    private static BodyPublisher getRedirectedPublisher(HttpRequest originalRequest,
+            int originalStatusCode,
+            String newMethod) {
+        if (originalStatusCode == 303 || originalRequest.method().equalsIgnoreCase(newMethod)
+                || !originalRequest.bodyPublisher().isPresent()) {
+            return HttpRequest.BodyPublishers.noBody();
+        }
+
+        return originalRequest.bodyPublisher().get();
+    }
+
+    // This implements the exact same rules as the ones applied in RedirectFilter
+    private static boolean isRedirecting(int statusCode) {
+        return statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 307 || statusCode == 308;
+    }
+
+    // This implements the exact same rules as the ones applied in RedirectFilter
+    private static String getRedirectedMethod(int statusCode, String originalMethod) {
+        switch (statusCode) {
+            case 301 :
+            case 302 :
+                return originalMethod.equals("POST") ? "GET" : originalMethod;
+            case 303 :
+                return "GET";
+            case 307 :
+            case 308 :
+                return originalMethod;
+            default :
+                return originalMethod;
+        }
+    }
 }

src/test/java/org/kohsuke/github/GHWorkflowRunTest.java

@@ -311,7 +311,7 @@ public void testArtifacts() throws IOException {
         checkArtifactProperties(artifacts.get(0), "artifact1");
         checkArtifactProperties(artifacts.get(1), "artifact2");
 
-        // Test download
+        // Test download from upload-artifact@v3 infrastructure
         String artifactContent = artifacts.get(0).download((is) -> {
             try (ZipInputStream zis = new ZipInputStream(is)) {
                 StringBuilder sb = new StringBuilder();
@@ -329,7 +329,25 @@ public void testArtifacts() throws IOException {
             }
         });
 
-        assertThat(artifactContent, is("artifact1"));
+        // Test download from upload-artifact@v4 infrastructure
+        artifactContent = artifacts.get(1).download((is) -> {
+            try (ZipInputStream zis = new ZipInputStream(is)) {
+                StringBuilder sb = new StringBuilder();
+
+                ZipEntry ze = zis.getNextEntry();
+                assertThat(ze.getName(), is("artifact2.txt"));
+
+                // the scanner has to be kept open to avoid closing zis
+                Scanner scanner = new Scanner(zis);
+                while (scanner.hasNextLine()) {
+                    sb.append(scanner.nextLine());
+                }
+
+                return sb.toString();
+            }
+        });
+
+        assertThat(artifactContent, is("artifact2"));
 
         // Test GHRepository#getArtifact(long) as we are sure we have artifacts around
         GHArtifact artifactById = repo.getArtifact(artifacts.get(0).getId());