diff --git a/mocks/vcs/mocks/mock_MockClient.go b/mocks/vcs/mocks/mock_MockClient.go index 3cd90e40..f37f1943 100644 --- a/mocks/vcs/mocks/mock_MockClient.go +++ b/mocks/vcs/mocks/mock_MockClient.go @@ -328,22 +328,31 @@ func (_c *MockClient_Email_Call) RunAndReturn(run func() string) *MockClient_Ema } // GetAuthHeaders provides a mock function for the type MockClient -func (_mock *MockClient) GetAuthHeaders() map[string]string { - ret := _mock.Called() +func (_mock *MockClient) GetAuthHeaders(ctx context.Context) (map[string]string, error) { + ret := _mock.Called(ctx) if len(ret) == 0 { panic("no return value specified for GetAuthHeaders") } var r0 map[string]string - if returnFunc, ok := ret.Get(0).(func() map[string]string); ok { - r0 = returnFunc() + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context) (map[string]string, error)); ok { + return returnFunc(ctx) + } + if returnFunc, ok := ret.Get(0).(func(context.Context) map[string]string); ok { + r0 = returnFunc(ctx) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(map[string]string) } } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context) error); ok { + r1 = returnFunc(ctx) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // MockClient_GetAuthHeaders_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetAuthHeaders' @@ -352,23 +361,30 @@ type MockClient_GetAuthHeaders_Call struct { } // GetAuthHeaders is a helper method to define mock.On call -func (_e *MockClient_Expecter) GetAuthHeaders() *MockClient_GetAuthHeaders_Call { - return &MockClient_GetAuthHeaders_Call{Call: _e.mock.On("GetAuthHeaders")} +// - ctx context.Context +func (_e *MockClient_Expecter) GetAuthHeaders(ctx interface{}) *MockClient_GetAuthHeaders_Call { + return &MockClient_GetAuthHeaders_Call{Call: _e.mock.On("GetAuthHeaders", ctx)} } -func (_c *MockClient_GetAuthHeaders_Call) Run(run func()) *MockClient_GetAuthHeaders_Call { +func (_c *MockClient_GetAuthHeaders_Call) Run(run func(ctx context.Context)) *MockClient_GetAuthHeaders_Call { _c.Call.Run(func(args mock.Arguments) { - run() + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + run( + arg0, + ) }) return _c } -func (_c *MockClient_GetAuthHeaders_Call) Return(stringToString map[string]string) *MockClient_GetAuthHeaders_Call { - _c.Call.Return(stringToString) +func (_c *MockClient_GetAuthHeaders_Call) Return(stringToString map[string]string, err error) *MockClient_GetAuthHeaders_Call { + _c.Call.Return(stringToString, err) return _c } -func (_c *MockClient_GetAuthHeaders_Call) RunAndReturn(run func() map[string]string) *MockClient_GetAuthHeaders_Call { +func (_c *MockClient_GetAuthHeaders_Call) RunAndReturn(run func(ctx context.Context) (map[string]string, error)) *MockClient_GetAuthHeaders_Call { _c.Call.Return(run) return _c } diff --git a/pkg/archive/README.md b/pkg/archive/README.md index a2565de6..cd0324d5 100644 --- a/pkg/archive/README.md +++ b/pkg/archive/README.md @@ -204,14 +204,16 @@ The manager gets VCS-specific auth headers: **GitHub**: ```go -// Returns: {"Authorization": "Bearer ghp_xxxxx"} -authHeaders := githubClient.GetAuthHeaders() +// Returns: {"Authorization": "Bearer "} +// For GitHub App auth, the token is fetched fresh from the installation transport, +// so the call may make a network request and must be passed a context. +authHeaders, err := githubClient.GetAuthHeaders(ctx) ``` **GitLab**: ```go // Returns: {"PRIVATE-TOKEN": "glpat_xxxxx"} -authHeaders := gitlabClient.GetAuthHeaders() +authHeaders, err := gitlabClient.GetAuthHeaders(ctx) ``` ### 4. Cache Lookup @@ -291,9 +293,19 @@ https://api.github.com/repos/{owner}/{repo}/zipball/{merge_commit_sha} **Authentication**: ``` -Authorization: Bearer ghp_xxxxxxxxxxxxx +Authorization: Bearer ``` +Supported token sources: +- Classic PAT (`ghp_*`) or fine-grained PAT (`github_pat_*`) via `KUBECHECKS_VCS_TOKEN`. +- GitHub App installation token (`ghs_*`) — fetched fresh from the installation transport + on each archive download; no static token is needed in config. +- OAuth user tokens (`gho_*`) are also accepted when set via `KUBECHECKS_VCS_TOKEN`. + +The REST API zipball endpoint 302s to a signed codeload URL on a different host; +Go's `http.Client` strips the `Authorization` header on that cross-host redirect by +default, so the bearer token is not leaked to the signed URL. + **Merge Commit SHA**: - GitHub provides `merge_commit_sha` in PR API response - This is a real commit representing the merged state @@ -308,10 +320,24 @@ Authorization: Bearer ghp_xxxxxxxxxxxxx ```go // pkg/vcs/github_client/client.go -func (c *Client) GetAuthHeaders() map[string]string { - return map[string]string{ - "Authorization": fmt.Sprintf("Bearer %s", c.cfg.VcsToken), +func (c *Client) GetAuthHeaders(ctx context.Context) (map[string]string, error) { + token := c.cfg.VcsToken + if c.appTokenSource != nil { + // GitHub App: fetch a fresh installation token. + t, err := c.appTokenSource.Token(ctx) + if err != nil { + return nil, errors.Wrap(err, "failed to fetch GitHub App installation token") + } + token = t } + if token == "" { + // Surfaces as a typed authError to PostArchiveErrorMessage so the PR comment + // explains it's a config problem, not a transient one to retry. + return nil, errors.New("no GitHub credentials configured for archive download") + } + return map[string]string{ + "Authorization": fmt.Sprintf("Bearer %s", token), + }, nil } func (c *Client) DownloadArchive(ctx context.Context, pr vcs.PullRequest) (string, error) { @@ -375,10 +401,10 @@ PRIVATE-TOKEN: glpat_xxxxxxxxxxxxx ```go // pkg/vcs/gitlab_client/client.go -func (c *Client) GetAuthHeaders() map[string]string { +func (c *Client) GetAuthHeaders(_ context.Context) (map[string]string, error) { return map[string]string{ "PRIVATE-TOKEN": c.cfg.VcsToken, - } + }, nil } func (c *Client) DownloadArchive(ctx context.Context, pr vcs.PullRequest) (string, error) { @@ -716,10 +742,16 @@ Kubechecks will immediately fall back to git mode. ### Authentication -- **GitHub**: Uses Bearer token (PAT or App token) +- **GitHub**: Uses Bearer token. Sources: + - Classic PAT, fine-grained PAT, or OAuth user token from `KUBECHECKS_VCS_TOKEN`. + - GitHub App installation token — fetched fresh from the installation transport at + download time, so rotation is handled automatically and no static token is required. - **GitLab**: Uses Private token - **Storage**: Tokens are stored in memory, never written to disk - **Headers**: Auth headers are added per-request, not globally +- **Cross-host redirects**: The GitHub REST zipball endpoint 302s to a signed codeload + URL. Go's `http.Client` strips `Authorization` on cross-host redirects by default, so + the bearer token is not leaked to the codeload host. ### Archive Integrity diff --git a/pkg/archive/download_test.go b/pkg/archive/download_test.go index a63c438d..f5872a14 100644 --- a/pkg/archive/download_test.go +++ b/pkg/archive/download_test.go @@ -1,13 +1,20 @@ package archive import ( + "archive/zip" + "bytes" "context" "fmt" "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" "testing" "github.com/pkg/errors" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIsRetriableDownloadError(t *testing.T) { @@ -136,3 +143,83 @@ func TestIsRetriableDownloadError(t *testing.T) { }) } } + +// buildTestZip returns a small in-memory zip mirroring the GitHub archive layout: an +// explicit top-level directory entry `{repo}-{sha}/` followed by a file underneath. +// The directory entry is what the extractor uses to detect and strip the wrapper folder. +func buildTestZip(t *testing.T) []byte { + t.Helper() + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + dirHdr := &zip.FileHeader{Name: "repo-deadbeef/"} + dirHdr.SetMode(0o755 | os.ModeDir) + _, err := zw.CreateHeader(dirHdr) + require.NoError(t, err) + + fileHdr := &zip.FileHeader{Name: "repo-deadbeef/README.md"} + fileHdr.SetMode(0o644) + w, err := zw.CreateHeader(fileHdr) + require.NoError(t, err) + _, err = w.Write([]byte("hello from zip")) + require.NoError(t, err) + require.NoError(t, zw.Close()) + return buf.Bytes() +} + +// TestDownloader_FollowsRedirect locks in the redirect behavior the GitHub zipball flow +// depends on: api.github.com responds with 302 to a signed codeload URL, and the +// downloader must follow that redirect to retrieve the archive. Guards against a future +// http.Client customization (e.g. CheckRedirect: ErrUseLastResponse) silently breaking +// archive downloads. +// +// Note: this test does NOT assert Authorization-header stripping. httptest servers all +// bind to 127.0.0.1, and Go's net/http compares hostnames (not host:port) for the +// sensitive-header decision, so within httptest the header is always forwarded. The +// cross-host stripping for api.github.com → codeload.github.com is a Go stdlib invariant +// covered by the standard library's own tests. +func TestDownloader_FollowsRedirect(t *testing.T) { + zipBytes := buildTestZip(t) + + var ( + apiHits int + codeloadHits int + ) + + // "codeload" stand-in: serves the archive bytes. + codeload := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + codeloadHits++ + w.Header().Set("Content-Type", "application/zip") + _, _ = w.Write(zipBytes) + })) + defer codeload.Close() + + // "api" stand-in: requires Authorization, 302s to codeload (mirroring how + // api.github.com/repos/.../zipball/SHA redirects to a signed codeload URL). + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + apiHits++ + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization"), + "original request to API must carry the Bearer token") + http.Redirect(w, r, codeload.URL+"/signed-archive.zip?token=abc", http.StatusFound) + })) + defer api.Close() + + targetDir := filepath.Join(t.TempDir(), "extract") + d := NewDownloader() + extractedPath, err := d.DownloadAndExtract( + context.Background(), + api.URL+"/repos/owner/repo/zipball/deadbeef", + targetDir, + map[string]string{"Authorization": "Bearer test-token"}, + ) + require.NoError(t, err) + + assert.Equal(t, 1, apiHits, "API endpoint should be hit exactly once") + assert.Equal(t, 1, codeloadHits, "Downloader must follow the 302 and fetch from the redirect target") + assert.True(t, strings.HasSuffix(extractedPath, "repo-deadbeef"), + "extractor should strip the top-level wrapper directory: got %q", extractedPath) + + body, err := os.ReadFile(filepath.Join(extractedPath, "README.md")) + require.NoError(t, err) + assert.Equal(t, "hello from zip", string(body)) +} diff --git a/pkg/archive/manager.go b/pkg/archive/manager.go index 81a12d54..5c5dc218 100644 --- a/pkg/archive/manager.go +++ b/pkg/archive/manager.go @@ -23,6 +23,16 @@ type urlParseError struct{ err error } func (e *urlParseError) Error() string { return e.err.Error() } func (e *urlParseError) Unwrap() error { return e.err } +// authError wraps failures from vcs.Client.GetAuthHeaders. These are config/permission +// issues (missing credentials, bad App private key, JWT exchange failure) rather than +// transient network blips — retrying via replan won't make the credentials valid. +// Without this typing, such failures would fall through PostArchiveErrorMessage's +// catch-all and show a misleading "transient error, comment to retry" message. +type authError struct{ err error } + +func (e *authError) Error() string { return e.err.Error() } +func (e *authError) Unwrap() error { return e.err } + // Manager manages archive-based repository access // It provides a similar interface to git.RepoManager but uses VCS archives instead type Manager struct { @@ -61,10 +71,12 @@ func (m *Manager) Clone(ctx context.Context, cloneURL, branchName string, pr vcs return nil, errors.Wrap(err, "failed to get archive URL from VCS") } - // Extract merge commit SHA from archive URL for cache key - // Archive URLs contain the SHA: https://github.com/owner/repo/archive/{sha}.zip + // Extract merge commit SHA from archive URL for cache key. + // Archive URLs contain the SHA, e.g.: + // GitHub: https://api.github.com/repos/owner/repo/zipball/{sha} + // GitLab: https://gitlab.com/api/v4/projects/{id}/repository/archive.zip?sha={ref} // IMPORTANT: Must use merge commit SHA, not HEAD SHA, as cache key! - // Otherwise, cache returns stale archives when new commits are pushed to existing PR + // Otherwise, cache returns stale archives when new commits are pushed to existing PR. mergeCommitSHA, err := extractSHAFromArchiveURL(archiveURL) if err != nil { return nil, &urlParseError{err: errors.Wrap(err, "failed to extract merge commit SHA from archive URL")} @@ -77,8 +89,12 @@ func (m *Manager) Clone(ctx context.Context, cloneURL, branchName string, pr vcs Str("head_sha", pr.SHA). Msg("extracted merge commit SHA from archive URL") - // Get authentication headers for archive download - authHeaders := m.vcsClient.GetAuthHeaders() + // Get authentication headers for archive download. For GitHub App auth this fetches + // a fresh installation token, so the call needs the request context. + authHeaders, err := m.vcsClient.GetAuthHeaders(ctx) + if err != nil { + return nil, &authError{err: errors.Wrap(err, "failed to get archive auth headers")} + } // Download and extract archive (or get from cache) extractedPath, err := m.cache.GetOrDownload(ctx, archiveURL, mergeCommitSHA, authHeaders) @@ -186,6 +202,7 @@ func (m *Manager) PostArchiveErrorMessage(ctx context.Context, pr vcs.PullReques // Classify by the error itself first; ctx.Err() is a fallback for cases where the // context was cancelled for an unrelated reason after Clone returned. var urlErr *urlParseError + var authErr *authError var httpErr *HTTPError hasHTTP := errors.As(cloneErr, &httpErr) @@ -241,6 +258,15 @@ func (m *Manager) PostArchiveErrorMessage(ctx context.Context, pr vcs.PullReques message = "⚠️ Access denied downloading the repository archive (HTTP 403 Forbidden).\n\n" + "Check that the kubechecks VCS token has sufficient repository permissions." + case errors.As(cloneErr, &authErr): + // Auth header construction failed before any HTTP call (e.g. no credentials + // configured, GitHub App installation token fetch failed, malformed PEM). + // Permanent — retrying via replan won't fix the configuration. + message = "⚠️ Kubechecks could not obtain credentials to download the repository archive.\n\n" + + "This is a configuration problem — check that the kubechecks VCS token or " + + "GitHub App credentials are valid. Comment `" + replan + "` will not help until " + + "the configuration is fixed." + case errors.As(cloneErr, &urlErr): // Unrecognized archive URL format — this is a bug, not something the user can retry message = "⚠️ Kubechecks could not parse the archive URL returned by the VCS.\n\n" + @@ -301,10 +327,25 @@ func (m *Manager) PostArchiveErrorMessage(ctx context.Context, pr vcs.PullReques // extractSHAFromArchiveURL extracts the commit SHA from an archive URL // Supports GitHub and GitLab archive URL formats: -// - GitHub: https://github.com/owner/repo/archive/{sha}.zip +// - GitHub REST API zipball: https://api.github.com/repos/owner/repo/zipball/{sha} +// - GitHub web archive (legacy): https://github.com/owner/repo/archive/{sha}.zip // - GitLab: https://gitlab.com/api/v4/projects/{encoded}/repository/archive.zip?sha={ref} func extractSHAFromArchiveURL(archiveURL string) (string, error) { - // Try GitHub format first: /archive/{sha}.zip or /archive/{sha}.tar.gz + // GitHub REST API zipball/tarball: /zipball/{sha} or /tarball/{sha} (no extension) + for _, marker := range []string{"/zipball/", "/tarball/"} { + if !strings.Contains(archiveURL, marker) { + continue + } + parts := strings.Split(archiveURL, marker) + // Strip any query string that may follow the SHA. + sha := strings.SplitN(parts[len(parts)-1], "?", 2)[0] + if sha == "" { + return "", fmt.Errorf("empty SHA extracted from archive URL: %s", archiveURL) + } + return sha, nil + } + + // GitHub web archive (legacy): /archive/{sha}.zip or /archive/{sha}.tar.gz if strings.Contains(archiveURL, "/archive/") { // Extract filename from URL path parts := strings.Split(archiveURL, "/archive/") diff --git a/pkg/archive/manager_sha_test.go b/pkg/archive/manager_sha_test.go index 7fed295a..6ac0502a 100644 --- a/pkg/archive/manager_sha_test.go +++ b/pkg/archive/manager_sha_test.go @@ -16,17 +16,39 @@ func TestExtractSHAFromArchiveURL(t *testing.T) { }{ // GitHub formats { - name: "GitHub zip", + name: "GitHub REST API zipball", + url: "https://api.github.com/repos/zapier/kubechecks/zipball/abc123def456", + wantSHA: "abc123def456", + }, + { + name: "GitHub Enterprise REST API zipball", + url: "https://github.example.com/api/v3/repos/zapier/kubechecks/zipball/abc123def456", + wantSHA: "abc123def456", + }, + { + name: "GitHub REST API tarball", + url: "https://api.github.com/repos/zapier/kubechecks/tarball/deadbeef", + wantSHA: "deadbeef", + }, + { + name: "GitHub REST API zipball full SHA", + url: "https://api.github.com/repos/owner/repo/zipball/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + wantSHA: "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + }, + { + // Legacy /archive/{sha}.zip format kept for backward compat with any cached or + // in-flight URLs that predate the switch to /zipball/. + name: "GitHub zip legacy", url: "https://github.com/zapier/kubechecks/archive/abc123def456.zip", wantSHA: "abc123def456", }, { - name: "GitHub Enterprise", + name: "GitHub Enterprise legacy archive", url: "https://github.example.com/zapier/kubechecks/archive/abc123def456.zip", wantSHA: "abc123def456", }, { - name: "GitHub full SHA", + name: "GitHub legacy full SHA", url: "https://github.com/owner/repo/archive/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2.zip", wantSHA: "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", }, @@ -69,6 +91,11 @@ func TestExtractSHAFromArchiveURL(t *testing.T) { url: "https://github.com/owner/repo/archive/", wantErr: true, }, + { + name: "GitHub zipball URL with empty SHA", + url: "https://api.github.com/repos/owner/repo/zipball/", + wantErr: true, + }, { name: "GitLab URL with empty sha param", url: "https://gitlab.com/api/v4/projects/group%2Frepo/repository/archive.zip?sha=", diff --git a/pkg/archive/manager_test.go b/pkg/archive/manager_test.go index 6fc41524..b67eae6e 100644 --- a/pkg/archive/manager_test.go +++ b/pkg/archive/manager_test.go @@ -127,6 +127,15 @@ func TestPostArchiveErrorMessage(t *testing.T) { wantMsgContains: []string{"parse", "kubechecks logs"}, wantMsgExcludes: []string{"kubechecks replan"}, }, + { + // Auth header construction failed before HTTP call — config problem, + // replan won't help until credentials are fixed. + name: "authError — config problem, no useful retry", + ctx: func() context.Context { return context.Background() }, + cloneErr: &authError{err: fmt.Errorf("failed to fetch GitHub App installation token")}, + wantMsgContains: []string{"credentials", "configuration", "GitHub App"}, + wantMsgExcludes: []string{"transient error"}, + }, { // 400/422/405/410 are permanent — no replan suggestion name: "HTTP 422 unprocessable — permanent 4xx, no retry", diff --git a/pkg/vcs/github_client/archive.go b/pkg/vcs/github_client/archive.go index 3f870a85..07b9b6bb 100644 --- a/pkg/vcs/github_client/archive.go +++ b/pkg/vcs/github_client/archive.go @@ -123,18 +123,27 @@ func (c *Client) DownloadArchive(ctx context.Context, pr vcs.PullRequest) (strin mergeCommitSHA := *ghPR.MergeCommitSHA - // Construct archive URL - // Format: https://github.com/{owner}/{repo}/archive/{sha}.zip - // Or for enterprise: https://{base_url}/{owner}/{repo}/archive/{sha}.zip + // Use the REST API zipball endpoint rather than the public web /archive/{sha}.zip URL. + // The web URL on github.com does not honor `Authorization: Bearer` for fine-grained + // PATs or App installation tokens on private repos and returns 404. The API endpoint + // honors all token types and 302s to a signed codeload URL on a different host; + // Go's http.Client strips the Authorization header on that cross-host redirect by + // default, so the signed URL (already authenticated via query string) is fetched + // without leaking the token. See issue #525. + // + // Enterprise detection mirrors createHttpClient: GHE requires both VcsBaseUrl and + // VcsUploadUrl to be set. VcsBaseUrl alone isn't a reliable signal because config + // defaults it to "https://github.com" when unset, so checking it would route + // cloud users into the enterprise branch and build a bogus + // `https://github.com/repos/...` URL that 404s. var archiveURL string - if c.cfg.VcsBaseUrl != "" { - // GitHub Enterprise - baseURL := strings.TrimSuffix(c.cfg.VcsBaseUrl, "/api/v3") - baseURL = strings.TrimSuffix(baseURL, "/") - archiveURL = fmt.Sprintf("%s/%s/%s/archive/%s.zip", baseURL, pr.Owner, pr.Name, mergeCommitSHA) + if c.cfg.VcsUploadUrl != "" && c.cfg.VcsBaseUrl != "" { + // GitHub Enterprise: VcsBaseUrl is the API base (typically includes /api/v3). + baseURL := strings.TrimSuffix(c.cfg.VcsBaseUrl, "/") + archiveURL = fmt.Sprintf("%s/repos/%s/%s/zipball/%s", baseURL, pr.Owner, pr.Name, mergeCommitSHA) } else { // GitHub.com - archiveURL = fmt.Sprintf("https://github.com/%s/%s/archive/%s.zip", pr.Owner, pr.Name, mergeCommitSHA) + archiveURL = fmt.Sprintf("https://api.github.com/repos/%s/%s/zipball/%s", pr.Owner, pr.Name, mergeCommitSHA) } log.Debug(). diff --git a/pkg/vcs/github_client/client.go b/pkg/vcs/github_client/client.go index fd896a74..63f0ab98 100644 --- a/pkg/vcs/github_client/client.go +++ b/pkg/vcs/github_client/client.go @@ -21,11 +21,24 @@ import ( var tracer = otel.Tracer("pkg/vcs/github_client") +// installationTokenSource fetches a fresh installation token. Implemented by +// *ghinstallation.Transport in production; stubbed in tests to exercise the App auth +// branch of GetAuthHeaders without spinning up a real JWT signer. +type installationTokenSource interface { + Token(ctx context.Context) (string, error) +} + type Client struct { shurcoolClient *githubv4.Client googleClient *GClient cfg config.ServerConfig + // appTokenSource is set when the client is authenticated as a GitHub App. It is used + // by GetAuthHeaders to fetch a fresh installation token for archive downloads, since + // the archive downloader uses a separate http.Client and must attach the auth header + // itself rather than going through the SDK's authenticated transport. + appTokenSource installationTokenSource + // archiveRetry overrides retry parameters for DownloadArchive. Zero value uses defaults. archiveRetry retryConfig @@ -50,7 +63,7 @@ func CreateGithubClient(ctx context.Context, cfg config.ServerConfig) (*Client, shurcoolClient *githubv4.Client ) - githubClient, err := createHttpClient(ctx, cfg) + githubClient, appTransport, err := createHttpClient(ctx, cfg) if err != nil { return nil, errors.Wrap(err, "failed to create github http client") } @@ -81,6 +94,9 @@ func CreateGithubClient(ctx context.Context, cfg config.ServerConfig) (*Client, username: cfg.VcsUsername, email: cfg.VcsEmail, } + if appTransport != nil { + client.appTokenSource = appTransport + } if client.username == "" || client.email == "" { user, _, err := googleClient.Users.Get(ctx, "") @@ -105,17 +121,21 @@ func CreateGithubClient(ctx context.Context, cfg config.ServerConfig) (*Client, return client, nil } -func createHttpClient(ctx context.Context, cfg config.ServerConfig) (*http.Client, error) { +// createHttpClient returns the authenticated http.Client used by the go-github SDK. +// For GitHub App auth it also returns the underlying ghinstallation.Transport so callers +// can fetch installation tokens directly (used by GetAuthHeaders for archive downloads). +// For PAT auth the second return value is nil. +func createHttpClient(ctx context.Context, cfg config.ServerConfig) (*http.Client, *ghinstallation.Transport, error) { // Initialize the GitHub client with app key if provided if cfg.IsGithubApp() { appTransport, err := ghinstallation.New( http.DefaultTransport, cfg.GithubAppID, cfg.GithubInstallationID, []byte(cfg.GithubPrivateKey), ) if err != nil { - return nil, errors.Wrap(err, "failed to create github app transport") + return nil, nil, errors.Wrap(err, "failed to create github app transport") } - return &http.Client{Transport: appTransport}, nil + return &http.Client{Transport: appTransport}, appTransport, nil } // Initialize the GitHub client with access token if app key is not provided @@ -125,10 +145,10 @@ func createHttpClient(ctx context.Context, cfg config.ServerConfig) (*http.Clien ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: vcsToken}, ) - return oauth2.NewClient(ctx, ts), nil + return oauth2.NewClient(ctx, ts), nil, nil } - return nil, errors.New("Either GitHub token or GitHub App credentials (App ID, Installation ID, Private Key) must be set") + return nil, nil, errors.New("Either GitHub token or GitHub App credentials (App ID, Installation ID, Private Key) must be set") } func (c *Client) Username() string { return c.username } @@ -145,13 +165,25 @@ func (c *Client) CloneUsername() string { } } -// GetAuthHeaders returns HTTP headers needed for authenticated archive downloads -func (c *Client) GetAuthHeaders() map[string]string { - // GitHub accepts: Authorization: Bearer or Authorization: token - // Using Bearer format as it's the modern standard - return map[string]string{ - "Authorization": fmt.Sprintf("Bearer %s", c.cfg.VcsToken), +// GetAuthHeaders returns HTTP headers needed for authenticated archive downloads. +// For GitHub App auth it fetches a fresh installation token from the underlying transport; +// for PAT auth it returns the configured token. GitHub accepts both `Bearer ` and +// `token `; Bearer is the modern form. +func (c *Client) GetAuthHeaders(ctx context.Context) (map[string]string, error) { + token := c.cfg.VcsToken + if c.appTokenSource != nil { + t, err := c.appTokenSource.Token(ctx) + if err != nil { + return nil, errors.Wrap(err, "failed to fetch GitHub App installation token") + } + token = t } + if token == "" { + return nil, errors.New("no GitHub credentials configured for archive download") + } + return map[string]string{ + "Authorization": fmt.Sprintf("Bearer %s", token), + }, nil } var nilPr vcs.PullRequest diff --git a/pkg/vcs/github_client/client_test.go b/pkg/vcs/github_client/client_test.go index 483a2c31..2034d048 100644 --- a/pkg/vcs/github_client/client_test.go +++ b/pkg/vcs/github_client/client_test.go @@ -505,10 +505,57 @@ func TestClient_GetAuthHeaders(t *testing.T) { }, } - headers := c.GetAuthHeaders() + headers, err := c.GetAuthHeaders(context.Background()) + require.NoError(t, err) assert.Equal(t, "Bearer ghp_test_token_12345", headers["Authorization"]) } +func TestClient_GetAuthHeaders_NoCredentials(t *testing.T) { + // Neither PAT nor App transport configured — should error rather than emit + // an empty `Bearer ` header that the VCS will reject. + c := &Client{cfg: config.ServerConfig{}} + + _, err := c.GetAuthHeaders(context.Background()) + require.Error(t, err) +} + +// stubTokenSource is a testing installationTokenSource that returns a fixed token +// (or error) without needing a real ghinstallation.Transport / JWT signer. +type stubTokenSource struct { + token string + err error +} + +func (s stubTokenSource) Token(_ context.Context) (string, error) { + return s.token, s.err +} + +func TestClient_GetAuthHeaders_GitHubApp(t *testing.T) { + // In App mode VcsToken is empty; the Bearer token must come from the installation + // token source, not the empty config field. + c := &Client{ + cfg: config.ServerConfig{}, + appTokenSource: stubTokenSource{token: "ghs_installation_token_xyz"}, + } + + headers, err := c.GetAuthHeaders(context.Background()) + require.NoError(t, err) + assert.Equal(t, "Bearer ghs_installation_token_xyz", headers["Authorization"]) +} + +func TestClient_GetAuthHeaders_GitHubApp_TokenFetchError(t *testing.T) { + // Token() failure (e.g. expired private key, network failure during JWT exchange) + // must surface as an error rather than emit an empty Bearer header. + c := &Client{ + cfg: config.ServerConfig{}, + appTokenSource: stubTokenSource{err: fmt.Errorf("upstream JWT exchange failed")}, + } + + _, err := c.GetAuthHeaders(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "installation token") +} + func TestClient_VerifyHook(t *testing.T) { tests := []struct { name string @@ -913,7 +960,7 @@ func TestClient_DownloadArchive_HappyPath(t *testing.T) { url, err := c.DownloadArchive(context.Background(), pr) require.NoError(t, err) - assert.Equal(t, "https://github.com/owner/repo/archive/merge-abc123.zip", url) + assert.Equal(t, "https://api.github.com/repos/owner/repo/zipball/merge-abc123", url) mockPR.AssertExpectations(t) } @@ -952,7 +999,7 @@ func TestClient_DownloadArchive_StaleMergeCommitThenReady(t *testing.T) { url, err := c.DownloadArchive(context.Background(), pr) require.NoError(t, err) // Must use the fresh merge SHA, not the stale one - assert.Equal(t, "https://github.com/owner/repo/archive/fresh-new-merge.zip", url) + assert.Equal(t, "https://api.github.com/repos/owner/repo/zipball/fresh-new-merge", url) assert.NotContains(t, url, "stale-old-merge") mockPR.AssertExpectations(t) } @@ -1036,7 +1083,10 @@ func TestClient_DownloadArchive_GHEnterprise(t *testing.T) { c := &Client{ googleClient: &GClient{PullRequests: mockPR}, - cfg: config.ServerConfig{VcsBaseUrl: "https://github.example.com/api/v3"}, + cfg: config.ServerConfig{ + VcsBaseUrl: "https://github.example.com/api/v3", + VcsUploadUrl: "https://github.example.com/api/uploads", + }, archiveRetry: fastRetry, } @@ -1050,7 +1100,40 @@ func TestClient_DownloadArchive_GHEnterprise(t *testing.T) { url, err := c.DownloadArchive(context.Background(), pr) require.NoError(t, err) - assert.Equal(t, "https://github.example.com/myorg/myrepo/archive/merge-sha.zip", url) + assert.Equal(t, "https://github.example.com/api/v3/repos/myorg/myrepo/zipball/merge-sha", url) + mockPR.AssertExpectations(t) +} + +func TestClient_DownloadArchive_DefaultedVcsBaseUrl(t *testing.T) { + // pkg/config defaults VcsBaseUrl to "https://github.com" when unset, so a cloud + // configuration arrives at DownloadArchive with a non-empty VcsBaseUrl. The cloud + // branch must still fire (api.github.com), not the enterprise branch which would + // build a bogus `https://github.com/repos/...` URL. See issue #525 review. + mockPR := NewMockPullRequestsServicesWithGet(t, + prResponse{ + headSHA: "sha1", + mergeCommitSHA: "merge-sha", + mergeable: github.Ptr(true), + }, + ) + + c := &Client{ + googleClient: &GClient{PullRequests: mockPR}, + cfg: config.ServerConfig{VcsBaseUrl: "https://github.com"}, // no VcsUploadUrl + archiveRetry: fastRetry, + } + + pr := vcs.PullRequest{ + Owner: "owner", + Name: "repo", + FullName: "owner/repo", + CheckID: 1, + SHA: "sha1", + } + + url, err := c.DownloadArchive(context.Background(), pr) + require.NoError(t, err) + assert.Equal(t, "https://api.github.com/repos/owner/repo/zipball/merge-sha", url) mockPR.AssertExpectations(t) } diff --git a/pkg/vcs/gitlab_client/client.go b/pkg/vcs/gitlab_client/client.go index 0e553125..37b75343 100644 --- a/pkg/vcs/gitlab_client/client.go +++ b/pkg/vcs/gitlab_client/client.go @@ -101,11 +101,11 @@ func (c *Client) CloneUsername() string { return c.username } func (c *Client) GetName() string { return "gitlab" } // GetAuthHeaders returns HTTP headers needed for authenticated archive downloads -func (c *Client) GetAuthHeaders() map[string]string { +func (c *Client) GetAuthHeaders(_ context.Context) (map[string]string, error) { // GitLab uses PRIVATE-TOKEN header for authentication return map[string]string{ "PRIVATE-TOKEN": c.cfg.VcsToken, - } + }, nil } // VerifyHook returns an err if the webhook isn't valid diff --git a/pkg/vcs/gitlab_client/client_test.go b/pkg/vcs/gitlab_client/client_test.go index b21ab745..a13ae318 100644 --- a/pkg/vcs/gitlab_client/client_test.go +++ b/pkg/vcs/gitlab_client/client_test.go @@ -253,7 +253,8 @@ func TestClient_GetAuthHeaders(t *testing.T) { }, } - headers := c.GetAuthHeaders() + headers, err := c.GetAuthHeaders(context.Background()) + require.NoError(t, err) assert.Equal(t, "test-token-12345", headers["PRIVATE-TOKEN"]) } diff --git a/pkg/vcs/types.go b/pkg/vcs/types.go index cdc59ac8..ef5f10f7 100644 --- a/pkg/vcs/types.go +++ b/pkg/vcs/types.go @@ -43,8 +43,10 @@ type Client interface { // DownloadArchive downloads a repository archive for a specific commit SHA // Returns the archive URL that can be used to download the zip file DownloadArchive(ctx context.Context, pr PullRequest) (string, error) - // GetAuthHeaders returns HTTP headers needed for authenticated archive downloads - GetAuthHeaders() map[string]string + // GetAuthHeaders returns HTTP headers needed for authenticated archive downloads. + // For GitHub App auth, this fetches a fresh installation token, so the call may + // perform a network request and must be passed a context. + GetAuthHeaders(ctx context.Context) (map[string]string, error) // PostReviewSuggestions posts a PR review with inline code suggestions. // Each suggestion targets a specific file+line in the PR diff.