diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 13b607b405..91fb9c1923 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -257,7 +257,7 @@ func formatGitHubValidationDetail(validationErr github.Error) string { func sanitizeGitHubValidationText(value string) string { // Tool errors are plain text; keep quoted branch patterns readable. - sanitized := strings.ReplaceAll(sanitize.Sanitize(value), "'", "'") + sanitized := sanitize.Title(value) return strings.Join(strings.Fields(sanitized), " ") } diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 9ea31b2ebf..9bddd041f4 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -100,7 +100,7 @@ type WithCategoryNoOrder struct { func fragmentToDiscussion(fragment NodeFragment) *github.Discussion { return &github.Discussion{ Number: github.Ptr(int(fragment.Number)), - Title: github.Ptr(sanitize.Sanitize(string(fragment.Title))), + Title: github.Ptr(sanitize.Title(string(fragment.Title))), HTMLURL: github.Ptr(string(fragment.URL)), CreatedAt: &github.Timestamp{Time: fragment.CreatedAt.Time}, UpdatedAt: &github.Timestamp{Time: fragment.UpdatedAt.Time}, @@ -361,7 +361,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { // like ListDiscussions and GetDiscussionComments). response := map[string]any{ "number": int(d.Number), - "title": sanitize.Sanitize(string(d.Title)), + "title": sanitize.Title(string(d.Title)), "body": sanitize.Sanitize(string(d.Body)), "url": string(d.URL), "closed": bool(d.Closed), diff --git a/pkg/github/issues.go b/pkg/github/issues.go index fd7ea36873..40d82fc86a 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1194,7 +1194,7 @@ func GetIssueParent(ctx context.Context, client *githubv4.Client, deps ToolDepen return MarshalledTextResult(map[string]any{ "parent": map[string]any{ "number": int(parent.Number), - "title": sanitize.Sanitize(string(parent.Title)), + "title": sanitize.Title(string(parent.Title)), "state": string(parent.State), "url": string(parent.URL), "repository": string(parent.Repository.NameWithOwner), @@ -1995,7 +1995,7 @@ func sanitizeIssueTitleAndBody(issue *github.Issue) { return } if issue.Title != nil { - issue.Title = github.Ptr(sanitize.Sanitize(*issue.Title)) + issue.Title = github.Ptr(sanitize.Title(*issue.Title)) } if issue.Body != nil { issue.Body = github.Ptr(sanitize.Sanitize(*issue.Body)) diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index f8cf9f307c..a5769274d4 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -622,7 +622,7 @@ type MinimalPullRequestRef struct { func newMinimalPullRequestRef(number int, title, state, url, repository string) MinimalPullRequestRef { return MinimalPullRequestRef{ Number: number, - Title: sanitize.Sanitize(title), + Title: sanitize.Title(title), State: state, URL: url, Repository: repository, @@ -646,7 +646,7 @@ type MinimalIssueRef struct { func newMinimalIssueRef(number int, title, state, url, repository string) MinimalIssueRef { return MinimalIssueRef{ Number: number, - Title: sanitize.Sanitize(title), + Title: sanitize.Title(title), State: state, URL: url, Repository: repository, @@ -814,7 +814,7 @@ func convertToMinimalPullRequestReview(review *github.PullRequestReview) Minimal func convertToMinimalIssue(issue *github.Issue) MinimalIssue { m := MinimalIssue{ Number: issue.GetNumber(), - Title: sanitize.Sanitize(issue.GetTitle()), + Title: sanitize.Title(issue.GetTitle()), Body: sanitize.Sanitize(issue.GetBody()), State: issue.GetState(), StateReason: issue.GetStateReason(), @@ -925,7 +925,7 @@ func fragmentToMinimalIssue(fragment IssueFragment) MinimalIssue { func fragmentWithoutFieldValuesToMinimalIssue(fragment issueFragmentWithoutFieldValues) MinimalIssue { m := MinimalIssue{ Number: int(fragment.Number), - Title: sanitize.Sanitize(string(fragment.Title)), + Title: sanitize.Title(string(fragment.Title)), Body: sanitize.Sanitize(string(fragment.Body)), State: string(fragment.State), Comments: int(fragment.Comments.TotalCount), @@ -1084,7 +1084,7 @@ func convertToMinimalFileContentResponse(resp *github.RepositoryContentResponse) func convertToMinimalPullRequest(pr *github.PullRequest) MinimalPullRequest { m := MinimalPullRequest{ Number: pr.GetNumber(), - Title: sanitize.Sanitize(pr.GetTitle()), + Title: sanitize.Title(pr.GetTitle()), Body: sanitize.Sanitize(pr.GetBody()), State: pr.GetState(), Draft: pr.GetDraft(), @@ -1279,7 +1279,7 @@ func convertIssueToMinimalProjectItemContent(issue *github.Issue) *MinimalProjec ID: issue.GetID(), NodeID: issue.GetNodeID(), Number: issue.GetNumber(), - Title: sanitize.Sanitize(issue.GetTitle()), + Title: sanitize.Title(issue.GetTitle()), State: issue.GetState(), StateReason: issue.GetStateReason(), HTMLURL: issue.GetHTMLURL(), @@ -1316,7 +1316,7 @@ func convertPullRequestToMinimalProjectItemContent(pr *github.PullRequest) *Mini ID: pr.GetID(), NodeID: pr.GetNodeID(), Number: pr.GetNumber(), - Title: sanitize.Sanitize(pr.GetTitle()), + Title: sanitize.Title(pr.GetTitle()), State: pr.GetState(), HTMLURL: pr.GetHTMLURL(), Repository: pullRequestRepositoryFullName(pr), @@ -1353,7 +1353,7 @@ func convertDraftIssueToMinimalProjectItemContent(draftIssue *github.ProjectV2Dr m := &MinimalProjectItemContent{ ID: draftIssue.GetID(), NodeID: draftIssue.GetNodeID(), - Title: sanitize.Sanitize(draftIssue.GetTitle()), + Title: sanitize.Title(draftIssue.GetTitle()), CreatedAt: formatProjectTimestamp(draftIssue.CreatedAt), UpdatedAt: formatProjectTimestamp(draftIssue.UpdatedAt), } @@ -1612,7 +1612,7 @@ func minimalProjectPullRequestRefFromPullRequest(pr *github.PullRequest) minimal } return minimalProjectPullRequestRef{ Number: pr.GetNumber(), - Title: sanitize.Sanitize(pr.GetTitle()), + Title: sanitize.Title(pr.GetTitle()), State: pr.GetState(), HTMLURL: pr.GetHTMLURL(), Repository: pullRequestRepositoryFullName(pr), @@ -1634,7 +1634,7 @@ func minimalProjectPullRequestRefFromMap(value map[string]any) minimalProjectPul return minimalProjectPullRequestRef{ Number: intFromAny(value["number"]), - Title: sanitize.Sanitize(stringFromMap(value, "title")), + Title: sanitize.Title(stringFromMap(value, "title")), State: stringFromMap(value, "state"), HTMLURL: htmlURL, Repository: repository, @@ -2038,7 +2038,7 @@ func convertToMinimalRelease(release *github.RepositoryRelease) MinimalRelease { m := MinimalRelease{ ID: release.GetID(), TagName: release.GetTagName(), - Name: sanitize.Sanitize(release.GetName()), + Name: sanitize.Title(release.GetName()), Body: sanitize.Sanitize(release.GetBody()), HTMLURL: release.GetHTMLURL(), Prerelease: release.GetPrerelease(), diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 8dfa19b4a2..1fba420974 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -2981,7 +2981,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.Title(headline), CommittedDate: r.Commit.CommittedDate.Format("2006-01-02T15:04:05Z"), Author: BlameAuthor{ Name: string(r.Commit.Author.Name), diff --git a/pkg/github/sanitize_coverage_test.go b/pkg/github/sanitize_coverage_test.go index 59747e9780..9e404c2b94 100644 --- a/pkg/github/sanitize_coverage_test.go +++ b/pkg/github/sanitize_coverage_test.go @@ -257,6 +257,111 @@ func Test_SearchIssueResult_SanitizesTitleAndBody(t *testing.T) { assert.Equal(t, sanitizedText, decoded.Body) } +func Test_MinimalConverters_TitlePreservesVisibleText(t *testing.T) { + title := "[bug] can't add a connection to toolkits in desktop app" + + tests := []struct { + name string + got func() string + }{ + { + name: "issue title (REST)", + got: func() string { + return convertToMinimalIssue(&github.Issue{Title: github.Ptr(title)}).Title + }, + }, + { + name: "issue title (GraphQL)", + got: func() string { + return fragmentWithoutFieldValuesToMinimalIssue(issueFragmentWithoutFieldValues{ + Title: githubv4.String(title), + }).Title + }, + }, + { + name: "pull request title", + got: func() string { + return convertToMinimalPullRequest(&github.PullRequest{Title: github.Ptr(title)}).Title + }, + }, + { + name: "release name", + got: func() string { + return convertToMinimalRelease(&github.RepositoryRelease{Name: github.Ptr(title)}).Name + }, + }, + { + name: "project item content title (issue)", + got: func() string { + return convertIssueToMinimalProjectItemContent(&github.Issue{Title: github.Ptr(title)}).Title + }, + }, + { + name: "project item content title (pull request)", + got: func() string { + return convertPullRequestToMinimalProjectItemContent(&github.PullRequest{Title: github.Ptr(title)}).Title + }, + }, + { + name: "project item content title (draft issue)", + got: func() string { + return convertDraftIssueToMinimalProjectItemContent(&github.ProjectV2DraftIssue{Title: github.Ptr(title)}).Title + }, + }, + { + name: "project pull request ref title (from *github.PullRequest)", + got: func() string { + return minimalProjectPullRequestRefFromPullRequest(&github.PullRequest{Title: github.Ptr(title)}).Title + }, + }, + { + name: "project pull request ref title (from map)", + got: func() string { + return minimalProjectPullRequestRefFromMap(map[string]any{"title": title}).Title + }, + }, + { + name: "issue ref title (shared constructor)", + got: func() string { + return newMinimalIssueRef(1, title, "OPEN", "https://github.com/o/r/issues/1", "o/r").Title + }, + }, + { + name: "pull request ref title (shared constructor)", + got: func() string { + return newMinimalPullRequestRef(1, title, "OPEN", "https://github.com/o/r/pull/1", "o/r").Title + }, + }, + { + name: "issue dependency ref title", + got: func() string { + return issueToDependencyRef(&github.Issue{Title: github.Ptr(title)}).Title + }, + }, + { + name: "discussion title", + got: func() string { + discussion := fragmentToDiscussion(NodeFragment{Title: githubv4.String(title)}) + return discussion.GetTitle() + }, + }, + { + name: "search issue result title", + got: func() string { + issue := &github.Issue{Title: github.Ptr(title)} + sanitizeIssueTitleAndBody(issue) + return issue.GetTitle() + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, title, tt.got()) + }) + } +} + // Test_SanitizeIssueTitleAndBody exercises the shared helper directly, including its nil-safety, // since it backs both search_issues and search_pull_requests. func Test_SanitizeIssueTitleAndBody(t *testing.T) { diff --git a/pkg/sanitize/sanitize.go b/pkg/sanitize/sanitize.go index 80d367029b..7c4ca58095 100644 --- a/pkg/sanitize/sanitize.go +++ b/pkg/sanitize/sanitize.go @@ -36,6 +36,45 @@ func Sanitize(input string) string { return FilterCodeFenceMetadata(FilterInvisibleCharacters(normalized)) } +// Title sanitizes short metadata fields such as issue and pull request titles. +// It applies the same HTML and invisible-character policy as Sanitize, then +// restores the punctuation that policy HTML-escapes so visible characters +// remain as themselves (for example, "can't" instead of "can't"). +// +// Angle brackets stay escaped. Decoding < / > would reconstitute markup +// from entity-encoded tags, including nested &amp;lt; payloads. +func Title(input string) string { + return restoreVisiblePunctuation(Sanitize(input)) +} + +// visiblePunctuationUnescaper inverts html.EscapeString for apostrophe, quote, +// and ampersand only. It must not include < or >. +var visiblePunctuationUnescaper = strings.NewReplacer( + "'", "'", + """, `"`, + """, `"`, + "'", "'", + "&", "&", +) + +func restoreVisiblePunctuation(input string) string { + if !strings.Contains(input, "&") { + return input + } + out := input + // Peel stacked & prefixes (&#39; → ' → ') without ever + // turning < / > into angle brackets. Each Replace shortens the + // string or is a no-op, so this is bounded by len(input). + for range len(input) { + next := visiblePunctuationUnescaper.Replace(out) + if next == out { + return out + } + out = next + } + return out +} + // 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..775e69695e 100644 --- a/pkg/sanitize/sanitize_test.go +++ b/pkg/sanitize/sanitize_test.go @@ -581,6 +581,10 @@ var invariantCorpus = []string{ "surrogate \xed\xa0\x80 encoded", strings.Repeat("clean ascii prose. ", 64), strings.Repeat("caf\u00e9 \u4e16\u754c \U0001F600\uFE0F ", 32), + "[bug] can't add a connection to toolkits in desktop app", + "<script>alert(1)</script>", + "&lt;script&gt;alert(1)&lt;/script&gt;", + "&amp;lt;script&amp;gt;alert(1)&amp;lt;/script&amp;gt;", } // TestHTMLInertBytesAreFixedPointsOfThePolicy is the load-bearing check on the @@ -718,6 +722,103 @@ func TestSanitizeStillStripsMaliciousContent(t *testing.T) { } for _, in := range payloads { require.NotEqual(t, in, Sanitize(in), "Sanitize left payload %q untouched", in) + require.NotEqual(t, in, Title(in), "Title left payload %q untouched", in) + require.NotContains(t, Title(in), "alert(1)Hello", + expected: "Hello", + }, + { + name: "leaves entity-encoded tags escaped", + input: "<script>alert(1)</script>Hello", + expected: "<script>alert(1)</script>Hello", + }, + { + name: "strips invisible characters", + input: "Fix\u200B the bug", + expected: "Fix the bug", + }, + { + name: "decodes then strips entity-encoded invisible characters", + input: "Hello​World", + expected: "HelloWorld", + }, + { + name: "empty string", + input: "", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Title(tt.input) + assert.Equal(t, tt.expected, got) + assert.NotContains(t, got, "'") + assert.NotContains(t, got, """) + assert.NotContains(t, got, """) + assert.NotContains(t, got, "alert(1)", + "<script>alert(1)</script>", + "&lt;script&gt;alert(1)&lt;/script&gt;", + "&amp;lt;script&amp;gt;alert(1)&amp;lt;/script&amp;gt;", + "&amp;amp;lt;script&amp;amp;gt;alert(1)&amp;amp;lt;/script&amp;amp;gt;", + "<script>alert(1)</script>", + } + for _, in := range payloads { + got := Title(in) + require.NotContains(t, got, " %q", in, got) + require.Equal(t, got, Title(got), "Title not idempotent on nested encoding %q", in) + } +} + +func TestTitleIsIdempotent(t *testing.T) { + for _, in := range invariantCorpus { + once := Title(in) + require.Equal(t, once, Title(once), "Title not idempotent on %q", in) + } +} + +func TestTitleDoesNotAllocateForCleanASCII(t *testing.T) { + clean := []string{ + "Fix flaky converter test for issue comments on large pages", + strings.Repeat("clean ascii prose. ", 512), + } + for _, in := range clean { + require.Equal(t, in, Title(in)) + require.Zero(t, testing.AllocsPerRun(20, func() { sink = Title(in) }), + "Title allocated for clean input %q", in) } }