From 2f85b82e931a96adabaa908b0e79d7c111e60782 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 3 Sep 2026 13:32:53 +0200 Subject: [PATCH] fix(sanitize): preserve plain-text title characters Add an entity-aware plain-text sanitizer for titles, release names, headlines, and validation text while keeping encoded markup inert and Markdown body handling unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/errors/error.go | 3 +- pkg/errors/error_test.go | 7 +- pkg/github/discussions.go | 4 +- pkg/github/issues.go | 4 +- pkg/github/minimal_types.go | 22 ++--- pkg/github/repositories.go | 2 +- pkg/github/sanitize_coverage_test.go | 27 +++++- pkg/sanitize/sanitize.go | 118 ++++++++++++++++++++++- pkg/sanitize/sanitize_test.go | 136 +++++++++++++++++++++++++++ 9 files changed, 295 insertions(+), 28 deletions(-) diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 13b607b405..cc3e542cc9 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -256,8 +256,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.PlainText(value) return strings.Join(strings.Fields(sanitized), " ") } diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index 9938c12df9..bdf7cb16bf 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -703,13 +703,13 @@ func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) { originalErr := &github.ErrorResponse{ Response: response, - Message: "Validation Failed\u202e", + Message: "Validation Failed\u202e for AT&T", Errors: []github.Error{ { Resource: "GitRef", Field: "ref", Code: "custom", - Message: "ref name does not match the required pattern 'feature/*'\u202e", + Message: `ref name does not match the required pattern 'feature/*' or "release/*"` + "\u202e", }, }, DocumentationURL: "https://docs.github.test/private?token=secret-doc-token", @@ -724,7 +724,8 @@ func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) { ) text := requireErrorText(t, result) - assert.Equal(t, "failed to create branch: Validation Failed\nGitRef.ref (custom): ref name does not match the required pattern 'feature/*'", text) + assert.Equal(t, `failed to create branch: Validation Failed for AT&T +GitRef.ref (custom): ref name does not match the required pattern 'feature/*' or "release/*"`, text) assert.NotContains(t, text, "create ref") assert.NotContains(t, text, "https://") assert.NotContains(t, text, "secret-") diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 9ea31b2ebf..8a601da86a 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.PlainText(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.PlainText(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..9a83e9c2cc 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.PlainText(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.PlainText(*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..ce0213f3ef 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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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.PlainText(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..17f16caec2 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.PlainText(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..1b423334f4 100644 --- a/pkg/github/sanitize_coverage_test.go +++ b/pkg/github/sanitize_coverage_test.go @@ -13,19 +13,19 @@ import ( ) // maliciousText contains an HTML payload plus invisible/hidden-instruction characters, -// mirroring the classes of untrusted content pkg/sanitize.Sanitize is meant to strip: +// mirroring the classes of untrusted content the shared sanitizers are meant to strip: // disallowed HTML tags and zero-width/BiDi control characters that can hide instructions // from a human reviewer while still being interpreted by a model. const maliciousText = "Hello\u200BWorld" -// sanitizedText is what maliciousText becomes after sanitize.Sanitize: the after", + expected: "beforeafter", + }, + { + name: "raw formatting element", + input: "bold and italic", + expected: "bold and italic", + }, + { + name: "named entity encoded element remains inert", + input: "<script>alert(1)</script>", + expected: "<script>alert(1)</script>", + }, + { + name: "decimal entity encoded element remains inert", + input: "<script>alert(1)</script>", + expected: "<script>alert(1)</script>", + }, + { + name: "hexadecimal entity encoded element remains inert", + input: "<script>alert(1)</script>", + expected: "<script>alert(1)</script>", + }, + { + name: "double encoded element remains inert", + input: "&lt;script&gt;", + expected: "&lt;script&gt;", + }, + { + name: "triply encoded element remains inert", + input: "&amp;lt;script&amp;gt;", + expected: "&amp;lt;script&amp;gt;", + }, + { + name: "double encoded punctuation remains encoded once", + input: "can&#39;t", + expected: "can&#39;t", + }, + { + name: "malformed element is stripped", + input: "before bold", + expected: "githubmcpplaintexttoken1xbold", + }, + { + name: "literal angle brackets are neutralized", + input: "1 < 2 > 0", + expected: "1 < 2 > 0", + }, + { + name: "literal invisible and bidi characters", + input: "Hello\u200B\u202EWorld", + expected: "HelloWorld", + }, + { + name: "encoded invisible and bidi characters", + input: "Hello​‮World", + expected: "HelloWorld", + }, + { + name: "nul characters are normalized", + input: "Hello\x00�World", + expected: "Hello��World", + }, + { + name: "malformed utf8 is normalized", + input: "Hello\xffWorld", + expected: "Hello\uFFFDWorld", + }, + { + name: "code fence metadata", + input: "```steal secrets\ncode\n```", + expected: "```\ncode\n```", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := PlainText(tt.input) + require.Equal(t, tt.expected, result) + require.Equal(t, result, PlainText(result)) + }) + } +} + // TestSanitizeFiltersInvisibleCharactersAfterEntityDecoding covers the core // regression from issue #3101: invisible/bidi characters encoded as HTML // character entities are decoded by FilterHTMLTags, so the invisible-character @@ -665,6 +782,25 @@ func TestSanitizeIsIdempotent(t *testing.T) { } } +func TestPlainTextIsIdempotent(t *testing.T) { + for _, in := range invariantCorpus { + once := PlainText(in) + require.Equal(t, once, PlainText(once), "PlainText not idempotent on %q", in) + } +} + +func FuzzPlainTextIsIdempotent(f *testing.F) { + for _, seed := range invariantCorpus { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, in string) { + once := PlainText(in) + if twice := PlainText(once); twice != once { + t.Fatalf("PlainText not idempotent on %q: first %q, second %q", in, once, twice) + } + }) +} + // TestSanitizeDoesNotAllocateForCleanASCII pins the allocation contract from // issue #3117: ordinary clean text passes through without being copied. func TestSanitizeDoesNotAllocateForCleanASCII(t *testing.T) {