Skip to content

Commit 4f4917b

Browse files
Sanitize remaining issue-ref and blame headline response paths
Route every MinimalIssueRef/MinimalPullRequestRef construction through shared constructors that sanitize the user-authored title, so issue_dependency_read, issue_dependency_write and find_duplicate no longer forward raw issue titles. Also sanitize the get_file_blame commit message headline, after truncation so the headline is still cut at the author's real first line break. Extends the sanitization regression suite with the project status update body, both ref constructors and the dependency ref, and adds tool-level regression tests for find_duplicate and get_file_blame. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent ab2b623 commit 4f4917b

8 files changed

Lines changed: 188 additions & 30 deletions

pkg/github/find_duplicate.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,15 @@ func FindDuplicate(t translations.TranslationHelperFunc) inventory.ServerTool {
152152
return utils.NewToolResultError("ranked duplicate detection is unavailable: the semantic-similarity endpoint returned issues without ranking metadata (the server-side duplicate-ranking feature is not enabled for this caller or repository)"), nil, nil
153153
}
154154
candidates = append(candidates, duplicateCandidate{
155-
Issue: MinimalIssueRef{
156-
Number: res.Issue.Number,
157-
Title: res.Issue.Title,
158-
State: res.Issue.State,
159-
URL: res.Issue.HTMLURL,
160-
},
155+
// Candidates are always scoped to the requested repository, so the
156+
// ref's repository field is left empty as it was before.
157+
Issue: newMinimalIssueRef(
158+
res.Issue.Number,
159+
res.Issue.Title,
160+
res.Issue.State,
161+
res.Issue.HTMLURL,
162+
"",
163+
),
161164
Score: res.Score,
162165
Confidence: res.Confidence,
163166
LikelyDuplicate: res.LikelyDuplicate,

pkg/github/find_duplicate_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,52 @@ func Test_FindDuplicate_RankedResults(t *testing.T) {
120120
assert.False(t, candidates[1].LikelyDuplicate)
121121
}
122122

123+
// Test_FindDuplicate_SanitizesIssueTitle asserts that candidate issue titles, which are
124+
// user-authored content from an arbitrary repository, are sanitized before being returned.
125+
// Without this the tool would forward hidden-instruction payloads straight to the model.
126+
func Test_FindDuplicate_SanitizesIssueTitle(t *testing.T) {
127+
serverTool := FindDuplicate(translations.NullTranslationHelper)
128+
129+
rankedResults := []map[string]any{
130+
{
131+
"issue": map[string]any{
132+
"number": 456,
133+
"title": maliciousText,
134+
"state": "open",
135+
"html_url": "https://github.com/owner/repo/issues/456",
136+
},
137+
"score": 0.95,
138+
"confidence": "high",
139+
"likely_duplicate": true,
140+
},
141+
}
142+
143+
handler := func(w http.ResponseWriter, _ *http.Request) {
144+
w.WriteHeader(http.StatusOK)
145+
_, _ = w.Write(MustMarshal(rankedResults))
146+
}
147+
148+
client := mustNewGHClient(t, NewMockedHTTPClient(WithRequestMatchHandler(endpointSemanticallySimilar, http.HandlerFunc(handler))))
149+
deps := BaseDeps{Client: client}
150+
toolHandler := serverTool.Handler(deps)
151+
152+
request := createMCPRequest(map[string]any{
153+
"owner": "owner",
154+
"repo": "repo",
155+
"issue_number": float64(123),
156+
})
157+
result, err := toolHandler(ContextWithDeps(context.Background(), deps), &request)
158+
require.NoError(t, err)
159+
require.False(t, result.IsError, "expected result to not be an error")
160+
161+
text := getTextResult(t, result)
162+
var candidates []duplicateCandidate
163+
require.NoError(t, json.Unmarshal([]byte(text.Text), &candidates))
164+
require.Len(t, candidates, 1)
165+
assert.Equal(t, sanitizedText, candidates[0].Issue.Title)
166+
assert.NotContains(t, text.Text, "<script>")
167+
}
168+
123169
func Test_FindDuplicate_OmitsUnsetParams(t *testing.T) {
124170
serverTool := FindDuplicate(translations.NullTranslationHelper)
125171

pkg/github/issue_dependencies.go

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -171,16 +171,17 @@ func issueToDependencyRef(issue *github.Issue) MinimalIssueRef {
171171
if issue == nil {
172172
return MinimalIssueRef{}
173173
}
174-
ref := MinimalIssueRef{
175-
Number: issue.GetNumber(),
176-
Title: issue.GetTitle(),
177-
State: strings.ToUpper(issue.GetState()),
178-
URL: issue.GetHTMLURL(),
179-
}
174+
var repository string
180175
if owner, repo, ok := parseRepositoryURL(issue.GetRepositoryURL()); ok {
181-
ref.Repository = owner + "/" + repo
176+
repository = owner + "/" + repo
182177
}
183-
return ref
178+
return newMinimalIssueRef(
179+
issue.GetNumber(),
180+
issue.GetTitle(),
181+
strings.ToUpper(issue.GetState()),
182+
issue.GetHTMLURL(),
183+
repository,
184+
)
184185
}
185186

186187
// IssueDependencyWrite creates a tool to add or remove an issue dependency

pkg/github/issues.go

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2196,27 +2196,27 @@ func fetchIssueReadEnrichment(ctx context.Context, gqlClient *githubv4.Client, n
21962196

21972197
if p := n.Issue.Parent; p != nil {
21982198
enrichment.Parent = &issueReadParent{
2199-
Ref: MinimalIssueRef{
2200-
Number: int(p.Number),
2201-
Title: sanitize.Sanitize(string(p.Title)),
2202-
State: string(p.State),
2203-
URL: string(p.URL),
2204-
Repository: string(p.Repository.NameWithOwner),
2205-
},
2199+
Ref: newMinimalIssueRef(
2200+
int(p.Number),
2201+
string(p.Title),
2202+
string(p.State),
2203+
string(p.URL),
2204+
string(p.Repository.NameWithOwner),
2205+
),
22062206
AuthorLogin: string(p.Author.Login),
22072207
}
22082208
}
22092209

22102210
closing := make([]issueReadClosingPullRequest, 0, len(n.Issue.ClosedByPullRequestsReferences.Nodes))
22112211
for _, pr := range n.Issue.ClosedByPullRequestsReferences.Nodes {
22122212
closing = append(closing, issueReadClosingPullRequest{
2213-
Ref: MinimalPullRequestRef{
2214-
Number: int(pr.Number),
2215-
Title: sanitize.Sanitize(string(pr.Title)),
2216-
State: string(pr.State),
2217-
URL: string(pr.URL),
2218-
Repository: string(pr.Repository.NameWithOwner),
2219-
},
2213+
Ref: newMinimalPullRequestRef(
2214+
int(pr.Number),
2215+
string(pr.Title),
2216+
string(pr.State),
2217+
string(pr.URL),
2218+
string(pr.Repository.NameWithOwner),
2219+
),
22202220
AuthorLogin: string(pr.Author.Login),
22212221
})
22222222
}

pkg/github/minimal_types.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,19 @@ type MinimalPullRequestRef struct {
616616
Repository string `json:"repository,omitempty"`
617617
}
618618

619+
// newMinimalPullRequestRef builds a MinimalPullRequestRef, sanitizing the user-authored
620+
// title. Callers should use it rather than the struct literal so every tool surfacing a
621+
// pull request reference strips untrusted content identically.
622+
func newMinimalPullRequestRef(number int, title, state, url, repository string) MinimalPullRequestRef {
623+
return MinimalPullRequestRef{
624+
Number: number,
625+
Title: sanitize.Sanitize(title),
626+
State: state,
627+
URL: url,
628+
Repository: repository,
629+
}
630+
}
631+
619632
// MinimalIssueRef is a compact reference to a related issue (e.g. a parent issue).
620633
// Its keys mirror the get_parent (GetIssueParent) response shape.
621634
type MinimalIssueRef struct {
@@ -626,6 +639,20 @@ type MinimalIssueRef struct {
626639
Repository string `json:"repository,omitempty"`
627640
}
628641

642+
// newMinimalIssueRef builds a MinimalIssueRef, sanitizing the user-authored title. Callers
643+
// should use it rather than the struct literal so every tool surfacing an issue reference
644+
// (issue_read's parent, issue_dependency_read/write, find_duplicate) strips untrusted
645+
// content identically.
646+
func newMinimalIssueRef(number int, title, state, url, repository string) MinimalIssueRef {
647+
return MinimalIssueRef{
648+
Number: number,
649+
Title: sanitize.Sanitize(title),
650+
State: state,
651+
URL: url,
652+
Repository: repository,
653+
}
654+
}
655+
629656
// MinimalSubIssuesSummary holds the native GraphQL subIssuesSummary counts for an issue.
630657
type MinimalSubIssuesSummary struct {
631658
Total int `json:"total"`

pkg/github/repositories.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/github/github-mcp-server/pkg/ifc"
1717
"github.com/github/github-mcp-server/pkg/inventory"
1818
"github.com/github/github-mcp-server/pkg/octicons"
19+
"github.com/github/github-mcp-server/pkg/sanitize"
1920
"github.com/github/github-mcp-server/pkg/scopes"
2021
"github.com/github/github-mcp-server/pkg/translations"
2122
"github.com/github/github-mcp-server/pkg/utils"
@@ -2950,8 +2951,10 @@ func GetFileBlame(t translations.TranslationHelperFunc) inventory.ServerTool {
29502951
}
29512952
headline = strings.TrimRight(headline, " \t\r")
29522953
bc := BlameCommit{
2953-
SHA: sha,
2954-
MessageHeadline: headline,
2954+
SHA: sha,
2955+
// Sanitized after truncation so the headline is cut at the author's real
2956+
// first line break rather than one introduced by sanitization.
2957+
MessageHeadline: sanitize.Sanitize(headline),
29552958
CommittedDate: r.Commit.CommittedDate.Format("2006-01-02T15:04:05Z"),
29562959
Author: BlameAuthor{
29572960
Name: string(r.Commit.Author.Name),

pkg/github/repositories_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6075,6 +6075,54 @@ func Test_GetFileBlame(t *testing.T) {
60756075
assert.Nil(t, br.Commits["xyz789abc123"].Author.Login, "anonymous author has no login")
60766076
},
60776077
},
6078+
{
6079+
// Commit messages are user-authored and untrusted: the headline must be
6080+
// truncated at the author's real first line break and then sanitized.
6081+
name: "blame commit message headline is sanitized",
6082+
mockedClient: githubv4mock.NewMockedHTTPClient(
6083+
githubv4mock.NewQueryMatcher(
6084+
blameQueryShape{},
6085+
makeBlameVars("testowner", "testrepo", "HEAD", "README.md"),
6086+
githubv4mock.DataResponse(map[string]any{
6087+
"repository": map[string]any{
6088+
"defaultBranchRef": map[string]any{"name": "main"},
6089+
"object": map[string]any{
6090+
"__typename": "Commit",
6091+
"blame": map[string]any{
6092+
"ranges": []map[string]any{
6093+
{
6094+
"startingLine": 1, "endingLine": 3, "age": 1,
6095+
"commit": map[string]any{
6096+
"oid": "badc0ffee0000",
6097+
"message": maliciousText + "\n\nLong body that should not appear.",
6098+
"committedDate": "2024-01-03T10:00:00Z",
6099+
"author": map[string]any{
6100+
"name": "Bob Developer", "email": "bob@example.com",
6101+
"user": nil,
6102+
},
6103+
},
6104+
},
6105+
},
6106+
},
6107+
},
6108+
},
6109+
}),
6110+
),
6111+
),
6112+
requestArgs: map[string]any{
6113+
"owner": "testowner",
6114+
"repo": "testrepo",
6115+
"path": "README.md",
6116+
},
6117+
validateResponse: func(t *testing.T, result string) {
6118+
var br BlameResult
6119+
require.NoError(t, json.Unmarshal([]byte(result), &br))
6120+
require.Contains(t, br.Commits, "badc0ffee0000")
6121+
assert.Equal(t, sanitizedText, br.Commits["badc0ffee0000"].MessageHeadline)
6122+
assert.NotContains(t, result, "<script>")
6123+
assert.NotContains(t, result, "Long body that should not appear")
6124+
},
6125+
},
60786126
{
60796127
name: "successful blame with annotated tag ref",
60806128
mockedClient: githubv4mock.NewMockedHTTPClient(

pkg/github/sanitize_coverage_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"encoding/json"
55
"net/url"
66
"testing"
7+
"time"
78

89
"github.com/google/go-github/v89/github"
910
"github.com/shurcooL/githubv4"
@@ -194,6 +195,35 @@ func Test_MinimalConverters_SanitizeUserAuthoredText(t *testing.T) {
194195
}).Title
195196
},
196197
},
198+
{
199+
name: "project status update body (projects_get / projects_list)",
200+
got: func() string {
201+
return convertToMinimalStatusUpdate(statusUpdateNode{
202+
Body: githubv4.NewString(githubv4.String(maliciousText)),
203+
CreatedAt: githubv4.DateTime{Time: time.Unix(0, 0).UTC()},
204+
}).Body
205+
},
206+
},
207+
{
208+
name: "issue ref title (shared constructor)",
209+
got: func() string {
210+
return newMinimalIssueRef(1, maliciousText, "OPEN", "https://github.com/o/r/issues/1", "o/r").Title
211+
},
212+
},
213+
{
214+
name: "pull request ref title (shared constructor)",
215+
got: func() string {
216+
return newMinimalPullRequestRef(1, maliciousText, "OPEN", "https://github.com/o/r/pull/1", "o/r").Title
217+
},
218+
},
219+
{
220+
name: "issue dependency ref title (issue_dependency_read / issue_dependency_write)",
221+
got: func() string {
222+
return issueToDependencyRef(&github.Issue{
223+
Title: github.Ptr(maliciousText),
224+
}).Title
225+
},
226+
},
197227
}
198228

199229
for _, tt := range tests {

0 commit comments

Comments
 (0)