From 1c4c824e68252a60bab5810c421cd8c0220e338e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 14:57:38 +0200 Subject: [PATCH] fix: preserve Markdown content in GitHub responses Route Markdown and code-bearing response fields through a fidelity-preserving content path while retaining metadata sanitization for titles. Add exact converter and sanitizer coverage for useful Markdown and invisible-character filtering. Refs #2202 Refs #3165 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69c5ab30-9815-4c07-8385-11a206e68f66 --- pkg/github/discussions.go | 2 +- pkg/github/discussions_test.go | 2 +- pkg/github/issues.go | 20 +++++++- pkg/github/minimal_types.go | 24 +++++----- pkg/github/projects.go | 2 +- pkg/github/repositories.go | 16 ++++++- pkg/github/repositories_test.go | 2 +- pkg/github/sanitize_coverage_test.go | 68 ++++++++++++++++------------ pkg/sanitize/sanitize.go | 5 ++ pkg/sanitize/sanitize_test.go | 15 ++++++ 10 files changed, 109 insertions(+), 47 deletions(-) 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/issues.go b/pkg/github/issues.go index fd7ea36873..01b448469b 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1118,6 +1118,9 @@ func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependenc subIssues = filteredSubIssues } + for _, subIssue := range subIssues { + sanitizeSubIssueTitleAndBody(subIssue) + } r, err := json.Marshal(subIssues) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -1708,6 +1711,7 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", resp, body), nil } + sanitizeSubIssueTitleAndBody(subIssue) r, err := json.Marshal(subIssue) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -1739,6 +1743,7 @@ func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, re return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to remove sub-issue", resp, body), nil } + sanitizeSubIssueTitleAndBody(subIssue) r, err := json.Marshal(subIssue) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -1788,6 +1793,7 @@ func ReprioritizeSubIssue(ctx context.Context, client *github.Client, owner stri return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to reprioritize sub-issue", resp, body), nil } + sanitizeSubIssueTitleAndBody(subIssue) r, err := json.Marshal(subIssue) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -1998,7 +2004,19 @@ func sanitizeIssueTitleAndBody(issue *github.Issue) { issue.Title = github.Ptr(sanitize.Sanitize(*issue.Title)) } if issue.Body != nil { - issue.Body = github.Ptr(sanitize.Sanitize(*issue.Body)) + issue.Body = github.Ptr(sanitize.Content(*issue.Body)) + } +} + +func sanitizeSubIssueTitleAndBody(issue *github.SubIssue) { + if issue == nil { + return + } + if issue.Title != nil { + issue.Title = github.Ptr(sanitize.Sanitize(*issue.Title)) + } + if issue.Body != nil { + issue.Body = github.Ptr(sanitize.Content(*issue.Body)) } } diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index f8cf9f307c..3f237df1d3 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -204,7 +204,7 @@ type MinimalDiscussionComment struct { func newMinimalDiscussionComment(id string, body string, isAnswer bool) MinimalDiscussionComment { return MinimalDiscussionComment{ ID: id, - Body: sanitize.Sanitize(body), + Body: sanitize.Content(body), IsAnswer: isAnswer, } } @@ -797,7 +797,7 @@ func convertToMinimalPullRequestReview(review *github.PullRequestReview) Minimal m := MinimalPullRequestReview{ ID: review.GetID(), State: review.GetState(), - Body: sanitize.Sanitize(review.GetBody()), + Body: sanitize.Content(review.GetBody()), HTMLURL: review.GetHTMLURL(), User: convertToMinimalUser(review.GetUser()), CommitID: review.GetCommitID(), @@ -815,7 +815,7 @@ func convertToMinimalIssue(issue *github.Issue) MinimalIssue { m := MinimalIssue{ Number: issue.GetNumber(), Title: sanitize.Sanitize(issue.GetTitle()), - Body: sanitize.Sanitize(issue.GetBody()), + Body: sanitize.Content(issue.GetBody()), State: issue.GetState(), StateReason: issue.GetStateReason(), Draft: issue.GetDraft(), @@ -926,7 +926,7 @@ func fragmentWithoutFieldValuesToMinimalIssue(fragment issueFragmentWithoutField m := MinimalIssue{ Number: int(fragment.Number), Title: sanitize.Sanitize(string(fragment.Title)), - Body: sanitize.Sanitize(string(fragment.Body)), + Body: sanitize.Content(string(fragment.Body)), State: string(fragment.State), Comments: int(fragment.Comments.TotalCount), CreatedAt: fragment.CreatedAt.Format(time.RFC3339), @@ -1015,7 +1015,7 @@ func convertToMinimalIssuesResponseWithoutFieldValues(fragment issueQueryFragmen func convertToMinimalIssueComment(comment *github.IssueComment) MinimalIssueComment { m := MinimalIssueComment{ ID: comment.GetID(), - Body: sanitize.Sanitize(comment.GetBody()), + Body: sanitize.Content(comment.GetBody()), HTMLURL: comment.GetHTMLURL(), User: convertToMinimalUser(comment.GetUser()), AuthorAssociation: comment.GetAuthorAssociation(), @@ -1064,7 +1064,7 @@ func convertToMinimalFileContentResponse(resp *github.RepositoryContentResponse) m.Commit = &MinimalFileCommit{ SHA: resp.Commit.GetSHA(), - Message: sanitize.Sanitize(resp.Commit.GetMessage()), + Message: sanitize.Content(resp.Commit.GetMessage()), HTMLURL: resp.Commit.GetHTMLURL(), } @@ -1085,7 +1085,7 @@ func convertToMinimalPullRequest(pr *github.PullRequest) MinimalPullRequest { m := MinimalPullRequest{ Number: pr.GetNumber(), Title: sanitize.Sanitize(pr.GetTitle()), - Body: sanitize.Sanitize(pr.GetBody()), + Body: sanitize.Content(pr.GetBody()), State: pr.GetState(), Draft: pr.GetDraft(), Merged: pr.GetMerged(), @@ -1794,7 +1794,7 @@ func newMinimalCommitFromCore(sha, htmlURL string, commit *github.Commit, author if commit != nil { minimalCommit.Commit = &MinimalCommitInfo{ - Message: sanitize.Sanitize(commit.GetMessage()), + Message: sanitize.Content(commit.GetMessage()), } if commit.Author != nil { @@ -2000,7 +2000,7 @@ func convertToMinimalPullRequestCommits(commits []*github.RepositoryCommit) []Mi } if commit.Commit != nil { - minimalCommit.Message = sanitize.Sanitize(commit.Commit.GetMessage()) + minimalCommit.Message = sanitize.Content(commit.Commit.GetMessage()) minimalCommit.Author = convertToMinimalCommitAuthor(commit.Commit.Author) } @@ -2039,7 +2039,7 @@ func convertToMinimalRelease(release *github.RepositoryRelease) MinimalRelease { ID: release.GetID(), TagName: release.GetTagName(), Name: sanitize.Sanitize(release.GetName()), - Body: sanitize.Sanitize(release.GetBody()), + Body: sanitize.Content(release.GetBody()), HTMLURL: release.GetHTMLURL(), Prerelease: release.GetPrerelease(), Draft: release.GetDraft(), @@ -2095,7 +2095,7 @@ func convertToMinimalWorkflowRun(workflowRun *github.WorkflowRun) MinimalWorkflo if headCommit := workflowRun.GetHeadCommit(); headCommit != nil && headCommit.GetMessage() != "" { minimalRun.HeadCommit = &MinimalWorkflowRunHeadCommit{ - Message: sanitize.Sanitize(headCommit.GetMessage()), + Message: sanitize.Content(headCommit.GetMessage()), } } @@ -2280,7 +2280,7 @@ func convertToMinimalReviewThread(thread reviewThreadNode) MinimalReviewThread { func convertToMinimalReviewComment(c reviewCommentNode) MinimalReviewComment { m := MinimalReviewComment{ - Body: sanitize.Sanitize(string(c.Body)), + Body: sanitize.Content(string(c.Body)), Path: string(c.Path), Author: string(c.Author.Login), HTMLURL: c.URL.String(), diff --git a/pkg/github/projects.go b/pkg/github/projects.go index fe702cfbbe..28e15d3bd1 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -266,7 +266,7 @@ func convertToMinimalStatusUpdate(node statusUpdateNode) MinimalProjectStatusUpd return MinimalProjectStatusUpdate{ ID: fmt.Sprintf("%v", node.ID), - Body: sanitize.Sanitize(derefString(node.Body)), + Body: sanitize.Content(derefString(node.Body)), Status: derefString(node.Status), CreatedAt: node.CreatedAt.Time.Format(time.RFC3339), StartDate: derefString(node.StartDate), diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 8dfa19b4a2..9b9c7957c0 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -2233,6 +2233,7 @@ func GetLatestRelease(t translations.TranslationHelperFunc) inventory.ServerTool return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get latest release", resp, body), nil, nil } + sanitizeReleaseNameAndBody(release) r, err := json.Marshal(release) if err != nil { return nil, nil, fmt.Errorf("failed to marshal response: %w", err) @@ -2319,6 +2320,7 @@ func GetReleaseByTag(t translations.TranslationHelperFunc) inventory.ServerTool return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get release by tag", resp, body), nil, nil } + sanitizeReleaseNameAndBody(release) r, err := json.Marshal(release) if err != nil { return nil, nil, fmt.Errorf("failed to marshal response: %w", err) @@ -2338,6 +2340,18 @@ func GetReleaseByTag(t translations.TranslationHelperFunc) inventory.ServerTool ) } +func sanitizeReleaseNameAndBody(release *github.RepositoryRelease) { + if release == nil { + return + } + if release.Name != nil { + release.Name = github.Ptr(sanitize.Sanitize(*release.Name)) + } + if release.Body != nil { + release.Body = github.Ptr(sanitize.Content(*release.Body)) + } +} + // ListStarredRepositories creates a tool to list starred repositories for the authenticated user or a specified user. func ListStarredRepositories(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( @@ -2981,7 +2995,7 @@ func GetFileBlame(t translations.TranslationHelperFunc) inventory.ServerTool { SHA: sha, // Sanitized after truncation so the headline is cut at the author's real // first line break rather than one introduced by sanitization. - MessageHeadline: sanitize.Sanitize(headline), + MessageHeadline: sanitize.Content(headline), CommittedDate: r.Commit.CommittedDate.Format("2006-01-02T15:04:05Z"), Author: BlameAuthor{ Name: string(r.Commit.Author.Name), diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 71b04faa3e..a81f5a7a71 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -6200,7 +6200,7 @@ func Test_GetFileBlame(t *testing.T) { var br BlameResult require.NoError(t, json.Unmarshal([]byte(result), &br)) require.Contains(t, br.Commits, "badc0ffee0000") - assert.Equal(t, sanitizedText, br.Commits["badc0ffee0000"].MessageHeadline) + assert.Equal(t, sanitizedContentText, br.Commits["badc0ffee0000"].MessageHeadline) assert.NotContains(t, result, "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 👩‍💻")) +}