diff --git a/pkg/github/context_tools_test.go b/pkg/github/context_tools_test.go index 365a019ab..510372cd9 100644 --- a/pkg/github/context_tools_test.go +++ b/pkg/github/context_tools_test.go @@ -192,10 +192,7 @@ func Test_GetMe_IFC_InsidersMode(t *testing.T) { require.NoError(t, err) assert.Equal(t, "trusted", 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]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) } diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 2a601c319..2346e40ca 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -31,7 +31,6 @@ 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 ab8611afb..98585e291 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -298,11 +298,7 @@ Options are: // attachIFC adds the IFC label to a successful tool result when // InsidersMode is enabled. If the visibility lookup fails the - // label is omitted rather than misclassifying the result. If - // only the collaborators lookup fails for a private repo we - // fall back to the owner so the reader set is never empty. The - // label matches list_issues semantics: per-repo visibility, - // integrity always untrusted. + // label is omitted rather than misclassifying the result. attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult { if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode { return r @@ -311,19 +307,10 @@ Options are: if err != nil { return r } - var readers []string - if isPrivate { - if collaborators, err := FetchRepoCollaborators(ctx, client, owner, repo); err == nil { - readers = collaborators - } - if len(readers) == 0 { - readers = []string{owner} - } - } if r.Meta == nil { r.Meta = mcp.Meta{} } - r.Meta["ifc"] = ifc.LabelListIssues(isPrivate, readers) + r.Meta["ifc"] = ifc.LabelListIssues(isPrivate) return r } @@ -1034,36 +1021,18 @@ func searchIssuesIFCPostProcess(deps ToolDependencies) searchPostProcessFn { uniqueRepos := uniqueSearchIssuesRepos(result) visibilities := make([]bool, 0, len(uniqueRepos)) - readerSets := make([][]string, 0, len(uniqueRepos)) for _, r := range uniqueRepos { isPrivate, err := FetchRepoIsPrivate(ctx, client, r.owner, r.repo) if err != nil { return } visibilities = append(visibilities, isPrivate) - if !isPrivate { - readerSets = append(readerSets, nil) - continue - } - collaborators, err := FetchRepoCollaborators(ctx, client, r.owner, r.repo) - if err != nil { - return - } - // Preserve an empty collaborator set as-is. Substituting the - // owner here would corrupt the cross-repo intersection (the - // owner could appear in another repo's collaborator list and - // widen the joined reader set incorrectly). - readerSets = append(readerSets, collaborators) } - label, ok := ifc.LabelSearchIssues(visibilities, readerSets) - if !ok { - return - } if callResult.Meta == nil { callResult.Meta = mcp.Meta{} } - callResult.Meta["ifc"] = label + callResult.Meta["ifc"] = ifc.LabelSearchIssues(visibilities) } } @@ -1728,22 +1697,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { if result.Meta == nil { result.Meta = mcp.Meta{} } - var readers []string - if isPrivate { - 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) + result.Meta["ifc"] = ifc.LabelListIssues(isPrivate) } return result, nil, nil }) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index ed92c49ab..d23c22ed5 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -297,10 +297,6 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { handlers := map[string]http.HandlerFunc{ GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue), GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockComments), - GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.User{ - {Login: github.Ptr("octocat")}, - {Login: github.Ptr("alice")}, - }), } if repoStatus != 0 && repoStatus != http.StatusOK { handlers[GetReposByOwnerByRepo] = mockResponse(t, repoStatus, "boom") @@ -356,7 +352,7 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"public"}, ifcMap["confidentiality"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) t.Run("insiders mode enabled on private repo with get_comments emits private untrusted", func(t *testing.T) { @@ -374,7 +370,7 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"octocat", "alice"}, ifcMap["confidentiality"]) + assert.Equal(t, "private", ifcMap["confidentiality"]) }) t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) { @@ -829,12 +825,10 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { } type repoFixture struct { - owner string - repo string - isPrivate bool - collaborators []string - repoStatus int - collaboratorsStatus int + owner string + repo string + isPrivate bool + repoStatus int } repoHandlers := func(repos []repoFixture) map[string]http.HandlerFunc { @@ -842,10 +836,6 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { for _, r := range repos { repoByPath["/repos/"+r.owner+"/"+r.repo] = r } - collaboratorsByPath := map[string]repoFixture{} - for _, r := range repos { - collaboratorsByPath["/repos/"+r.owner+"/"+r.repo+"/collaborators"] = r - } return map[string]http.HandlerFunc{ GetReposByOwnerByRepo: func(w http.ResponseWriter, req *http.Request) { r, ok := repoByPath[req.URL.Path] @@ -864,25 +854,6 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { w.WriteHeader(http.StatusOK) _, _ = w.Write(body) }, - GetReposCollaboratorsByOwnerByRepo: func(w http.ResponseWriter, req *http.Request) { - r, ok := collaboratorsByPath[req.URL.Path] - if !ok { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("[]")) - return - } - if r.collaboratorsStatus != 0 && r.collaboratorsStatus != http.StatusOK { - w.WriteHeader(r.collaboratorsStatus) - return - } - users := make([]*github.User, len(r.collaborators)) - for i, login := range r.collaborators { - users[i] = &github.User{Login: github.Ptr(login)} - } - body, _ := json.Marshal(users) - w.WriteHeader(http.StatusOK) - _, _ = w.Write(body) - }, } } @@ -909,7 +880,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { assert.Nil(t, result.Meta) }) - t.Run("insiders mode enabled with single public repo emits public untrusted", func(t *testing.T) { + t.Run("insiders mode all public emits public untrusted", func(t *testing.T) { searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "public-repo", 1)}} deps := BaseDeps{ Client: github.NewClient(makeMockClient(searchResult, []repoFixture{{owner: "octocat", repo: "public-repo"}})), @@ -925,17 +896,17 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"public"}, ifcMap["confidentiality"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) - t.Run("insiders mode mixed public and private keeps the private readers", func(t *testing.T) { + t.Run("insiders mode mixed public and private emits private untrusted", func(t *testing.T) { searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{ makeIssue("octocat", "private-repo", 1), makeIssue("octocat", "public-repo", 2), }} deps := BaseDeps{ Client: github.NewClient(makeMockClient(searchResult, []repoFixture{ - {owner: "octocat", repo: "private-repo", isPrivate: true, collaborators: []string{"alice"}}, + {owner: "octocat", repo: "private-repo", isPrivate: true}, {owner: "octocat", repo: "public-repo"}, })), Flags: FeatureFlags{InsidersMode: true}, @@ -950,32 +921,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"alice"}, ifcMap["confidentiality"]) - }) - - t.Run("insiders mode two private repos intersect collaborators", func(t *testing.T) { - searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{ - makeIssue("octocat", "repo-a", 1), - makeIssue("octocat", "repo-b", 2), - }} - deps := BaseDeps{ - Client: github.NewClient(makeMockClient(searchResult, []repoFixture{ - {owner: "octocat", repo: "repo-a", isPrivate: true, collaborators: []string{"alice", "bob", "carol"}}, - {owner: "octocat", repo: "repo-b", isPrivate: true, collaborators: []string{"bob", "carol", "dan"}}, - })), - 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) - ifcMap := unmarshalIFC(t, result.Meta["ifc"]) - assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"bob", "carol"}, ifcMap["confidentiality"]) + assert.Equal(t, "private", ifcMap["confidentiality"]) }) t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) { @@ -999,27 +945,6 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { } }) - t.Run("insiders mode skips ifc label when collaborators lookup fails", func(t *testing.T) { - searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "private-repo", 1)}} - deps := BaseDeps{ - Client: github.NewClient(makeMockClient(searchResult, []repoFixture{ - {owner: "octocat", repo: "private-repo", isPrivate: true, collaboratorsStatus: http.StatusInternalServerError}, - })), - 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, "tool call should still succeed when collaborators lookup fails") - - if result.Meta != nil { - _, hasIFC := result.Meta["ifc"] - assert.False(t, hasIFC, "ifc label should be omitted when collaborators lookup fails") - } - }) - t.Run("insiders mode empty results emits public untrusted", func(t *testing.T) { searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{}} deps := BaseDeps{ @@ -1036,7 +961,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"public"}, ifcMap["confidentiality"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) } @@ -1804,24 +1729,13 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { 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]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) - t.Run("insiders mode enabled on private repo emits private untrusted label with collaborators", func(t *testing.T) { + 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)) - 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}, } @@ -1842,36 +1756,7 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { 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") - 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"]) + assert.Equal(t, "private", ifcMap["confidentiality"]) }) } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 507677ee5..c51516e29 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -654,34 +654,6 @@ 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 -} - // FetchRepoIsPrivate returns whether a repository is private. It is a thin // wrapper around the GitHub Repositories.Get endpoint provided as a shared // helper for IFC label computation across tools. @@ -769,17 +741,15 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool } // attachIFC adds the IFC label to a successful tool result when - // InsidersMode is enabled. The visibility and (for private - // repositories) collaborators lookups are performed lazily on - // first use. If the visibility lookup fails we skip the label - // rather than misclassify the result; the failure is not cached - // so a later return path can retry. If only the collaborators - // lookup fails for a private repo we fall back to the owner so - // the reader set is never empty. + // InsidersMode is enabled. The visibility lookup is performed + // lazily on first use and cached because GetFileContents has + // many possible return paths and would otherwise re-fetch on + // each. If the visibility lookup fails we skip the label rather + // than misclassify the result; the failure is not cached so a + // later return path can retry. var ( ifcLabelKnown bool ifcIsPrivate bool - ifcReaders []string ) attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult { if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode { @@ -791,20 +761,12 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool return r } ifcIsPrivate = isPrivate - if ifcIsPrivate { - if collaborators, err := FetchRepoCollaborators(ctx, client, owner, repo); err == nil { - ifcReaders = collaborators - } - if len(ifcReaders) == 0 { - ifcReaders = []string{owner} - } - } ifcLabelKnown = true } if r.Meta == nil { r.Meta = mcp.Meta{} } - r.Meta["ifc"] = ifc.LabelGetFileContents(ifcIsPrivate, ifcReaders) + r.Meta["ifc"] = ifc.LabelGetFileContents(ifcIsPrivate) return r } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index ceaa95901..913be5997 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -492,10 +492,6 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { "default_branch": "main", "private": isPrivate, }), - GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.User{ - {Login: github.Ptr("octocat")}, - {Login: github.Ptr("alice")}, - }), GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) encodedContent := base64.StdEncoding.EncodeToString(mockRawContent) @@ -558,10 +554,7 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { 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]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) t.Run("insiders mode enabled on private repo emits private trusted label", func(t *testing.T) { @@ -586,9 +579,7 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap)) assert.Equal(t, "trusted", ifcMap["integrity"]) - confList, ok := ifcMap["confidentiality"].([]any) - require.True(t, ok, "confidentiality should be a list") - assert.Equal(t, []any{"octocat", "alice"}, confList) + assert.Equal(t, "private", ifcMap["confidentiality"]) }) t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) { @@ -3351,6 +3342,7 @@ func Test_ListReleases(t *testing.T) { }) } } + func Test_GetLatestRelease(t *testing.T) { serverTool := GetLatestRelease(translations.NullTranslationHelper) tool := serverTool.Tool diff --git a/pkg/github/search.go b/pkg/github/search.go index 8edfc948a..a44add8bb 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -164,7 +164,7 @@ func SearchRepositories(t translations.TranslationHelperFunc) inventory.ServerTo callResult := utils.NewToolResultText(string(r)) if deps.GetFlags(ctx).InsidersMode { - attachSearchRepositoriesIFCLabel(ctx, client, result.Repositories, callResult) + attachSearchRepositoriesIFCLabel(result.Repositories, callResult) } return callResult, nil, nil }, @@ -173,47 +173,24 @@ func SearchRepositories(t translations.TranslationHelperFunc) inventory.ServerTo // attachSearchRepositoriesIFCLabel joins per-repository IFC labels across // every matched repository and attaches the result to callResult. Visibility -// is read directly from the search response (no extra API call); collaborators -// are fetched once per private repository. If any collaborators lookup fails -// the label is omitted to avoid misclassifying the result. The join math is -// shared with search_issues via ifc.LabelSearchIssues: integrity is always -// untrusted, and confidentiality is the intersection of the reader sets of -// the matched private repositories (public matches contribute the universe -// set and drop out without shrinking it). -func attachSearchRepositoriesIFCLabel(ctx context.Context, client *github.Client, repos []*github.Repository, callResult *mcp.CallToolResult) { +// is read directly from the search response — no extra API call. The join +// math is shared with search_issues via ifc.LabelSearchIssues: integrity is +// always untrusted; confidentiality is private if any matched repository is +// private, otherwise public. +func attachSearchRepositoriesIFCLabel(repos []*github.Repository, callResult *mcp.CallToolResult) { if callResult == nil || callResult.IsError { return } visibilities := make([]bool, 0, len(repos)) - readerSets := make([][]string, 0, len(repos)) for _, repo := range repos { - isPrivate := repo.GetPrivate() - visibilities = append(visibilities, isPrivate) - if !isPrivate { - readerSets = append(readerSets, nil) - continue - } - owner := repo.GetOwner().GetLogin() - name := repo.GetName() - if owner == "" || name == "" { - return - } - collaborators, err := FetchRepoCollaborators(ctx, client, owner, name) - if err != nil { - return - } - readerSets = append(readerSets, collaborators) + visibilities = append(visibilities, repo.GetPrivate()) } - label, ok := ifc.LabelSearchIssues(visibilities, readerSets) - if !ok { - return - } if callResult.Meta == nil { callResult.Meta = mcp.Meta{} } - callResult.Meta["ifc"] = label + callResult.Meta["ifc"] = ifc.LabelSearchIssues(visibilities) } // SearchCode creates a tool to search for code across GitHub repositories. diff --git a/pkg/github/search_test.go b/pkg/github/search_test.go index 0c4a30c32..13e787a67 100644 --- a/pkg/github/search_test.go +++ b/pkg/github/search_test.go @@ -173,11 +173,9 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { serverTool := SearchRepositories(translations.NullTranslationHelper) type repoFixture struct { - owner string - name string - isPrivate bool - collaborators []string - collaboratorsStatus int + owner string + name string + isPrivate bool } makeRepo := func(r repoFixture) *github.Repository { @@ -198,33 +196,8 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { for _, r := range repos { searchResult.Repositories = append(searchResult.Repositories, makeRepo(r)) } - - collaboratorsByPath := map[string]repoFixture{} - for _, r := range repos { - collaboratorsByPath["/repos/"+r.owner+"/"+r.name+"/collaborators"] = r - } - return MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetSearchRepositories: mockResponse(t, http.StatusOK, searchResult), - GetReposCollaboratorsByOwnerByRepo: func(w http.ResponseWriter, req *http.Request) { - r, ok := collaboratorsByPath[req.URL.Path] - if !ok { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("[]")) - return - } - if r.collaboratorsStatus != 0 && r.collaboratorsStatus != http.StatusOK { - w.WriteHeader(r.collaboratorsStatus) - return - } - users := make([]*github.User, len(r.collaborators)) - for i, login := range r.collaborators { - users[i] = &github.User{Login: github.Ptr(login)} - } - body, _ := json.Marshal(users) - w.WriteHeader(http.StatusOK) - _, _ = w.Write(body) - }, }) } @@ -262,13 +235,13 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"public"}, ifcMap["confidentiality"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) - t.Run("insiders mode mixed public and private keeps the private readers", func(t *testing.T) { + t.Run("insiders mode any private match emits private untrusted", func(t *testing.T) { deps := BaseDeps{ Client: github.NewClient(makeMockClient([]repoFixture{ - {owner: "octocat", name: "private-repo", isPrivate: true, collaborators: []string{"alice"}}, + {owner: "octocat", name: "private-repo", isPrivate: true}, {owner: "octocat", name: "public-repo"}, })), Flags: FeatureFlags{InsidersMode: true}, @@ -283,48 +256,7 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"alice"}, ifcMap["confidentiality"]) - }) - - t.Run("insiders mode two private repos intersect collaborators", func(t *testing.T) { - deps := BaseDeps{ - Client: github.NewClient(makeMockClient([]repoFixture{ - {owner: "octocat", name: "repo-a", isPrivate: true, collaborators: []string{"alice", "bob", "carol"}}, - {owner: "octocat", name: "repo-b", isPrivate: true, collaborators: []string{"bob", "carol", "dan"}}, - })), - 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) - ifcMap := unmarshalIFC(t, result.Meta["ifc"]) - assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"bob", "carol"}, ifcMap["confidentiality"]) - }) - - t.Run("insiders mode skips ifc label when collaborators lookup fails", func(t *testing.T) { - deps := BaseDeps{ - Client: github.NewClient(makeMockClient([]repoFixture{ - {owner: "octocat", name: "private-repo", isPrivate: true, collaboratorsStatus: http.StatusInternalServerError}, - })), - 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, "tool call should still succeed when collaborators lookup fails") - - if result.Meta != nil { - _, hasIFC := result.Meta["ifc"] - assert.False(t, hasIFC, "ifc label should be omitted when collaborators lookup fails") - } + assert.Equal(t, "private", ifcMap["confidentiality"]) }) t.Run("insiders mode empty results emits public untrusted", func(t *testing.T) { @@ -342,7 +274,7 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { require.NotNil(t, result.Meta) ifcMap := unmarshalIFC(t, result.Meta["ifc"]) assert.Equal(t, "untrusted", ifcMap["integrity"]) - assert.Equal(t, []any{"public"}, ifcMap["confidentiality"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) }) } diff --git a/pkg/ifc/ifc.go b/pkg/ifc/ifc.go index 61c81e255..e6eeb407b 100644 --- a/pkg/ifc/ifc.go +++ b/pkg/ifc/ifc.go @@ -13,19 +13,20 @@ const ( type Confidentiality string const ( - ConfidentialityPublic Confidentiality = "public" + ConfidentialityPublic Confidentiality = "public" + ConfidentialityPrivate Confidentiality = "private" ) type SecurityLabel struct { - Integrity Integrity `json:"integrity"` - Confidentiality []Confidentiality `json:"confidentiality"` + Integrity Integrity `json:"integrity"` + Confidentiality Confidentiality `json:"confidentiality"` } // PublicTrusted returns a label for trusted, publicly readable data. func PublicTrusted() SecurityLabel { return SecurityLabel{ Integrity: IntegrityTrusted, - Confidentiality: []Confidentiality{ConfidentialityPublic}, + Confidentiality: ConfidentialityPublic, } } @@ -33,45 +34,42 @@ func PublicTrusted() SecurityLabel { func PublicUntrusted() SecurityLabel { return SecurityLabel{ Integrity: IntegrityUntrusted, - Confidentiality: []Confidentiality{ConfidentialityPublic}, + Confidentiality: ConfidentialityPublic, } } -// PrivateTrusted returns a label for trusted data restricted to the given readers. -func PrivateTrusted(readers []string) SecurityLabel { +// PrivateTrusted returns a label for trusted data restricted to the readers +// of the originating repository. The reader set is opaque on the wire (a +// single "private" marker); the client engine resolves the concrete readers +// from the GitHub API on demand at egress decision time. +func PrivateTrusted() SecurityLabel { return SecurityLabel{ Integrity: IntegrityTrusted, - Confidentiality: toConfidentiality(readers), + Confidentiality: ConfidentialityPrivate, } } -// PrivateUntrusted returns a label for untrusted data restricted to the given readers. -func PrivateUntrusted(readers []string) SecurityLabel { +// PrivateUntrusted returns a label for untrusted data restricted to the +// readers of the originating repository. See PrivateTrusted for the reader +// resolution model. +func PrivateUntrusted() SecurityLabel { return SecurityLabel{ Integrity: IntegrityUntrusted, - Confidentiality: toConfidentiality(readers), + Confidentiality: ConfidentialityPrivate, } } -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). +// restricted to their collaborators (resolved client-side from the marker). // Issue contents are attacker-controllable, so integrity is always untrusted. -func LabelListIssues(isPrivate bool, readers []string) SecurityLabel { +func LabelListIssues(isPrivate bool) SecurityLabel { if isPrivate { - return PrivateUntrusted(readers) + return PrivateUntrusted() } return PublicUntrusted() } @@ -80,86 +78,31 @@ func LabelListIssues(isPrivate bool, readers []string) SecurityLabel { // Public repository file contents may be authored by anyone via pull requests // and are therefore untrusted. In private repositories only collaborators can // land changes, so contents are treated as trusted. -func LabelGetFileContents(isPrivate bool, readers []string) SecurityLabel { +func LabelGetFileContents(isPrivate bool) SecurityLabel { if isPrivate { - return PrivateTrusted(readers) + return PrivateTrusted() } return PublicUntrusted() } -// LabelSearchIssues returns the IFC label for a search_issues result, joining -// per-repository labels across all matched repositories. +// LabelSearchIssues returns the IFC label for a multi-repository search +// result, joining per-repository labels across all matched repositories. +// Used by both search_issues and search_repositories. // -// Integrity is always untrusted because issue contents are user-authored. +// Integrity is always untrusted because results expose user-authored content. // -// Confidentiality follows the IFC meet (greatest lower bound): the private -// side dominates because a reader of the combined result must be authorised -// to read every matched repository. Public repositories contribute the -// universe set and therefore drop out of the intersection without shrinking -// it. +// Confidentiality follows the IFC meet (greatest lower bound): if any matched +// repository is private the joined label is private; otherwise public. The +// reader set is opaque (the "private" marker); the client engine resolves +// concrete readers on demand at egress decision time. // -// - If no repositories matched (empty result set), the label is -// public-untrusted because no repository data is leaked. -// - If every matched repository is public, the joined readers are -// ["public"]. -// - Otherwise the joined readers are the intersection of the reader sets -// of the matched private repositories only. -// -// repoVisibilities[i] reports whether the i-th matched repository is private; -// readerSets[i] is that repository's reader set (only consulted for private -// repos). The two slices must have the same length; the second return value -// is false when they do not, in which case the caller should omit the label -// rather than emit one computed from inconsistent inputs. -func LabelSearchIssues(repoVisibilities []bool, readerSets [][]string) (SecurityLabel, bool) { - if len(repoVisibilities) != len(readerSets) { - return SecurityLabel{}, false - } - if len(repoVisibilities) == 0 { - return PublicUntrusted(), true - } - privateReaderSets := make([][]string, 0, len(repoVisibilities)) - for i, isPrivate := range repoVisibilities { +// An empty result set is treated as public-untrusted (no repository data is +// leaked). +func LabelSearchIssues(repoVisibilities []bool) SecurityLabel { + for _, isPrivate := range repoVisibilities { if isPrivate { - privateReaderSets = append(privateReaderSets, readerSets[i]) + return PrivateUntrusted() } } - if len(privateReaderSets) == 0 { - return PublicUntrusted(), true - } - return PrivateUntrusted(intersectReaders(privateReaderSets)), true -} - -// intersectReaders returns the readers present in every set, preserving the -// order from the first set. Empty input yields nil. -func intersectReaders(sets [][]string) []string { - if len(sets) == 0 { - return nil - } - counts := make(map[string]int, len(sets[0])) - for _, login := range sets[0] { - if _, seen := counts[login]; seen { - continue - } - counts[login] = 1 - } - for _, set := range sets[1:] { - seen := make(map[string]struct{}, len(set)) - for _, login := range set { - if _, dup := seen[login]; dup { - continue - } - seen[login] = struct{}{} - if _, ok := counts[login]; ok { - counts[login]++ - } - } - } - out := make([]string, 0, len(counts)) - for _, login := range sets[0] { - if counts[login] == len(sets) { - out = append(out, login) - delete(counts, login) - } - } - return out + return PublicUntrusted() } diff --git a/pkg/ifc/ifc_test.go b/pkg/ifc/ifc_test.go index 644244a52..669f5ff0c 100644 --- a/pkg/ifc/ifc_test.go +++ b/pkg/ifc/ifc_test.go @@ -12,86 +12,39 @@ func TestLabelSearchIssues(t *testing.T) { tests := []struct { name string visibilities []bool - readers [][]string - wantOK bool - wantIntegrity Integrity - wantConfidential []Confidentiality + wantConfidential Confidentiality }{ { name: "empty result is treated as public", - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{ConfidentialityPublic}, + wantConfidential: ConfidentialityPublic, }, { name: "single public repo", visibilities: []bool{false}, - readers: [][]string{nil}, - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{ConfidentialityPublic}, + wantConfidential: ConfidentialityPublic, }, { - name: "mixed public and private keeps the private reader set", - visibilities: []bool{true, false}, - readers: [][]string{{"alice"}, nil}, - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{"alice"}, + name: "all public repos stay public", + visibilities: []bool{false, false, false}, + wantConfidential: ConfidentialityPublic, }, { - name: "two private repos with intersecting collaborators", - visibilities: []bool{true, true}, - readers: [][]string{{"alice", "bob", "carol"}, {"bob", "carol", "dan"}}, - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{"bob", "carol"}, + name: "any private match flips to private", + visibilities: []bool{false, true, false}, + wantConfidential: ConfidentialityPrivate, }, { - name: "private repos with no overlap yield empty reader set", + name: "all private repos stay private", visibilities: []bool{true, true}, - readers: [][]string{{"alice"}, {"bob"}}, - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{}, - }, - { - name: "two private plus one public intersects only the private sets", - visibilities: []bool{true, false, true}, - readers: [][]string{{"alice", "bob", "carol"}, nil, {"bob", "carol", "dan"}}, - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{"bob", "carol"}, - }, - { - name: "intersection preserves first-set order and dedupes", - visibilities: []bool{true, true, true}, - readers: [][]string{{"alice", "bob", "alice"}, {"bob", "alice"}, {"alice", "bob"}}, - wantOK: true, - wantIntegrity: IntegrityUntrusted, - wantConfidential: []Confidentiality{"alice", "bob"}, - }, - { - name: "mismatched slice lengths return ok=false", - visibilities: []bool{true, true}, - readers: [][]string{{"alice"}}, - wantOK: false, + wantConfidential: ConfidentialityPrivate, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - label, ok := LabelSearchIssues(tc.visibilities, tc.readers) - assert.Equal(t, tc.wantOK, ok) - if !tc.wantOK { - return - } - assert.Equal(t, tc.wantIntegrity, label.Integrity) - if len(tc.wantConfidential) == 0 { - assert.Empty(t, label.Confidentiality) - return - } + label := LabelSearchIssues(tc.visibilities) + assert.Equal(t, IntegrityUntrusted, label.Integrity) assert.Equal(t, tc.wantConfidential, label.Confidentiality) }) }