diff --git a/internal/runtime/github_token_service.go b/internal/runtime/github_token_service.go index d9058116..e9a63ba2 100644 --- a/internal/runtime/github_token_service.go +++ b/internal/runtime/github_token_service.go @@ -18,6 +18,10 @@ import ( const githubTokenResponseLimit = 64 << 10 +// Rejection reasons come from an untrusted response body, so cap them well +// short of anything a terminal or log line needs to display in full. +const githubTokenRejectionReasonRuneLimit = 200 + var githubInstallationTokenPattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) var retryAfterSecondsPattern = regexp.MustCompile(`^[0-9]{1,10}$`) @@ -132,8 +136,8 @@ func (c *AgentGitHubTokens) mint(ctx context.Context, mintURL, repository, workf } defer func() { _ = response.Body.Close() }() if response.StatusCode < 200 || response.StatusCode >= 300 { - _, _ = io.Copy(io.Discard, io.LimitReader(response.Body, githubTokenResponseLimit)) - return "", githubTokenStatusError(response.StatusCode, response.Header.Get("Retry-After"), purpose) + errorBody, _ := io.ReadAll(io.LimitReader(response.Body, githubTokenResponseLimit)) + return "", githubTokenStatusError(response.StatusCode, response.Header.Get("Retry-After"), purpose, errorBody) } payload, err := io.ReadAll(io.LimitReader(response.Body, githubTokenResponseLimit+1)) if err != nil { @@ -193,10 +197,13 @@ func agentGitHubTokenURL(endpoint, jobID, endpointName string) (string, error) { return u.String(), nil } -func githubTokenStatusError(status int, retryAfter, purpose string) error { +func githubTokenStatusError(status int, retryAfter, purpose string, body []byte) error { credential := "GitHub " + purpose + " token" switch status { case http.StatusBadRequest: + if reason := githubTokenRejectionReason(body); reason != "" { + return fmt.Errorf("%s request was rejected: %s", credential, reason) + } return fmt.Errorf("%s request was rejected", credential) case http.StatusUnauthorized, http.StatusForbidden: return fmt.Errorf("%s request was denied", credential) @@ -215,3 +222,30 @@ func githubTokenStatusError(status int, retryAfter, purpose string) error { return fmt.Errorf("%s service returned HTTP %d", credential, status) } } + +var githubTokenControlCharPattern = regexp.MustCompile(`[\x00-\x1F\x7F]+`) + +// githubTokenRejectionReason turns a 400 response body into an operator-facing +// reason: the JSON "message" field the Agent API renders for its errors, or +// the raw body when it isn't that shape. +func githubTokenRejectionReason(body []byte) string { + trimmed := bytes.TrimSpace(body) + if len(trimmed) == 0 { + return "" + } + var decoded struct { + Message string `json:"message"` + } + if json.Unmarshal(trimmed, &decoded) == nil && decoded.Message != "" { + return sanitizeGithubTokenRejectionReason(decoded.Message) + } + return sanitizeGithubTokenRejectionReason(string(trimmed)) +} + +func sanitizeGithubTokenRejectionReason(reason string) string { + reason = strings.TrimSpace(githubTokenControlCharPattern.ReplaceAllString(reason, " ")) + if runes := []rune(reason); len(runes) > githubTokenRejectionReasonRuneLimit { + reason = string(runes[:githubTokenRejectionReasonRuneLimit]) + } + return reason +} diff --git a/internal/runtime/github_token_service_test.go b/internal/runtime/github_token_service_test.go index bfd58a75..5159ce1e 100644 --- a/internal/runtime/github_token_service_test.go +++ b/internal/runtime/github_token_service_test.go @@ -144,28 +144,33 @@ func TestAgentGitHubTokensRejectsUnsafeConfigurationAndRepository(t *testing.T) } func TestAgentGitHubTokensRejectsRedirectsAndUntrustedResponses(t *testing.T) { - secret := "ghs_must_not_leak" - for _, test := range []struct { + // Every response carries its own distinct secret so a leak names the branch that + // leaked it. Only the 400 branch may echo its own body: that body is the Agent + // API's rejection reason, which operators need to see. + const leakPrefix = "ghs_must_not_leak" + leaked := func(name string) string { return leakPrefix + "_" + name } + tests := []struct { name string status int body string retryAfter string want string }{ - {"rejected", http.StatusBadRequest, secret, "", "rejected"}, - {"unauthorized", http.StatusUnauthorized, secret, "", "denied"}, - {"denied", http.StatusForbidden, secret, "", "denied"}, - {"disabled", http.StatusNotFound, secret, "", "not enabled"}, - {"rate limited", http.StatusServiceUnavailable, secret, "60", "retry after 60 seconds"}, - {"unsafe retry header", http.StatusServiceUnavailable, secret, secret, "temporarily unavailable"}, - {"unexpected status", http.StatusBadGateway, secret, "", "HTTP 502"}, + {"rejected", http.StatusBadRequest, leaked("rejected"), "", "rejected"}, + {"unauthorized", http.StatusUnauthorized, leaked("unauthorized"), "", "denied"}, + {"denied", http.StatusForbidden, leaked("denied"), "", "denied"}, + {"disabled", http.StatusNotFound, leaked("disabled"), "", "not enabled"}, + {"rate limited", http.StatusServiceUnavailable, leaked("rate_limited"), "60", "retry after 60 seconds"}, + {"unsafe retry header", http.StatusServiceUnavailable, leaked("unsafe_body"), leaked("unsafe_header"), "temporarily unavailable"}, + {"unexpected status", http.StatusBadGateway, leaked("unexpected_status"), "", "HTTP 502"}, {"malformed JSON", http.StatusOK, `{"token":`, "", "decode"}, {"unknown field", http.StatusOK, `{"token":"ghs_valid","other":true}`, "", "unknown field"}, {"trailing JSON", http.StatusOK, `{"token":"ghs_valid"}{}`, "", "trailing data"}, {"empty token", http.StatusOK, `{"token":""}`, "", "invalid token"}, {"invalid token", http.StatusOK, `{"token":"secret with spaces"}`, "", "invalid token"}, {"oversized", http.StatusOK, `{"token":"ghs_valid"}` + strings.Repeat(" ", githubTokenResponseLimit), "", "exceeds"}, - } { + } + for _, test := range tests { t.Run(test.name, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if test.retryAfter != "" { @@ -180,8 +185,20 @@ func TestAgentGitHubTokensRejectsRedirectsAndUntrustedResponses(t *testing.T) { t.Fatal(err) } _, err = provider.WorkflowToken(context.Background(), "buildkite/buildkite-gha", "ci.yml", map[string]string{"contents": "read"}) - if err == nil || !strings.Contains(err.Error(), test.want) || strings.Contains(err.Error(), secret) { - t.Fatalf("WorkflowToken() error = %v, want %q without response data", err, test.want) + if err == nil || !strings.Contains(err.Error(), test.want) { + t.Fatalf("WorkflowToken() error = %v, want %q", err, test.want) + } + if test.status == http.StatusBadRequest && !strings.Contains(err.Error(), test.body) { + t.Fatalf("WorkflowToken() error = %v, want the rejection reason from the response body", err) + } + for _, other := range tests { + echoesOwnBody := other.name == test.name && test.status == http.StatusBadRequest + if strings.HasPrefix(other.body, leakPrefix) && !echoesOwnBody && strings.Contains(err.Error(), other.body) { + t.Fatalf("WorkflowToken() error = %v, leaked the %q response body", err, other.name) + } + if strings.HasPrefix(other.retryAfter, leakPrefix) && strings.Contains(err.Error(), other.retryAfter) { + t.Fatalf("WorkflowToken() error = %v, leaked the %q retry header", err, other.name) + } } }) } @@ -205,6 +222,93 @@ func TestAgentGitHubTokensRejectsRedirectsAndUntrustedResponses(t *testing.T) { } } +func TestAgentGitHubTokensAppendsSanitizedRejectionReason(t *testing.T) { + for _, test := range []struct { + name string + status int + body string + retryAfter string + want string + }{ + { + name: "JSON message", + status: http.StatusBadRequest, + body: `{"message":"Repository provider refused to issue token"}`, + want: "GitHub workflow token request was rejected: Repository provider refused to issue token", + }, + { + name: "plain text body", + status: http.StatusBadRequest, + body: "Repository provider refused to issue token", + want: "GitHub workflow token request was rejected: Repository provider refused to issue token", + }, + { + name: "empty body", + status: http.StatusBadRequest, + body: "", + want: "GitHub workflow token request was rejected", + }, + { + name: "rate limited unaffected by body", + status: http.StatusServiceUnavailable, + body: `{"message":"Repository provider refused to issue token"}`, + retryAfter: "30", + want: "GitHub workflow token service is temporarily unavailable; retry after 30 seconds", + }, + } { + t.Run(test.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if test.retryAfter != "" { + w.Header().Set("Retry-After", test.retryAfter) + } + w.WriteHeader(test.status) + _, _ = io.WriteString(w, test.body) + })) + defer server.Close() + provider, err := NewAgentGitHubTokens(AgentGitHubTokenConfig{Endpoint: server.URL, JobID: testCacheJobID, JobToken: "job-token"}) + if err != nil { + t.Fatal(err) + } + _, err = provider.WorkflowToken(context.Background(), "buildkite/buildkite-gha", "ci.yml", map[string]string{"contents": "read"}) + if err == nil || err.Error() != test.want { + t.Fatalf("WorkflowToken() error = %v, want %q", err, test.want) + } + }) + } +} + +func TestAgentGitHubTokensCollapsesAndTruncatesRejectionReason(t *testing.T) { + body := "line one\r\n\nline two\t\t" + strings.Repeat("z", 300) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = io.WriteString(w, body) + })) + defer server.Close() + provider, err := NewAgentGitHubTokens(AgentGitHubTokenConfig{Endpoint: server.URL, JobID: testCacheJobID, JobToken: "job-token"}) + if err != nil { + t.Fatal(err) + } + _, err = provider.WorkflowToken(context.Background(), "buildkite/buildkite-gha", "ci.yml", map[string]string{"contents": "read"}) + if err == nil { + t.Fatal("WorkflowToken() succeeded, want error") + } + const prefix = "GitHub workflow token request was rejected: " + message := err.Error() + if !strings.HasPrefix(message, prefix) { + t.Fatalf("WorkflowToken() error = %q, want prefix %q", message, prefix) + } + reason := strings.TrimPrefix(message, prefix) + if strings.ContainsAny(reason, "\r\n\t") { + t.Fatalf("reason retains control characters: %q", reason) + } + if !strings.Contains(reason, "line one line two") { + t.Fatalf("reason lost its collapsed text: %q", reason) + } + if got := len([]rune(reason)); got != githubTokenRejectionReasonRuneLimit { + t.Fatalf("reason length = %d, want %d", got, githubTokenRejectionReasonRuneLimit) + } +} + func TestAgentGitHubTokensHonorsCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel()