Skip to content

Commit 57d4875

Browse files
fix(sanitize): preserve code in sanitized bodies and cover remaining read paths
Full Sanitize runs bluemonday, which escapes entities and silently truncates a fenced code block at the first '<'. Comment and review bodies are the most code-dense fields the server returns, so applying it there corrupts content delivered to the model. Add sanitize.FilterBody (invisible characters + code fence metadata, no HTML filtering) and use it for issue comment, PR review, PR review comment and sub-issue bodies. Titles keep the full Sanitize treatment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b8dd0f23-2c6e-47c1-bad5-c1d0c9361f13
1 parent fe386b2 commit 57d4875

6 files changed

Lines changed: 190 additions & 5 deletions

File tree

pkg/github/issues.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -951,7 +951,7 @@ func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependenc
951951
subIssue.Title = github.Ptr(sanitize.Sanitize(*subIssue.Title))
952952
}
953953
if subIssue.Body != nil {
954-
subIssue.Body = github.Ptr(sanitize.Sanitize(*subIssue.Body))
954+
subIssue.Body = github.Ptr(sanitize.FilterBody(*subIssue.Body))
955955
}
956956
}
957957

pkg/github/issues_test.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4738,6 +4738,56 @@ func Test_GetSubIssues(t *testing.T) {
47384738
}
47394739
}
47404740

4741+
func Test_GetSubIssues_Sanitization(t *testing.T) {
4742+
serverTool := IssueRead(translations.NullTranslationHelper)
4743+
4744+
hiddenPayload := "Sub-issue\U000E0001\U000E0049\U000E0067\U000E006E\U000E006F\U000E0072\U000E0065"
4745+
bodyWithCode := "Repro:\n```go\nif a<b { fmt.Println(\"x\") }\n```"
4746+
4747+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
4748+
GetReposIssuesSubIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*github.Issue{
4749+
{
4750+
Number: github.Ptr(123),
4751+
Title: github.Ptr(hiddenPayload),
4752+
Body: github.Ptr(hiddenPayload),
4753+
State: github.Ptr("open"),
4754+
},
4755+
{
4756+
Number: github.Ptr(124),
4757+
Title: github.Ptr("Sub-issue 2"),
4758+
Body: github.Ptr(bodyWithCode),
4759+
State: github.Ptr("open"),
4760+
},
4761+
}),
4762+
})
4763+
4764+
deps := BaseDeps{
4765+
Client: mustNewGHClient(t, mockedClient),
4766+
GQLClient: githubv4.NewClient(nil),
4767+
RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute),
4768+
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
4769+
}
4770+
handler := serverTool.Handler(deps)
4771+
4772+
request := createMCPRequest(map[string]any{
4773+
"method": "get_sub_issues",
4774+
"owner": "owner",
4775+
"repo": "repo",
4776+
"issue_number": float64(42),
4777+
})
4778+
4779+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
4780+
require.NoError(t, err)
4781+
4782+
var returnedSubIssues []*github.Issue
4783+
require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returnedSubIssues))
4784+
require.Len(t, returnedSubIssues, 2)
4785+
4786+
assert.Equal(t, "Sub-issue", returnedSubIssues[0].GetTitle(), "hidden characters must be stripped from titles")
4787+
assert.Equal(t, "Sub-issue", returnedSubIssues[0].GetBody(), "hidden characters must be stripped from bodies")
4788+
assert.Equal(t, bodyWithCode, returnedSubIssues[1].GetBody(), "code content must survive sanitization")
4789+
}
4790+
47414791
func TestAddIssueComment(t *testing.T) {
47424792
t.Parallel()
47434793

pkg/github/minimal_types.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -645,7 +645,7 @@ func convertToMinimalPullRequestReview(review *github.PullRequestReview) Minimal
645645
m := MinimalPullRequestReview{
646646
ID: review.GetID(),
647647
State: review.GetState(),
648-
Body: review.GetBody(),
648+
Body: sanitize.FilterBody(review.GetBody()),
649649
HTMLURL: review.GetHTMLURL(),
650650
User: convertToMinimalUser(review.GetUser()),
651651
CommitID: review.GetCommitID(),
@@ -821,9 +821,9 @@ func convertToMinimalIssuesResponse(fragment IssueQueryFragment) MinimalIssuesRe
821821
func convertToMinimalIssueComment(comment *github.IssueComment) MinimalIssueComment {
822822
m := MinimalIssueComment{
823823
ID: comment.GetID(),
824-
// Bodies carry the same invisible-glyph / HTML injection surface as
824+
// Comment bodies carry the same invisible-glyph injection surface as
825825
// issue and PR bodies, which the read paths already sanitize.
826-
Body: sanitize.Sanitize(comment.GetBody()),
826+
Body: sanitize.FilterBody(comment.GetBody()),
827827
HTMLURL: comment.GetHTMLURL(),
828828
User: convertToMinimalUser(comment.GetUser()),
829829
AuthorAssociation: comment.GetAuthorAssociation(),
@@ -1911,7 +1911,7 @@ func convertToMinimalReviewThread(thread reviewThreadNode) MinimalReviewThread {
19111911

19121912
func convertToMinimalReviewComment(c reviewCommentNode) MinimalReviewComment {
19131913
m := MinimalReviewComment{
1914-
Body: string(c.Body),
1914+
Body: sanitize.FilterBody(string(c.Body)),
19151915
Path: string(c.Path),
19161916
Author: string(c.Author.Login),
19171917
HTMLURL: c.URL.String(),

pkg/github/minimal_types_test.go

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package github
2+
3+
import (
4+
"net/url"
5+
"testing"
6+
7+
"github.com/google/go-github/v89/github"
8+
"github.com/shurcooL/githubv4"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// bodyWithHiddenPayload embeds Unicode tag characters, which are invisible to a
14+
// human reviewer but legible to a model.
15+
const bodyWithHiddenPayload = "Looks good\U000E0001\U000E0049\U000E0067\U000E006E\U000E006F\U000E0072\U000E0065"
16+
17+
const bodyWithCode = "Compare with:\n```go\nif a<b { fmt.Println(\"x\") }\n```\nand <Foo/> in JSX."
18+
19+
func TestConvertToMinimalIssueCommentSanitizesBody(t *testing.T) {
20+
t.Run("strips hidden characters", func(t *testing.T) {
21+
m := convertToMinimalIssueComment(&github.IssueComment{
22+
ID: github.Ptr(int64(1)),
23+
Body: github.Ptr(bodyWithHiddenPayload),
24+
})
25+
assert.Equal(t, "Looks good", m.Body)
26+
})
27+
28+
t.Run("preserves code content", func(t *testing.T) {
29+
m := convertToMinimalIssueComment(&github.IssueComment{
30+
ID: github.Ptr(int64(1)),
31+
Body: github.Ptr(bodyWithCode),
32+
})
33+
assert.Equal(t, bodyWithCode, m.Body)
34+
})
35+
}
36+
37+
func TestConvertToMinimalPullRequestReviewSanitizesBody(t *testing.T) {
38+
t.Run("strips hidden characters", func(t *testing.T) {
39+
m := convertToMinimalPullRequestReview(&github.PullRequestReview{
40+
ID: github.Ptr(int64(1)),
41+
Body: github.Ptr(bodyWithHiddenPayload),
42+
})
43+
assert.Equal(t, "Looks good", m.Body)
44+
})
45+
46+
t.Run("preserves code content", func(t *testing.T) {
47+
m := convertToMinimalPullRequestReview(&github.PullRequestReview{
48+
ID: github.Ptr(int64(1)),
49+
Body: github.Ptr(bodyWithCode),
50+
})
51+
assert.Equal(t, bodyWithCode, m.Body)
52+
})
53+
}
54+
55+
func TestConvertToMinimalReviewCommentSanitizesBody(t *testing.T) {
56+
commentURL, err := url.Parse("https://github.com/owner/repo/pull/1#discussion_r1")
57+
require.NoError(t, err)
58+
59+
t.Run("strips hidden characters", func(t *testing.T) {
60+
m := convertToMinimalReviewComment(reviewCommentNode{
61+
Body: githubv4.String(bodyWithHiddenPayload),
62+
Path: githubv4.String("main.go"),
63+
URL: githubv4.URI{URL: commentURL},
64+
})
65+
assert.Equal(t, "Looks good", m.Body)
66+
})
67+
68+
t.Run("preserves code content", func(t *testing.T) {
69+
m := convertToMinimalReviewComment(reviewCommentNode{
70+
Body: githubv4.String(bodyWithCode),
71+
Path: githubv4.String("main.go"),
72+
URL: githubv4.URI{URL: commentURL},
73+
})
74+
assert.Equal(t, bodyWithCode, m.Body)
75+
})
76+
}

pkg/sanitize/sanitize.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@ func Sanitize(input string) string {
1515
return FilterHTMLTags(FilterCodeFenceMetadata(FilterInvisibleCharacters(input)))
1616
}
1717

18+
// FilterBody strips the injection surface that matters for markdown bodies —
19+
// invisible glyphs and hidden code-fence info strings — without running the
20+
// HTML filter. Bodies routinely contain code (generics, JSX, shell redirects),
21+
// and HTML filtering silently truncates a fenced block at the first '<', which
22+
// would corrupt the content delivered to the model.
23+
func FilterBody(input string) string {
24+
return FilterCodeFenceMetadata(FilterInvisibleCharacters(input))
25+
}
26+
1827
// FilterInvisibleCharacters removes invisible or control characters that should not appear
1928
// in user-facing titles or bodies. This includes:
2029
// - Unicode tag characters: U+E0001, U+E0020–U+E007F

pkg/sanitize/sanitize_test.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,3 +300,53 @@ func TestSanitizeRemovesInvisibleCodeFenceMetadata(t *testing.T) {
300300
result := Sanitize(input)
301301
assert.Equal(t, expected, result)
302302
}
303+
304+
func TestFilterBody(t *testing.T) {
305+
tests := []struct {
306+
name string
307+
input string
308+
expected string
309+
}{
310+
{
311+
name: "removes unicode tag characters",
312+
input: "hello\U000E0001\U000E0068\U000E0069world",
313+
expected: "helloworld",
314+
},
315+
{
316+
name: "removes bidi overrides",
317+
input: "safe\u202Ereversed\u202C",
318+
expected: "safereversed",
319+
},
320+
{
321+
name: "strips hidden code fence metadata",
322+
input: "```steal secrets\nfmt.Println(42)\n```",
323+
expected: "```\nfmt.Println(42)\n```",
324+
},
325+
{
326+
name: "preserves angle brackets in prose",
327+
input: "a < b && c > d",
328+
expected: "a < b && c > d",
329+
},
330+
{
331+
name: "preserves code fences containing angle brackets",
332+
input: "```go\nif a<b { fmt.Println(\"x\") }\n```",
333+
expected: "```go\nif a<b { fmt.Println(\"x\") }\n```",
334+
},
335+
{
336+
name: "preserves html-like markup",
337+
input: "use <Foo/> component",
338+
expected: "use <Foo/> component",
339+
},
340+
{
341+
name: "empty string",
342+
input: "",
343+
expected: "",
344+
},
345+
}
346+
347+
for _, tt := range tests {
348+
t.Run(tt.name, func(t *testing.T) {
349+
assert.Equal(t, tt.expected, FilterBody(tt.input))
350+
})
351+
}
352+
}

0 commit comments

Comments
 (0)