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
@@ -84,15 +84,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@v4.0.0
 
   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@v4
       with:
@@ -103,6 +103,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/java/org/kohsuke/github/GitHubClient.java

@@ -452,7 +452,7 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Bo
 
         int retries = retryCount;
         sendRequestTraceId.set(Integer.toHexString(request.hashCode()));
-        GitHubConnectorRequest connectorRequest = prepareConnectorRequest(request);
+        GitHubConnectorRequest connectorRequest = prepareConnectorRequest(request, authorizationProvider);
         do {
             GitHubConnectorResponse connectorResponse = null;
             try {
@@ -492,7 +492,7 @@ private void detectKnownErrors(GitHubConnectorResponse connectorResponse,
         detectOTPRequired(connectorResponse);
         detectInvalidCached404Response(connectorResponse, request);
         detectExpiredToken(connectorResponse, request);
-        detectRedirect(connectorResponse);
+        detectRedirect(connectorResponse, request);
         if (rateLimitHandler.isError(connectorResponse)) {
             rateLimitHandler.onError(connectorResponse);
             throw new RetryRequestException();
@@ -514,32 +514,106 @@ private void detectExpiredToken(GitHubConnectorResponse connectorResponse, GitHu
         if (Objects.isNull(originalAuthorization) || originalAuthorization.isEmpty()) {
             return;
         }
-        GitHubConnectorRequest updatedRequest = prepareConnectorRequest(request);
+        GitHubConnectorRequest updatedRequest = prepareConnectorRequest(request, authorizationProvider);
         String updatedAuthorization = updatedRequest.header("Authorization");
         if (!originalAuthorization.equals(updatedAuthorization)) {
             throw new RetryRequestException(updatedRequest);
         }
     }
 
-    private void detectRedirect(GitHubConnectorResponse connectorResponse) throws IOException {
-        if (connectorResponse.statusCode() == HTTP_MOVED_PERM || connectorResponse.statusCode() == HTTP_MOVED_TEMP) {
-            // GitHubClient depends on GitHubConnector implementations to follow any redirects automatically
-            // If this is not done and a redirect is requested, throw in order to maintain security and consistency
-            throw new HttpException(
-                    "GitHubConnnector did not automatically follow redirect.\n"
-                            + "Change your http client configuration to automatically follow redirects as appropriate.",
+    private void detectRedirect(GitHubConnectorResponse connectorResponse, GitHubRequest request) throws IOException {
+        if (isRedirecting(connectorResponse.statusCode())) {
+            // For redirects, GitHub expects the Authorization header to be removed.
+            // GitHubConnector implementations can follow any redirects automatically as long as they remove the header
+            // as well.
+            // Okhttp does this.
+            // https://github.com/square/okhttp/blob/f9dfd4e8cc070ca2875a67d8f7ad939d95e7e296/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L313-L318
+            // GitHubClient always strips Authorization from detected redirects for security.
+            // 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.
+            // See also https://github.com/arduino/report-size-deltas/pull/83 for more context
+
+            GitHubConnectorRequest updatedRequest = prepareRedirectRequest(connectorResponse, request);
+            throw new RetryRequestException(updatedRequest);
+        }
+    }
+
+    private GitHubConnectorRequest prepareRedirectRequest(GitHubConnectorResponse connectorResponse,
+            GitHubRequest request) throws IOException {
+        URI requestUri = URI.create(request.url().toString());
+        URI redirectedUri = getRedirectedUri(requestUri, connectorResponse);
+        // If we switch ports on the same host, we consider that as a different host
+        // This is slightly different from Redirect#NORMAL, but needed for local testing
+        boolean sameHost = redirectedUri.getHost().equalsIgnoreCase(request.url().getHost())
+                && redirectedUri.getPort() == request.url().getPort();
+
+        // mimicking the behavior of Redirect#NORMAL which was the behavior we used before
+        // Always redirect, except from HTTPS URLs to HTTP URLs.
+        if (!requestUri.getScheme().equalsIgnoreCase(redirectedUri.getScheme())
+                && !"https".equalsIgnoreCase(redirectedUri.getScheme())) {
+            throw new HttpException("Attemped to redirect to a different scheme and the target scheme as not https.",
                     connectorResponse.statusCode(),
                     "Redirect",
                     connectorResponse.request().url().toString());
         }
+
+        String redirectedMethod = getRedirectedMethod(connectorResponse.statusCode(), request.method());
+
+        // let's build the new redirected request
+        GitHubRequest.Builder<?> requestBuilder = request.toBuilder()
+                .setRawUrlPath(redirectedUri.toString())
+                .method(redirectedMethod);
+        // if we redirect to a different host (even https), we remove the Authorization header
+        AuthorizationProvider provider = authorizationProvider;
+        if (!sameHost) {
+            requestBuilder.removeHeader("Authorization");
+            provider = AuthorizationProvider.ANONYMOUS;
+        }
+        return prepareConnectorRequest(requestBuilder.build(), provider);
     }
 
-    private GitHubConnectorRequest prepareConnectorRequest(GitHubRequest request) throws IOException {
+    private static URI getRedirectedUri(URI requestUri, GitHubConnectorResponse connectorResponse) throws IOException {
+        URI redirectedURI;
+        redirectedURI = Optional.of(connectorResponse.header("Location"))
+                .map(URI::create)
+                .orElseThrow(() -> new IOException("Invalid redirection"));
+
+        // redirect could be relative to original URL, but if not
+        // then redirect is used.
+        redirectedURI = requestUri.resolve(redirectedURI);
+        return redirectedURI;
+    }
+
+    // This implements the exact same rules as the ones applied in jdk.internal.net.http.RedirectFilter
+    private static boolean isRedirecting(int statusCode) {
+        return statusCode == HTTP_MOVED_PERM || statusCode == HTTP_MOVED_TEMP || statusCode == 303 || statusCode == 307
+                || statusCode == 308;
+    }
+
+    // This implements the exact same rules as the ones applied in jdk.internal.net.http.RedirectFilter
+    private static String getRedirectedMethod(int statusCode, String originalMethod) {
+        switch (statusCode) {
+            case HTTP_MOVED_PERM :
+            case HTTP_MOVED_TEMP :
+                return originalMethod.equals("POST") ? "GET" : originalMethod;
+            case 303 :
+                return "GET";
+            case 307 :
+            case 308 :
+                return originalMethod;
+            default :
+                return originalMethod;
+        }
+    }
+
+    private static GitHubConnectorRequest prepareConnectorRequest(GitHubRequest request,
+            AuthorizationProvider authorizationProvider) throws IOException {
         GitHubRequest.Builder<?> builder = request.toBuilder();
         // if the authentication is needed but no credential is given, try it anyway (so that some calls
         // that do work with anonymous access in the reduced form should still work.)
         if (!request.allHeaders().containsKey("Authorization")) {
-            String authorization = getEncodedAuthorization();
+            String authorization = authorizationProvider.getEncodedAuthorization();
             if (authorization != null) {
                 builder.setHeader("Authorization", authorization);
             }
@@ -725,7 +799,8 @@ private void detectInvalidCached404Response(GitHubConnectorResponse connectorRes
             // "If-Modified-Since" or "If-None-Match" values.
             // This makes GitHub give us current data (not incorrectly cached data)
             throw new RetryRequestException(
-                    prepareConnectorRequest(request.toBuilder().setHeader("Cache-Control", "no-cache").build()));
+                    prepareConnectorRequest(request.toBuilder().setHeader("Cache-Control", "no-cache").build(),
+                            authorizationProvider));
         }
     }
 

src/main/java/org/kohsuke/github/GitHubRequest.java

@@ -425,6 +425,18 @@ public B withApiUrl(String url) {
             return (B) this;
         }
 
+        /**
+         * Removes the named request HTTP header.
+         *
+         * @param name
+         *            the name
+         * @return the request builder
+         */
+        public B removeHeader(String name) {
+            headers.remove(name);
+            return (B) this;
+        }
+
         /**
          * Sets the request HTTP header.
          * <p>

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

@@ -31,7 +31,17 @@ public class HttpClientGitHubConnector implements GitHubConnector {
      * Instantiates a new HttpClientGitHubConnector with a default HttpClient.
      */
     public HttpClientGitHubConnector() {
-        this(HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL).build());
+        // GitHubClient handles redirects manually as Java HttpClient copies all the headers when redirecting
+        // 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());
     }
 
     /**

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

@@ -355,6 +355,10 @@ public void testLogs() throws IOException {
     @SuppressWarnings("resource")
     @Test
     public void testArtifacts() throws IOException {
+        // Recorded with Authorization, then manually updated
+        snapshotNotAllowed();
+
+        mockGitHub.customizeRecordSpec(recordSpecBuilder -> recordSpecBuilder.captureHeader("Authorization"));
         GHWorkflow workflow = repo.getWorkflow(ARTIFACTS_WORKFLOW_PATH);
 
         long latestPreexistingWorkflowRunId = getLatestPreexistingWorkflowRunId();
@@ -382,7 +386,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();
@@ -400,7 +404,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());