Skip to content

Commit 0cdcd4a

Browse files
authored
Add ifc label for get_file_contents tool (#2454)
* Add ifc label for get_file_contents tool Emits an IFC SecurityLabel on the get_file_contents tool result when the InsidersMode flag is enabled, mirroring the pattern landed for get_me in Public repositories are labelled PublicUntrusted (anyone can author file content via pull requests). Private repositories are labelled PrivateTrusted with the repository owner as a placeholder reader, since only collaborators can land changes there. Full collaborator enumeration is intentionally deferred to a follow-up shared helper. A new exported FetchRepoIsPrivate helper wraps Repositories.Get for visibility lookups; it is invoked lazily and only when InsidersMode is on, so non-insiders pay no extra round trip. Visibility lookup failures skip the label rather than fail the user-facing call. Refs github/copilot-mcp-core#1623, github/copilot-mcp-core#1389. * get_file_contents: address Copilot review findings - FetchRepoIsPrivate: tighten doc to 'returns whether a repository is private' and close the underlying *github.Response body. - attachIFC: skip emitting the ifc label when the repository visibility lookup fails, instead of falling through to PublicUntrusted (which would mislabel a private or unknown-visibility repo as public). The failure is no longer cached so a subsequent return path can retry. - Add a test asserting the tool still succeeds and omits result.Meta ["ifc"] when the visibility lookup returns 500.
1 parent 5259513 commit 0cdcd4a

3 files changed

Lines changed: 226 additions & 7 deletions

File tree

pkg/github/repositories.go

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"strings"
1111

1212
ghErrors "github.com/github/github-mcp-server/pkg/errors"
13+
"github.com/github/github-mcp-server/pkg/ifc"
1314
"github.com/github/github-mcp-server/pkg/inventory"
1415
"github.com/github/github-mcp-server/pkg/octicons"
1516
"github.com/github/github-mcp-server/pkg/scopes"
@@ -681,6 +682,20 @@ func FetchRepoCollaborators(ctx context.Context, client *github.Client, owner, r
681682
return logins, nil
682683
}
683684

685+
// FetchRepoIsPrivate returns whether a repository is private. It is a thin
686+
// wrapper around the GitHub Repositories.Get endpoint provided as a shared
687+
// helper for IFC label computation across tools.
688+
func FetchRepoIsPrivate(ctx context.Context, client *github.Client, owner, repo string) (bool, error) {
689+
r, resp, err := client.Repositories.Get(ctx, owner, repo)
690+
if resp != nil {
691+
defer func() { _ = resp.Body.Close() }()
692+
}
693+
if err != nil {
694+
return false, err
695+
}
696+
return r.GetPrivate(), nil
697+
}
698+
684699
// GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository.
685700
func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool {
686701
return NewTool(
@@ -753,6 +768,46 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
753768
return utils.NewToolResultError("failed to get GitHub client"), nil, nil
754769
}
755770

771+
// attachIFC adds the IFC label to a successful tool result when
772+
// InsidersMode is enabled. The visibility and (for private
773+
// repositories) collaborators lookups are performed lazily on
774+
// first use. If the visibility lookup fails we skip the label
775+
// rather than misclassify the result; the failure is not cached
776+
// so a later return path can retry. If only the collaborators
777+
// lookup fails for a private repo we fall back to the owner so
778+
// the reader set is never empty.
779+
var (
780+
ifcLabelKnown bool
781+
ifcIsPrivate bool
782+
ifcReaders []string
783+
)
784+
attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult {
785+
if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode {
786+
return r
787+
}
788+
if !ifcLabelKnown {
789+
isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo)
790+
if err != nil {
791+
return r
792+
}
793+
ifcIsPrivate = isPrivate
794+
if ifcIsPrivate {
795+
if collaborators, err := FetchRepoCollaborators(ctx, client, owner, repo); err == nil {
796+
ifcReaders = collaborators
797+
}
798+
if len(ifcReaders) == 0 {
799+
ifcReaders = []string{owner}
800+
}
801+
}
802+
ifcLabelKnown = true
803+
}
804+
if r.Meta == nil {
805+
r.Meta = mcp.Meta{}
806+
}
807+
r.Meta["ifc"] = ifc.LabelGetFileContents(ifcIsPrivate, ifcReaders)
808+
return r
809+
}
810+
756811
rawOpts, fallbackUsed, err := resolveGitReference(ctx, client, owner, repo, ref, sha)
757812
if err != nil {
758813
return utils.NewToolResultError(fmt.Sprintf("failed to resolve git reference: %s", err)), nil, nil
@@ -774,7 +829,8 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
774829
// The path does not point to a file or directory.
775830
// Instead let's try to find it in the Git Tree by matching the end of the path.
776831
if err != nil || (fileContent == nil && dirContent == nil) {
777-
return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
832+
res, data, err := matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
833+
return attachIFC(res), data, err
778834
}
779835

780836
if fileContent != nil && fileContent.SHA != nil {
@@ -804,7 +860,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
804860
Text: "",
805861
MIMEType: "text/plain",
806862
}
807-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
863+
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
808864
}
809865

810866
// For files >= 1MB, return a ResourceLink instead of content
@@ -817,10 +873,10 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
817873
Title: fmt.Sprintf("File: %s", path),
818874
Size: &size,
819875
}
820-
return utils.NewToolResultResourceLink(
876+
return attachIFC(utils.NewToolResultResourceLink(
821877
fmt.Sprintf("File %s is too large to display (%d bytes). Use the download URL to fetch the content: %s (SHA: %s)%s",
822878
path, fileSize, fileContent.GetDownloadURL(), fileSHA, successNote),
823-
resourceLink), nil, nil
879+
resourceLink)), nil, nil
824880
}
825881

826882
// For files < 1MB, get content directly from Contents API
@@ -848,7 +904,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
848904
Text: content,
849905
MIMEType: contentType,
850906
}
851-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
907+
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
852908
}
853909

854910
// Binary content - encode as base64 blob
@@ -858,14 +914,14 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
858914
Blob: []byte(blobContent),
859915
MIMEType: contentType,
860916
}
861-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
917+
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
862918
} else if dirContent != nil {
863919
// file content or file SHA is nil which means it's a directory
864920
r, err := json.Marshal(dirContent)
865921
if err != nil {
866922
return utils.NewToolResultError("failed to marshal response"), nil, nil
867923
}
868-
return utils.NewToolResultText(string(r)), nil, nil
924+
return attachIFC(utils.NewToolResultText(string(r))), nil, nil
869925
}
870926

871927
return utils.NewToolResultError("failed to get file contents"), nil, nil

pkg/github/repositories_test.go

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,158 @@ func Test_GetFileContents(t *testing.T) {
477477
}
478478
}
479479

480+
func Test_GetFileContents_IFC_InsidersMode(t *testing.T) {
481+
t.Parallel()
482+
483+
serverTool := GetFileContents(translations.NullTranslationHelper)
484+
485+
mockRawContent := []byte("hello")
486+
487+
makeMockClient := func(isPrivate bool) *http.Client {
488+
return MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
489+
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
490+
GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, map[string]any{
491+
"name": "repo",
492+
"default_branch": "main",
493+
"private": isPrivate,
494+
}),
495+
GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.User{
496+
{Login: github.Ptr("octocat")},
497+
{Login: github.Ptr("alice")},
498+
}),
499+
GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) {
500+
w.WriteHeader(http.StatusOK)
501+
encodedContent := base64.StdEncoding.EncodeToString(mockRawContent)
502+
fileContent := &github.RepositoryContent{
503+
Name: github.Ptr("README.md"),
504+
Path: github.Ptr("README.md"),
505+
SHA: github.Ptr("abc123"),
506+
Type: github.Ptr("file"),
507+
Content: github.Ptr(encodedContent),
508+
Size: github.Ptr(len(mockRawContent)),
509+
Encoding: github.Ptr("base64"),
510+
}
511+
contentBytes, _ := json.Marshal(fileContent)
512+
_, _ = w.Write(contentBytes)
513+
},
514+
})
515+
}
516+
517+
reqParams := map[string]any{
518+
"owner": "octocat",
519+
"repo": "repo",
520+
"path": "README.md",
521+
"ref": "refs/heads/main",
522+
}
523+
524+
t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) {
525+
deps := BaseDeps{
526+
Client: github.NewClient(makeMockClient(false)),
527+
Flags: FeatureFlags{InsidersMode: false},
528+
}
529+
handler := serverTool.Handler(deps)
530+
531+
request := createMCPRequest(reqParams)
532+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
533+
require.NoError(t, err)
534+
require.False(t, result.IsError)
535+
536+
assert.Nil(t, result.Meta, "result meta should be nil when insiders mode is disabled")
537+
})
538+
539+
t.Run("insiders mode enabled on public repo emits public untrusted label", func(t *testing.T) {
540+
deps := BaseDeps{
541+
Client: github.NewClient(makeMockClient(false)),
542+
Flags: FeatureFlags{InsidersMode: true},
543+
}
544+
handler := serverTool.Handler(deps)
545+
546+
request := createMCPRequest(reqParams)
547+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
548+
require.NoError(t, err)
549+
require.False(t, result.IsError)
550+
551+
require.NotNil(t, result.Meta)
552+
ifcLabel, ok := result.Meta["ifc"]
553+
require.True(t, ok, "result meta should contain ifc key")
554+
555+
ifcJSON, err := json.Marshal(ifcLabel)
556+
require.NoError(t, err)
557+
var ifcMap map[string]any
558+
require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap))
559+
560+
assert.Equal(t, "untrusted", ifcMap["integrity"])
561+
confList, ok := ifcMap["confidentiality"].([]any)
562+
require.True(t, ok, "confidentiality should be a list")
563+
require.Len(t, confList, 1)
564+
assert.Equal(t, "public", confList[0])
565+
})
566+
567+
t.Run("insiders mode enabled on private repo emits private trusted label", func(t *testing.T) {
568+
deps := BaseDeps{
569+
Client: github.NewClient(makeMockClient(true)),
570+
Flags: FeatureFlags{InsidersMode: true},
571+
}
572+
handler := serverTool.Handler(deps)
573+
574+
request := createMCPRequest(reqParams)
575+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
576+
require.NoError(t, err)
577+
require.False(t, result.IsError)
578+
579+
require.NotNil(t, result.Meta)
580+
ifcLabel, ok := result.Meta["ifc"]
581+
require.True(t, ok, "result meta should contain ifc key")
582+
583+
ifcJSON, err := json.Marshal(ifcLabel)
584+
require.NoError(t, err)
585+
var ifcMap map[string]any
586+
require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap))
587+
588+
assert.Equal(t, "trusted", ifcMap["integrity"])
589+
confList, ok := ifcMap["confidentiality"].([]any)
590+
require.True(t, ok, "confidentiality should be a list")
591+
assert.Equal(t, []any{"octocat", "alice"}, confList)
592+
})
593+
594+
t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) {
595+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
596+
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
597+
GetReposByOwnerByRepo: mockResponse(t, http.StatusInternalServerError, "boom"),
598+
GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) {
599+
w.WriteHeader(http.StatusOK)
600+
encodedContent := base64.StdEncoding.EncodeToString(mockRawContent)
601+
fileContent := &github.RepositoryContent{
602+
Name: github.Ptr("README.md"),
603+
Path: github.Ptr("README.md"),
604+
SHA: github.Ptr("abc123"),
605+
Type: github.Ptr("file"),
606+
Content: github.Ptr(encodedContent),
607+
Size: github.Ptr(len(mockRawContent)),
608+
Encoding: github.Ptr("base64"),
609+
}
610+
contentBytes, _ := json.Marshal(fileContent)
611+
_, _ = w.Write(contentBytes)
612+
},
613+
})
614+
deps := BaseDeps{
615+
Client: github.NewClient(mockedClient),
616+
Flags: FeatureFlags{InsidersMode: true},
617+
}
618+
handler := serverTool.Handler(deps)
619+
620+
request := createMCPRequest(reqParams)
621+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
622+
require.NoError(t, err)
623+
require.False(t, result.IsError, "tool call should still succeed when visibility lookup fails")
624+
625+
if result.Meta != nil {
626+
_, hasIFC := result.Meta["ifc"]
627+
assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails")
628+
}
629+
})
630+
}
631+
480632
func Test_ForkRepository(t *testing.T) {
481633
// Verify tool definition once
482634
serverTool := ForkRepository(translations.NullTranslationHelper)

pkg/ifc/ifc.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,3 +75,14 @@ func LabelListIssues(isPrivate bool, readers []string) SecurityLabel {
7575
}
7676
return PublicUntrusted()
7777
}
78+
79+
// LabelGetFileContents returns the IFC label for a get_file_contents result.
80+
// Public repository file contents may be authored by anyone via pull requests
81+
// and are therefore untrusted. In private repositories only collaborators can
82+
// land changes, so contents are treated as trusted.
83+
func LabelGetFileContents(isPrivate bool, readers []string) SecurityLabel {
84+
if isPrivate {
85+
return PrivateTrusted(readers)
86+
}
87+
return PublicUntrusted()
88+
}

0 commit comments

Comments
 (0)