Skip to content

Commit 1ebe446

Browse files
gsmetbitwiseman
gsmet
and
bitwiseman
authored
Add support for artifacts uploaded by actions/upload-artifact@v4 (hub4j#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 hub4j#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>
1 parent ddbbc7a commit 1ebe446

118 files changed

Lines changed: 2641 additions & 1256 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.github/workflows/maven-build.yml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
name: CI
22

3-
on:
3+
on:
44
push:
55
branches:
66
- main
@@ -84,15 +84,15 @@ jobs:
8484
env:
8585
MAVEN_OPTS: ${{ env.JAVA_11_PLUS_MAVEN_OPTS }}
8686
run: mvn -B clean install -D enable-ci --file pom.xml "-Dsurefire.argLine=--add-opens java.base/java.net=ALL-UNNAMED"
87-
- name: Codecov Report
87+
- name: Codecov Report
8888
if: matrix.os == 'ubuntu' && matrix.java == '17'
8989
uses: codecov/codecov-action@v4.1.0
9090

9191
test-java-8:
9292
name: test Java 8 (no-build)
9393
needs: build
9494
runs-on: ubuntu-latest
95-
steps:
95+
steps:
9696
- uses: actions/checkout@v4
9797
- uses: actions/download-artifact@v4
9898
with:
@@ -103,6 +103,6 @@ jobs:
103103
with:
104104
java-version: 8
105105
distribution: 'temurin'
106-
cache: 'maven'
106+
cache: 'maven'
107107
- name: Maven Test (no build) Java 8
108108
run: mvn -B surefire:test -DfailIfNoTests -Dsurefire.excludesFile=src/test/resources/slow-or-flaky-tests.txt

‎src/main/java/org/kohsuke/github/GitHubClient.java‎

Lines changed: 88 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Bo
452452

453453
int retries = retryCount;
454454
sendRequestTraceId.set(Integer.toHexString(request.hashCode()));
455-
GitHubConnectorRequest connectorRequest = prepareConnectorRequest(request);
455+
GitHubConnectorRequest connectorRequest = prepareConnectorRequest(request, authorizationProvider);
456456
do {
457457
GitHubConnectorResponse connectorResponse = null;
458458
try {
@@ -492,7 +492,7 @@ private void detectKnownErrors(GitHubConnectorResponse connectorResponse,
492492
detectOTPRequired(connectorResponse);
493493
detectInvalidCached404Response(connectorResponse, request);
494494
detectExpiredToken(connectorResponse, request);
495-
detectRedirect(connectorResponse);
495+
detectRedirect(connectorResponse, request);
496496
if (rateLimitHandler.isError(connectorResponse)) {
497497
rateLimitHandler.onError(connectorResponse);
498498
throw new RetryRequestException();
@@ -514,32 +514,106 @@ private void detectExpiredToken(GitHubConnectorResponse connectorResponse, GitHu
514514
if (Objects.isNull(originalAuthorization) || originalAuthorization.isEmpty()) {
515515
return;
516516
}
517-
GitHubConnectorRequest updatedRequest = prepareConnectorRequest(request);
517+
GitHubConnectorRequest updatedRequest = prepareConnectorRequest(request, authorizationProvider);
518518
String updatedAuthorization = updatedRequest.header("Authorization");
519519
if (!originalAuthorization.equals(updatedAuthorization)) {
520520
throw new RetryRequestException(updatedRequest);
521521
}
522522
}
523523

524-
private void detectRedirect(GitHubConnectorResponse connectorResponse) throws IOException {
525-
if (connectorResponse.statusCode() == HTTP_MOVED_PERM || connectorResponse.statusCode() == HTTP_MOVED_TEMP) {
526-
// GitHubClient depends on GitHubConnector implementations to follow any redirects automatically
527-
// If this is not done and a redirect is requested, throw in order to maintain security and consistency
528-
throw new HttpException(
529-
"GitHubConnnector did not automatically follow redirect.\n"
530-
+ "Change your http client configuration to automatically follow redirects as appropriate.",
524+
private void detectRedirect(GitHubConnectorResponse connectorResponse, GitHubRequest request) throws IOException {
525+
if (isRedirecting(connectorResponse.statusCode())) {
526+
// For redirects, GitHub expects the Authorization header to be removed.
527+
// GitHubConnector implementations can follow any redirects automatically as long as they remove the header
528+
// as well.
529+
// Okhttp does this.
530+
// https://github.com/square/okhttp/blob/f9dfd4e8cc070ca2875a67d8f7ad939d95e7e296/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L313-L318
531+
// GitHubClient always strips Authorization from detected redirects for security.
532+
// This problem was discovered when upload-artifact@v4 was released as the new
533+
// service we are redirected to for downloading the artifacts doesn't support
534+
// having the Authorization header set.
535+
// See also https://github.com/arduino/report-size-deltas/pull/83 for more context
536+
537+
GitHubConnectorRequest updatedRequest = prepareRedirectRequest(connectorResponse, request);
538+
throw new RetryRequestException(updatedRequest);
539+
}
540+
}
541+
542+
private GitHubConnectorRequest prepareRedirectRequest(GitHubConnectorResponse connectorResponse,
543+
GitHubRequest request) throws IOException {
544+
URI requestUri = URI.create(request.url().toString());
545+
URI redirectedUri = getRedirectedUri(requestUri, connectorResponse);
546+
// If we switch ports on the same host, we consider that as a different host
547+
// This is slightly different from Redirect#NORMAL, but needed for local testing
548+
boolean sameHost = redirectedUri.getHost().equalsIgnoreCase(request.url().getHost())
549+
&& redirectedUri.getPort() == request.url().getPort();
550+
551+
// mimicking the behavior of Redirect#NORMAL which was the behavior we used before
552+
// Always redirect, except from HTTPS URLs to HTTP URLs.
553+
if (!requestUri.getScheme().equalsIgnoreCase(redirectedUri.getScheme())
554+
&& !"https".equalsIgnoreCase(redirectedUri.getScheme())) {
555+
throw new HttpException("Attemped to redirect to a different scheme and the target scheme as not https.",
531556
connectorResponse.statusCode(),
532557
"Redirect",
533558
connectorResponse.request().url().toString());
534559
}
560+
561+
String redirectedMethod = getRedirectedMethod(connectorResponse.statusCode(), request.method());
562+
563+
// let's build the new redirected request
564+
GitHubRequest.Builder<?> requestBuilder = request.toBuilder()
565+
.setRawUrlPath(redirectedUri.toString())
566+
.method(redirectedMethod);
567+
// if we redirect to a different host (even https), we remove the Authorization header
568+
AuthorizationProvider provider = authorizationProvider;
569+
if (!sameHost) {
570+
requestBuilder.removeHeader("Authorization");
571+
provider = AuthorizationProvider.ANONYMOUS;
572+
}
573+
return prepareConnectorRequest(requestBuilder.build(), provider);
535574
}
536575

537-
private GitHubConnectorRequest prepareConnectorRequest(GitHubRequest request) throws IOException {
576+
private static URI getRedirectedUri(URI requestUri, GitHubConnectorResponse connectorResponse) throws IOException {
577+
URI redirectedURI;
578+
redirectedURI = Optional.of(connectorResponse.header("Location"))
579+
.map(URI::create)
580+
.orElseThrow(() -> new IOException("Invalid redirection"));
581+
582+
// redirect could be relative to original URL, but if not
583+
// then redirect is used.
584+
redirectedURI = requestUri.resolve(redirectedURI);
585+
return redirectedURI;
586+
}
587+
588+
// This implements the exact same rules as the ones applied in jdk.internal.net.http.RedirectFilter
589+
private static boolean isRedirecting(int statusCode) {
590+
return statusCode == HTTP_MOVED_PERM || statusCode == HTTP_MOVED_TEMP || statusCode == 303 || statusCode == 307
591+
|| statusCode == 308;
592+
}
593+
594+
// This implements the exact same rules as the ones applied in jdk.internal.net.http.RedirectFilter
595+
private static String getRedirectedMethod(int statusCode, String originalMethod) {
596+
switch (statusCode) {
597+
case HTTP_MOVED_PERM :
598+
case HTTP_MOVED_TEMP :
599+
return originalMethod.equals("POST") ? "GET" : originalMethod;
600+
case 303 :
601+
return "GET";
602+
case 307 :
603+
case 308 :
604+
return originalMethod;
605+
default :
606+
return originalMethod;
607+
}
608+
}
609+
610+
private static GitHubConnectorRequest prepareConnectorRequest(GitHubRequest request,
611+
AuthorizationProvider authorizationProvider) throws IOException {
538612
GitHubRequest.Builder<?> builder = request.toBuilder();
539613
// if the authentication is needed but no credential is given, try it anyway (so that some calls
540614
// that do work with anonymous access in the reduced form should still work.)
541615
if (!request.allHeaders().containsKey("Authorization")) {
542-
String authorization = getEncodedAuthorization();
616+
String authorization = authorizationProvider.getEncodedAuthorization();
543617
if (authorization != null) {
544618
builder.setHeader("Authorization", authorization);
545619
}
@@ -725,7 +799,8 @@ private void detectInvalidCached404Response(GitHubConnectorResponse connectorRes
725799
// "If-Modified-Since" or "If-None-Match" values.
726800
// This makes GitHub give us current data (not incorrectly cached data)
727801
throw new RetryRequestException(
728-
prepareConnectorRequest(request.toBuilder().setHeader("Cache-Control", "no-cache").build()));
802+
prepareConnectorRequest(request.toBuilder().setHeader("Cache-Control", "no-cache").build(),
803+
authorizationProvider));
729804
}
730805
}
731806

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,18 @@ public B withApiUrl(String url) {
425425
return (B) this;
426426
}
427427

428+
/**
429+
* Removes the named request HTTP header.
430+
*
431+
* @param name
432+
* the name
433+
* @return the request builder
434+
*/
435+
public B removeHeader(String name) {
436+
headers.remove(name);
437+
return (B) this;
438+
}
439+
428440
/**
429441
* Sets the request HTTP header.
430442
* <p>

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,17 @@ public class HttpClientGitHubConnector implements GitHubConnector {
3131
* Instantiates a new HttpClientGitHubConnector with a default HttpClient.
3232
*/
3333
public HttpClientGitHubConnector() {
34-
this(HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL).build());
34+
// GitHubClient handles redirects manually as Java HttpClient copies all the headers when redirecting
35+
// even when redirecting to a different host which is problematic as we don't want
36+
// to push the Authorization header when redirected to a different host.
37+
// This problem was discovered when upload-artifact@v4 was released as the new
38+
// service we are redirected to for downloading the artifacts doesn't support
39+
// having the Authorization header set.
40+
// The new implementation does not push the Authorization header when redirected
41+
// to a different host, which is similar to what Okhttp is doing:
42+
// https://github.com/square/okhttp/blob/f9dfd4e8cc070ca2875a67d8f7ad939d95e7e296/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L313-L318
43+
// See also https://github.com/arduino/report-size-deltas/pull/83 for more context
44+
this(HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NEVER).build());
3545
}
3646

3747
/**

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

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,10 @@ public void testLogs() throws IOException {
355355
@SuppressWarnings("resource")
356356
@Test
357357
public void testArtifacts() throws IOException {
358+
// Recorded with Authorization, then manually updated
359+
snapshotNotAllowed();
360+
361+
mockGitHub.customizeRecordSpec(recordSpecBuilder -> recordSpecBuilder.captureHeader("Authorization"));
358362
GHWorkflow workflow = repo.getWorkflow(ARTIFACTS_WORKFLOW_PATH);
359363

360364
long latestPreexistingWorkflowRunId = getLatestPreexistingWorkflowRunId();
@@ -382,7 +386,7 @@ public void testArtifacts() throws IOException {
382386
checkArtifactProperties(artifacts.get(0), "artifact1");
383387
checkArtifactProperties(artifacts.get(1), "artifact2");
384388

385-
// Test download
389+
// Test download from upload-artifact@v3 infrastructure
386390
String artifactContent = artifacts.get(0).download((is) -> {
387391
try (ZipInputStream zis = new ZipInputStream(is)) {
388392
StringBuilder sb = new StringBuilder();
@@ -400,7 +404,25 @@ public void testArtifacts() throws IOException {
400404
}
401405
});
402406

403-
assertThat(artifactContent, is("artifact1"));
407+
// Test download from upload-artifact@v4 infrastructure
408+
artifactContent = artifacts.get(1).download((is) -> {
409+
try (ZipInputStream zis = new ZipInputStream(is)) {
410+
StringBuilder sb = new StringBuilder();
411+
412+
ZipEntry ze = zis.getNextEntry();
413+
assertThat(ze.getName(), is("artifact2.txt"));
414+
415+
// the scanner has to be kept open to avoid closing zis
416+
Scanner scanner = new Scanner(zis);
417+
while (scanner.hasNextLine()) {
418+
sb.append(scanner.nextLine());
419+
}
420+
421+
return sb.toString();
422+
}
423+
});
424+
425+
assertThat(artifactContent, is("artifact2"));
404426

405427
// Test GHRepository#getArtifact(long) as we are sure we have artifacts around
406428
GHArtifact artifactById = repo.getArtifact(artifacts.get(0).getId());

0 commit comments

Comments
 (0)