Skip to content

Commit 207fe79

Browse files
louimclaude
andcommitted
Use upstream sanitizers for rename titles and dismissal messages
Rename titles now use sanitize.PlainText like issue titles (#3216), so a title such as can't "quote" AT&T no longer comes back HTML-escaped. The dismissal message uses sanitize.Content like review bodies (#3177). The new converter fields are added to the sanitizer coverage table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 9912c6c commit 207fe79

3 files changed

Lines changed: 62 additions & 16 deletions

File tree

‎pkg/github/issues_events_test.go‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func Test_GetIssueEvents(t *testing.T) {
108108
ID: new(int64(456)),
109109
Event: new("renamed"),
110110
Actor: &github.User{Login: new("outsider")},
111-
Rename: &github.Rename{From: new("old title"), To: new("new title")},
111+
Rename: &github.Rename{From: new("old title"), To: new(`can't "quote" AT&T`)},
112112
CommitID: new("abc123"),
113113
},
114114
}
@@ -145,7 +145,7 @@ func Test_GetIssueEvents(t *testing.T) {
145145
Event: "renamed",
146146
CommitID: "abc123",
147147
RenamedFrom: "old title",
148-
RenamedTo: "new title",
148+
RenamedTo: `can't "quote" AT&T`,
149149
},
150150
},
151151
},
@@ -439,14 +439,14 @@ func Test_GetIssueTimeline_TrimsAndSanitizes(t *testing.T) {
439439
assert.NotContains(t, raw[0], "parents")
440440
}
441441
442-
// Test_GetIssueTimeline_PreservesMarkdown checks that comment bodies and commit messages go
443-
// through sanitize.Content like get_comments does: Markdown and code survive, invisible
444-
// characters are stripped.
445-
func Test_GetIssueTimeline_PreservesMarkdown(t *testing.T) {
442+
// Test_GetIssueTimeline_PreservesAuthoredText checks that bodies and commit messages keep their
443+
// Markdown and code, and that rename titles keep quotes and ampersands, as on get_comments and get.
444+
func Test_GetIssueTimeline_PreservesAuthoredText(t *testing.T) {
446445
t.Parallel()
447446
448447
body := "Render <Foo/> here.\n\n```go\nif a<b {}\n```\n"
449448
message := "Fix comparison: if a<b {}"
449+
title := `can't "quote" AT&T`
450450

451451
serverTool := IssueReadWithEvents(translations.NullTranslationHelper)
452452
deps := issueEventsDeps(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -462,6 +462,11 @@ func Test_GetIssueTimeline_PreservesMarkdown(t *testing.T) {
462462
SHA: new("abc123"),
463463
Message: new(message),
464464
},
465+
{
466+
Event: new("renamed"),
467+
Actor: &github.User{Login: new("maintainer")},
468+
Rename: &github.Rename{From: new("old title"), To: new(title)},
469+
},
465470
}),
466471
}), false)
467472
@@ -477,10 +482,11 @@ func Test_GetIssueTimeline_PreservesMarkdown(t *testing.T) {
477482
478483
var returned []MinimalTimelineItem
479484
require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned))
480-
require.Len(t, returned, 2)
485+
require.Len(t, returned, 3)
481486
482487
assert.Equal(t, body, returned[0].Body)
483488
assert.Equal(t, message, returned[1].Message)
489+
assert.Equal(t, title, returned[2].RenamedTo)
484490
}
485491
486492
func Test_GetIssueEvent(t *testing.T) {
@@ -769,8 +775,6 @@ func Test_GetIssueEvent_CarriesDismissedReview(t *testing.T) {
769775
assert.Equal(t, "changes_requested", returned.DismissedReview.State)
770776
assert.Equal(t, int64(456), returned.DismissedReview.ReviewID)
771777
assert.Equal(t, "def456", returned.DismissedReview.DismissalCommitID)
772-
// The dismissal message is author-supplied, so it is sanitized like any other body text.
773-
assert.NotContains(t, returned.DismissedReview.DismissalMessage, "<script>")
774778
}
775779

776780
func Test_GetIssueTimeline_CarriesCommitAuthorship(t *testing.T) {

‎pkg/github/minimal_types.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1176,8 +1176,8 @@ func convertToMinimalIssueEvent(event *github.IssueEvent) MinimalIssueEvent {
11761176
m.Milestone = milestone.GetTitle()
11771177
}
11781178
if rename := event.GetRename(); rename != nil {
1179-
m.RenamedFrom = sanitize.Sanitize(rename.GetFrom())
1180-
m.RenamedTo = sanitize.Sanitize(rename.GetTo())
1179+
m.RenamedFrom = sanitize.PlainText(rename.GetFrom())
1180+
m.RenamedTo = sanitize.PlainText(rename.GetTo())
11811181
}
11821182
if issue := event.GetIssue(); issue != nil {
11831183
ref := newMinimalIssueRef(
@@ -1202,16 +1202,15 @@ func convertToMinimalIssueEvent(event *github.IssueEvent) MinimalIssueEvent {
12021202
m.DismissedReview = &MinimalDismissedReview{
12031203
State: dismissed.GetState(),
12041204
ReviewID: dismissed.GetReviewID(),
1205-
DismissalMessage: sanitize.Sanitize(dismissed.GetDismissalMessage()),
1205+
DismissalMessage: sanitize.Content(dismissed.GetDismissalMessage()),
12061206
DismissalCommitID: dismissed.GetDismissalCommitID(),
12071207
}
12081208
}
12091209

12101210
return m
12111211
}
12121212

1213-
// convertToMinimalTimelineItem trims a REST timeline entry. Bodies and commit messages use
1214-
// sanitize.Content to keep their Markdown intact, matching get_comments.
1213+
// convertToMinimalTimelineItem trims a REST timeline entry.
12151214
func convertToMinimalTimelineItem(item *github.Timeline) MinimalTimelineItem {
12161215
if item == nil {
12171216
return MinimalTimelineItem{}
@@ -1250,8 +1249,8 @@ func convertToMinimalTimelineItem(item *github.Timeline) MinimalTimelineItem {
12501249
m.Milestone = milestone.GetTitle()
12511250
}
12521251
if rename := item.GetRename(); rename != nil {
1253-
m.RenamedFrom = sanitize.Sanitize(rename.GetFrom())
1254-
m.RenamedTo = sanitize.Sanitize(rename.GetTo())
1252+
m.RenamedFrom = sanitize.PlainText(rename.GetFrom())
1253+
m.RenamedTo = sanitize.PlainText(rename.GetTo())
12551254
}
12561255
if reviewer := item.GetReviewer(); reviewer != nil {
12571256
m.RequestedReviewer = reviewer.GetLogin()

‎pkg/github/sanitize_coverage_test.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,49 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) {
230230
}).Title
231231
},
232232
},
233+
{
234+
name: "issue event renamed title",
235+
got: func() string {
236+
return convertToMinimalIssueEvent(&github.IssueEvent{
237+
Rename: &github.Rename{From: new(maliciousText)},
238+
}).RenamedFrom
239+
},
240+
},
241+
{
242+
name: "issue event dismissal message",
243+
content: true,
244+
got: func() string {
245+
return convertToMinimalIssueEvent(&github.IssueEvent{
246+
DismissedReview: &github.DismissedReview{DismissalMessage: new(maliciousText)},
247+
}).DismissedReview.DismissalMessage
248+
},
249+
},
250+
{
251+
name: "timeline renamed title",
252+
got: func() string {
253+
return convertToMinimalTimelineItem(&github.Timeline{
254+
Rename: &github.Rename{To: new(maliciousText)},
255+
}).RenamedTo
256+
},
257+
},
258+
{
259+
name: "timeline comment body",
260+
content: true,
261+
got: func() string {
262+
return convertToMinimalTimelineItem(&github.Timeline{
263+
Body: new(maliciousText),
264+
}).Body
265+
},
266+
},
267+
{
268+
name: "timeline commit message",
269+
content: true,
270+
got: func() string {
271+
return convertToMinimalTimelineItem(&github.Timeline{
272+
Message: new(maliciousText),
273+
}).Message
274+
},
275+
},
233276
}
234277

235278
for _, tt := range tests {

0 commit comments

Comments
 (0)