Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/errors/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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), " ")
}

Expand Down
7 changes: 4 additions & 3 deletions pkg/errors/error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -703,13 +703,13 @@ func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) {

originalErr := &github.ErrorResponse{
Response: response,
Message: "Validation <script>secret-script</script>Failed\u202e",
Message: "Validation <script>secret-script</script>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",
Expand All @@ -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-")
Expand Down
4 changes: 2 additions & 2 deletions pkg/github/discussions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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),
Expand Down
4 changes: 2 additions & 2 deletions pkg/github/issues.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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))
Expand Down
22 changes: 11 additions & 11 deletions pkg/github/minimal_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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),
Expand All @@ -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,
Expand Down Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion pkg/github/repositories.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
27 changes: 22 additions & 5 deletions pkg/github/sanitize_coverage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "<script>alert(1)</script>Hello\u200BWorld"

// sanitizedText is what maliciousText becomes after sanitize.Sanitize: the <script> tag
// (and its content) is stripped by the HTML policy, and the zero-width space is removed.
// sanitizedText is what maliciousText becomes after sanitization: the <script> tag and
// its content are stripped by the HTML policy, and the zero-width space is removed.
const sanitizedText = "HelloWorld"

// Test_MinimalConverters_SanitizeUserAuthoredText is a table-driven regression test asserting
// that every convertToMinimal* helper which surfaces untrusted, user-authored prose (issue and
// PR titles/bodies, comments, reviews, review comments, releases, commit messages) applies
// pkg/sanitize.Sanitize consistently. This guards against the inconsistent coverage described in
// PR titles/bodies, comments, reviews, review comments, releases, commit messages) applies the
// appropriate shared sanitizer consistently. This guards against the inconsistent coverage described in
// https://github.com/github/github-mcp-server/issues/3106.
func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) {
tests := []struct {
Expand Down Expand Up @@ -257,6 +257,23 @@ func Test_SearchIssueResult_SanitizesTitleAndBody(t *testing.T) {
assert.Equal(t, sanitizedText, decoded.Body)
}

func Test_TitleSanitizationPreservesToolOutputText(t *testing.T) {
issue := convertToMinimalIssue(&github.Issue{
Title: github.Ptr(`can't "quote" AT&T`),
Body: github.Ptr(`<b>"quoted"</b>`),
})

result := MarshalledTextResult(map[string]string{
"body": issue.Body,
"title": issue.Title,
})

assert.Equal(t,
`{"body":"\u003cb\u003e\u0026#34;quoted\u0026#34;\u003c/b\u003e","title":"can't \"quote\" AT\u0026T"}`,
getTextResult(t, result).Text,
)
}

// 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) {
Expand Down
118 changes: 116 additions & 2 deletions pkg/sanitize/sanitize.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
package sanitize

import (
stdhtml "html"
"strconv"
"strings"
"sync"
"unicode"
"unicode/utf8"

"github.com/microcosm-cc/bluemonday"
nethtml "golang.org/x/net/html"
)

var policy *bluemonday.Policy
var policyOnce sync.Once
var (
policy *bluemonday.Policy
policyOnce sync.Once
plainTextPolicy *bluemonday.Policy
plainTextPolicyOnce sync.Once
)

func Sanitize(input string) string {
// The invisible-character and code-fence filters both run before and after
Expand All @@ -36,6 +43,106 @@ func Sanitize(input string) string {
return FilterCodeFenceMetadata(FilterInvisibleCharacters(normalized))
}

// PlainText sanitizes user-authored text that must not contain HTML.
func PlainText(input string) string {
filtered := FilterCodeFenceMetadata(FilterInvisibleCharacters(input))
if filtered == "" {
return ""
}

tokenizer := nethtml.NewTokenizer(strings.NewReader(filtered))
var marked strings.Builder
var text []string
for {
tokenType := tokenizer.Next()
if tokenType == nethtml.ErrorToken {
break
}
if tokenType == nethtml.TextToken {
marker := plainTextMarker(len(text))
marked.WriteString(marker)
text = append(text, neutralizePlainTextAngles(tokenizer.Token().Data))
continue
}
marked.Write(tokenizer.Raw())
}

sanitized := restorePlainText(getPlainTextPolicy().Sanitize(marked.String()), text)
return FilterCodeFenceMetadata(FilterInvisibleCharacters(sanitized))
}

const plainTextMarkerPrefix = "githubmcpplaintexttoken"

func plainTextMarker(index int) string {
return plainTextMarkerPrefix + strconv.Itoa(index) + "x"
}

func restorePlainText(marked string, values []string) string {
var restored strings.Builder
for marked != "" {
start := strings.Index(marked, plainTextMarkerPrefix)
if start < 0 {
restored.WriteString(marked)
break
}
restored.WriteString(marked[:start])
marked = marked[start+len(plainTextMarkerPrefix):]

end := strings.IndexByte(marked, 'x')
if end < 0 {
restored.WriteString(plainTextMarkerPrefix)
restored.WriteString(marked)
break
}
index, err := strconv.Atoi(marked[:end])
if err != nil || index < 0 || index >= len(values) {
restored.WriteString(plainTextMarkerPrefix)
restored.WriteString(marked[:end+1])
} else {
restored.WriteString(values[index])
}
marked = marked[end+1:]
}
return restored.String()
}

func neutralizePlainTextAngles(input string) string {
input = FilterInvisibleCharacters(input)
input = strings.ReplaceAll(input, "\x00", string(utf8.RuneError))
input = strings.ReplaceAll(input, "\r\n", "\n")
input = strings.ReplaceAll(input, "\r", "\n")
input = neutralizeNestedEntities(input)
input = strings.ReplaceAll(input, "<", "&lt;")
return strings.ReplaceAll(input, ">", "&gt;")
}

func neutralizeNestedEntities(input string) string {
var neutralized strings.Builder
for {
start := strings.IndexByte(input, '&')
if start < 0 {
neutralized.WriteString(input)
return neutralized.String()
}
neutralized.WriteString(input[:start])
input = input[start:]

end := strings.IndexByte(input[1:], '&')
if end < 0 {
end = len(input)
} else {
end++
}
if candidate := input[:end]; stdhtml.UnescapeString(candidate) != candidate {
neutralized.WriteString("&amp;")
input = input[1:]
} else {
neutralized.WriteByte('&')
input = input[1:]
}
}
}

// 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
Expand Down Expand Up @@ -335,6 +442,13 @@ func getPolicy() *bluemonday.Policy {
return policy
}

func getPlainTextPolicy() *bluemonday.Policy {
plainTextPolicyOnce.Do(func() {
plainTextPolicy = bluemonday.StrictPolicy()
})
return plainTextPolicy
}

func shouldRemoveRune(r rune) bool {
switch r {
case 0x200B, // ZERO WIDTH SPACE
Expand Down
Loading
Loading