From 936665839090ccea61e4cae6036fdf15af25b70e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=B6khan=20Arkan?= Date: Mon, 11 May 2026 18:47:35 +0300 Subject: [PATCH 1/2] Add ifc label for list_issues tool Emits an IFC SecurityLabel on the list_issues tool result when the InsidersMode flag is enabled, mirroring the pattern landed for get_me in #2432. Public repositories are labelled PublicUntrusted; private repositories are labelled PrivateUntrusted with the repository owner as a placeholder reader (full collaborator enumeration is intentionally deferred to a follow-up shared helper). A new IsPrivate field is added to the ListIssues GraphQL query types so visibility is available without a second round trip. Refs github/copilot-mcp-core#1623, github/copilot-mcp-core#1389. --- pkg/github/issues.go | 43 ++++++++++-- pkg/github/issues_test.go | 139 +++++++++++++++++++++++++++++++++++++- pkg/ifc/ifc.go | 50 +++++++++++++- 3 files changed, 224 insertions(+), 8 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 81161626bb..3f2ae02d33 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -10,6 +10,7 @@ import ( "time" ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/ifc" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/sanitize" "github.com/github/github-mcp-server/pkg/scopes" @@ -130,6 +131,7 @@ type IssueFragment struct { // Common interface for all issue query types type IssueQueryResult interface { GetIssueFragment() IssueQueryFragment + GetIsPrivate() bool } type IssueQueryFragment struct { @@ -146,28 +148,32 @@ type IssueQueryFragment struct { // ListIssuesQuery is the root query structure for fetching issues with optional label filtering. type ListIssuesQuery struct { Repository struct { - Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + IsPrivate githubv4.Boolean } `graphql:"repository(owner: $owner, name: $repo)"` } // ListIssuesQueryTypeWithLabels is the query structure for fetching issues with optional label filtering. type ListIssuesQueryTypeWithLabels struct { Repository struct { - Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + IsPrivate githubv4.Boolean } `graphql:"repository(owner: $owner, name: $repo)"` } // ListIssuesQueryWithSince is the query structure for fetching issues without label filtering but with since filtering. type ListIssuesQueryWithSince struct { Repository struct { - Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + IsPrivate githubv4.Boolean } `graphql:"repository(owner: $owner, name: $repo)"` } // ListIssuesQueryTypeWithLabelsWithSince is the query structure for fetching issues with both label and since filtering. type ListIssuesQueryTypeWithLabelsWithSince struct { Repository struct { - Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + IsPrivate githubv4.Boolean } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -176,18 +182,28 @@ func (q *ListIssuesQueryTypeWithLabels) GetIssueFragment() IssueQueryFragment { return q.Repository.Issues } +func (q *ListIssuesQueryTypeWithLabels) GetIsPrivate() bool { return bool(q.Repository.IsPrivate) } + func (q *ListIssuesQuery) GetIssueFragment() IssueQueryFragment { return q.Repository.Issues } +func (q *ListIssuesQuery) GetIsPrivate() bool { return bool(q.Repository.IsPrivate) } + func (q *ListIssuesQueryWithSince) GetIssueFragment() IssueQueryFragment { return q.Repository.Issues } +func (q *ListIssuesQueryWithSince) GetIsPrivate() bool { return bool(q.Repository.IsPrivate) } + func (q *ListIssuesQueryTypeWithLabelsWithSince) GetIssueFragment() IssueQueryFragment { return q.Repository.Issues } +func (q *ListIssuesQueryTypeWithLabelsWithSince) GetIsPrivate() bool { + return bool(q.Repository.IsPrivate) +} + func getIssueQueryType(hasLabels bool, hasSince bool) any { switch { case hasLabels && hasSince: @@ -1568,11 +1584,28 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } var resp MinimalIssuesResponse + var isPrivate bool if queryResult, ok := issueQuery.(IssueQueryResult); ok { resp = convertToMinimalIssuesResponse(queryResult.GetIssueFragment()) + isPrivate = queryResult.GetIsPrivate() } - return MarshalledTextResult(resp), nil, nil + result := MarshalledTextResult(resp) + if deps.GetFlags(ctx).InsidersMode { + if result.Meta == nil { + result.Meta = mcp.Meta{} + } + // TODO(fides): for private repos, populate readers with the + // repository's collaborator logins. Using [owner] as a + // placeholder until a shared visibility/collaborators helper + // lands (tracked under copilot-mcp-core#1623). + var readers []string + if isPrivate { + readers = []string{owner} + } + result.Meta["ifc"] = ifc.LabelListIssues(isPrivate, readers) + } + return result, nil, nil }) } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 9c20824746..5f61003347 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1117,6 +1117,7 @@ func Test_ListIssues(t *testing.T) { }, "totalCount": 2, }, + "isPrivate": false, }, }) @@ -1132,6 +1133,7 @@ func Test_ListIssues(t *testing.T) { }, "totalCount": 2, }, + "isPrivate": false, }, }) @@ -1147,6 +1149,7 @@ func Test_ListIssues(t *testing.T) { }, "totalCount": 1, }, + "isPrivate": false, }, }) @@ -1272,8 +1275,8 @@ func Test_ListIssues(t *testing.T) { } // Define the actual query strings that match the implementation - qBasicNoLabels := "query($after:String$direction:OrderDirection!$first:Int!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" - qWithLabels := "query($after:String$direction:OrderDirection!$first:Int!$labels:[String!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" + qBasicNoLabels := "query($after:String$direction:OrderDirection!$first:Int!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + qWithLabels := "query($after:String$direction:OrderDirection!$first:Int!$labels:[String!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -1349,6 +1352,138 @@ func Test_ListIssues(t *testing.T) { } } +func Test_ListIssues_IFC_InsidersMode(t *testing.T) { + t.Parallel() + + serverTool := ListIssues(translations.NullTranslationHelper) + + mockIssues := []map[string]any{ + { + "number": 1, + "title": "An issue", + "body": "body", + "state": "OPEN", + "databaseId": 1, + "createdAt": "2023-01-01T00:00:00Z", + "updatedAt": "2023-01-01T00:00:00Z", + "author": map[string]any{"login": "user1"}, + "labels": map[string]any{"nodes": []map[string]any{}}, + "comments": map[string]any{"totalCount": 0}, + }, + } + + pageInfo := map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + } + + makeResponse := func(isPrivate bool) githubv4mock.GQLResponse { + return githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issues": map[string]any{ + "nodes": mockIssues, + "pageInfo": pageInfo, + "totalCount": 1, + }, + "isPrivate": isPrivate, + }, + }) + } + + query := "query($after:String$direction:OrderDirection!$first:Int!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + + vars := map[string]any{ + "owner": "octocat", + "repo": "hello", + "states": []any{"OPEN", "CLOSED"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + } + + reqParams := map[string]any{"owner": "octocat", "repo": "hello"} + + t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) { + matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(false)) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) + deps := BaseDeps{ + GQLClient: gqlClient, + Flags: FeatureFlags{InsidersMode: false}, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(reqParams) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + assert.Nil(t, result.Meta, "result meta should be nil when insiders mode is disabled") + }) + + t.Run("insiders mode enabled on public repo emits public untrusted label", func(t *testing.T) { + matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(false)) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) + deps := BaseDeps{ + GQLClient: gqlClient, + Flags: FeatureFlags{InsidersMode: true}, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(reqParams) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + require.NotNil(t, result.Meta) + ifcLabel, ok := result.Meta["ifc"] + require.True(t, ok, "result meta should contain ifc key") + + ifcJSON, err := json.Marshal(ifcLabel) + require.NoError(t, err) + var ifcMap map[string]any + require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap)) + + assert.Equal(t, "untrusted", ifcMap["integrity"]) + confList, ok := ifcMap["confidentiality"].([]any) + require.True(t, ok, "confidentiality should be a list") + require.Len(t, confList, 1) + assert.Equal(t, "public", confList[0]) + }) + + t.Run("insiders mode enabled on private repo emits private untrusted label", func(t *testing.T) { + matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(true)) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) + deps := BaseDeps{ + GQLClient: gqlClient, + Flags: FeatureFlags{InsidersMode: true}, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(reqParams) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + require.NotNil(t, result.Meta) + ifcLabel, ok := result.Meta["ifc"] + require.True(t, ok, "result meta should contain ifc key") + + ifcJSON, err := json.Marshal(ifcLabel) + require.NoError(t, err) + var ifcMap map[string]any + require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap)) + + assert.Equal(t, "untrusted", ifcMap["integrity"]) + confList, ok := ifcMap["confidentiality"].([]any) + require.True(t, ok, "confidentiality should be a list") + require.Len(t, confList, 1) + assert.Equal(t, "octocat", confList[0]) + }) +} + func Test_UpdateIssue(t *testing.T) { // Verify tool definition serverTool := IssueWrite(translations.NullTranslationHelper) diff --git a/pkg/ifc/ifc.go b/pkg/ifc/ifc.go index cf0d72114f..43b39fc450 100644 --- a/pkg/ifc/ifc.go +++ b/pkg/ifc/ifc.go @@ -21,9 +21,57 @@ type SecurityLabel struct { Confidentiality []Confidentiality `json:"confidentiality"` } -func LabelGetMe() SecurityLabel { +// PublicTrusted returns a label for trusted, publicly readable data. +func PublicTrusted() SecurityLabel { return SecurityLabel{ Integrity: IntegrityTrusted, Confidentiality: []Confidentiality{ConfidentialityPublic}, } } + +// PublicUntrusted returns a label for untrusted, publicly readable data. +func PublicUntrusted() SecurityLabel { + return SecurityLabel{ + Integrity: IntegrityUntrusted, + Confidentiality: []Confidentiality{ConfidentialityPublic}, + } +} + +// PrivateTrusted returns a label for trusted data restricted to the given readers. +func PrivateTrusted(readers []string) SecurityLabel { + return SecurityLabel{ + Integrity: IntegrityTrusted, + Confidentiality: toConfidentiality(readers), + } +} + +// PrivateUntrusted returns a label for untrusted data restricted to the given readers. +func PrivateUntrusted(readers []string) SecurityLabel { + return SecurityLabel{ + Integrity: IntegrityUntrusted, + Confidentiality: toConfidentiality(readers), + } +} + +func toConfidentiality(readers []string) []Confidentiality { + out := make([]Confidentiality, len(readers)) + for i, r := range readers { + out[i] = Confidentiality(r) + } + return out +} + +func LabelGetMe() SecurityLabel { + return PublicTrusted() +} + +// LabelListIssues returns the IFC label for a list_issues result. +// Public repositories are universally readable; private repositories are +// restricted to the provided reader set (typically repository collaborators). +// Issue contents are attacker-controllable, so integrity is always untrusted. +func LabelListIssues(isPrivate bool, readers []string) SecurityLabel { + if isPrivate { + return PrivateUntrusted(readers) + } + return PublicUntrusted() +} From 6d9f188790ce6fd9fe0f424b49d49135717a7563 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=B6khan=20Arkan?= Date: Tue, 12 May 2026 11:55:02 +0300 Subject: [PATCH 2/2] list_issues: populate readers with repo collaborators Addresses Joanna's review feedback: for private repositories, populate the IFC confidentiality reader set with the repository's collaborator logins instead of the [owner] placeholder. Adds an exported FetchRepoCollaborators helper in pkg/github/repositories.go that paginates through Repositories.ListCollaborators. Mirrors the helper in github-mcp-server-remote (without the cache for now; cache can land in a follow-up). The lookup is invoked only for private repos under InsidersMode; if it fails we fall back to [owner] so the reader set is never empty for a private repo. --- pkg/github/helper_test.go | 1 + pkg/github/issues.go | 17 +++++++++++----- pkg/github/issues_test.go | 40 +++++++++++++++++++++++++++++++++++--- pkg/github/repositories.go | 28 ++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index ff752f5f37..67a05fd6c0 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -31,6 +31,7 @@ const ( GetReposByOwnerByRepo = "GET /repos/{owner}/{repo}" GetReposBranchesByOwnerByRepo = "GET /repos/{owner}/{repo}/branches" GetReposTagsByOwnerByRepo = "GET /repos/{owner}/{repo}/tags" + GetReposCollaboratorsByOwnerByRepo = "GET /repos/{owner}/{repo}/collaborators" GetReposCommitsByOwnerByRepo = "GET /repos/{owner}/{repo}/commits" GetReposCommitsByOwnerByRepoByRef = "GET /repos/{owner}/{repo}/commits/{ref}" GetReposContentsByOwnerByRepoByPath = "GET /repos/{owner}/{repo}/contents/{path}" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 3f2ae02d33..e3e1f6b223 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1595,13 +1595,20 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { if result.Meta == nil { result.Meta = mcp.Meta{} } - // TODO(fides): for private repos, populate readers with the - // repository's collaborator logins. Using [owner] as a - // placeholder until a shared visibility/collaborators helper - // lands (tracked under copilot-mcp-core#1623). var readers []string if isPrivate { - readers = []string{owner} + restClient, err := deps.GetClient(ctx) + if err == nil { + if collaborators, err := FetchRepoCollaborators(ctx, restClient, owner, repo); err == nil { + readers = collaborators + } + } + // Fall back to the repository owner so the reader set is + // never empty for a private repository even if the + // collaborators lookup fails. + if len(readers) == 0 { + readers = []string{owner} + } } result.Meta["ifc"] = ifc.LabelListIssues(isPrivate, readers) } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 5f61003347..49ce2dde9c 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1453,10 +1453,18 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { assert.Equal(t, "public", confList[0]) }) - t.Run("insiders mode enabled on private repo emits private untrusted label", func(t *testing.T) { + t.Run("insiders mode enabled on private repo emits private untrusted label with collaborators", func(t *testing.T) { matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(true)) gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) + restClient := github.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.User{ + {Login: github.Ptr("octocat")}, + {Login: github.Ptr("alice")}, + {Login: github.Ptr("bob")}, + }), + })) deps := BaseDeps{ + Client: restClient, GQLClient: gqlClient, Flags: FeatureFlags{InsidersMode: true}, } @@ -1479,8 +1487,34 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { assert.Equal(t, "untrusted", ifcMap["integrity"]) confList, ok := ifcMap["confidentiality"].([]any) require.True(t, ok, "confidentiality should be a list") - require.Len(t, confList, 1) - assert.Equal(t, "octocat", confList[0]) + assert.Equal(t, []any{"octocat", "alice", "bob"}, confList) + }) + + t.Run("insiders mode enabled on private repo falls back to owner when collaborators lookup fails", func(t *testing.T) { + matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(true)) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) + restClient := github.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusInternalServerError, "boom"), + })) + deps := BaseDeps{ + Client: restClient, + GQLClient: gqlClient, + Flags: FeatureFlags{InsidersMode: true}, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(reqParams) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + require.NotNil(t, result.Meta) + ifcJSON, err := json.Marshal(result.Meta["ifc"]) + require.NoError(t, err) + var ifcMap map[string]any + require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap)) + + assert.Equal(t, []any{"octocat"}, ifcMap["confidentiality"]) }) } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 0ebacc6668..c946d6308e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -653,6 +653,34 @@ func CreateRepository(t translations.TranslationHelperFunc) inventory.ServerTool ) } +// FetchRepoCollaborators returns the login names of all collaborators on a +// repository. It is provided as a shared helper for IFC label computation so +// tools can populate the reader set for private repositories. The full list +// is fetched eagerly via pagination; callers are expected to invoke this only +// when needed (e.g. private repos under InsidersMode). +func FetchRepoCollaborators(ctx context.Context, client *github.Client, owner, repo string) ([]string, error) { + opts := &github.ListCollaboratorsOptions{ + ListOptions: github.ListOptions{PerPage: 100}, + } + var logins []string + for { + page, resp, err := client.Repositories.ListCollaborators(ctx, owner, repo, opts) + if err != nil { + return nil, err + } + for _, c := range page { + if login := c.GetLogin(); login != "" { + logins = append(logins, login) + } + } + if resp == nil || resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return logins, nil +} + // GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository. func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool(