diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 8643acc7ef..9d9d02f82a 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -8,6 +8,7 @@ import ( "github.com/github/github-mcp-server/pkg/ifc" "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/sanitize" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/github/github-mcp-server/pkg/utils" @@ -99,7 +100,7 @@ type WithCategoryNoOrder struct { func fragmentToDiscussion(fragment NodeFragment) *github.Discussion { return &github.Discussion{ Number: github.Ptr(int(fragment.Number)), - Title: github.Ptr(string(fragment.Title)), + Title: github.Ptr(sanitize.Sanitize(string(fragment.Title))), HTMLURL: github.Ptr(string(fragment.URL)), CreatedAt: &github.Timestamp{Time: fragment.CreatedAt.Time}, UpdatedAt: &github.Timestamp{Time: fragment.UpdatedAt.Time}, @@ -360,8 +361,8 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { // like ListDiscussions and GetDiscussionComments). response := map[string]any{ "number": int(d.Number), - "title": string(d.Title), - "body": string(d.Body), + "title": sanitize.Sanitize(string(d.Title)), + "body": sanitize.Sanitize(string(d.Body)), "url": string(d.URL), "closed": bool(d.Closed), "isAnswered": bool(d.IsAnswered), @@ -520,18 +521,10 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve return utils.NewToolResultError(err.Error()), nil, nil } for _, c := range q.Repository.Discussion.Comments.Nodes { - comment := MinimalDiscussionComment{ - ID: fmt.Sprintf("%v", c.ID), - Body: string(c.Body), - IsAnswer: bool(c.IsAnswer), - ReplyTotalCount: c.Replies.TotalCount, - } + comment := newMinimalDiscussionComment(fmt.Sprintf("%v", c.ID), string(c.Body), bool(c.IsAnswer)) + comment.ReplyTotalCount = c.Replies.TotalCount for _, r := range c.Replies.Nodes { - comment.Replies = append(comment.Replies, MinimalDiscussionComment{ - ID: fmt.Sprintf("%v", r.ID), - Body: string(r.Body), - IsAnswer: bool(r.IsAnswer), - }) + comment.Replies = append(comment.Replies, newMinimalDiscussionComment(fmt.Sprintf("%v", r.ID), string(r.Body), bool(r.IsAnswer))) } comments = append(comments, comment) } @@ -562,11 +555,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve return utils.NewToolResultError(err.Error()), nil, nil } for _, c := range q.Repository.Discussion.Comments.Nodes { - comments = append(comments, MinimalDiscussionComment{ - ID: fmt.Sprintf("%v", c.ID), - Body: string(c.Body), - IsAnswer: bool(c.IsAnswer), - }) + comments = append(comments, newMinimalDiscussionComment(fmt.Sprintf("%v", c.ID), string(c.Body), bool(c.IsAnswer))) } pageInfo = q.Repository.Discussion.Comments.PageInfo totalCount = q.Repository.Discussion.Comments.TotalCount diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index c2c0426218..a41a903d4e 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -553,6 +553,30 @@ func Test_GetDiscussion(t *testing.T) { expectError: true, errContains: "discussion not found", }, + { + name: "sanitizes malicious title and body", + response: githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{"discussion": map[string]any{ + "number": 1, + "title": maliciousText, + "body": maliciousText, + "url": "https://github.com/owner/repo/discussions/1", + "createdAt": "2025-04-25T12:00:00Z", + "closed": false, + "isAnswered": false, + "category": map[string]any{"name": "General"}, + }}, + }), + expectError: false, + expected: map[string]any{ + "number": float64(1), + "title": sanitizedText, + "body": sanitizedText, + "url": "https://github.com/owner/repo/discussions/1", + "closed": false, + "isAnswered": false, + }, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/github/find_duplicate.go b/pkg/github/find_duplicate.go index 4831f15c5d..e32179ce8d 100644 --- a/pkg/github/find_duplicate.go +++ b/pkg/github/find_duplicate.go @@ -152,12 +152,15 @@ func FindDuplicate(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError("ranked duplicate detection is unavailable: the semantic-similarity endpoint returned issues without ranking metadata (the server-side duplicate-ranking feature is not enabled for this caller or repository)"), nil, nil } candidates = append(candidates, duplicateCandidate{ - Issue: MinimalIssueRef{ - Number: res.Issue.Number, - Title: res.Issue.Title, - State: res.Issue.State, - URL: res.Issue.HTMLURL, - }, + // Candidates are always scoped to the requested repository, so the + // ref's repository field is left empty as it was before. + Issue: newMinimalIssueRef( + res.Issue.Number, + res.Issue.Title, + res.Issue.State, + res.Issue.HTMLURL, + "", + ), Score: res.Score, Confidence: res.Confidence, LikelyDuplicate: res.LikelyDuplicate, diff --git a/pkg/github/find_duplicate_test.go b/pkg/github/find_duplicate_test.go index 4384c92198..be5c3dda81 100644 --- a/pkg/github/find_duplicate_test.go +++ b/pkg/github/find_duplicate_test.go @@ -120,6 +120,52 @@ func Test_FindDuplicate_RankedResults(t *testing.T) { assert.False(t, candidates[1].LikelyDuplicate) } +// Test_FindDuplicate_SanitizesIssueTitle asserts that candidate issue titles, which are +// user-authored content from an arbitrary repository, are sanitized before being returned. +// Without this the tool would forward hidden-instruction payloads straight to the model. +func Test_FindDuplicate_SanitizesIssueTitle(t *testing.T) { + serverTool := FindDuplicate(translations.NullTranslationHelper) + + rankedResults := []map[string]any{ + { + "issue": map[string]any{ + "number": 456, + "title": maliciousText, + "state": "open", + "html_url": "https://github.com/owner/repo/issues/456", + }, + "score": 0.95, + "confidence": "high", + "likely_duplicate": true, + }, + } + + handler := func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write(MustMarshal(rankedResults)) + } + + client := mustNewGHClient(t, NewMockedHTTPClient(WithRequestMatchHandler(endpointSemanticallySimilar, http.HandlerFunc(handler)))) + deps := BaseDeps{Client: client} + toolHandler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + }) + result, err := toolHandler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "expected result to not be an error") + + text := getTextResult(t, result) + var candidates []duplicateCandidate + require.NoError(t, json.Unmarshal([]byte(text.Text), &candidates)) + require.Len(t, candidates, 1) + assert.Equal(t, sanitizedText, candidates[0].Issue.Title) + assert.NotContains(t, text.Text, "Hello\u200BWorld" + +// sanitizedText is what maliciousText becomes after sanitize.Sanitize: the \n+\u200B\n" + + t.Run("commit file patch (get_commit)", func(t *testing.T) { + commit := convertToMinimalCommit(&github.RepositoryCommit{ + Files: []*github.CommitFile{{Filename: github.Ptr("a.go"), Patch: github.Ptr(patch)}}, + }, commitDetailFullPatch) + require.Len(t, commit.Files, 1) + assert.Equal(t, patch, commit.Files[0].Patch) + }) + + t.Run("pull request file patch (get_pull_request_files)", func(t *testing.T) { + files := convertToMinimalPRFiles([]*github.CommitFile{ + {Filename: github.Ptr("a.go"), Patch: github.Ptr(patch)}, + }) + require.Len(t, files, 1) + assert.Equal(t, patch, files[0].Patch) + }) +} + +// Test_Discussion_SanitizesUserAuthoredText covers the discussion helpers, which previously +// applied no sanitization at all to titles, bodies, or comments despite being user-authored, +// untrusted content equivalent to issue/PR text. +func Test_Discussion_SanitizesUserAuthoredText(t *testing.T) { + t.Run("discussion title (fragmentToDiscussion, used by list_discussions)", func(t *testing.T) { + discussion := fragmentToDiscussion(NodeFragment{Title: githubv4.String(maliciousText)}) + require.NotNil(t, discussion.Title) + assert.Equal(t, sanitizedText, *discussion.Title) + }) + + t.Run("discussion comment body (newMinimalDiscussionComment, used by get_discussion_comments)", func(t *testing.T) { + comment := newMinimalDiscussionComment("id", maliciousText, false) + assert.Equal(t, sanitizedText, comment.Body) + }) +} diff --git a/pkg/github/search_utils.go b/pkg/github/search_utils.go index dc800171aa..52d735ca0b 100644 --- a/pkg/github/search_utils.go +++ b/pkg/github/search_utils.go @@ -204,6 +204,12 @@ func searchHandler( return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, errorPrefix, resp, body), nil } + // result.Issues are raw *github.Issue objects marshaled directly below rather than through + // a convertToMinimal* helper (see minimal_types.go), so Title/Body must be sanitized here. + for _, iss := range result.Issues { + sanitizeIssueTitleAndBody(iss) + } + filtered := false var payload any = result if len(cfg.fields) > 0 {