Skip to content

Commit 9ad99c5

Browse files
authored
Add ifc label for search_issues tool (#2456)
* Add ifc label for search_issues tool Emits an IFC SecurityLabel on the search_issues tool result when the InsidersMode flag is enabled, mirroring the pattern landed for get_me in #2432, list_issues in #2453, and get_file_contents in #2454. Search results may span multiple repositories, so the label is the IFC join of the per-repository labels: - Integrity is always untrusted (issues are user-authored). - If any matched repository is public, the joined readers are ["public"] (the public side dominates the lub). - Otherwise the joined readers are the intersection of the collaborator sets across all matched private repositories. - Empty result sets are labelled public-untrusted (no data leaked). The shared searchHandler in search_utils.go gains an additive variadic 'searchOption' hook so SearchIssues can attach _meta.ifc without duplicating the search call. SearchPullRequests is unaffected; it does not pass any options. If any per-repository visibility or collaborators lookup fails the label is omitted entirely, consistent with get_file_contents, to avoid misclassifying the result. Refs github/copilot-mcp-core#1623, github/copilot-mcp-core#1389. Note: this PR is chained on #2454 (gokhanarkan/fides-get-file-contents) because it depends on the FetchRepoIsPrivate and FetchRepoCollaborators helpers introduced there. GitHub will retarget the base to main once #2454 merges. * search_issues: address Copilot review findings - LabelSearchIssues now returns (SecurityLabel, bool); the bool is false when len(repoVisibilities) != len(readerSets), so callers can omit the label rather than emit one computed from inconsistent inputs. - searchIssuesIFCPostProcess no longer substitutes [owner] when the collaborators API returns an empty list. The substitution was inconsistent with the cross-repo intersection semantics: the owner could appear in another matched private repo's collaborator list and thereby widen the joined reader set incorrectly. Empty collaborator sets are now passed through unchanged. - Add a subtest exercising the collaborators-failure branch (500 on /repos/{owner}/{repo}/collaborators), asserting the tool still succeeds and result.Meta["ifc"] is absent. - Extend the LabelSearchIssues table tests with the slice-length mismatch case. Addresses the three Copilot findings on #2456. * search_issues: flip IFC join to intersection (private wins) Address Joanna's review feedback on #2456: a reader of a multi-repo result must be authorised to read every matched private repository, so the IFC join is the meet (intersection over private repos) rather than the join. Public matches contribute the universe set and drop out of the intersection without shrinking it. - LabelSearchIssues: collect only the private reader sets, then intersect. Empty result and all-public remain public-untrusted. - TestLabelSearchIssues: flip the mixed public+private expectation and add a 'two private + one public' case to lock in the new semantics. - Test_SearchIssues_IFC_InsidersMode: mixed subtest now expects the private repo's reader set instead of public.
1 parent 59fa9a7 commit 9ad99c5

5 files changed

Lines changed: 544 additions & 5 deletions

File tree

pkg/github/issues.go

Lines changed: 105 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,6 @@ func GetIssueLabels(ctx context.Context, client *githubv4.Client, owner string,
543543
}
544544

545545
return utils.NewToolResultText(string(out)), nil
546-
547546
}
548547

549548
// ListIssueTypes creates a tool to list defined issue types for an organization. This can be used to understand supported issue type values for creating or updating issues.
@@ -838,7 +837,6 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo
838837
}
839838

840839
return utils.NewToolResultText(string(r)), nil
841-
842840
}
843841

844842
func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int) (*mcp.CallToolResult, error) {
@@ -978,11 +976,115 @@ func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool {
978976
},
979977
[]scopes.Scope{scopes.Repo},
980978
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
981-
result, err := searchHandler(ctx, deps.GetClient, args, "issue", "failed to search issues")
979+
var options []searchOption
980+
if deps.GetFlags(ctx).InsidersMode {
981+
options = append(options, withSearchPostProcess(searchIssuesIFCPostProcess(deps)))
982+
}
983+
result, err := searchHandler(ctx, deps.GetClient, args, "issue", "failed to search issues", options...)
982984
return result, nil, err
983985
})
984986
}
985987

988+
// searchIssuesIFCPostProcess returns a searchPostProcessFn that attaches the
989+
// IFC label for a search_issues result. It looks up the visibility (and, for
990+
// private repos, collaborators) of every repository represented in the search
991+
// payload and joins the labels via ifc.LabelSearchIssues. If any per-repo
992+
// lookup fails the label is omitted to avoid misclassifying the result.
993+
func searchIssuesIFCPostProcess(deps ToolDependencies) searchPostProcessFn {
994+
return func(ctx context.Context, result *github.IssuesSearchResult, callResult *mcp.CallToolResult) {
995+
if callResult == nil || callResult.IsError || result == nil {
996+
return
997+
}
998+
999+
client, err := deps.GetClient(ctx)
1000+
if err != nil {
1001+
return
1002+
}
1003+
1004+
uniqueRepos := uniqueSearchIssuesRepos(result)
1005+
visibilities := make([]bool, 0, len(uniqueRepos))
1006+
readerSets := make([][]string, 0, len(uniqueRepos))
1007+
for _, r := range uniqueRepos {
1008+
isPrivate, err := FetchRepoIsPrivate(ctx, client, r.owner, r.repo)
1009+
if err != nil {
1010+
return
1011+
}
1012+
visibilities = append(visibilities, isPrivate)
1013+
if !isPrivate {
1014+
readerSets = append(readerSets, nil)
1015+
continue
1016+
}
1017+
collaborators, err := FetchRepoCollaborators(ctx, client, r.owner, r.repo)
1018+
if err != nil {
1019+
return
1020+
}
1021+
// Preserve an empty collaborator set as-is. Substituting the
1022+
// owner here would corrupt the cross-repo intersection (the
1023+
// owner could appear in another repo's collaborator list and
1024+
// widen the joined reader set incorrectly).
1025+
readerSets = append(readerSets, collaborators)
1026+
}
1027+
1028+
label, ok := ifc.LabelSearchIssues(visibilities, readerSets)
1029+
if !ok {
1030+
return
1031+
}
1032+
if callResult.Meta == nil {
1033+
callResult.Meta = mcp.Meta{}
1034+
}
1035+
callResult.Meta["ifc"] = label
1036+
}
1037+
}
1038+
1039+
type searchIssuesRepoRef struct {
1040+
owner string
1041+
repo string
1042+
}
1043+
1044+
// uniqueSearchIssuesRepos extracts the owner/repo pairs of every issue in the
1045+
// search result, preserving order of first appearance and deduplicating.
1046+
func uniqueSearchIssuesRepos(result *github.IssuesSearchResult) []searchIssuesRepoRef {
1047+
if result == nil {
1048+
return nil
1049+
}
1050+
seen := make(map[string]struct{})
1051+
var out []searchIssuesRepoRef
1052+
for _, issue := range result.Issues {
1053+
if issue == nil {
1054+
continue
1055+
}
1056+
owner, repo, ok := parseRepositoryURL(issue.GetRepositoryURL())
1057+
if !ok {
1058+
continue
1059+
}
1060+
key := owner + "/" + repo
1061+
if _, dup := seen[key]; dup {
1062+
continue
1063+
}
1064+
seen[key] = struct{}{}
1065+
out = append(out, searchIssuesRepoRef{owner: owner, repo: repo})
1066+
}
1067+
return out
1068+
}
1069+
1070+
// parseRepositoryURL extracts the owner and repo from a GitHub API repository
1071+
// URL of the form https://api.github.com/repos/{owner}/{repo}.
1072+
func parseRepositoryURL(repoURL string) (string, string, bool) {
1073+
if repoURL == "" {
1074+
return "", "", false
1075+
}
1076+
const marker = "/repos/"
1077+
idx := strings.LastIndex(repoURL, marker)
1078+
if idx < 0 {
1079+
return "", "", false
1080+
}
1081+
parts := strings.Split(strings.Trim(repoURL[idx+len(marker):], "/"), "/")
1082+
if len(parts) < 2 || parts[0] == "" || parts[1] == "" {
1083+
return "", "", false
1084+
}
1085+
return parts[0], parts[1], true
1086+
}
1087+
9861088
// IssueWrite creates a tool to create a new or update an existing issue in a GitHub repository.
9871089
// IssueWriteUIResourceURI is the URI for the issue_write tool's MCP App UI resource.
9881090
const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write"

pkg/github/issues_test.go

Lines changed: 237 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,6 @@ func Test_AddIssueComment(t *testing.T) {
381381
require.NoError(t, err)
382382
assert.Equal(t, fmt.Sprintf("%d", tc.expectedComment.GetID()), minimalResponse.ID)
383383
assert.Equal(t, tc.expectedComment.GetHTMLURL(), minimalResponse.URL)
384-
385384
})
386385
}
387386
}
@@ -693,6 +692,243 @@ func Test_SearchIssues(t *testing.T) {
693692
}
694693
}
695694

695+
func Test_SearchIssues_IFC_InsidersMode(t *testing.T) {
696+
t.Parallel()
697+
698+
serverTool := SearchIssues(translations.NullTranslationHelper)
699+
700+
makeIssue := func(owner, repo string, number int) *github.Issue {
701+
return &github.Issue{
702+
Number: github.Ptr(number),
703+
Title: github.Ptr("issue"),
704+
State: github.Ptr("open"),
705+
RepositoryURL: github.Ptr("https://api.github.com/repos/" + owner + "/" + repo),
706+
User: &github.User{Login: github.Ptr("u")},
707+
}
708+
}
709+
710+
type repoFixture struct {
711+
owner string
712+
repo string
713+
isPrivate bool
714+
collaborators []string
715+
repoStatus int
716+
collaboratorsStatus int
717+
}
718+
719+
repoHandlers := func(repos []repoFixture) map[string]http.HandlerFunc {
720+
repoByPath := map[string]repoFixture{}
721+
for _, r := range repos {
722+
repoByPath["/repos/"+r.owner+"/"+r.repo] = r
723+
}
724+
collaboratorsByPath := map[string]repoFixture{}
725+
for _, r := range repos {
726+
collaboratorsByPath["/repos/"+r.owner+"/"+r.repo+"/collaborators"] = r
727+
}
728+
return map[string]http.HandlerFunc{
729+
GetReposByOwnerByRepo: func(w http.ResponseWriter, req *http.Request) {
730+
r, ok := repoByPath[req.URL.Path]
731+
if !ok {
732+
w.WriteHeader(http.StatusNotFound)
733+
return
734+
}
735+
if r.repoStatus != 0 && r.repoStatus != http.StatusOK {
736+
w.WriteHeader(r.repoStatus)
737+
return
738+
}
739+
body, _ := json.Marshal(map[string]any{
740+
"name": r.repo,
741+
"private": r.isPrivate,
742+
})
743+
w.WriteHeader(http.StatusOK)
744+
_, _ = w.Write(body)
745+
},
746+
GetReposCollaboratorsByOwnerByRepo: func(w http.ResponseWriter, req *http.Request) {
747+
r, ok := collaboratorsByPath[req.URL.Path]
748+
if !ok {
749+
w.WriteHeader(http.StatusOK)
750+
_, _ = w.Write([]byte("[]"))
751+
return
752+
}
753+
if r.collaboratorsStatus != 0 && r.collaboratorsStatus != http.StatusOK {
754+
w.WriteHeader(r.collaboratorsStatus)
755+
return
756+
}
757+
users := make([]*github.User, len(r.collaborators))
758+
for i, login := range r.collaborators {
759+
users[i] = &github.User{Login: github.Ptr(login)}
760+
}
761+
body, _ := json.Marshal(users)
762+
w.WriteHeader(http.StatusOK)
763+
_, _ = w.Write(body)
764+
},
765+
}
766+
}
767+
768+
makeMockClient := func(searchResult *github.IssuesSearchResult, repos []repoFixture) *http.Client {
769+
handlers := repoHandlers(repos)
770+
handlers[GetSearchIssues] = mockResponse(t, http.StatusOK, searchResult)
771+
return MockHTTPClientWithHandlers(handlers)
772+
}
773+
774+
reqParams := map[string]any{"query": "bug"}
775+
776+
t.Run("insiders mode disabled omits ifc label", func(t *testing.T) {
777+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "public-repo", 1)}}
778+
deps := BaseDeps{
779+
Client: github.NewClient(makeMockClient(searchResult, []repoFixture{{owner: "octocat", repo: "public-repo"}})),
780+
Flags: FeatureFlags{InsidersMode: false},
781+
}
782+
handler := serverTool.Handler(deps)
783+
784+
request := createMCPRequest(reqParams)
785+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
786+
require.NoError(t, err)
787+
require.False(t, result.IsError)
788+
assert.Nil(t, result.Meta)
789+
})
790+
791+
t.Run("insiders mode enabled with single public repo emits public untrusted", func(t *testing.T) {
792+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "public-repo", 1)}}
793+
deps := BaseDeps{
794+
Client: github.NewClient(makeMockClient(searchResult, []repoFixture{{owner: "octocat", repo: "public-repo"}})),
795+
Flags: FeatureFlags{InsidersMode: true},
796+
}
797+
handler := serverTool.Handler(deps)
798+
799+
request := createMCPRequest(reqParams)
800+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
801+
require.NoError(t, err)
802+
require.False(t, result.IsError)
803+
804+
require.NotNil(t, result.Meta)
805+
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
806+
assert.Equal(t, "untrusted", ifcMap["integrity"])
807+
assert.Equal(t, []any{"public"}, ifcMap["confidentiality"])
808+
})
809+
810+
t.Run("insiders mode mixed public and private keeps the private readers", func(t *testing.T) {
811+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{
812+
makeIssue("octocat", "private-repo", 1),
813+
makeIssue("octocat", "public-repo", 2),
814+
}}
815+
deps := BaseDeps{
816+
Client: github.NewClient(makeMockClient(searchResult, []repoFixture{
817+
{owner: "octocat", repo: "private-repo", isPrivate: true, collaborators: []string{"alice"}},
818+
{owner: "octocat", repo: "public-repo"},
819+
})),
820+
Flags: FeatureFlags{InsidersMode: true},
821+
}
822+
handler := serverTool.Handler(deps)
823+
824+
request := createMCPRequest(reqParams)
825+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
826+
require.NoError(t, err)
827+
require.False(t, result.IsError)
828+
829+
require.NotNil(t, result.Meta)
830+
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
831+
assert.Equal(t, "untrusted", ifcMap["integrity"])
832+
assert.Equal(t, []any{"alice"}, ifcMap["confidentiality"])
833+
})
834+
835+
t.Run("insiders mode two private repos intersect collaborators", func(t *testing.T) {
836+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{
837+
makeIssue("octocat", "repo-a", 1),
838+
makeIssue("octocat", "repo-b", 2),
839+
}}
840+
deps := BaseDeps{
841+
Client: github.NewClient(makeMockClient(searchResult, []repoFixture{
842+
{owner: "octocat", repo: "repo-a", isPrivate: true, collaborators: []string{"alice", "bob", "carol"}},
843+
{owner: "octocat", repo: "repo-b", isPrivate: true, collaborators: []string{"bob", "carol", "dan"}},
844+
})),
845+
Flags: FeatureFlags{InsidersMode: true},
846+
}
847+
handler := serverTool.Handler(deps)
848+
849+
request := createMCPRequest(reqParams)
850+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
851+
require.NoError(t, err)
852+
require.False(t, result.IsError)
853+
854+
require.NotNil(t, result.Meta)
855+
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
856+
assert.Equal(t, "untrusted", ifcMap["integrity"])
857+
assert.Equal(t, []any{"bob", "carol"}, ifcMap["confidentiality"])
858+
})
859+
860+
t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) {
861+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "broken", 1)}}
862+
deps := BaseDeps{
863+
Client: github.NewClient(makeMockClient(searchResult, []repoFixture{
864+
{owner: "octocat", repo: "broken", repoStatus: http.StatusInternalServerError},
865+
})),
866+
Flags: FeatureFlags{InsidersMode: true},
867+
}
868+
handler := serverTool.Handler(deps)
869+
870+
request := createMCPRequest(reqParams)
871+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
872+
require.NoError(t, err)
873+
require.False(t, result.IsError, "tool call should still succeed when visibility lookup fails")
874+
875+
if result.Meta != nil {
876+
_, hasIFC := result.Meta["ifc"]
877+
assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails")
878+
}
879+
})
880+
881+
t.Run("insiders mode skips ifc label when collaborators lookup fails", func(t *testing.T) {
882+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "private-repo", 1)}}
883+
deps := BaseDeps{
884+
Client: github.NewClient(makeMockClient(searchResult, []repoFixture{
885+
{owner: "octocat", repo: "private-repo", isPrivate: true, collaboratorsStatus: http.StatusInternalServerError},
886+
})),
887+
Flags: FeatureFlags{InsidersMode: true},
888+
}
889+
handler := serverTool.Handler(deps)
890+
891+
request := createMCPRequest(reqParams)
892+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
893+
require.NoError(t, err)
894+
require.False(t, result.IsError, "tool call should still succeed when collaborators lookup fails")
895+
896+
if result.Meta != nil {
897+
_, hasIFC := result.Meta["ifc"]
898+
assert.False(t, hasIFC, "ifc label should be omitted when collaborators lookup fails")
899+
}
900+
})
901+
902+
t.Run("insiders mode empty results emits public untrusted", func(t *testing.T) {
903+
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{}}
904+
deps := BaseDeps{
905+
Client: github.NewClient(makeMockClient(searchResult, nil)),
906+
Flags: FeatureFlags{InsidersMode: true},
907+
}
908+
handler := serverTool.Handler(deps)
909+
910+
request := createMCPRequest(reqParams)
911+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
912+
require.NoError(t, err)
913+
require.False(t, result.IsError)
914+
915+
require.NotNil(t, result.Meta)
916+
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
917+
assert.Equal(t, "untrusted", ifcMap["integrity"])
918+
assert.Equal(t, []any{"public"}, ifcMap["confidentiality"])
919+
})
920+
}
921+
922+
func unmarshalIFC(t *testing.T, ifcLabel any) map[string]any {
923+
t.Helper()
924+
require.NotNil(t, ifcLabel, "ifc label should be present")
925+
ifcJSON, err := json.Marshal(ifcLabel)
926+
require.NoError(t, err)
927+
var ifcMap map[string]any
928+
require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap))
929+
return ifcMap
930+
}
931+
696932
func Test_CreateIssue(t *testing.T) {
697933
// Verify tool definition once
698934
serverTool := IssueWrite(translations.NullTranslationHelper)

0 commit comments

Comments
 (0)