diff --git a/internal/dispatch/gcf/mintsrc/mintcore/github.go.embed b/internal/dispatch/gcf/mintsrc/mintcore/github.go.embed index 265e514d54..2ff0e0b5a1 100644 --- a/internal/dispatch/gcf/mintsrc/mintcore/github.go.embed +++ b/internal/dispatch/gcf/mintsrc/mintcore/github.go.embed @@ -22,6 +22,30 @@ import ( "time" ) +// GitHubAPIError is returned when the GitHub API responds with a non-success +// status code. It preserves the upstream status code and response body so +// callers can distinguish client errors (4xx) from server errors (5xx) and +// surface GitHub's error message in logs and client responses. +type GitHubAPIError struct { + StatusCode int + Body string + Action string // what we were doing, e.g. "creating installation token" +} + +func (e *GitHubAPIError) Error() string { + if e.Body != "" { + return fmt.Sprintf("%s returned status %d: %s", e.Action, e.StatusCode, e.Body) + } + return fmt.Sprintf("%s returned status %d", e.Action, e.StatusCode) +} + +// readErrorBody reads up to maxBytes from r and returns the content as a +// trimmed string. +func readErrorBody(r io.Reader, maxBytes int64) string { + body, _ := io.ReadAll(io.LimitReader(r, maxBytes)) + return strings.TrimSpace(string(body)) +} + // installationResponse is the response from GET /repos/{owner}/{repo}/installation. type installationResponse struct { ID int64 `json:"id"` @@ -253,8 +277,12 @@ func FindInstallation(ctx context.Context, httpClient HTTPDoer, githubBaseURL, j defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return 0, fmt.Errorf("getting installation for %s/%s returned status %d", org, repo, resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return 0, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: fmt.Sprintf("getting installation for %s/%s", org, repo), + } } var inst installationResponse @@ -293,8 +321,12 @@ func FindOrgInstallation(ctx context.Context, httpClient HTTPDoer, githubBaseURL defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return 0, fmt.Errorf("getting org installation for %s returned status %d", org, resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return 0, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: fmt.Sprintf("getting org installation for %s", org), + } } var inst installationResponse @@ -340,8 +372,12 @@ func GetOrgVariable(ctx context.Context, httpClient HTTPDoer, githubBaseURL, ins return "", false, nil } if resp.StatusCode != http.StatusOK { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return "", false, fmt.Errorf("getting org variable %s returned status %d", name, resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return "", false, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: fmt.Sprintf("getting org variable %s", name), + } } var varResp orgVariableResponse @@ -386,8 +422,12 @@ func createInstallationTokenWithPermissions(ctx context.Context, httpClient HTTP defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return "", fmt.Errorf("creating installation token returned status %d", resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return "", &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: "creating installation token", + } } var tokenResp installationTokenResponse @@ -453,8 +493,12 @@ func CreateInstallationToken(ctx context.Context, httpClient HTTPDoer, githubBas defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return "", "", nil, fmt.Errorf("creating installation token returned status %d", resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return "", "", nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: "creating installation token", + } } var tokenResp installationTokenResponse diff --git a/internal/dispatch/gcf/mintsrc/mintcore/handler.go.embed b/internal/dispatch/gcf/mintsrc/mintcore/handler.go.embed index 8b951277d4..3bb5728aab 100644 --- a/internal/dispatch/gcf/mintsrc/mintcore/handler.go.embed +++ b/internal/dispatch/gcf/mintsrc/mintcore/handler.go.embed @@ -365,12 +365,12 @@ func (h *Handler) mintToken(ctx context.Context, org, role string, repos []strin installationID, err = FindInstallation(ctx, h.httpClient, h.githubBaseURL, jwt, org, repos[0]) } if err != nil { - return "", "", nil, &mintError{status: http.StatusBadGateway, msg: err.Error()} + return "", "", nil, &mintError{status: upstreamStatus(err), msg: err.Error()} } token, expiresAt, granted, err := CreateInstallationToken(ctx, h.httpClient, h.githubBaseURL, jwt, installationID, role, repos) if err != nil { - return "", "", nil, &mintError{status: http.StatusBadGateway, msg: err.Error()} + return "", "", nil, &mintError{status: upstreamStatus(err), msg: err.Error()} } if granted != nil { @@ -384,7 +384,7 @@ func (h *Handler) mintToken(ctx context.Context, org, role string, repos []strin func (h *Handler) mintTokenCrossOrg(ctx context.Context, claims *Claims, targetOrg, role string, repos []string) (string, string, *GrantedScope, error) { allowlist, err := h.loadForeignAllowlist(ctx, targetOrg, role) if err != nil { - return "", "", nil, &mintError{status: http.StatusBadGateway, msg: err.Error()} + return "", "", nil, &mintError{status: upstreamStatus(err), msg: err.Error()} } if len(allowlist) == 0 { return "", "", nil, &mintError{status: http.StatusForbidden, msg: "foreign caller not authorized for target org"} @@ -462,7 +462,7 @@ func (h *Handler) fetchForeignAllowlist(ctx context.Context, targetOrg, role str installationID, err := FindOrgInstallation(ctx, h.httpClient, h.githubBaseURL, jwt, targetOrg) if err != nil { - return nil, fmt.Errorf("finding org installation on %s: %v", targetOrg, err) + return nil, fmt.Errorf("finding org installation on %s: %w", targetOrg, err) } allowlist, err := ReadForeignAllowlist(ctx, h.httpClient, h.githubBaseURL, jwt, installationID, targetOrg, role) @@ -537,6 +537,18 @@ func (h *Handler) lookupRoleAppID(role string) (string, error) { return appID, nil } +// upstreamStatus inspects err for a GitHubAPIError. If GitHub returned a +// 4xx client error the caller's request is at fault and will never succeed on +// retry, so we forward that status. For 5xx or non-GitHub errors we return 502 +// (Bad Gateway) so clients know to retry. +func upstreamStatus(err error) int { + var ghErr *GitHubAPIError + if errors.As(err, &ghErr) && ghErr.StatusCode >= 400 && ghErr.StatusCode < 500 { + return ghErr.StatusCode + } + return http.StatusBadGateway +} + // mintError is an HTTP-aware error carrying a status code for the response. type mintError struct { status int diff --git a/internal/mintcore/github.go b/internal/mintcore/github.go index 265e514d54..2ff0e0b5a1 100644 --- a/internal/mintcore/github.go +++ b/internal/mintcore/github.go @@ -22,6 +22,30 @@ import ( "time" ) +// GitHubAPIError is returned when the GitHub API responds with a non-success +// status code. It preserves the upstream status code and response body so +// callers can distinguish client errors (4xx) from server errors (5xx) and +// surface GitHub's error message in logs and client responses. +type GitHubAPIError struct { + StatusCode int + Body string + Action string // what we were doing, e.g. "creating installation token" +} + +func (e *GitHubAPIError) Error() string { + if e.Body != "" { + return fmt.Sprintf("%s returned status %d: %s", e.Action, e.StatusCode, e.Body) + } + return fmt.Sprintf("%s returned status %d", e.Action, e.StatusCode) +} + +// readErrorBody reads up to maxBytes from r and returns the content as a +// trimmed string. +func readErrorBody(r io.Reader, maxBytes int64) string { + body, _ := io.ReadAll(io.LimitReader(r, maxBytes)) + return strings.TrimSpace(string(body)) +} + // installationResponse is the response from GET /repos/{owner}/{repo}/installation. type installationResponse struct { ID int64 `json:"id"` @@ -253,8 +277,12 @@ func FindInstallation(ctx context.Context, httpClient HTTPDoer, githubBaseURL, j defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return 0, fmt.Errorf("getting installation for %s/%s returned status %d", org, repo, resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return 0, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: fmt.Sprintf("getting installation for %s/%s", org, repo), + } } var inst installationResponse @@ -293,8 +321,12 @@ func FindOrgInstallation(ctx context.Context, httpClient HTTPDoer, githubBaseURL defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return 0, fmt.Errorf("getting org installation for %s returned status %d", org, resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return 0, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: fmt.Sprintf("getting org installation for %s", org), + } } var inst installationResponse @@ -340,8 +372,12 @@ func GetOrgVariable(ctx context.Context, httpClient HTTPDoer, githubBaseURL, ins return "", false, nil } if resp.StatusCode != http.StatusOK { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return "", false, fmt.Errorf("getting org variable %s returned status %d", name, resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return "", false, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: fmt.Sprintf("getting org variable %s", name), + } } var varResp orgVariableResponse @@ -386,8 +422,12 @@ func createInstallationTokenWithPermissions(ctx context.Context, httpClient HTTP defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return "", fmt.Errorf("creating installation token returned status %d", resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return "", &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: "creating installation token", + } } var tokenResp installationTokenResponse @@ -453,8 +493,12 @@ func CreateInstallationToken(ctx context.Context, httpClient HTTPDoer, githubBas defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) - return "", "", nil, fmt.Errorf("creating installation token returned status %d", resp.StatusCode) + body := readErrorBody(resp.Body, 4096) + return "", "", nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + Body: body, + Action: "creating installation token", + } } var tokenResp installationTokenResponse diff --git a/internal/mintcore/github_test.go b/internal/mintcore/github_test.go index f7d3dfd450..7bb6767a87 100644 --- a/internal/mintcore/github_test.go +++ b/internal/mintcore/github_test.go @@ -419,21 +419,86 @@ func TestReadForeignAllowlist_EmptyVariable(t *testing.T) { func TestFindOrgInstallation_NotFound(t *testing.T) { mockGH := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"message":"Not Found"}`)) })) defer mockGH.Close() _, err := FindOrgInstallation(t.Context(), http.DefaultClient, mockGH.URL, "fake-jwt", "myorg") require.Error(t, err) assert.Contains(t, err.Error(), "status 404") + + var ghErr *GitHubAPIError + require.ErrorAs(t, err, &ghErr) + assert.Equal(t, 404, ghErr.StatusCode) + assert.Contains(t, ghErr.Body, "Not Found") +} + +func TestCreateInstallationToken_NonCreatedStatus_IncludesBody(t *testing.T) { + ghBody := `{"message":"Validation Failed","errors":[{"resource":"Repository","code":"invalid"}]}` + mockGH := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + w.Write([]byte(ghBody)) + })) + defer mockGH.Close() + + _, _, _, err := CreateInstallationToken(t.Context(), http.DefaultClient, mockGH.URL, "fake-jwt", 42, "coder", []string{"stale-repo"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "422") + assert.Contains(t, err.Error(), "Validation Failed") + + var ghErr *GitHubAPIError + require.ErrorAs(t, err, &ghErr) + assert.Equal(t, http.StatusUnprocessableEntity, ghErr.StatusCode) + assert.Contains(t, ghErr.Body, "Validation Failed") +} + +func TestCreateInstallationToken_5xxStatus_IncludesBody(t *testing.T) { + mockGH := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(`{"message":"Internal Server Error"}`)) + })) + defer mockGH.Close() + + _, _, _, err := CreateInstallationToken(t.Context(), http.DefaultClient, mockGH.URL, "fake-jwt", 42, "coder", []string{"repo"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "500") + assert.Contains(t, err.Error(), "Internal Server Error") + + var ghErr *GitHubAPIError + require.ErrorAs(t, err, &ghErr) + assert.Equal(t, http.StatusInternalServerError, ghErr.StatusCode) +} + +func TestFindInstallation_ErrorIncludesBody(t *testing.T) { + mockGH := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message":"Resource not accessible by integration"}`)) + })) + defer mockGH.Close() + + _, err := FindInstallation(t.Context(), http.DefaultClient, mockGH.URL, "fake-jwt", "myorg", "my-repo") + require.Error(t, err) + assert.Contains(t, err.Error(), "403") + assert.Contains(t, err.Error(), "Resource not accessible") + + var ghErr *GitHubAPIError + require.ErrorAs(t, err, &ghErr) + assert.Equal(t, http.StatusForbidden, ghErr.StatusCode) } func TestGetOrgVariable_ErrorStatus(t *testing.T) { mockGH := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message":"Resource not accessible"}`)) })) defer mockGH.Close() _, _, err := GetOrgVariable(t.Context(), http.DefaultClient, mockGH.URL, "ghs_policy", "pool-org", "VAR") require.Error(t, err) assert.Contains(t, err.Error(), "status 403") + + var ghErr *GitHubAPIError + require.ErrorAs(t, err, &ghErr) + assert.Equal(t, 403, ghErr.StatusCode) + assert.Contains(t, ghErr.Body, "Resource not accessible") } diff --git a/internal/mintcore/handler.go b/internal/mintcore/handler.go index 8b951277d4..3bb5728aab 100644 --- a/internal/mintcore/handler.go +++ b/internal/mintcore/handler.go @@ -365,12 +365,12 @@ func (h *Handler) mintToken(ctx context.Context, org, role string, repos []strin installationID, err = FindInstallation(ctx, h.httpClient, h.githubBaseURL, jwt, org, repos[0]) } if err != nil { - return "", "", nil, &mintError{status: http.StatusBadGateway, msg: err.Error()} + return "", "", nil, &mintError{status: upstreamStatus(err), msg: err.Error()} } token, expiresAt, granted, err := CreateInstallationToken(ctx, h.httpClient, h.githubBaseURL, jwt, installationID, role, repos) if err != nil { - return "", "", nil, &mintError{status: http.StatusBadGateway, msg: err.Error()} + return "", "", nil, &mintError{status: upstreamStatus(err), msg: err.Error()} } if granted != nil { @@ -384,7 +384,7 @@ func (h *Handler) mintToken(ctx context.Context, org, role string, repos []strin func (h *Handler) mintTokenCrossOrg(ctx context.Context, claims *Claims, targetOrg, role string, repos []string) (string, string, *GrantedScope, error) { allowlist, err := h.loadForeignAllowlist(ctx, targetOrg, role) if err != nil { - return "", "", nil, &mintError{status: http.StatusBadGateway, msg: err.Error()} + return "", "", nil, &mintError{status: upstreamStatus(err), msg: err.Error()} } if len(allowlist) == 0 { return "", "", nil, &mintError{status: http.StatusForbidden, msg: "foreign caller not authorized for target org"} @@ -462,7 +462,7 @@ func (h *Handler) fetchForeignAllowlist(ctx context.Context, targetOrg, role str installationID, err := FindOrgInstallation(ctx, h.httpClient, h.githubBaseURL, jwt, targetOrg) if err != nil { - return nil, fmt.Errorf("finding org installation on %s: %v", targetOrg, err) + return nil, fmt.Errorf("finding org installation on %s: %w", targetOrg, err) } allowlist, err := ReadForeignAllowlist(ctx, h.httpClient, h.githubBaseURL, jwt, installationID, targetOrg, role) @@ -537,6 +537,18 @@ func (h *Handler) lookupRoleAppID(role string) (string, error) { return appID, nil } +// upstreamStatus inspects err for a GitHubAPIError. If GitHub returned a +// 4xx client error the caller's request is at fault and will never succeed on +// retry, so we forward that status. For 5xx or non-GitHub errors we return 502 +// (Bad Gateway) so clients know to retry. +func upstreamStatus(err error) int { + var ghErr *GitHubAPIError + if errors.As(err, &ghErr) && ghErr.StatusCode >= 400 && ghErr.StatusCode < 500 { + return ghErr.StatusCode + } + return http.StatusBadGateway +} + // mintError is an HTTP-aware error carrying a status code for the response. type mintError struct { status int diff --git a/internal/mintcore/handler_test.go b/internal/mintcore/handler_test.go index 07001d60fb..82567aa990 100644 --- a/internal/mintcore/handler_test.go +++ b/internal/mintcore/handler_test.go @@ -1272,8 +1272,9 @@ func TestHandler_InstallationNotFound(t *testing.T) { req.Header.Set("Authorization", "Bearer "+token) env.handler.ServeHTTP(rec, req) - if rec.Code != http.StatusBadGateway { - t.Fatalf("expected 502, got %d: %s", rec.Code, rec.Body.String()) + // GitHub 404 is a client error (4xx) — forwarded instead of collapsed to 502. + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d: %s", rec.Code, rec.Body.String()) } var resp map[string]string @@ -1283,6 +1284,102 @@ func TestHandler_InstallationNotFound(t *testing.T) { } } +func TestHandler_GitHubTokenCreation422_ForwardedAsIs(t *testing.T) { + t.Setenv("ROLE_APP_IDS", `{"coder":"200"}`) + + pemData, err := generateTestRSAKey() + if err != nil { + t.Fatalf("generating test key: %v", err) + } + + env := newTestOIDCEnv(t, &fakePEMAccessor{ + pems: map[string][]byte{"coder": pemData}, + }) + token := env.signToken(t, nil) + + github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/repos/test-org/test-repo/installation" && r.Method == http.MethodGet: + json.NewEncoder(w).Encode(installationResponse{ + ID: 12345, Account: struct { + Login string `json:"login"` + }{Login: "test-org"}, + }) + case strings.HasPrefix(r.URL.Path, "/app/installations/12345/access_tokens") && r.Method == http.MethodPost: + // Simulate GitHub rejecting the token request with 422 + // (e.g. one of the requested repos was transferred). + w.WriteHeader(http.StatusUnprocessableEntity) + w.Write([]byte(`{"message":"Validation Failed","errors":[{"resource":"Repository","code":"invalid"}]}`)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer github.Close() + env.handler.githubBaseURL = github.URL + + body := `{"role":"coder","repos":["test-repo"]}` + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/v1/token", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+token) + env.handler.ServeHTTP(rec, req) + + // 422 is a client error — forwarded instead of collapsed to 502. + if rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("expected 422, got %d: %s", rec.Code, rec.Body.String()) + } + + var resp map[string]string + json.NewDecoder(rec.Body).Decode(&resp) + if resp["error"] != "mint failed" { + t.Fatalf("expected 'mint failed', got: %s", resp["error"]) + } +} + +func TestHandler_GitHub5xx_Returns502(t *testing.T) { + t.Setenv("ROLE_APP_IDS", `{"coder":"200"}`) + + pemData, err := generateTestRSAKey() + if err != nil { + t.Fatalf("generating test key: %v", err) + } + + env := newTestOIDCEnv(t, &fakePEMAccessor{ + pems: map[string][]byte{"coder": pemData}, + }) + token := env.signToken(t, nil) + + github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/repos/test-org/test-repo/installation" && r.Method == http.MethodGet: + json.NewEncoder(w).Encode(installationResponse{ + ID: 12345, Account: struct { + Login string `json:"login"` + }{Login: "test-org"}, + }) + case strings.HasPrefix(r.URL.Path, "/app/installations/12345/access_tokens") && r.Method == http.MethodPost: + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(`{"message":"Internal Server Error"}`)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer github.Close() + env.handler.githubBaseURL = github.URL + + body := `{"role":"coder","repos":["test-repo"]}` + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/v1/token", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+token) + env.handler.ServeHTTP(rec, req) + + // 5xx errors should still be 502 (Bad Gateway). + if rec.Code != http.StatusBadGateway { + t.Fatalf("expected 502, got %d: %s", rec.Code, rec.Body.String()) + } +} + func TestHandler_LargeBody(t *testing.T) { h := mustNewHandler(t, &fakePEMAccessor{}, &fakeOIDCVerifier{}) largePayload := bytes.Repeat([]byte("x"), 128<<10)