Add support for artifacts uploaded by actions/upload-artifact@v4 (#1791)

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

The artifacts upload by actions/upload-artifact@v4 are hosted on a new
infrastructure which has several constraints:
- we will have an error if we push the Authorization header to it, which
  was the case when using the Java 11 HttpClient (and is considered a
  bad practice so it is good to have fixed it anyway)
- the host name is dynamic so our test infrastructure was having
  problems with proxying the request

All these problems are sorted out by this pull request and we are now
testing an artifact uploaded by v3 and one uploaded by v4.

Fixes #1790

* Add support for Git longpaths on Windows CI

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

* Move redirect handling to GitHubClient

* WIP

* Make snapshot file names based on mapping file information

* For redirect host is only same if ports are also same

* Delete .vscode/launch.json

* Verify removal of header when redirecting

---------

Co-authored-by: Liam Newman <bitwiseman@gmail.com>

.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.1.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());