fix(archive): use api.github.com zipball endpoint and fix App auth#532
Open
fasher wants to merge 1 commit into
Open
fix(archive): use api.github.com zipball endpoint and fix App auth#532fasher wants to merge 1 commit into
fasher wants to merge 1 commit into
Conversation
a386fd4 to
382265c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes archive downloads for private GitHub.com repositories by switching from the public web /archive/{sha}.zip URL to the authenticated REST API zipball endpoint, and by correcting GitHub App archive authentication so a valid installation token is used.
Changes:
- Switch GitHub archive URL generation to
.../repos/{owner}/{repo}/zipball/{sha}(cloud + GHE) and fix cloud-vs-enterprise detection. - Update
vcs.Client.GetAuthHeadersto acceptcontext.Contextand return(headers, error), and implement GitHub App installation-token fetching for archive downloads. - Extend SHA extraction to support
/zipball/{sha}and/tarball/{sha}formats and add/adjust unit tests (including redirect-follow behavior for zipball).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vcs/types.go | Update GetAuthHeaders interface to be context-aware and error-returning. |
| pkg/vcs/gitlab_client/client.go | Adapt GitLab implementation to new GetAuthHeaders(ctx) (map, error) signature. |
| pkg/vcs/gitlab_client/client_test.go | Update tests for the new GetAuthHeaders signature. |
| pkg/vcs/github_client/client.go | Add GitHub App installation token source + context-aware GetAuthHeaders with error handling. |
| pkg/vcs/github_client/client_test.go | Add coverage for GitHub App token fetching and “no credentials” errors; update archive URL expectations. |
| pkg/vcs/github_client/archive.go | Switch archive URL generation to REST zipball endpoint; fix enterprise-vs-cloud branching. |
| pkg/archive/README.md | Update documentation for new URL format and new GetAuthHeaders signature/token sources. |
| pkg/archive/manager.go | Call new GetAuthHeaders(ctx) and propagate errors; extend SHA extraction for zipball/tarball. |
| pkg/archive/manager_sha_test.go | Extend SHA extraction tests for zipball/tarball + empty-SHA cases. |
| pkg/archive/download_test.go | Add redirect-follow test using an in-memory zip payload. |
| mocks/vcs/mocks/mock_MockClient.go | Regenerate/update mock for new GetAuthHeaders(ctx) (map, error) signature. |
Files not reviewed (1)
- mocks/vcs/mocks/mock_MockClient.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The public web archive URL (github.com/{o}/{r}/archive/{sha}.zip) does
not honor `Authorization: Bearer` for fine-grained PATs or GitHub App
installation tokens on private repos and 404s. The REST API zipball
endpoint accepts all token types and 302s to a signed codeload URL.
Also fixes GitHub App auth for archive downloads: `GetAuthHeaders`
previously returned `Bearer ` with an empty token in App mode because
`cfg.VcsToken` is unset. The client now holds an installation token
source (satisfied by `ghinstallation.Transport`) and fetches a fresh
token on each call.
GHE detection switched to require both VcsBaseUrl and VcsUploadUrl,
mirroring createHttpClient — VcsBaseUrl alone is unreliable because
config defaults it to https://github.com when unset.
Fixes zapier#525
Signed-off-by: Asher Frenkel <asher.frenkel@gmail.com>
382265c to
70799f4
Compare
Contributor
|
@Greyeye could you review? Kubechecks has been unusable for months for me. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #525.
github.com/{o}/{r}/archive/{sha}.zip) to the REST API zipball endpoint (api.github.com/repos/{o}/{r}/zipball/{sha}). The web URL does not honorAuthorization: Bearerfor fine-grained PATs or GitHub App installation tokens on private repos and returns 404; the API endpoint accepts all token types and 302s to a signed codeload URL. Go'shttp.Clientstrips theAuthorizationheader on the cross-host redirect, so the bearer token is not leaked.GetAuthHeaderspreviously emittedAuthorization: Bearer(empty token) in App mode becausecfg.VcsTokenis unset. The client now holds aninstallationTokenSource(satisfied by*ghinstallation.Transport) and fetches a fresh installation token per call. Thevcs.Clientinterface signature changes toGetAuthHeaders(ctx) (map, error); GitLab + mocks updated.DownloadArchive. The original PR draft keyed offVcsBaseUrl != "", butpkg/config/config.godefaultsVcsBaseUrltohttps://github.comwhen unset — so cloud configs were taking the enterprise branch and producing a bogushttps://github.com/repos/.../zipball/...URL. Detection now requires bothVcsBaseUrlandVcsUploadUrl, matchingcreateHttpClient. (Caught by Codex review and verified with a regression test.)extractSHAFromArchiveURLto recognize/zipball/{sha}and/tarball/{sha}(no extension) alongside the legacy/archive/{sha}.zipand GitLab?sha=formats.Test plan
go build ./...go vet ./...go test ./...— full suite passesTestClient_DownloadArchive_HappyPath/…_StaleMergeCommitThenReady/…_GHEnterpriseupdated for new URL formatTestClient_DownloadArchive_DefaultedVcsBaseUrl— regression test for the defaulted-VcsBaseUrlcloud caseTestClient_GetAuthHeaders_NoCredentials— no PAT / no App transport → error rather than empty BearerTestClient_GetAuthHeaders_GitHubApp/…_TokenFetchError— App-auth happy path andToken()failure surface correctlyTestDownloader_FollowsRedirect— httptest 302 → 200 with a real zip, locks in that the downloader follows redirects (the zipball flow depends on it)TestExtractSHAFromArchiveURLextended for/zipball/,/tarball/, GHE API, and empty-SHA error casesNotes
vcs.Client.GetAuthHeadersinterface signature changed (() (map)→(ctx) (map, error)). All in-tree implementations and the generated mock are updated.pkg/archive/README.mdupdated to reflect the new signature and document the supported token sources (classic PAT, fine-grained PAT, OAuth user token, App installation token).