github-featured/github--github-mcp-server
2
0
Fork 0
Code Issues Pull Requests Deployments Models Agent Notes Packages Projects Releases Wiki Activity

fix(errors): safely format GitHub validation failures

Browse Source 
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>
This commit is contained in:
Sam Morrow
2026-08-18 23:51:31 +02:00
parent 4e6329eb46
commit bf47e3eca9
3 changed files with 130 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/errors/error.go
+51 -34
View File
@@ -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), "&#39;", "'")
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
+72 -19
View File
@@ -689,7 +689,53 @@ 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: 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/*'\u202e",
},
},
DocumentationURL: "https://docs.github.test/private?token=secret-doc-token",
}
wrappedErr := fmt.Errorf("create ref: %w", originalErr)
result := NewGitHubAPIErrorResponse(
ctx,
"failed to create branch",
&github.Response{Response: response},
wrappedErr,
)
text := requireErrorText(t, result)
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("ordinary validation errors retain resource field and code", func(t *testing.T) {
ctx := ContextWithGitHubErrors(context.Background())
originalErr := &github.ErrorResponse{
@@ -697,40 +743,47 @@ func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) {
Message: "Validation Failed",
Errors: []github.Error{
{
Resource: "GitRef",
Field: "ref",
Code: "custom",
Message: "ref name does not match the required pattern 'feature/*'",
Resource: "Repository",
Field: "name",
Code: "invalid",
},
},
DocumentationURL: "https://docs.github.com/rest/git/refs#create-a-reference",
}
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, "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: Reference already exists", text)
})
t.Run("wrapped ErrorResponse is still unwrapped", func(t *testing.T) {
t.Run("non-422 ErrorResponse preserves the existing error contract", func(t *testing.T) {
ctx := ContextWithGitHubErrors(context.Background())
originalErr := fmt.Errorf("create ref: %w", &github.ErrorResponse{
Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
Message: "Validation Failed",
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, "failed to create branch", nil, originalErr)
result := NewGitHubAPIErrorResponse(ctx, "API call failed", 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, "API call failed: "+originalErr.Error(), text)
})
}
pkg/github/repositories_test.go
+7 -1
View File
@@ -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
}