diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 9ea31b2ebf..8678ed2a6b 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -362,7 +362,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { response := map[string]any{ "number": int(d.Number), "title": sanitize.Sanitize(string(d.Title)), - "body": sanitize.Sanitize(string(d.Body)), + "body": sanitize.Content(string(d.Body)), "url": string(d.URL), "closed": bool(d.Closed), "isAnswered": bool(d.IsAnswered), diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index a41a903d4e..111372a9ef 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -571,7 +571,7 @@ func Test_GetDiscussion(t *testing.T) { expected: map[string]any{ "number": float64(1), "title": sanitizedText, - "body": sanitizedText, + "body": sanitizedContentText, "url": "https://github.com/owner/repo/discussions/1", "closed": false, "isAnswered": false, diff --git a/pkg/github/find_duplicate_test.go b/pkg/github/find_duplicate_test.go index 9e20d958b4..9539575896 100644 --- a/pkg/github/find_duplicate_test.go +++ b/pkg/github/find_duplicate_test.go @@ -165,7 +165,6 @@ func Test_FindDuplicate_SanitizesIssueTitle(t *testing.T) { 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 HelloWorld" -// Test_MinimalConverters_SanitizeUserAuthoredText is a table-driven regression test asserting -// that every convertToMinimal* helper which surfaces untrusted, user-authored prose (issue and -// PR titles/bodies, comments, reviews, review comments, releases, commit messages) applies -// pkg/sanitize.Sanitize consistently. This guards against the inconsistent coverage described in -// https://github.com/github/github-mcp-server/issues/3106. +// Test_MinimalConverters_SanitizeUserAuthoredText covers the converter fields that expose +// untrusted titles, bodies, comments, reviews, releases, and commit messages. func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { tests := []struct { - name string - got func() string + name string + content bool + got func() string }{ { name: "issue title (REST)", @@ -41,7 +35,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "issue body (REST)", + name: "issue body (REST)", + content: true, got: func() string { return convertToMinimalIssue(&github.Issue{ Body: github.Ptr(maliciousText), @@ -49,7 +44,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "issue comment body", + name: "issue comment body", + content: true, got: func() string { return convertToMinimalIssueComment(&github.IssueComment{ Body: github.Ptr(maliciousText), @@ -65,7 +61,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "pull request body", + name: "pull request body", + content: true, got: func() string { return convertToMinimalPullRequest(&github.PullRequest{ Body: github.Ptr(maliciousText), @@ -73,7 +70,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "pull request review body", + name: "pull request review body", + content: true, got: func() string { return convertToMinimalPullRequestReview(&github.PullRequestReview{ Body: github.Ptr(maliciousText), @@ -81,7 +79,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "pull request review comment body (GraphQL)", + name: "pull request review comment body (GraphQL)", + content: true, got: func() string { return convertToMinimalReviewComment(reviewCommentNode{ Body: githubv4.String(maliciousText), @@ -98,7 +97,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "release body", + name: "release body", + content: true, got: func() string { return convertToMinimalRelease(&github.RepositoryRelease{ Body: github.Ptr(maliciousText), @@ -106,7 +106,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "commit message (get_commit / list_commits)", + name: "commit message (get_commit / list_commits)", + content: true, got: func() string { commit := convertToMinimalCommit(&github.RepositoryCommit{ Commit: &github.Commit{Message: github.Ptr(maliciousText)}, @@ -116,7 +117,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "commit message (search_commits)", + name: "commit message (search_commits)", + content: true, got: func() string { item := convertCommitResultToMinimalCommit(&github.CommitResult{ Commit: &github.Commit{Message: github.Ptr(maliciousText)}, @@ -126,7 +128,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "pull request commit message (list_pull_request_commits)", + name: "pull request commit message (list_pull_request_commits)", + content: true, got: func() string { commits := convertToMinimalPullRequestCommits([]*github.RepositoryCommit{ {Commit: &github.Commit{Message: github.Ptr(maliciousText)}}, @@ -136,7 +139,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "file commit message (create/update/delete file)", + name: "file commit message (create/update/delete file)", + content: true, got: func() string { resp := convertToMinimalFileContentResponse(&github.RepositoryContentResponse{ Commit: github.Commit{Message: github.Ptr(maliciousText)}, @@ -146,7 +150,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "workflow run head commit message", + name: "workflow run head commit message", + content: true, got: func() string { run := convertToMinimalWorkflowRun(&github.WorkflowRun{ HeadCommit: &github.HeadCommit{Message: github.Ptr(maliciousText)}, @@ -196,7 +201,8 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { }, }, { - name: "project status update body (projects_get / projects_list)", + name: "project status update body (projects_get / projects_list)", + content: true, got: func() string { return convertToMinimalStatusUpdate(statusUpdateNode{ Body: githubv4.NewString(githubv4.String(maliciousText)), @@ -228,7 +234,11 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, sanitizedText, tt.got()) + expected := sanitizedText + if tt.content { + expected = sanitizedContentText + } + assert.Equal(t, expected, tt.got()) }) } } @@ -254,7 +264,7 @@ func Test_SearchIssueResult_SanitizesTitleAndBody(t *testing.T) { require.NoError(t, json.Unmarshal(out, &decoded)) assert.Equal(t, sanitizedText, decoded.Title) - assert.Equal(t, sanitizedText, decoded.Body) + assert.Equal(t, sanitizedContentText, decoded.Body) } // Test_SanitizeIssueTitleAndBody exercises the shared helper directly, including its nil-safety, @@ -280,7 +290,7 @@ func Test_SanitizeIssueTitleAndBody(t *testing.T) { require.NotNil(t, issue.Title) require.NotNil(t, issue.Body) assert.Equal(t, sanitizedText, *issue.Title) - assert.Equal(t, sanitizedText, *issue.Body) + assert.Equal(t, sanitizedContentText, *issue.Body) }) } @@ -319,6 +329,6 @@ func Test_Discussion_SanitizesUserAuthoredText(t *testing.T) { 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) + assert.Equal(t, sanitizedContentText, comment.Body) }) } diff --git a/pkg/sanitize/sanitize.go b/pkg/sanitize/sanitize.go index 80d367029b..9636b48928 100644 --- a/pkg/sanitize/sanitize.go +++ b/pkg/sanitize/sanitize.go @@ -36,6 +36,11 @@ func Sanitize(input string) string { return FilterCodeFenceMetadata(FilterInvisibleCharacters(normalized)) } +// Content preserves Markdown and code content while removing invisible characters. +func Content(input string) string { + return FilterInvisibleCharacters(input) +} + // FilterInvisibleCharacters removes invisible or control characters that should not appear // in user-facing titles or bodies. This includes: // - Unicode tag characters: U+E0001, U+E0020–U+E007F diff --git a/pkg/sanitize/sanitize_test.go b/pkg/sanitize/sanitize_test.go index 2b54bdb5f9..d7ec45048e 100644 --- a/pkg/sanitize/sanitize_test.go +++ b/pkg/sanitize/sanitize_test.go @@ -722,3 +722,18 @@ func TestSanitizeStillStripsMaliciousContent(t *testing.T) { } var sink string + +func TestContentPreservesMarkdownAndCode(t *testing.T) { + content := "普通 prose with $5 and $x^2$, :rocket:, ✈️, 👩‍💻.\n\n" + + "[link](https://example.com/a?b=c) ![badge](https://example.com/b.svg)\n\n" + + "
Details
cell
\n\n" + + "```uncommon-language\nx & y\n```\n\n" + + " inline `code` and footnote[^1]\n\n[^1]: note" + + require.Equal(t, content, Content(content)) +} + +func TestContentRemovesOnlyUnconditionalInvisibleCharacters(t *testing.T) { + require.Equal(t, "left right", Content("left\u200B right")) + require.Equal(t, "✈️ and 👩‍💻", Content("✈️ and 👩‍💻")) +}