Skip to content

Commit 883f58d

Browse files
authored
Add ifc label for issue_read tool (#2457)
* 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. * Add ifc label for issue_read tool Emits an IFC SecurityLabel on the issue_read tool result when the InsidersMode flag is enabled, mirroring the pattern landed for get_me in #2432, list_issues in #2453, get_file_contents in #2454, and search_issues in #2456. issue_read operates on a single issue in a single repository so the label has the same per-repo semantics as list_issues; the helper ifc.LabelListIssues is reused directly. Integrity is always untrusted (issue contents, comments, and label descriptions are user-authored). Public repos are labelled PublicUntrusted; private repos are labelled PrivateUntrusted with the repository's collaborator logins, falling back to [owner] when the collaborators lookup fails. The IssueRead handler dispatches to four sub-functions (GetIssue, GetIssueComments, GetSubIssues, GetIssueLabels). The IFC label is attached at the dispatch site via a single attachIFC closure, so all four method branches emit the label without changes to the underlying helpers. Visibility-lookup failures cause the label to be omitted entirely (consistent with get_file_contents and search_issues). A future cleanup PR can extract attachIFC into a shared helper now that get_file_contents, search_issues, and issue_read use near-identical closures; intentionally not bundled here to keep the diff minimal. Refs github/copilot-mcp-core#1623, github/copilot-mcp-core#1389. Note: chained on #2456 (gokhanarkan/fides-search-issues), which is in turn chained on #2454. GitHub will retarget the base to main once those merge. * issue_read: simplify attachIFC by dropping unused lazy-cache Address Joanna's review feedback on #2457: the dispatch switch returns on exactly one branch, so attachIFC runs at most once per request. The ifcLabelKnown / ifcIsPrivate / ifcReaders cache variables were never reused across calls and only added complexity. Inline the visibility and collaborators lookups directly into the closure and drop the cache. Behaviour is identical; a follow-up can add real per-request caching across handlers if needed.
1 parent 9ad99c5 commit 883f58d

2 files changed

Lines changed: 156 additions & 4 deletions

File tree

pkg/github/issues.go

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -296,19 +296,50 @@ Options are:
296296
return utils.NewToolResultErrorFromErr("failed to get GitHub graphql client", err), nil, nil
297297
}
298298

299+
// attachIFC adds the IFC label to a successful tool result when
300+
// InsidersMode is enabled. If the visibility lookup fails the
301+
// label is omitted rather than misclassifying the result. If
302+
// only the collaborators lookup fails for a private repo we
303+
// fall back to the owner so the reader set is never empty. The
304+
// label matches list_issues semantics: per-repo visibility,
305+
// integrity always untrusted.
306+
attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult {
307+
if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode {
308+
return r
309+
}
310+
isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo)
311+
if err != nil {
312+
return r
313+
}
314+
var readers []string
315+
if isPrivate {
316+
if collaborators, err := FetchRepoCollaborators(ctx, client, owner, repo); err == nil {
317+
readers = collaborators
318+
}
319+
if len(readers) == 0 {
320+
readers = []string{owner}
321+
}
322+
}
323+
if r.Meta == nil {
324+
r.Meta = mcp.Meta{}
325+
}
326+
r.Meta["ifc"] = ifc.LabelListIssues(isPrivate, readers)
327+
return r
328+
}
329+
299330
switch method {
300331
case "get":
301332
result, err := GetIssue(ctx, client, deps, owner, repo, issueNumber)
302-
return result, nil, err
333+
return attachIFC(result), nil, err
303334
case "get_comments":
304335
result, err := GetIssueComments(ctx, client, deps, owner, repo, issueNumber, pagination)
305-
return result, nil, err
336+
return attachIFC(result), nil, err
306337
case "get_sub_issues":
307338
result, err := GetSubIssues(ctx, client, deps, owner, repo, issueNumber, pagination)
308-
return result, nil, err
339+
return attachIFC(result), nil, err
309340
case "get_labels":
310341
result, err := GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber)
311-
return result, nil, err
342+
return attachIFC(result), nil, err
312343
default:
313344
return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil
314345
}

pkg/github/issues_test.go

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,127 @@ func Test_GetIssue(t *testing.T) {
275275
}
276276
}
277277

278+
func Test_IssueRead_IFC_InsidersMode(t *testing.T) {
279+
t.Parallel()
280+
281+
serverTool := IssueRead(translations.NullTranslationHelper)
282+
283+
mockIssue := &github.Issue{
284+
Number: github.Ptr(1),
285+
Title: github.Ptr("Test"),
286+
Body: github.Ptr("body"),
287+
State: github.Ptr("open"),
288+
HTMLURL: github.Ptr("https://github.com/octocat/repo/issues/1"),
289+
User: &github.User{Login: github.Ptr("u")},
290+
}
291+
292+
mockComments := []*github.IssueComment{
293+
{Body: github.Ptr("hello"), User: &github.User{Login: github.Ptr("u")}},
294+
}
295+
296+
makeMockClient := func(isPrivate bool, repoStatus int) *http.Client {
297+
handlers := map[string]http.HandlerFunc{
298+
GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue),
299+
GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockComments),
300+
GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.User{
301+
{Login: github.Ptr("octocat")},
302+
{Login: github.Ptr("alice")},
303+
}),
304+
}
305+
if repoStatus != 0 && repoStatus != http.StatusOK {
306+
handlers[GetReposByOwnerByRepo] = mockResponse(t, repoStatus, "boom")
307+
} else {
308+
handlers[GetReposByOwnerByRepo] = mockResponse(t, http.StatusOK, map[string]any{
309+
"name": "repo",
310+
"private": isPrivate,
311+
})
312+
}
313+
return MockHTTPClientWithHandlers(handlers)
314+
}
315+
316+
getReq := map[string]any{
317+
"method": "get",
318+
"owner": "octocat",
319+
"repo": "repo",
320+
"issue_number": float64(1),
321+
}
322+
commentsReq := map[string]any{
323+
"method": "get_comments",
324+
"owner": "octocat",
325+
"repo": "repo",
326+
"issue_number": float64(1),
327+
}
328+
329+
t.Run("insiders mode disabled omits ifc label", func(t *testing.T) {
330+
deps := BaseDeps{
331+
Client: github.NewClient(makeMockClient(false, 0)),
332+
Flags: FeatureFlags{InsidersMode: false},
333+
}
334+
handler := serverTool.Handler(deps)
335+
336+
request := createMCPRequest(getReq)
337+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
338+
require.NoError(t, err)
339+
require.False(t, result.IsError)
340+
341+
assert.Nil(t, result.Meta)
342+
})
343+
344+
t.Run("insiders mode enabled on public repo emits public untrusted", func(t *testing.T) {
345+
deps := BaseDeps{
346+
Client: github.NewClient(makeMockClient(false, 0)),
347+
Flags: FeatureFlags{InsidersMode: true},
348+
}
349+
handler := serverTool.Handler(deps)
350+
351+
request := createMCPRequest(getReq)
352+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
353+
require.NoError(t, err)
354+
require.False(t, result.IsError)
355+
356+
require.NotNil(t, result.Meta)
357+
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
358+
assert.Equal(t, "untrusted", ifcMap["integrity"])
359+
assert.Equal(t, []any{"public"}, ifcMap["confidentiality"])
360+
})
361+
362+
t.Run("insiders mode enabled on private repo with get_comments emits private untrusted", func(t *testing.T) {
363+
deps := BaseDeps{
364+
Client: github.NewClient(makeMockClient(true, 0)),
365+
Flags: FeatureFlags{InsidersMode: true},
366+
}
367+
handler := serverTool.Handler(deps)
368+
369+
request := createMCPRequest(commentsReq)
370+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
371+
require.NoError(t, err)
372+
require.False(t, result.IsError)
373+
374+
require.NotNil(t, result.Meta)
375+
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
376+
assert.Equal(t, "untrusted", ifcMap["integrity"])
377+
assert.Equal(t, []any{"octocat", "alice"}, ifcMap["confidentiality"])
378+
})
379+
380+
t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) {
381+
deps := BaseDeps{
382+
Client: github.NewClient(makeMockClient(false, http.StatusInternalServerError)),
383+
Flags: FeatureFlags{InsidersMode: true},
384+
}
385+
handler := serverTool.Handler(deps)
386+
387+
request := createMCPRequest(getReq)
388+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
389+
require.NoError(t, err)
390+
require.False(t, result.IsError, "tool call should still succeed when visibility lookup fails")
391+
392+
if result.Meta != nil {
393+
_, hasIFC := result.Meta["ifc"]
394+
assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails")
395+
}
396+
})
397+
}
398+
278399
func Test_AddIssueComment(t *testing.T) {
279400
// Verify tool definition once
280401
serverTool := AddIssueComment(translations.NullTranslationHelper)

0 commit comments

Comments
 (0)