fix(errors): safely format GitHub validation failures

Limit structured formatting to HTTP 422 responses, sanitize allowlisted validation fields, and omit request, response, and documentation metadata while preserving other error contracts.

Refs #3080

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: SamMorrowDrums

pkg/errors/error.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/github/github-mcp-server/pkg/sanitize"
 	"github.com/github/github-mcp-server/pkg/utils"
 	"github.com/google/go-github/v89/github"
 	"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -192,58 +193,74 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github
 			"%s: GitHub secondary rate limit exceeded. Wait before retrying.", message))
 	}
 
-	return utils.NewToolResultErrorFromErr(message, formattedGitHubAPIError(err))
+	return utils.NewToolResultErrorFromErr(message, formatGitHubValidationError(resp, err))
 }
 
-// formattedGitHubAPIError unwraps a github.ErrorResponse so tool results include
-// nested validation messages (for example repository ruleset violations) instead
-// of go-github's compact 422 dump.
-func formattedGitHubAPIError(err error) error {
+// formatGitHubValidationError exposes the parsed fields of 422 responses without
+// including request or response metadata from the underlying HTTP exchange.
+func formatGitHubValidationError(resp *github.Response, err error) error {
 	var ghErr *github.ErrorResponse
 	if !stderrors.As(err, &ghErr) {
 		return err
 	}
 
-	var parts []string
-	switch {
-	case ghErr.Response != nil && ghErr.Response.StatusCode != 0 && ghErr.Message != "":
-		parts = append(parts, fmt.Sprintf("HTTP %d %s", ghErr.Response.StatusCode, ghErr.Message))
-	case ghErr.Response != nil && ghErr.Response.StatusCode != 0:
-		parts = append(parts, fmt.Sprintf("HTTP %d", ghErr.Response.StatusCode))
-	case ghErr.Message != "":
-		parts = append(parts, ghErr.Message)
+	statusCode := 0
+	if ghErr.Response != nil {
+		statusCode = ghErr.Response.StatusCode
+	}
+	if statusCode == 0 && resp != nil {
+		statusCode = resp.StatusCode
+	}
+	if statusCode != http.StatusUnprocessableEntity {
+		return err
 	}
 
-	for _, item := range ghErr.Errors {
-		detail := strings.TrimSpace(item.Message)
-		if detail == "" {
-			var bits []string
-			if item.Resource != "" {
-				bits = append(bits, item.Resource)
-			}
-			if item.Field != "" {
-				bits = append(bits, item.Field)
-			}
-			if item.Code != "" {
-				bits = append(bits, item.Code)
-			}
-			detail = strings.Join(bits, " ")
-		}
-		if detail != "" {
+	parts := make([]string, 0, len(ghErr.Errors)+1)
+	if summary := sanitizeGitHubValidationText(ghErr.Message); summary != "" {
+		parts = append(parts, summary)
+	}
+	for _, validationErr := range ghErr.Errors {
+		if detail := formatGitHubValidationDetail(validationErr); detail != "" {
 			parts = append(parts, detail)
 		}
 	}
 
-	if ghErr.DocumentationURL != "" {
-		parts = append(parts, "See "+ghErr.DocumentationURL)
-	}
-
 	if len(parts) == 0 {
-		return err
+		return stderrors.New("GitHub API validation failed")
 	}
 	return stderrors.New(strings.Join(parts, "\n"))
 }
 
+func formatGitHubValidationDetail(validationErr github.Error) string {
+	resource := sanitizeGitHubValidationText(validationErr.Resource)
+	field := sanitizeGitHubValidationText(validationErr.Field)
+	code := sanitizeGitHubValidationText(validationErr.Code)
+	message := sanitizeGitHubValidationText(validationErr.Message)
+
+	location := strings.Trim(strings.Join([]string{resource, field}, "."), ".")
+	switch {
+	case location != "" && code != "":
+		location += " (" + code + ")"
+	case location == "":
+		location = code
+	}
+
+	switch {
+	case location != "" && message != "":
+		return location + ": " + message
+	case message != "":
+		return message
+	default:
+		return location
+	}
+}
+
+func sanitizeGitHubValidationText(value string) string {
+	// Tool errors are plain text; keep quoted branch patterns readable.
+	sanitized := strings.ReplaceAll(sanitize.Sanitize(value), "'", "'")
+	return strings.Join(strings.Fields(sanitized), " ")
+}
+
 // NewGitHubGraphQLErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware
 func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err error) *mcp.CallToolResult {
 	graphQLErr := newGitHubGraphQLError(message, err)

pkg/errors/error_test.go

@@ -689,48 +689,101 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) {
 }
 
 func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) {
-	t.Run("ruleset ErrorResponse includes nested validation messages", func(t *testing.T) {
+	t.Run("ruleset ErrorResponse includes sanitized structured validation messages", func(t *testing.T) {
 		ctx := ContextWithGitHubErrors(context.Background())
 
+		request, err := http.NewRequest(http.MethodPost, "https://api.github.test/repos/owner/repo/git/refs?private=secret-url-token", nil)
+		require.NoError(t, err)
+		request.Header.Set("Authorization", "Bearer secret-request-token")
+		response := &http.Response{
+			StatusCode: http.StatusUnprocessableEntity,
+			Request:    request,
+			Header:     http.Header{"X-Secret": []string{"secret-response-header"}},
+		}
+
 		originalErr := &github.ErrorResponse{
-			Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
-			Message:  "Validation Failed",
+			Response: response,
+			Message:  "Validation <script>secret-script</script>Failed\u202e",
 			Errors: []github.Error{
 				{
 					Resource: "GitRef",
 					Field:    "ref",
 					Code:     "custom",
-					Message:  "ref name does not match the required pattern 'feature/*'",
+					Message:  "ref name does not match the required pattern 'feature/*'\u202e",
 				},
 			},
-			DocumentationURL: "https://docs.github.com/rest/git/refs#create-a-reference",
+			DocumentationURL: "https://docs.github.test/private?token=secret-doc-token",
 		}
 
-		result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr)
+		wrappedErr := fmt.Errorf("create ref: %w", originalErr)
+		result := NewGitHubAPIErrorResponse(
+			ctx,
+			"failed to create branch",
+			&github.Response{Response: response},
+			wrappedErr,
+		)
 
 		text := requireErrorText(t, result)
-		assert.Contains(t, text, "failed to create branch")
-		assert.Contains(t, text, "HTTP 422 Validation Failed")
-		assert.Contains(t, text, "ref name does not match the required pattern 'feature/*'")
-		assert.Contains(t, text, "See https://docs.github.com/rest/git/refs#create-a-reference")
-		assert.NotContains(t, text, "Resource:")
+		assert.Equal(t, "failed to create branch: Validation Failed\nGitRef.ref (custom): ref name does not match the required pattern 'feature/*'", text)
+		assert.NotContains(t, text, "create ref")
+		assert.NotContains(t, text, "https://")
+		assert.NotContains(t, text, "secret-")
+		assert.NotContains(t, text, "Authorization")
+		assert.NotContains(t, text, "X-Secret")
+		assert.NotContains(t, text, "<script>")
+		assert.NotContains(t, text, "\u202e")
+		assertContextHasError(t, ctx, wrappedErr)
 	})
 
-	t.Run("wrapped ErrorResponse is still unwrapped", func(t *testing.T) {
+	t.Run("ordinary validation errors retain resource field and code", func(t *testing.T) {
 		ctx := ContextWithGitHubErrors(context.Background())
 
-		originalErr := fmt.Errorf("create ref: %w", &github.ErrorResponse{
+		originalErr := &github.ErrorResponse{
 			Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
 			Message:  "Validation Failed",
 			Errors: []github.Error{
-				{Message: "Changes must be made through a pull request."},
+				{
+					Resource: "Repository",
+					Field:    "name",
+					Code:     "invalid",
+				},
 			},
-		})
+		}
+
+		result := NewGitHubAPIErrorResponse(ctx, "API call failed", nil, originalErr)
+
+		text := requireErrorText(t, result)
+		assert.Equal(t, "API call failed: Validation Failed\nRepository.name (invalid)", text)
+	})
+
+	t.Run("top-level validation message is useful without nested errors", func(t *testing.T) {
+		ctx := ContextWithGitHubErrors(context.Background())
+
+		originalErr := &github.ErrorResponse{
+			Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
+			Message:  "Reference already exists",
+		}
 
 		result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr)
 
 		text := requireErrorText(t, result)
-		assert.Contains(t, text, "Changes must be made through a pull request.")
-		assert.NotContains(t, text, "create ref:")
+		assert.Equal(t, "failed to create branch: Reference already exists", text)
+	})
+
+	t.Run("non-422 ErrorResponse preserves the existing error contract", func(t *testing.T) {
+		ctx := ContextWithGitHubErrors(context.Background())
+
+		originalErr := &github.ErrorResponse{
+			Response: &http.Response{StatusCode: http.StatusConflict},
+			Message:  "Conflict",
+			Errors: []github.Error{
+				{Message: "Changes must be made through a pull request."},
+			},
+		}
+
+		result := NewGitHubAPIErrorResponse(ctx, "API call failed", nil, originalErr)
+
+		text := requireErrorText(t, result)
+		assert.Equal(t, "API call failed: "+originalErr.Error(), text)
 	})
 }

pkg/github/repositories_test.go

@@ -1008,6 +1008,7 @@ func Test_CreateBranch(t *testing.T) {
 		expectError    bool
 		expectedRef    *github.Reference
 		expectedErrMsg string
+		unexpectedErrs []string
 	}{
 		{
 			name: "successful branch creation with from_branch",
@@ -1096,7 +1097,8 @@ func Test_CreateBranch(t *testing.T) {
 				"from_branch": "main",
 			},
 			expectError:    true,
-			expectedErrMsg: "failed to create branch",
+			expectedErrMsg: "Reference already exists",
+			unexpectedErrs: []string{"422", "http://", "https://"},
 		},
 		{
 			name: "create branch surfaces ruleset validation details",
@@ -1127,6 +1129,7 @@ func Test_CreateBranch(t *testing.T) {
 			},
 			expectError:    true,
 			expectedErrMsg: "ref name does not match the required pattern 'feature/*'",
+			unexpectedErrs: []string{"422", "https://docs.github.com"},
 		},
 	}
 
@@ -1151,6 +1154,9 @@ func Test_CreateBranch(t *testing.T) {
 				require.True(t, result.IsError)
 				errorContent := getErrorResult(t, result)
 				assert.Contains(t, errorContent.Text, tc.expectedErrMsg)
+				for _, unexpectedErr := range tc.unexpectedErrs {
+					assert.NotContains(t, errorContent.Text, unexpectedErr)
+				}
 				return
 			}