fix(errors): safely format GitHub validation failures · github/github-mcp-server@fed13b5 · GitHub
Skip to content

Commit fed13b5

Browse files
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>
1 parent b0c9486 commit fed13b5

3 files changed

Lines changed: 128 additions & 52 deletions

File tree

‎pkg/errors/error.go‎

Lines changed: 51 additions & 34 deletions

‎pkg/errors/error_test.go‎

Lines changed: 70 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -689,48 +689,101 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) {
689689
}
690690

691691
func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) {
692-
t.Run("ruleset ErrorResponse includes nested validation messages", func(t *testing.T) {
692+
t.Run("ruleset ErrorResponse includes sanitized structured validation messages", func(t *testing.T) {
693693
ctx := ContextWithGitHubErrors(context.Background())
694694

695+
request, err := http.NewRequest(http.MethodPost, "https://api.github.test/repos/owner/repo/git/refs?private=secret-url-token", nil)
696+
require.NoError(t, err)
697+
request.Header.Set("Authorization", "Bearer secret-request-token")
698+
response := &http.Response{
699+
StatusCode: http.StatusUnprocessableEntity,
700+
Request: request,
701+
Header: http.Header{"X-Secret": []string{"secret-response-header"}},
702+
}
703+
695704
originalErr := &github.ErrorResponse{
696-
Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
697-
Message: "Validation Failed",
705+
Response: response,
706+
Message: "Validation <script>secret-script</script>Failed\u202e",
698707
Errors: []github.Error{
699708
{
700709
Resource: "GitRef",
701710
Field: "ref",
702711
Code: "custom",
703-
Message: "ref name does not match the required pattern 'feature/*'",
712+
Message: "ref name does not match the required pattern 'feature/*'\u202e",
704713
},
705714
},
706-
DocumentationURL: "https://docs.github.com/rest/git/refs#create-a-reference",
715+
DocumentationURL: "https://docs.github.test/private?token=secret-doc-token",
707716
}
708717

709-
result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr)
718+
wrappedErr := fmt.Errorf("create ref: %w", originalErr)
719+
result := NewGitHubAPIErrorResponse(
720+
ctx,
721+
"failed to create branch",
722+
&github.Response{Response: response},
723+
wrappedErr,
724+
)
710725

711726
text := requireErrorText(t, result)
712-
assert.Contains(t, text, "failed to create branch")
713-
assert.Contains(t, text, "HTTP 422 Validation Failed")
714-
assert.Contains(t, text, "ref name does not match the required pattern 'feature/*'")
715-
assert.Contains(t, text, "See https://docs.github.com/rest/git/refs#create-a-reference")
716-
assert.NotContains(t, text, "Resource:")
727+
assert.Equal(t, "failed to create branch: Validation Failed\nGitRef.ref (custom): ref name does not match the required pattern 'feature/*'", text)
728+
assert.NotContains(t, text, "create ref")
729+
assert.NotContains(t, text, "https://")
730+
assert.NotContains(t, text, "secret-")
731+
assert.NotContains(t, text, "Authorization")
732+
assert.NotContains(t, text, "X-Secret")
733+
assert.NotContains(t, text, "<script>")
734+
assert.NotContains(t, text, "\u202e")
735+
assertContextHasError(t, ctx, wrappedErr)
717736
})
718737

719-
t.Run("wrapped ErrorResponse is still unwrapped", func(t *testing.T) {
738+
t.Run("ordinary validation errors retain resource field and code", func(t *testing.T) {
720739
ctx := ContextWithGitHubErrors(context.Background())
721740

722-
originalErr := fmt.Errorf("create ref: %w", &github.ErrorResponse{
741+
originalErr := &github.ErrorResponse{
723742
Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
724743
Message: "Validation Failed",
725744
Errors: []github.Error{
726-
{Message: "Changes must be made through a pull request."},
745+
{
746+
Resource: "Repository",
747+
Field: "name",
748+
Code: "invalid",
749+
},
727750
},
728-
})
751+
}
752+
753+
result := NewGitHubAPIErrorResponse(ctx, "API call failed", nil, originalErr)
754+
755+
text := requireErrorText(t, result)
756+
assert.Equal(t, "API call failed: Validation Failed\nRepository.name (invalid)", text)
757+
})
758+
759+
t.Run("top-level validation message is useful without nested errors", func(t *testing.T) {
760+
ctx := ContextWithGitHubErrors(context.Background())
761+
762+
originalErr := &github.ErrorResponse{
763+
Response: &http.Response{StatusCode: http.StatusUnprocessableEntity},
764+
Message: "Reference already exists",
765+
}
729766

730767
result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr)
731768

732769
text := requireErrorText(t, result)
733-
assert.Contains(t, text, "Changes must be made through a pull request.")
734-
assert.NotContains(t, text, "create ref:")
770+
assert.Equal(t, "failed to create branch: Reference already exists", text)
771+
})
772+
773+
t.Run("non-422 ErrorResponse preserves the existing error contract", func(t *testing.T) {
774+
ctx := ContextWithGitHubErrors(context.Background())
775+
776+
originalErr := &github.ErrorResponse{
777+
Response: &http.Response{StatusCode: http.StatusConflict},
778+
Message: "Conflict",
779+
Errors: []github.Error{
780+
{Message: "Changes must be made through a pull request."},
781+
},
782+
}
783+
784+
result := NewGitHubAPIErrorResponse(ctx, "API call failed", nil, originalErr)
785+
786+
text := requireErrorText(t, result)
787+
assert.Equal(t, "API call failed: "+originalErr.Error(), text)
735788
})
736789
}

‎pkg/github/repositories_test.go‎

Lines changed: 7 additions & 1 deletion

0 commit comments

Comments
 (0)