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.
This commit is contained in:
@@ -10,6 +10,7 @@ import (
|
||||
"strings"
|
||||
|
||||
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/octicons"
|
||||
"github.com/github/github-mcp-server/pkg/scopes"
|
||||
@@ -681,6 +682,20 @@ func FetchRepoCollaborators(ctx context.Context, client *github.Client, owner, r
|
||||
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.
|
||||
func FetchRepoIsPrivate(ctx context.Context, client *github.Client, owner, repo string) (bool, error) {
|
||||
r, resp, err := client.Repositories.Get(ctx, owner, repo)
|
||||
if resp != nil {
|
||||
defer func() { _ = resp.Body.Close() }()
|
||||
}
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
return r.GetPrivate(), 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(
|
||||
@@ -753,6 +768,46 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
|
||||
return utils.NewToolResultError("failed to get GitHub client"), nil, nil
|
||||
}
|
||||
|
||||
// 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.
|
||||
var (
|
||||
ifcLabelKnown bool
|
||||
ifcIsPrivate bool
|
||||
ifcReaders []string
|
||||
)
|
||||
attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult {
|
||||
if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode {
|
||||
return r
|
||||
}
|
||||
if !ifcLabelKnown {
|
||||
isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo)
|
||||
if err != nil {
|
||||
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)
|
||||
return r
|
||||
}
|
||||
|
||||
rawOpts, fallbackUsed, err := resolveGitReference(ctx, client, owner, repo, ref, sha)
|
||||
if err != nil {
|
||||
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
|
||||
// The path does not point to a file or directory.
|
||||
// Instead let's try to find it in the Git Tree by matching the end of the path.
|
||||
if err != nil || (fileContent == nil && dirContent == nil) {
|
||||
return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
|
||||
res, data, err := matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
|
||||
return attachIFC(res), data, err
|
||||
}
|
||||
|
||||
if fileContent != nil && fileContent.SHA != nil {
|
||||
@@ -804,7 +860,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
|
||||
Text: "",
|
||||
MIMEType: "text/plain",
|
||||
}
|
||||
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
|
||||
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
|
||||
}
|
||||
|
||||
// For files >= 1MB, return a ResourceLink instead of content
|
||||
@@ -817,10 +873,10 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
|
||||
Title: fmt.Sprintf("File: %s", path),
|
||||
Size: &size,
|
||||
}
|
||||
return utils.NewToolResultResourceLink(
|
||||
return attachIFC(utils.NewToolResultResourceLink(
|
||||
fmt.Sprintf("File %s is too large to display (%d bytes). Use the download URL to fetch the content: %s (SHA: %s)%s",
|
||||
path, fileSize, fileContent.GetDownloadURL(), fileSHA, successNote),
|
||||
resourceLink), nil, nil
|
||||
resourceLink)), nil, nil
|
||||
}
|
||||
|
||||
// For files < 1MB, get content directly from Contents API
|
||||
@@ -848,7 +904,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
|
||||
Text: content,
|
||||
MIMEType: contentType,
|
||||
}
|
||||
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
|
||||
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
|
||||
}
|
||||
|
||||
// Binary content - encode as base64 blob
|
||||
@@ -858,14 +914,14 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
|
||||
Blob: []byte(blobContent),
|
||||
MIMEType: contentType,
|
||||
}
|
||||
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
|
||||
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
|
||||
} else if dirContent != nil {
|
||||
// file content or file SHA is nil which means it's a directory
|
||||
r, err := json.Marshal(dirContent)
|
||||
if err != nil {
|
||||
return utils.NewToolResultError("failed to marshal response"), nil, nil
|
||||
}
|
||||
return utils.NewToolResultText(string(r)), nil, nil
|
||||
return attachIFC(utils.NewToolResultText(string(r))), nil, nil
|
||||
}
|
||||
|
||||
return utils.NewToolResultError("failed to get file contents"), nil, nil
|
||||
|
||||
@@ -477,6 +477,158 @@ func Test_GetFileContents(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func Test_GetFileContents_IFC_InsidersMode(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
serverTool := GetFileContents(translations.NullTranslationHelper)
|
||||
|
||||
mockRawContent := []byte("hello")
|
||||
|
||||
makeMockClient := func(isPrivate bool) *http.Client {
|
||||
return MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
|
||||
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
|
||||
GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, map[string]any{
|
||||
"name": "repo",
|
||||
"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)
|
||||
fileContent := &github.RepositoryContent{
|
||||
Name: github.Ptr("README.md"),
|
||||
Path: github.Ptr("README.md"),
|
||||
SHA: github.Ptr("abc123"),
|
||||
Type: github.Ptr("file"),
|
||||
Content: github.Ptr(encodedContent),
|
||||
Size: github.Ptr(len(mockRawContent)),
|
||||
Encoding: github.Ptr("base64"),
|
||||
}
|
||||
contentBytes, _ := json.Marshal(fileContent)
|
||||
_, _ = w.Write(contentBytes)
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
reqParams := map[string]any{
|
||||
"owner": "octocat",
|
||||
"repo": "repo",
|
||||
"path": "README.md",
|
||||
"ref": "refs/heads/main",
|
||||
}
|
||||
|
||||
t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) {
|
||||
deps := BaseDeps{
|
||||
Client: github.NewClient(makeMockClient(false)),
|
||||
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) {
|
||||
deps := BaseDeps{
|
||||
Client: github.NewClient(makeMockClient(false)),
|
||||
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 trusted label", func(t *testing.T) {
|
||||
deps := BaseDeps{
|
||||
Client: github.NewClient(makeMockClient(true)),
|
||||
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, "trusted", ifcMap["integrity"])
|
||||
confList, ok := ifcMap["confidentiality"].([]any)
|
||||
require.True(t, ok, "confidentiality should be a list")
|
||||
assert.Equal(t, []any{"octocat", "alice"}, confList)
|
||||
})
|
||||
|
||||
t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) {
|
||||
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
|
||||
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
|
||||
GetReposByOwnerByRepo: mockResponse(t, http.StatusInternalServerError, "boom"),
|
||||
GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
encodedContent := base64.StdEncoding.EncodeToString(mockRawContent)
|
||||
fileContent := &github.RepositoryContent{
|
||||
Name: github.Ptr("README.md"),
|
||||
Path: github.Ptr("README.md"),
|
||||
SHA: github.Ptr("abc123"),
|
||||
Type: github.Ptr("file"),
|
||||
Content: github.Ptr(encodedContent),
|
||||
Size: github.Ptr(len(mockRawContent)),
|
||||
Encoding: github.Ptr("base64"),
|
||||
}
|
||||
contentBytes, _ := json.Marshal(fileContent)
|
||||
_, _ = w.Write(contentBytes)
|
||||
},
|
||||
})
|
||||
deps := BaseDeps{
|
||||
Client: github.NewClient(mockedClient),
|
||||
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 visibility lookup fails")
|
||||
|
||||
if result.Meta != nil {
|
||||
_, hasIFC := result.Meta["ifc"]
|
||||
assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func Test_ForkRepository(t *testing.T) {
|
||||
// Verify tool definition once
|
||||
serverTool := ForkRepository(translations.NullTranslationHelper)
|
||||
|
||||
@@ -75,3 +75,14 @@ func LabelListIssues(isPrivate bool, readers []string) SecurityLabel {
|
||||
}
|
||||
return PublicUntrusted()
|
||||
}
|
||||
|
||||
// LabelGetFileContents returns the IFC label for a get_file_contents result.
|
||||
// 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 {
|
||||
if isPrivate {
|
||||
return PrivateTrusted(readers)
|
||||
}
|
||||
return PublicUntrusted()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user