Skip to content

Commit c34ff37

Browse files
RossTarrantCopilot
andcommitted
fix: address ifc label review feedback
Keep joined search integrity conservative for mixed public and private results, and split user-authored repo content from list_issues private-trusted labeling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2549290 commit c34ff37

9 files changed

Lines changed: 76 additions & 32 deletions

File tree

pkg/github/discussions.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ func ListDiscussions(t translations.TranslationHelperFunc) inventory.ServerTool
276276
result := utils.NewToolResultText(string(out))
277277
// Discussion content is user-authored (untrusted); confidentiality
278278
// follows repo visibility.
279-
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, owner, repo, result, ifc.LabelListIssues)
279+
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, owner, repo, result, ifc.LabelRepoUserContent)
280280
return result, nil, nil
281281
},
282282
)
@@ -384,7 +384,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
384384
result := utils.NewToolResultText(string(out))
385385
// Discussion content is user-authored (untrusted); confidentiality
386386
// follows repo visibility.
387-
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelListIssues)
387+
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelRepoUserContent)
388388
return result, nil, nil
389389
},
390390
)
@@ -592,7 +592,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
592592
result := utils.NewToolResultText(string(out))
593593
// Discussion comments are user-authored (untrusted); confidentiality
594594
// follows repo visibility.
595-
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelListIssues)
595+
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelRepoUserContent)
596596
return result, nil, nil
597597
},
598598
)

pkg/github/issues.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -804,7 +804,7 @@ Options are:
804804
// attachIFC adds the IFC label to a successful tool result when
805805
// IFC labels are enabled. If the visibility lookup fails the
806806
// label is omitted rather than misclassifying the result.
807-
attachIFC := newRepoVisibilityIFCLabeler(ctx, deps, client, owner, repo, ifc.LabelListIssues)
807+
attachIFC := newRepoVisibilityIFCLabeler(ctx, deps, client, owner, repo, ifc.LabelRepoUserContent)
808808

809809
switch method {
810810
case "get":

pkg/github/issues_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) {
356356
assert.Equal(t, "public", ifcMap["confidentiality"])
357357
})
358358

359-
t.Run("insiders mode enabled on private repo with get_comments emits private trusted", func(t *testing.T) {
359+
t.Run("insiders mode enabled on private repo with get_comments emits private untrusted", func(t *testing.T) {
360360
deps := BaseDeps{
361361
Client: mustNewGHClient(t, makeMockClient(true, 0)),
362362
featureChecker: featureCheckerFor(FeatureFlagIFCLabels),
@@ -370,7 +370,7 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) {
370370

371371
require.NotNil(t, result.Meta)
372372
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
373-
assert.Equal(t, "trusted", ifcMap["integrity"])
373+
assert.Equal(t, "untrusted", ifcMap["integrity"])
374374
assert.Equal(t, "private", ifcMap["confidentiality"])
375375
})
376376

@@ -1107,7 +1107,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) {
11071107
assert.Equal(t, "public", ifcMap["confidentiality"])
11081108
})
11091109

1110-
t.Run("insiders mode mixed public and private emits private trusted", func(t *testing.T) {
1110+
t.Run("insiders mode mixed public and private emits private untrusted", func(t *testing.T) {
11111111
searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{
11121112
makeIssue("octocat", "private-repo", 1),
11131113
makeIssue("octocat", "public-repo", 2),
@@ -1128,7 +1128,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) {
11281128

11291129
require.NotNil(t, result.Meta)
11301130
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
1131-
assert.Equal(t, "trusted", ifcMap["integrity"])
1131+
assert.Equal(t, "untrusted", ifcMap["integrity"])
11321132
assert.Equal(t, "private", ifcMap["confidentiality"])
11331133
})
11341134

pkg/github/pullrequests.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ Possible options:
115115
// visibility lookup fails the label is omitted rather than
116116
// misclassifying the result.
117117
attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult {
118-
return attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, r, ifc.LabelListIssues)
118+
return attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, r, ifc.LabelRepoUserContent)
119119
}
120120

121121
switch method {
@@ -1339,7 +1339,7 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool
13391339
result := utils.NewToolResultText(string(r))
13401340
// Pull request titles/bodies are user-authored (untrusted);
13411341
// confidentiality follows repo visibility.
1342-
result = attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, result, ifc.LabelListIssues)
1342+
result = attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, result, ifc.LabelRepoUserContent)
13431343
return result, nil, nil
13441344
})
13451345
}

pkg/github/repositories.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2121,9 +2121,10 @@ func ListStarredRepositories(t translations.TranslationHelperFunc) inventory.Ser
21212121
result := utils.NewToolResultText(string(r))
21222122
// A starred-repository listing exposes repository data across many
21232123
// repos; reuse the multi-repo join shared with search_repositories
2124-
// (public-only results stay public-untrusted; any private repo makes
2125-
// the joined result private-trusted). Visibility is read directly
2126-
// from the response, so no extra API call is needed.
2124+
// (public-only results stay public-untrusted, mixed-visibility
2125+
// results become private-untrusted, all-private results become
2126+
// private-trusted). Visibility is read directly from the response,
2127+
// so no extra API call is needed.
21272128
visibilities := make([]bool, 0, len(minimalRepos))
21282129
for _, mr := range minimalRepos {
21292130
visibilities = append(visibilities, mr.Private)

pkg/github/search.go

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,9 @@ func SearchRepositories(t translations.TranslationHelperFunc) inventory.ServerTo
173173
// every matched repository and attaches the result to callResult when IFC
174174
// labels are enabled. Visibility is read directly from the search response —
175175
// no extra API call. The join math is shared with search_issues via
176-
// ifc.LabelSearchIssues: public-only results stay public-untrusted, while any
177-
// private matched repository makes the joined result private-trusted. The
176+
// ifc.LabelSearchIssues: public-only results stay public-untrusted,
177+
// mixed-visibility results become private-untrusted, and all-private results
178+
// become private-trusted. The
178179
// feature-flag check is centralized here (mirroring the attach* helpers in
179180
// ifc_labels.go) so the handler can call this unconditionally.
180181
func attachSearchRepositoriesIFCLabel(ctx context.Context, deps ToolDependencies, repos []*github.Repository, callResult *mcp.CallToolResult) {
@@ -302,9 +303,9 @@ func SearchCode(t translations.TranslationHelperFunc) inventory.ServerTool {
302303
}
303304

304305
callResult := utils.NewToolResultText(string(r))
305-
// Code search spans repositories; the IFC label is the join across
306-
// every matched repository's visibility, read directly from the
307-
// search response.
306+
// Code search spans repositories; the IFC label is the conservative
307+
// join across every matched repository's visibility, read directly
308+
// from the search response.
308309
visibilities := make([]bool, 0, len(result.CodeResults))
309310
for _, code := range result.CodeResults {
310311
if code.Repository != nil {
@@ -593,9 +594,9 @@ func SearchCommits(t translations.TranslationHelperFunc) inventory.ServerTool {
593594
}
594595

595596
callResult := utils.NewToolResultText(string(r))
596-
// Commit search spans repositories; the IFC label is the join across
597-
// every matched repository's visibility, read directly from the
598-
// search response.
597+
// Commit search spans repositories; the IFC label is the conservative
598+
// join across every matched repository's visibility, read directly
599+
// from the search response.
599600
visibilities := make([]bool, 0, len(result.Commits))
600601
for _, commit := range result.Commits {
601602
if commit.Repository != nil {

pkg/github/search_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) {
238238
assert.Equal(t, "public", ifcMap["confidentiality"])
239239
})
240240

241-
t.Run("insiders mode any private match emits private trusted", func(t *testing.T) {
241+
t.Run("insiders mode mixed public and private emits private untrusted", func(t *testing.T) {
242242
deps := BaseDeps{
243243
Client: mustNewGHClient(t, makeMockClient([]repoFixture{
244244
{owner: "octocat", name: "private-repo", isPrivate: true},
@@ -255,7 +255,7 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) {
255255

256256
require.NotNil(t, result.Meta)
257257
ifcMap := unmarshalIFC(t, result.Meta["ifc"])
258-
assert.Equal(t, "trusted", ifcMap["integrity"])
258+
assert.Equal(t, "untrusted", ifcMap["integrity"])
259259
assert.Equal(t, "private", ifcMap["confidentiality"])
260260
})
261261

pkg/ifc/ifc.go

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,18 @@ func LabelListIssues(isPrivate bool) SecurityLabel {
8585
return PublicUntrusted()
8686
}
8787

88+
// LabelRepoUserContent returns the IFC label for user-authored content scoped
89+
// to a repository when that tool has not opted into a more specific integrity
90+
// policy. Confidentiality follows repository visibility, while integrity stays
91+
// untrusted because the payload can contain free-form issue, pull request,
92+
// discussion, review, or comment text.
93+
func LabelRepoUserContent(isPrivate bool) SecurityLabel {
94+
if isPrivate {
95+
return PrivateUntrusted()
96+
}
97+
return PublicUntrusted()
98+
}
99+
88100
// LabelGetFileContents returns the IFC label for a get_file_contents result.
89101
// Public repository file contents may be authored by anyone via pull requests
90102
// and are therefore untrusted. In private repositories only collaborators can
@@ -100,11 +112,13 @@ func LabelGetFileContents(isPrivate bool) SecurityLabel {
100112
// result, joining per-repository labels across all matched repositories.
101113
// Used by both search_issues and search_repositories.
102114
//
103-
// Public-only results are untrusted and public. If any matched repository is
104-
// private, the joined label is trusted and private because private repository
105-
// content is treated as trusted collaborator-authored data. The reader set is
106-
// opaque (the "private" marker); the client engine resolves concrete readers
107-
// on demand at egress decision time.
115+
// Public-only results are untrusted and public. All-private results are trusted
116+
// and private because private repository content is treated as trusted
117+
// collaborator-authored data. Mixed public/private results are untrusted and
118+
// private: the public items keep the joined payload's integrity untrusted,
119+
// while the private items keep the joined payload's confidentiality private.
120+
// The reader set is opaque (the "private" marker); the client engine resolves
121+
// concrete readers on demand at egress decision time.
108122
//
109123
// An empty result set is treated as public-untrusted (no repository data is
110124
// leaked).
@@ -119,12 +133,22 @@ func LabelGetFileContents(isPrivate bool) SecurityLabel {
119133
// until then they would invite unsafe declassification of a "public" item that
120134
// actually arrived alongside private data.
121135
func LabelSearchIssues(repoVisibilities []bool) SecurityLabel {
136+
var anyPrivate, anyPublic bool
122137
for _, isPrivate := range repoVisibilities {
123138
if isPrivate {
124-
return PrivateTrusted()
139+
anyPrivate = true
140+
} else {
141+
anyPublic = true
125142
}
126143
}
127-
return PublicUntrusted()
144+
switch {
145+
case anyPrivate && anyPublic:
146+
return PrivateUntrusted()
147+
case anyPrivate:
148+
return PrivateTrusted()
149+
default:
150+
return PublicUntrusted()
151+
}
128152
}
129153

130154
// LabelRepoMetadata returns the IFC label for structural repository metadata

pkg/ifc/ifc_test.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@ func TestLabelListIssues(t *testing.T) {
2424
})
2525
}
2626

27+
func TestLabelRepoUserContent(t *testing.T) {
28+
t.Parallel()
29+
30+
t.Run("public repo user content is untrusted and public", func(t *testing.T) {
31+
t.Parallel()
32+
label := LabelRepoUserContent(false)
33+
assert.Equal(t, IntegrityUntrusted, label.Integrity)
34+
assert.Equal(t, ConfidentialityPublic, label.Confidentiality)
35+
})
36+
37+
t.Run("private repo user content is untrusted and private", func(t *testing.T) {
38+
t.Parallel()
39+
label := LabelRepoUserContent(true)
40+
assert.Equal(t, IntegrityUntrusted, label.Integrity)
41+
assert.Equal(t, ConfidentialityPrivate, label.Confidentiality)
42+
})
43+
}
44+
2745
func TestLabelSearchIssues(t *testing.T) {
2846
t.Parallel()
2947

@@ -51,9 +69,9 @@ func TestLabelSearchIssues(t *testing.T) {
5169
wantConfidential: ConfidentialityPublic,
5270
},
5371
{
54-
name: "any private match flips to trusted private",
72+
name: "mixed public and private repos become untrusted private",
5573
visibilities: []bool{false, true, false},
56-
wantIntegrity: IntegrityTrusted,
74+
wantIntegrity: IntegrityUntrusted,
5775
wantConfidential: ConfidentialityPrivate,
5876
},
5977
{

0 commit comments

Comments
 (0)