Skip to content

Commit 9205304

Browse files
SamMorrowDrumsCopilot
SamMorrowDrums
and
Copilot
authored
fix: restore sanitizer content boundaries (#3219)
Preserve Markdown and HTML in body and commit-message fields while stripping invisible controls. Apply plain-text sanitization consistently to raw release and sub-issue titles and restore blame headline handling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent cf3a4bb commit 9205304

5 files changed

Lines changed: 48 additions & 34 deletions

File tree

‎pkg/github/issues.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2013,7 +2013,7 @@ func sanitizeSubIssueTitleAndBody(issue *github.SubIssue) {
20132013
return
20142014
}
20152015
if issue.Title != nil {
2016-
issue.Title = github.Ptr(sanitize.Sanitize(*issue.Title))
2016+
issue.Title = github.Ptr(sanitize.PlainText(*issue.Title))
20172017
}
20182018
if issue.Body != nil {
20192019
issue.Body = github.Ptr(sanitize.Content(*issue.Body))

‎pkg/github/issues_test.go‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5774,8 +5774,8 @@ func Test_AddSubIssue(t *testing.T) {
57745774
// Setup mock issue for success case (matches GitHub API response format)
57755775
mockIssue := &github.Issue{
57765776
Number: github.Ptr(42),
5777-
Title: github.Ptr("<int>\u200B"),
5778-
Body: github.Ptr("This is **Markdown**\u200B"),
5777+
Title: github.Ptr("<script>alert(1)</script>can't \"quote\" AT&T\u200B"),
5778+
Body: github.Ptr("<script>alert(1)</script>This is **Markdown**\u200B"),
57795779
State: github.Ptr("open"),
57805780
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"),
57815781
User: &github.User{
@@ -5970,8 +5970,8 @@ func Test_AddSubIssue(t *testing.T) {
59705970
err = json.Unmarshal([]byte(textContent.Text), &returnedIssue)
59715971
require.NoError(t, err)
59725972
assert.Equal(t, *tc.expectedIssue.Number, *returnedIssue.Number)
5973-
assert.Empty(t, *returnedIssue.Title)
5974-
assert.Equal(t, "This is **Markdown**", *returnedIssue.Body)
5973+
assert.Equal(t, "can't \"quote\" AT&T", *returnedIssue.Title)
5974+
assert.Equal(t, "<script>alert(1)</script>This is **Markdown**", *returnedIssue.Body)
59755975
assert.Equal(t, *tc.expectedIssue.State, *returnedIssue.State)
59765976
assert.Equal(t, *tc.expectedIssue.HTMLURL, *returnedIssue.HTMLURL)
59775977
assert.Equal(t, *tc.expectedIssue.User.Login, *returnedIssue.User.Login)
@@ -5999,8 +5999,8 @@ func Test_GetSubIssues(t *testing.T) {
59995999
mockSubIssues := []*github.Issue{
60006000
{
60016001
Number: github.Ptr(123),
6002-
Title: github.Ptr("<int>\u200B"),
6003-
Body: github.Ptr("This is **Markdown**\u200B"),
6002+
Title: github.Ptr("<script>alert(1)</script>can't \"quote\" AT&T\u200B"),
6003+
Body: github.Ptr("<script>alert(1)</script>This is **Markdown**\u200B"),
60046004
State: github.Ptr("open"),
60056005
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"),
60066006
User: &github.User{
@@ -6200,8 +6200,8 @@ func Test_GetSubIssues(t *testing.T) {
62006200
if i < len(tc.expectedSubIssues) {
62016201
assert.Equal(t, *tc.expectedSubIssues[i].Number, *subIssue.Number)
62026202
if i == 0 {
6203-
assert.Empty(t, *subIssue.Title)
6204-
assert.Equal(t, "This is **Markdown**", *subIssue.Body)
6203+
assert.Equal(t, "can't \"quote\" AT&T", *subIssue.Title)
6204+
assert.Equal(t, "<script>alert(1)</script>This is **Markdown**", *subIssue.Body)
62056205
} else {
62066206
assert.Equal(t, *tc.expectedSubIssues[i].Title, *subIssue.Title)
62076207
}
@@ -6657,8 +6657,8 @@ func Test_RemoveSubIssue(t *testing.T) {
66576657
// Setup mock issue for success case (matches GitHub API response format - the updated parent issue)
66586658
mockIssue := &github.Issue{
66596659
Number: github.Ptr(42),
6660-
Title: github.Ptr("<int>\u200B"),
6661-
Body: github.Ptr("This is **Markdown**\u200B"),
6660+
Title: github.Ptr("<script>alert(1)</script>can't \"quote\" AT&T\u200B"),
6661+
Body: github.Ptr("<script>alert(1)</script>This is **Markdown**\u200B"),
66626662
State: github.Ptr("open"),
66636663
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"),
66646664
User: &github.User{
@@ -6836,8 +6836,8 @@ func Test_RemoveSubIssue(t *testing.T) {
68366836
err = json.Unmarshal([]byte(textContent.Text), &returnedIssue)
68376837
require.NoError(t, err)
68386838
assert.Equal(t, *tc.expectedIssue.Number, *returnedIssue.Number)
6839-
assert.Empty(t, *returnedIssue.Title)
6840-
assert.Equal(t, "This is **Markdown**", *returnedIssue.Body)
6839+
assert.Equal(t, "can't \"quote\" AT&T", *returnedIssue.Title)
6840+
assert.Equal(t, "<script>alert(1)</script>This is **Markdown**", *returnedIssue.Body)
68416841
assert.Equal(t, *tc.expectedIssue.State, *returnedIssue.State)
68426842
assert.Equal(t, *tc.expectedIssue.HTMLURL, *returnedIssue.HTMLURL)
68436843
assert.Equal(t, *tc.expectedIssue.User.Login, *returnedIssue.User.Login)
@@ -6865,8 +6865,8 @@ func Test_ReprioritizeSubIssue(t *testing.T) {
68656865
// Setup mock issue for success case (matches GitHub API response format - the updated parent issue)
68666866
mockIssue := &github.Issue{
68676867
Number: github.Ptr(42),
6868-
Title: github.Ptr("<int>\u200B"),
6869-
Body: github.Ptr("This is **Markdown**\u200B"),
6868+
Title: github.Ptr("<script>alert(1)</script>can't \"quote\" AT&T\u200B"),
6869+
Body: github.Ptr("<script>alert(1)</script>This is **Markdown**\u200B"),
68706870
State: github.Ptr("open"),
68716871
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"),
68726872
User: &github.User{
@@ -7096,8 +7096,8 @@ func Test_ReprioritizeSubIssue(t *testing.T) {
70967096
err = json.Unmarshal([]byte(textContent.Text), &returnedIssue)
70977097
require.NoError(t, err)
70987098
assert.Equal(t, *tc.expectedIssue.Number, *returnedIssue.Number)
7099-
assert.Empty(t, *returnedIssue.Title)
7100-
assert.Equal(t, "This is **Markdown**", *returnedIssue.Body)
7099+
assert.Equal(t, "can't \"quote\" AT&T", *returnedIssue.Title)
7100+
assert.Equal(t, "<script>alert(1)</script>This is **Markdown**", *returnedIssue.Body)
71017101
assert.Equal(t, *tc.expectedIssue.State, *returnedIssue.State)
71027102
assert.Equal(t, *tc.expectedIssue.HTMLURL, *returnedIssue.HTMLURL)
71037103
assert.Equal(t, *tc.expectedIssue.User.Login, *returnedIssue.User.Login)

‎pkg/github/minimal_types.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ type MinimalDiscussionComment struct {
204204
func newMinimalDiscussionComment(id string, body string, isAnswer bool) MinimalDiscussionComment {
205205
return MinimalDiscussionComment{
206206
ID: id,
207-
Body: sanitize.Sanitize(body),
207+
Body: sanitize.Content(body),
208208
IsAnswer: isAnswer,
209209
}
210210
}
@@ -1015,7 +1015,7 @@ func convertToMinimalIssuesResponseWithoutFieldValues(fragment issueQueryFragmen
10151015
func convertToMinimalIssueComment(comment *github.IssueComment) MinimalIssueComment {
10161016
m := MinimalIssueComment{
10171017
ID: comment.GetID(),
1018-
Body: sanitize.Sanitize(comment.GetBody()),
1018+
Body: sanitize.Content(comment.GetBody()),
10191019
HTMLURL: comment.GetHTMLURL(),
10201020
User: convertToMinimalUser(comment.GetUser()),
10211021
AuthorAssociation: comment.GetAuthorAssociation(),
@@ -1064,7 +1064,7 @@ func convertToMinimalFileContentResponse(resp *github.RepositoryContentResponse)
10641064

10651065
m.Commit = &MinimalFileCommit{
10661066
SHA: resp.Commit.GetSHA(),
1067-
Message: sanitize.Sanitize(resp.Commit.GetMessage()),
1067+
Message: sanitize.Content(resp.Commit.GetMessage()),
10681068
HTMLURL: resp.Commit.GetHTMLURL(),
10691069
}
10701070

@@ -1794,7 +1794,7 @@ func newMinimalCommitFromCore(sha, htmlURL string, commit *github.Commit, author
17941794

17951795
if commit != nil {
17961796
minimalCommit.Commit = &MinimalCommitInfo{
1797-
Message: sanitize.Sanitize(commit.GetMessage()),
1797+
Message: sanitize.Content(commit.GetMessage()),
17981798
}
17991799

18001800
if commit.Author != nil {
@@ -2000,7 +2000,7 @@ func convertToMinimalPullRequestCommits(commits []*github.RepositoryCommit) []Mi
20002000
}
20012001

20022002
if commit.Commit != nil {
2003-
minimalCommit.Message = sanitize.Sanitize(commit.Commit.GetMessage())
2003+
minimalCommit.Message = sanitize.Content(commit.Commit.GetMessage())
20042004
minimalCommit.Author = convertToMinimalCommitAuthor(commit.Commit.Author)
20052005
}
20062006

@@ -2095,7 +2095,7 @@ func convertToMinimalWorkflowRun(workflowRun *github.WorkflowRun) MinimalWorkflo
20952095

20962096
if headCommit := workflowRun.GetHeadCommit(); headCommit != nil && headCommit.GetMessage() != "" {
20972097
minimalRun.HeadCommit = &MinimalWorkflowRunHeadCommit{
2098-
Message: sanitize.Sanitize(headCommit.GetMessage()),
2098+
Message: sanitize.Content(headCommit.GetMessage()),
20992099
}
21002100
}
21012101

@@ -2280,7 +2280,7 @@ func convertToMinimalReviewThread(thread reviewThreadNode) MinimalReviewThread {
22802280

22812281
func convertToMinimalReviewComment(c reviewCommentNode) MinimalReviewComment {
22822282
m := MinimalReviewComment{
2283-
Body: sanitize.Sanitize(string(c.Body)),
2283+
Body: sanitize.Content(string(c.Body)),
22842284
Path: string(c.Path),
22852285
Author: string(c.Author.Login),
22862286
HTMLURL: c.URL.String(),

‎pkg/github/repositories.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2233,6 +2233,7 @@ func GetLatestRelease(t translations.TranslationHelperFunc) inventory.ServerTool
22332233
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get latest release", resp, body), nil, nil
22342234
}
22352235

2236+
sanitizeReleaseNameAndBody(release)
22362237
r, err := json.Marshal(release)
22372238
if err != nil {
22382239
return nil, nil, fmt.Errorf("failed to marshal response: %w", err)
@@ -2319,6 +2320,7 @@ func GetReleaseByTag(t translations.TranslationHelperFunc) inventory.ServerTool
23192320
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get release by tag", resp, body), nil, nil
23202321
}
23212322

2323+
sanitizeReleaseNameAndBody(release)
23222324
r, err := json.Marshal(release)
23232325
if err != nil {
23242326
return nil, nil, fmt.Errorf("failed to marshal response: %w", err)
@@ -2338,6 +2340,18 @@ func GetReleaseByTag(t translations.TranslationHelperFunc) inventory.ServerTool
23382340
)
23392341
}
23402342

2343+
func sanitizeReleaseNameAndBody(release *github.RepositoryRelease) {
2344+
if release == nil {
2345+
return
2346+
}
2347+
if release.Name != nil {
2348+
release.Name = github.Ptr(sanitize.PlainText(*release.Name))
2349+
}
2350+
if release.Body != nil {
2351+
release.Body = github.Ptr(sanitize.Content(*release.Body))
2352+
}
2353+
}
2354+
23412355
// ListStarredRepositories creates a tool to list starred repositories for the authenticated user or a specified user.
23422356
func ListStarredRepositories(t translations.TranslationHelperFunc) inventory.ServerTool {
23432357
return NewTool(
@@ -2981,7 +2995,7 @@ func GetFileBlame(t translations.TranslationHelperFunc) inventory.ServerTool {
29812995
SHA: sha,
29822996
// Sanitized after truncation so the headline is cut at the author's real
29832997
// first line break rather than one introduced by sanitization.
2984-
MessageHeadline: sanitize.Content(headline),
2998+
MessageHeadline: sanitize.PlainText(headline),
29852999
CommittedDate: r.Commit.CommittedDate.Format("2006-01-02T15:04:05Z"),
29863000
Author: BlameAuthor{
29873001
Name: string(r.Commit.Author.Name),

‎pkg/github/repositories_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4854,8 +4854,8 @@ func Test_GetLatestRelease(t *testing.T) {
48544854
mockRelease := &github.RepositoryRelease{
48554855
ID: 1,
48564856
TagName: "v1.0.0",
4857-
Name: github.Ptr("First Release"),
4858-
Body: github.Ptr("<details>Notes</details>\u200B"),
4857+
Name: github.Ptr("<script>alert(1)</script>can't \"quote\" AT&T\u200B"),
4858+
Body: github.Ptr("<script>alert(1)</script><details>Notes</details>\u200B"),
48594859
}
48604860

48614861
tests := []struct {
@@ -4923,8 +4923,8 @@ func Test_GetLatestRelease(t *testing.T) {
49234923
err = json.Unmarshal([]byte(textContent.Text), &returnedRelease)
49244924
require.NoError(t, err)
49254925
assert.Equal(t, tc.expectedResult.TagName, returnedRelease.TagName)
4926-
assert.Equal(t, "First Release", *returnedRelease.Name)
4927-
assert.Equal(t, "<details>Notes</details>", *returnedRelease.Body)
4926+
assert.Equal(t, "can't \"quote\" AT&T", *returnedRelease.Name)
4927+
assert.Equal(t, "<script>alert(1)</script><details>Notes</details>", *returnedRelease.Body)
49284928
})
49294929
}
49304930
}
@@ -4947,8 +4947,8 @@ func Test_GetReleaseByTag(t *testing.T) {
49474947
mockRelease := &github.RepositoryRelease{
49484948
ID: 1,
49494949
TagName: "v1.0.0",
4950-
Name: github.Ptr("Release v1.0.0"),
4951-
Body: github.Ptr("<details>Notes</details>\u200B"),
4950+
Name: github.Ptr("<script>alert(1)</script>can't \"quote\" AT&T\u200B"),
4951+
Body: github.Ptr("<script>alert(1)</script><details>Notes</details>\u200B"),
49524952
Assets: []*github.ReleaseAsset{
49534953
{
49544954
ID: github.Ptr(int64(1)),
@@ -5088,9 +5088,9 @@ func Test_GetReleaseByTag(t *testing.T) {
50885088

50895089
assert.Equal(t, tc.expectedResult.ID, returnedRelease.ID)
50905090
assert.Equal(t, tc.expectedResult.TagName, returnedRelease.TagName)
5091-
assert.Equal(t, *tc.expectedResult.Name, *returnedRelease.Name)
5091+
assert.Equal(t, "can't \"quote\" AT&T", *returnedRelease.Name)
50925092
if tc.expectedResult.Body != nil {
5093-
assert.Equal(t, "<details>Notes</details>", *returnedRelease.Body)
5093+
assert.Equal(t, "<script>alert(1)</script><details>Notes</details>", *returnedRelease.Body)
50945094
}
50955095
if len(tc.expectedResult.Assets) > 0 {
50965096
require.Len(t, returnedRelease.Assets, len(tc.expectedResult.Assets))
@@ -6203,7 +6203,7 @@ func Test_GetFileBlame(t *testing.T) {
62036203
var br BlameResult
62046204
require.NoError(t, json.Unmarshal([]byte(result), &br))
62056205
require.Contains(t, br.Commits, "badc0ffee0000")
6206-
assert.Equal(t, sanitizedContentText, br.Commits["badc0ffee0000"].MessageHeadline)
6206+
assert.Equal(t, sanitizedText, br.Commits["badc0ffee0000"].MessageHeadline)
62076207
assert.NotContains(t, result, "<script>")
62086208
assert.NotContains(t, result, "Long body that should not appear")
62096209
},

0 commit comments

Comments
 (0)