Skip to content

Commit ab2b623

Browse files
Centralize sanitization of untrusted GitHub response fields
Sanitization was previously applied ad hoc at a handful of tool call sites (GetIssue, GetPullRequest, ListPullRequests) rather than in the shared convertToMinimal* converters, so equivalent user-authored text returned by other tools (issue comments, PR reviews, review comments, releases, commit messages, discussions, project item titles) was returned unsanitized. - Apply sanitize.Sanitize inside the convertToMinimal* helpers in minimal_types.go for issue/PR titles and bodies, issue comments, PR reviews, review comments, releases, commit messages, and project item content titles. This is the single, shared conversion point used by nearly every read tool, so fixing it there covers get/list issues, pull requests, comments, reviews, review comments, releases, commits, and project items consistently. - Add a sanitizeIssueTitleAndBody helper and use it for the two response paths that marshal a raw *github.Issue directly instead of a Minimal* type: search_issues (SearchIssueResult.MarshalJSON) and search_pull_requests (searchHandler). - Sanitize discussion titles/bodies/comments (list_discussions, get_discussion, get_discussion_comments), which previously had no sanitization at all, via a new newMinimalDiscussionComment constructor and inline fixes. - Sanitize project status update bodies. - Remove the now-redundant scattered sanitize calls in GetIssue, GetPullRequest, and ListPullRequests now that the shared converters sanitize on their own. Patches, diffs, and raw file contents are intentionally left untouched to preserve fidelity. Adds table-driven regression tests covering every touched converter, the search_issues/search_pull_requests raw-passthrough paths, and a fidelity check that patches/diffs are not altered. Fixes #3106 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent c54cd63 commit ab2b623

8 files changed

Lines changed: 385 additions & 72 deletions

File tree

pkg/github/discussions.go

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
"github.com/github/github-mcp-server/pkg/ifc"
1010
"github.com/github/github-mcp-server/pkg/inventory"
11+
"github.com/github/github-mcp-server/pkg/sanitize"
1112
"github.com/github/github-mcp-server/pkg/scopes"
1213
"github.com/github/github-mcp-server/pkg/translations"
1314
"github.com/github/github-mcp-server/pkg/utils"
@@ -99,7 +100,7 @@ type WithCategoryNoOrder struct {
99100
func fragmentToDiscussion(fragment NodeFragment) *github.Discussion {
100101
return &github.Discussion{
101102
Number: github.Ptr(int(fragment.Number)),
102-
Title: github.Ptr(string(fragment.Title)),
103+
Title: github.Ptr(sanitize.Sanitize(string(fragment.Title))),
103104
HTMLURL: github.Ptr(string(fragment.URL)),
104105
CreatedAt: &github.Timestamp{Time: fragment.CreatedAt.Time},
105106
UpdatedAt: &github.Timestamp{Time: fragment.UpdatedAt.Time},
@@ -360,8 +361,8 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
360361
// like ListDiscussions and GetDiscussionComments).
361362
response := map[string]any{
362363
"number": int(d.Number),
363-
"title": string(d.Title),
364-
"body": string(d.Body),
364+
"title": sanitize.Sanitize(string(d.Title)),
365+
"body": sanitize.Sanitize(string(d.Body)),
365366
"url": string(d.URL),
366367
"closed": bool(d.Closed),
367368
"isAnswered": bool(d.IsAnswered),
@@ -520,18 +521,10 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
520521
return utils.NewToolResultError(err.Error()), nil, nil
521522
}
522523
for _, c := range q.Repository.Discussion.Comments.Nodes {
523-
comment := MinimalDiscussionComment{
524-
ID: fmt.Sprintf("%v", c.ID),
525-
Body: string(c.Body),
526-
IsAnswer: bool(c.IsAnswer),
527-
ReplyTotalCount: c.Replies.TotalCount,
528-
}
524+
comment := newMinimalDiscussionComment(fmt.Sprintf("%v", c.ID), string(c.Body), bool(c.IsAnswer))
525+
comment.ReplyTotalCount = c.Replies.TotalCount
529526
for _, r := range c.Replies.Nodes {
530-
comment.Replies = append(comment.Replies, MinimalDiscussionComment{
531-
ID: fmt.Sprintf("%v", r.ID),
532-
Body: string(r.Body),
533-
IsAnswer: bool(r.IsAnswer),
534-
})
527+
comment.Replies = append(comment.Replies, newMinimalDiscussionComment(fmt.Sprintf("%v", r.ID), string(r.Body), bool(r.IsAnswer)))
535528
}
536529
comments = append(comments, comment)
537530
}
@@ -562,11 +555,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
562555
return utils.NewToolResultError(err.Error()), nil, nil
563556
}
564557
for _, c := range q.Repository.Discussion.Comments.Nodes {
565-
comments = append(comments, MinimalDiscussionComment{
566-
ID: fmt.Sprintf("%v", c.ID),
567-
Body: string(c.Body),
568-
IsAnswer: bool(c.IsAnswer),
569-
})
558+
comments = append(comments, newMinimalDiscussionComment(fmt.Sprintf("%v", c.ID), string(c.Body), bool(c.IsAnswer)))
570559
}
571560
pageInfo = q.Repository.Discussion.Comments.PageInfo
572561
totalCount = q.Repository.Discussion.Comments.TotalCount

pkg/github/discussions_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,30 @@ func Test_GetDiscussion(t *testing.T) {
553553
expectError: true,
554554
errContains: "discussion not found",
555555
},
556+
{
557+
name: "sanitizes malicious title and body",
558+
response: githubv4mock.DataResponse(map[string]any{
559+
"repository": map[string]any{"discussion": map[string]any{
560+
"number": 1,
561+
"title": maliciousText,
562+
"body": maliciousText,
563+
"url": "https://github.com/owner/repo/discussions/1",
564+
"createdAt": "2025-04-25T12:00:00Z",
565+
"closed": false,
566+
"isAnswered": false,
567+
"category": map[string]any{"name": "General"},
568+
}},
569+
}),
570+
expectError: false,
571+
expected: map[string]any{
572+
"number": float64(1),
573+
"title": sanitizedText,
574+
"body": sanitizedText,
575+
"url": "https://github.com/owner/repo/discussions/1",
576+
"closed": false,
577+
"isAnswered": false,
578+
},
579+
},
556580
}
557581
for _, tc := range tests {
558582
t.Run(tc.name, func(t *testing.T) {

pkg/github/issues.go

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -920,16 +920,6 @@ func GetIssue(ctx context.Context, client *github.Client, deps ToolDependencies,
920920
}
921921
}
922922

923-
// Sanitize title/body on response
924-
if issue != nil {
925-
if issue.Title != nil {
926-
issue.Title = github.Ptr(sanitize.Sanitize(*issue.Title))
927-
}
928-
if issue.Body != nil {
929-
issue.Body = github.Ptr(sanitize.Sanitize(*issue.Body))
930-
}
931-
}
932-
933923
minimalIssue := convertToMinimalIssue(issue)
934924

935925
// Always drop the verbose REST IssueFieldValues; enrich with the GraphQL
@@ -2003,9 +1993,31 @@ type SearchIssueResult struct {
20031993
FieldValues []MinimalFieldValue `json:"field_values,omitempty"`
20041994
}
20051995

1996+
// sanitizeIssueTitleAndBody mutates issue.Title and issue.Body in place, applying the shared
1997+
// untrusted-content sanitization policy (pkg/sanitize). It exists for the handful of response
1998+
// paths — search_issues and search_pull_requests — that marshal a raw *github.Issue directly
1999+
// instead of routing through one of the convertToMinimal* helpers in minimal_types.go, which
2000+
// sanitize on their own. It is a no-op for a nil issue or unset fields.
2001+
func sanitizeIssueTitleAndBody(issue *github.Issue) {
2002+
if issue == nil {
2003+
return
2004+
}
2005+
if issue.Title != nil {
2006+
issue.Title = github.Ptr(sanitize.Sanitize(*issue.Title))
2007+
}
2008+
if issue.Body != nil {
2009+
issue.Body = github.Ptr(sanitize.Sanitize(*issue.Body))
2010+
}
2011+
}
2012+
20062013
// MarshalJSON serializes SearchIssueResult, suppressing the raw issue_field_values from the
20072014
// embedded REST response in favour of the normalized field_values populated via GraphQL enrichment.
2015+
// It also sanitizes the embedded issue's Title and Body in place: search_issues is one of the few
2016+
// response paths that marshals a raw *github.Issue directly rather than routing through a
2017+
// convertToMinimal* helper (see minimal_types.go), so sanitization must happen here instead.
20082018
func (r SearchIssueResult) MarshalJSON() ([]byte, error) {
2019+
sanitizeIssueTitleAndBody(r.Issue)
2020+
20092021
issueBytes, err := json.Marshal(r.Issue)
20102022
if err != nil {
20112023
return nil, err

pkg/github/minimal_types.go

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,17 @@ type MinimalDiscussionComment struct {
198198
ReplyTotalCount int `json:"replyTotalCount,omitempty"`
199199
}
200200

201+
// newMinimalDiscussionComment is the single constructor for MinimalDiscussionComment,
202+
// ensuring the untrusted, user-authored body is sanitized consistently regardless of
203+
// which discussion query (with or without replies) produced it.
204+
func newMinimalDiscussionComment(id string, body string, isAnswer bool) MinimalDiscussionComment {
205+
return MinimalDiscussionComment{
206+
ID: id,
207+
Body: sanitize.Sanitize(body),
208+
IsAnswer: isAnswer,
209+
}
210+
}
211+
201212
// MinimalCodeSearchResult is the trimmed output type for code search results.
202213
type MinimalCodeSearchResult struct {
203214
TotalCount int `json:"total_count"`
@@ -759,7 +770,7 @@ func convertToMinimalPullRequestReview(review *github.PullRequestReview) Minimal
759770
m := MinimalPullRequestReview{
760771
ID: review.GetID(),
761772
State: review.GetState(),
762-
Body: review.GetBody(),
773+
Body: sanitize.Sanitize(review.GetBody()),
763774
HTMLURL: review.GetHTMLURL(),
764775
User: convertToMinimalUser(review.GetUser()),
765776
CommitID: review.GetCommitID(),
@@ -776,8 +787,8 @@ func convertToMinimalPullRequestReview(review *github.PullRequestReview) Minimal
776787
func convertToMinimalIssue(issue *github.Issue) MinimalIssue {
777788
m := MinimalIssue{
778789
Number: issue.GetNumber(),
779-
Title: issue.GetTitle(),
780-
Body: issue.GetBody(),
790+
Title: sanitize.Sanitize(issue.GetTitle()),
791+
Body: sanitize.Sanitize(issue.GetBody()),
781792
State: issue.GetState(),
782793
StateReason: issue.GetStateReason(),
783794
Draft: issue.GetDraft(),
@@ -977,7 +988,7 @@ func convertToMinimalIssuesResponseWithoutFieldValues(fragment issueQueryFragmen
977988
func convertToMinimalIssueComment(comment *github.IssueComment) MinimalIssueComment {
978989
m := MinimalIssueComment{
979990
ID: comment.GetID(),
980-
Body: comment.GetBody(),
991+
Body: sanitize.Sanitize(comment.GetBody()),
981992
HTMLURL: comment.GetHTMLURL(),
982993
User: convertToMinimalUser(comment.GetUser()),
983994
AuthorAssociation: comment.GetAuthorAssociation(),
@@ -1026,7 +1037,7 @@ func convertToMinimalFileContentResponse(resp *github.RepositoryContentResponse)
10261037

10271038
m.Commit = &MinimalFileCommit{
10281039
SHA: resp.Commit.GetSHA(),
1029-
Message: resp.Commit.GetMessage(),
1040+
Message: sanitize.Sanitize(resp.Commit.GetMessage()),
10301041
HTMLURL: resp.Commit.GetHTMLURL(),
10311042
}
10321043

@@ -1046,8 +1057,8 @@ func convertToMinimalFileContentResponse(resp *github.RepositoryContentResponse)
10461057
func convertToMinimalPullRequest(pr *github.PullRequest) MinimalPullRequest {
10471058
m := MinimalPullRequest{
10481059
Number: pr.GetNumber(),
1049-
Title: pr.GetTitle(),
1050-
Body: pr.GetBody(),
1060+
Title: sanitize.Sanitize(pr.GetTitle()),
1061+
Body: sanitize.Sanitize(pr.GetBody()),
10511062
State: pr.GetState(),
10521063
Draft: pr.GetDraft(),
10531064
Merged: pr.GetMerged(),
@@ -1241,7 +1252,7 @@ func convertIssueToMinimalProjectItemContent(issue *github.Issue) *MinimalProjec
12411252
ID: issue.GetID(),
12421253
NodeID: issue.GetNodeID(),
12431254
Number: issue.GetNumber(),
1244-
Title: issue.GetTitle(),
1255+
Title: sanitize.Sanitize(issue.GetTitle()),
12451256
State: issue.GetState(),
12461257
StateReason: issue.GetStateReason(),
12471258
HTMLURL: issue.GetHTMLURL(),
@@ -1278,7 +1289,7 @@ func convertPullRequestToMinimalProjectItemContent(pr *github.PullRequest) *Mini
12781289
ID: pr.GetID(),
12791290
NodeID: pr.GetNodeID(),
12801291
Number: pr.GetNumber(),
1281-
Title: pr.GetTitle(),
1292+
Title: sanitize.Sanitize(pr.GetTitle()),
12821293
State: pr.GetState(),
12831294
HTMLURL: pr.GetHTMLURL(),
12841295
Repository: pullRequestRepositoryFullName(pr),
@@ -1315,7 +1326,7 @@ func convertDraftIssueToMinimalProjectItemContent(draftIssue *github.ProjectV2Dr
13151326
m := &MinimalProjectItemContent{
13161327
ID: draftIssue.GetID(),
13171328
NodeID: draftIssue.GetNodeID(),
1318-
Title: draftIssue.GetTitle(),
1329+
Title: sanitize.Sanitize(draftIssue.GetTitle()),
13191330
CreatedAt: formatProjectTimestamp(draftIssue.CreatedAt),
13201331
UpdatedAt: formatProjectTimestamp(draftIssue.UpdatedAt),
13211332
}
@@ -1574,7 +1585,7 @@ func minimalProjectPullRequestRefFromPullRequest(pr *github.PullRequest) minimal
15741585
}
15751586
return minimalProjectPullRequestRef{
15761587
Number: pr.GetNumber(),
1577-
Title: pr.GetTitle(),
1588+
Title: sanitize.Sanitize(pr.GetTitle()),
15781589
State: pr.GetState(),
15791590
HTMLURL: pr.GetHTMLURL(),
15801591
Repository: pullRequestRepositoryFullName(pr),
@@ -1596,7 +1607,7 @@ func minimalProjectPullRequestRefFromMap(value map[string]any) minimalProjectPul
15961607

15971608
return minimalProjectPullRequestRef{
15981609
Number: intFromAny(value["number"]),
1599-
Title: stringFromMap(value, "title"),
1610+
Title: sanitize.Sanitize(stringFromMap(value, "title")),
16001611
State: stringFromMap(value, "state"),
16011612
HTMLURL: htmlURL,
16021613
Repository: repository,
@@ -1756,7 +1767,7 @@ func newMinimalCommitFromCore(sha, htmlURL string, commit *github.Commit, author
17561767

17571768
if commit != nil {
17581769
minimalCommit.Commit = &MinimalCommitInfo{
1759-
Message: commit.GetMessage(),
1770+
Message: sanitize.Sanitize(commit.GetMessage()),
17601771
}
17611772

17621773
if commit.Author != nil {
@@ -1959,7 +1970,7 @@ func convertToMinimalPullRequestCommits(commits []*github.RepositoryCommit) []Mi
19591970
}
19601971

19611972
if commit.Commit != nil {
1962-
minimalCommit.Message = commit.Commit.GetMessage()
1973+
minimalCommit.Message = sanitize.Sanitize(commit.Commit.GetMessage())
19631974
minimalCommit.Author = convertToMinimalCommitAuthor(commit.Commit.Author)
19641975
}
19651976

@@ -1997,8 +2008,8 @@ func convertToMinimalRelease(release *github.RepositoryRelease) MinimalRelease {
19972008
m := MinimalRelease{
19982009
ID: release.GetID(),
19992010
TagName: release.GetTagName(),
2000-
Name: release.GetName(),
2001-
Body: release.GetBody(),
2011+
Name: sanitize.Sanitize(release.GetName()),
2012+
Body: sanitize.Sanitize(release.GetBody()),
20022013
HTMLURL: release.GetHTMLURL(),
20032014
Prerelease: release.GetPrerelease(),
20042015
Draft: release.GetDraft(),
@@ -2054,7 +2065,7 @@ func convertToMinimalWorkflowRun(workflowRun *github.WorkflowRun) MinimalWorkflo
20542065

20552066
if headCommit := workflowRun.GetHeadCommit(); headCommit != nil && headCommit.GetMessage() != "" {
20562067
minimalRun.HeadCommit = &MinimalWorkflowRunHeadCommit{
2057-
Message: headCommit.GetMessage(),
2068+
Message: sanitize.Sanitize(headCommit.GetMessage()),
20582069
}
20592070
}
20602071

@@ -2239,7 +2250,7 @@ func convertToMinimalReviewThread(thread reviewThreadNode) MinimalReviewThread {
22392250

22402251
func convertToMinimalReviewComment(c reviewCommentNode) MinimalReviewComment {
22412252
m := MinimalReviewComment{
2242-
Body: string(c.Body),
2253+
Body: sanitize.Sanitize(string(c.Body)),
22432254
Path: string(c.Path),
22442255
Author: string(c.Author.Login),
22452256
HTMLURL: c.URL.String(),

pkg/github/projects.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
ghErrors "github.com/github/github-mcp-server/pkg/errors"
1616
"github.com/github/github-mcp-server/pkg/ifc"
1717
"github.com/github/github-mcp-server/pkg/inventory"
18+
"github.com/github/github-mcp-server/pkg/sanitize"
1819
"github.com/github/github-mcp-server/pkg/scopes"
1920
"github.com/github/github-mcp-server/pkg/translations"
2021
"github.com/github/github-mcp-server/pkg/utils"
@@ -265,7 +266,7 @@ func convertToMinimalStatusUpdate(node statusUpdateNode) MinimalProjectStatusUpd
265266

266267
return MinimalProjectStatusUpdate{
267268
ID: fmt.Sprintf("%v", node.ID),
268-
Body: derefString(node.Body),
269+
Body: sanitize.Sanitize(derefString(node.Body)),
269270
Status: derefString(node.Status),
270271
CreatedAt: node.CreatedAt.Time.Format(time.RFC3339),
271272
StartDate: derefString(node.StartDate),

pkg/github/pullrequests.go

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"github.com/github/github-mcp-server/pkg/ifc"
1818
"github.com/github/github-mcp-server/pkg/inventory"
1919
"github.com/github/github-mcp-server/pkg/octicons"
20-
"github.com/github/github-mcp-server/pkg/sanitize"
2120
"github.com/github/github-mcp-server/pkg/scopes"
2221
"github.com/github/github-mcp-server/pkg/translations"
2322
"github.com/github/github-mcp-server/pkg/utils"
@@ -185,16 +184,6 @@ func GetPullRequest(ctx context.Context, client *github.Client, deps ToolDepende
185184
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request", resp, body), nil
186185
}
187186

188-
// sanitize title/body on response
189-
if pr != nil {
190-
if pr.Title != nil {
191-
pr.Title = github.Ptr(sanitize.Sanitize(*pr.Title))
192-
}
193-
if pr.Body != nil {
194-
pr.Body = github.Ptr(sanitize.Sanitize(*pr.Body))
195-
}
196-
}
197-
198187
if ff.LockdownMode {
199188
if restricted, err := authorLockdownResult(ctx, cache, owner, repo, pr.GetUser().GetLogin(), lockdownPullRequestRestrictedMessage); restricted != nil || err != nil {
200189
return restricted, err
@@ -1454,19 +1443,6 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool
14541443
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list pull requests", resp, bodyBytes), nil, nil
14551444
}
14561445

1457-
// sanitize title/body on each PR
1458-
for _, pr := range prs {
1459-
if pr == nil {
1460-
continue
1461-
}
1462-
if pr.Title != nil {
1463-
pr.Title = github.Ptr(sanitize.Sanitize(*pr.Title))
1464-
}
1465-
if pr.Body != nil {
1466-
pr.Body = github.Ptr(sanitize.Sanitize(*pr.Body))
1467-
}
1468-
}
1469-
14701446
minimalPRs := make([]MinimalPullRequest, 0, len(prs))
14711447
for _, pr := range prs {
14721448
if pr != nil {

0 commit comments

Comments
 (0)