From 6f52ec153396c2863d5c1a81ecc6479edd4e8c1f Mon Sep 17 00:00:00 2001 From: Keshav Malik Date: Fri, 14 Aug 2026 18:27:00 +0530 Subject: [PATCH 1/6] Clarify symlink behavior for repository file writes --- README.md | 2 +- .../__toolsnaps__/create_or_update_file.snap | 2 +- pkg/github/__toolsnaps__/push_files.snap | 2 +- pkg/github/repositories.go | 4 +- pkg/github/tools.go | 9 +- pkg/github/toolset_instructions.go | 6 ++ pkg/github/toolset_instructions_test.go | 86 +++++++++++++++++++ 7 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 pkg/github/toolset_instructions_test.go diff --git a/README.md b/README.md index 78195d5827..6289b51139 100644 --- a/README.md +++ b/README.md @@ -1288,7 +1288,7 @@ The following sets of tools are available: - `content`: Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API. (string, required) - `message`: Commit message (string, required) - `owner`: Repository owner (username or organization) (string, required) - - `path`: Path where to create/update the file (string, required) + - `path`: Exact Git path to write. Writing to a symbolic link path rewrites the symbolic link's target path; use the linked file's path to update its contents. (string, required) - `repo`: Repository name (string, required) - `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional) diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index 85ad887649..3f899f5c4e 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -24,7 +24,7 @@ "type": "string" }, "path": { - "description": "Path where to create/update the file", + "description": "Exact Git path to write. Writing to a symbolic link path rewrites the symbolic link's target path; use the linked file's path to update its contents.", "type": "string" }, "repo": { diff --git a/pkg/github/__toolsnaps__/push_files.snap b/pkg/github/__toolsnaps__/push_files.snap index 798ad18451..9981878c69 100644 --- a/pkg/github/__toolsnaps__/push_files.snap +++ b/pkg/github/__toolsnaps__/push_files.snap @@ -21,7 +21,7 @@ "type": "string" }, "path": { - "description": "path to the file", + "description": "Exact Git path to write. Writing to a symbolic link path replaces the link with a regular file; use the linked file's path to update its contents.", "type": "string" } }, diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index aa0509ad46..bcd42d3dbe 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -433,7 +433,7 @@ SHA MUST be provided for existing file updates. }, "path": { Type: "string", - Description: "Path where to create/update the file", + Description: "Exact Git path to write. Writing to a symbolic link path rewrites the symbolic link's target path; use the linked file's path to update its contents.", }, "content": { Type: "string", @@ -1573,7 +1573,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Properties: map[string]*jsonschema.Schema{ "path": { Type: "string", - Description: "path to the file", + Description: "Exact Git path to write. Writing to a symbolic link path replaces the link with a regular file; use the linked file's path to update its contents.", }, "content": { Type: "string", diff --git a/pkg/github/tools.go b/pkg/github/tools.go index f9b51159b5..a720ef365a 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -38,10 +38,11 @@ var ( InstructionsFunc: generateContextToolsetInstructions, } ToolsetMetadataRepos = inventory.ToolsetMetadata{ - ID: "repos", - Description: "GitHub Repository related tools", - Default: true, - Icon: "repo", + ID: "repos", + Description: "GitHub Repository related tools", + Default: true, + Icon: "repo", + InstructionsFunc: generateReposToolsetInstructions, } ToolsetMetadataGit = inventory.ToolsetMetadata{ ID: "git", diff --git a/pkg/github/toolset_instructions.go b/pkg/github/toolset_instructions.go index 3b3a54eadd..8cd908f31d 100644 --- a/pkg/github/toolset_instructions.go +++ b/pkg/github/toolset_instructions.go @@ -9,6 +9,12 @@ func generateContextToolsetInstructions(_ *inventory.Inventory) string { return "Always call 'get_me' first to understand current user permissions and context." } +func generateReposToolsetInstructions(_ *inventory.Inventory) string { + return `## Repository file writes + +'get_file_contents' may return the target contents when a path is a symbolic link, but repository file writes use exact Git paths and do not follow symbolic links. To edit content that a symlink points to, write to the target path.` +} + func generateIssuesToolsetInstructions(_ *inventory.Inventory) string { return `## Issues diff --git a/pkg/github/toolset_instructions_test.go b/pkg/github/toolset_instructions_test.go new file mode 100644 index 0000000000..5e7dbb4cdc --- /dev/null +++ b/pkg/github/toolset_instructions_test.go @@ -0,0 +1,86 @@ +package github + +import ( + "strings" + "testing" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRepositoryInstructionsExplainSymlinkWriteSemantics(t *testing.T) { + t.Setenv("DISABLE_INSTRUCTIONS", "false") + + reposInventory, err := inventory.NewBuilder(). + SetTools([]inventory.ServerTool{{Toolset: ToolsetMetadataRepos}}). + WithToolsets([]string{"repos"}). + WithServerInstructions(). + Build() + require.NoError(t, err) + + instructions := strings.ToLower(reposInventory.Instructions()) + assert.Contains(t, instructions, "## repository file writes") + assert.Contains(t, instructions, "may return the target contents") + assert.Contains(t, instructions, "do not follow symbolic links") + + defaultInventory, err := inventory.NewBuilder(). + SetTools([]inventory.ServerTool{ + {Toolset: ToolsetMetadataContext}, + {Toolset: ToolsetMetadataRepos}, + }). + WithToolsets([]string{"default"}). + WithServerInstructions(). + Build() + require.NoError(t, err) + assert.Contains(t, strings.ToLower(defaultInventory.Instructions()), "## repository file writes") + + contextInventory, err := inventory.NewBuilder(). + SetTools([]inventory.ServerTool{{Toolset: ToolsetMetadataContext}}). + WithToolsets([]string{"context"}). + WithServerInstructions(). + Build() + require.NoError(t, err) + assert.NotContains(t, strings.ToLower(contextInventory.Instructions()), "## repository file writes") +} + +func TestFileWritePathsExplainSymlinkWriteSemantics(t *testing.T) { + createSchema, ok := CreateOrUpdateFile(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok) + pushSchema, ok := PushFiles(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok) + createPath := createSchema.Properties["path"] + require.NotNil(t, createPath) + pushFiles := pushSchema.Properties["files"] + require.NotNil(t, pushFiles) + require.NotNil(t, pushFiles.Items) + pushPath := pushFiles.Items.Properties["path"] + require.NotNil(t, pushPath) + + tools := []struct { + name string + description string + expectedBehavior string + }{ + { + name: "create_or_update_file", + description: createPath.Description, + expectedBehavior: "rewrites the symbolic link's target path", + }, + { + name: "push_files", + description: pushPath.Description, + expectedBehavior: "replaces the link with a regular file", + }, + } + + for _, tool := range tools { + t.Run(tool.name, func(t *testing.T) { + description := strings.ToLower(tool.description) + assert.Contains(t, description, "exact git path") + assert.Contains(t, description, tool.expectedBehavior) + }) + } +} From 434d3c69e57d1d8b74544d9c2b0427317c7f827e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 00:07:35 +0200 Subject: [PATCH 2/6] fix(repos): guard writes to symbolic links Detect existing symlinks through the Git tree and require an explicit opt-in before changing their targets. Return the resolved repository target so callers can safely update the linked file instead. Refs #2997 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 3 +- .../__toolsnaps__/create_or_update_file.snap | 7 +- pkg/github/helper_test.go | 1 + pkg/github/repositories.go | 25 ++- pkg/github/repositories_helper.go | 108 ++++++++++++ pkg/github/repositories_test.go | 159 ++++++++++++++++++ pkg/github/toolset_instructions.go | 2 +- pkg/github/toolset_instructions_test.go | 8 +- 8 files changed, 308 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 6289b51139..4bd100139a 100644 --- a/README.md +++ b/README.md @@ -1284,11 +1284,12 @@ The following sets of tools are available: - **create_or_update_file** - Create or update file - **Required OAuth Scopes**: `repo` + - `allow_symlink_write`: Set to true only to intentionally change a symbolic link's target. The content must be the new link target path. By default, writes to existing symbolic links are rejected to prevent replacing the link target with file contents returned by get_file_contents. (boolean, optional) - `branch`: Branch to create/update the file in (string, required) - `content`: Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API. (string, required) - `message`: Commit message (string, required) - `owner`: Repository owner (username or organization) (string, required) - - `path`: Exact Git path to write. Writing to a symbolic link path rewrites the symbolic link's target path; use the linked file's path to update its contents. (string, required) + - `path`: Exact Git path to write. Writing to a symbolic link path changes the link target to the supplied content; it does not update the linked file. (string, required) - `repo`: Repository name (string, required) - `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional) diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index 3f899f5c4e..5f31f4f030 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -7,6 +7,11 @@ "description": "Create or update a single file in a GitHub repository. \nIf updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.\n\nIn order to obtain the SHA of original file version before updating, use the following git command:\ngit rev-parse \u003cbranch\u003e:\u003cpath to file\u003e\n\nSHA MUST be provided for existing file updates.\n", "inputSchema": { "properties": { + "allow_symlink_write": { + "default": false, + "description": "Set to true only to intentionally change a symbolic link's target. The content must be the new link target path. By default, writes to existing symbolic links are rejected to prevent replacing the link target with file contents returned by get_file_contents.", + "type": "boolean" + }, "branch": { "description": "Branch to create/update the file in", "type": "string" @@ -24,7 +29,7 @@ "type": "string" }, "path": { - "description": "Exact Git path to write. Writing to a symbolic link path rewrites the symbolic link's target path; use the linked file's path to update its contents.", + "description": "Exact Git path to write. Writing to a symbolic link path changes the link target to the supplied content; it does not update the linked file.", "type": "string" }, "repo": { diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index f6737c5df0..5fc541d45d 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -45,6 +45,7 @@ const ( ListCollaborators = "GET /repos/{owner}/{repo}/collaborators" // Git endpoints + GetReposGitBlobsByOwnerByRepoByFileSHA = "GET /repos/{owner}/{repo}/git/blobs/{file_sha}" GetReposGitTreesByOwnerByRepoByTree = "GET /repos/{owner}/{repo}/git/trees/{tree}" GetReposGitRefByOwnerByRepoByRef = "GET /repos/{owner}/{repo}/git/ref/{ref:.*}" PostReposGitRefsByOwnerByRepo = "POST /repos/{owner}/{repo}/git/refs" diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index bcd42d3dbe..9e869e1791 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -433,7 +433,7 @@ SHA MUST be provided for existing file updates. }, "path": { Type: "string", - Description: "Exact Git path to write. Writing to a symbolic link path rewrites the symbolic link's target path; use the linked file's path to update its contents.", + Description: "Exact Git path to write. Writing to a symbolic link path changes the link target to the supplied content; it does not update the linked file.", }, "content": { Type: "string", @@ -451,6 +451,11 @@ SHA MUST be provided for existing file updates. Type: "string", Description: "The blob SHA of the file being replaced. Required if the file already exists.", }, + "allow_symlink_write": { + Type: "boolean", + Description: "Set to true only to intentionally change a symbolic link's target. The content must be the new link target path. By default, writes to existing symbolic links are rejected to prevent replacing the link target with file contents returned by get_file_contents.", + Default: json.RawMessage("false"), + }, }, Required: []string{"owner", "repo", "path", "content", "message", "branch"}, }, @@ -501,6 +506,11 @@ SHA MUST be provided for existing file updates. opts.SHA = github.Ptr(sha) } + allowSymlinkWrite, err := OptionalParam[bool](args, "allow_symlink_write") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + // Create or update the file client, err := deps.GetClient(ctx) if err != nil { @@ -541,6 +551,19 @@ SHA MUST be provided for existing file updates. "Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", sha, currentSHA, branch, path)), nil, nil } + if !allowSymlinkWrite { + symlinkTarget, isSymlink, respTree, err := symlinkTargetAtPath(ctx, client, owner, repo, branch, path) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to verify whether file path is a symbolic link", + respTree, + err, + ), nil, nil + } + if isSymlink { + return newSymlinkWriteBlockedResult(path, symlinkTarget), nil, nil + } + } } } else { // No SHA provided - check if file already exists diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 9795bdc109..74826fa355 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + pathpkg "path" "strings" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -90,6 +91,113 @@ func createReferenceFromDefaultBranch(ctx context.Context, client *github.Client return createdRef, nil } +const gitSymlinkMode = "120000" + +type symlinkWriteBlockedError struct { + Error string `json:"error"` + Path string `json:"path"` + SymlinkTarget string `json:"symlink_target"` + ResolvedTargetPath string `json:"resolved_target_path,omitempty"` + Message string `json:"message"` +} + +func newSymlinkWriteBlockedResult(path, target string) *mcp.CallToolResult { + resolvedTargetPath := resolveRepositorySymlinkTarget(path, target) + message := "The exact Git path is a symbolic link. get_file_contents may have returned the linked file's content and SHA, " + + "but create_or_update_file would write that content into the symlink itself. " + if resolvedTargetPath != "" { + message += fmt.Sprintf("Write to %q instead, or set allow_symlink_write to true only to intentionally change the link target.", resolvedTargetPath) + } else { + message += "The link target resolves outside this repository. Set allow_symlink_write to true only to intentionally change the link target." + } + + payload, _ := json.Marshal(symlinkWriteBlockedError{ + Error: "symlink_write_requires_explicit_opt_in", + Path: path, + SymlinkTarget: target, + ResolvedTargetPath: resolvedTargetPath, + Message: message, + }) + return utils.NewToolResultError(string(payload)) +} + +func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (string, bool, *github.Response, error) { + ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+treeish) + if err != nil { + return "", false, resp, err + } + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + headSHA := ref.GetObject().GetSHA() + if headSHA == "" { + return "", false, nil, fmt.Errorf("branch %q has no commit SHA", treeish) + } + + entry, resp, err := getTreeEntry(ctx, client, owner, repo, headSHA, path) + if err != nil { + return "", false, resp, err + } + if entry == nil { + return "", false, nil, fmt.Errorf("path %q exists according to the Contents API but was not found in the Git tree", path) + } + if entry.GetMode() != gitSymlinkMode { + return "", false, nil, nil + } + + target, resp, err := client.Git.GetBlobRaw(ctx, owner, repo, entry.GetSHA()) + if err != nil { + return "", false, resp, err + } + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + return string(target), true, nil, nil +} + +func getTreeEntry(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (*github.TreeEntry, *github.Response, error) { + segments := strings.Split(pathpkg.Clean(strings.TrimPrefix(path, "/")), "/") + for i, segment := range segments { + tree, resp, err := client.Git.GetTree(ctx, owner, repo, treeish, false) + if err != nil { + return nil, resp, err + } + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + + var matched *github.TreeEntry + for _, entry := range tree.Entries { + if entry.GetPath() == segment { + matched = entry + break + } + } + if matched == nil { + return nil, nil, nil + } + if i == len(segments)-1 { + return matched, nil, nil + } + if matched.GetType() != "tree" { + return nil, nil, nil + } + treeish = matched.GetSHA() + } + return nil, nil, nil +} + +func resolveRepositorySymlinkTarget(linkPath, target string) string { + if pathpkg.IsAbs(target) { + return "" + } + resolved := pathpkg.Clean(pathpkg.Join(pathpkg.Dir(linkPath), target)) + if resolved == ".." || strings.HasPrefix(resolved, "../") { + return "" + } + return resolved +} + // matchFiles searches for files in the Git tree that match the given path. // It's used when GetContents fails or returns unexpected results. func matchFiles(ctx context.Context, client *github.Client, owner, repo, ref, path string, rawOpts *raw.ContentOpts, rawAPIResponseCode int) (*mcp.CallToolResult, any, error) { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 10aef42d67..e32b05c2d7 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1790,6 +1790,45 @@ func Test_CreateOrUpdateFile(t *testing.T) { HTMLURL: github.Ptr("https://github.com/owner/repo/commit/def456abc789"), }, } + mockPathTree := func(mode string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + var tree *github.Tree + switch { + case strings.HasSuffix(r.URL.Path, "/head-sha"): + tree = &github.Tree{ + Entries: []*github.TreeEntry{ + { + Path: github.Ptr("docs"), + Mode: github.Ptr("040000"), + Type: github.Ptr("tree"), + SHA: github.Ptr("docs-tree"), + }, + }, + } + case strings.HasSuffix(r.URL.Path, "/docs-tree"): + tree = &github.Tree{ + Entries: []*github.TreeEntry{ + { + Path: github.Ptr("example.md"), + Mode: github.Ptr(mode), + Type: github.Ptr("blob"), + SHA: github.Ptr("example-sha"), + }, + }, + } + default: + require.FailNow(t, "unexpected tree request", r.URL.Path) + } + mockResponse(t, http.StatusOK, tree)(w, r) + } + } + mockBranchRef := mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/main"), + Object: &github.GitObject{ + SHA: github.Ptr("head-sha"), + Type: github.Ptr("commit"), + }, + }) tests := []struct { name string @@ -1798,6 +1837,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectError bool expectedContent *github.RepositoryContentResponse expectedErrMsg string + expectedErrMsgs []string }{ { name: "successful file creation", @@ -1831,6 +1871,8 @@ func Test_CreateOrUpdateFile(t *testing.T) { { name: "successful file update with SHA", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockBranchRef, + GetReposGitTreesByOwnerByRepoByTree: mockPathTree("100644"), "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), Type: github.Ptr("file"), @@ -1894,6 +1936,8 @@ func Test_CreateOrUpdateFile(t *testing.T) { { name: "sha validation - current sha matches", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockBranchRef, + GetReposGitTreesByOwnerByRepoByTree: mockPathTree("100644"), "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), Type: github.Ptr("file"), @@ -1931,6 +1975,118 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectError: false, expectedContent: mockFileResponse, }, + { + name: "rejects symbolic link update without explicit opt-in", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockBranchRef, + GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("other.md")) + }, + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "docs/example.md", + "content": "# Content returned by get_file_contents", + "message": "Update linked file", + "branch": "main", + "sha": "abc123def456", + }, + expectError: true, + expectedErrMsg: `"error":"symlink_write_requires_explicit_opt_in"`, + expectedErrMsgs: []string{ + `"symlink_target":"other.md"`, + `"resolved_target_path":"docs/other.md"`, + }, + }, + { + name: "resolves special-character branch before inspecting symlink", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: func(w http.ResponseWriter, r *http.Request) { + assert.Contains(t, r.URL.EscapedPath(), "/git/ref/heads/release%23candidate") + mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/release#candidate"), + Object: &github.GitObject{ + SHA: github.Ptr("head-sha"), + Type: github.Ptr("commit"), + }, + })(w, r) + }, + GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("other.md")) + }, + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "docs/example.md", + "content": "# Content returned by get_file_contents", + "message": "Update linked file", + "branch": "release#candidate", + "sha": "abc123def456", + }, + expectError: true, + expectedErrMsg: `"error":"symlink_write_requires_explicit_opt_in"`, + }, + { + name: "allows intentional symbolic link update with explicit opt-in", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ + "message": "Change link target", + "content": "b3RoZXIubWQ=", + "branch": "main", + "sha": "abc123def456", + }).andThen( + mockResponse(t, http.StatusOK, mockFileResponse), + ), + "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ + "message": "Change link target", + "content": "b3RoZXIubWQ=", + "branch": "main", + "sha": "abc123def456", + }).andThen( + mockResponse(t, http.StatusOK, mockFileResponse), + ), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "docs/example.md", + "content": "other.md", + "message": "Change link target", + "branch": "main", + "sha": "abc123def456", + "allow_symlink_write": true, + }, + expectError: false, + expectedContent: mockFileResponse, + }, { name: "sha validation - stale sha detected", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -2074,6 +2230,9 @@ func Test_CreateOrUpdateFile(t *testing.T) { require.True(t, result.IsError) errorContent := getErrorResult(t, result) assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + for _, expectedErrMsg := range tc.expectedErrMsgs { + assert.Contains(t, errorContent.Text, expectedErrMsg) + } return } diff --git a/pkg/github/toolset_instructions.go b/pkg/github/toolset_instructions.go index 8cd908f31d..01a4cab848 100644 --- a/pkg/github/toolset_instructions.go +++ b/pkg/github/toolset_instructions.go @@ -12,7 +12,7 @@ func generateContextToolsetInstructions(_ *inventory.Inventory) string { func generateReposToolsetInstructions(_ *inventory.Inventory) string { return `## Repository file writes -'get_file_contents' may return the target contents when a path is a symbolic link, but repository file writes use exact Git paths and do not follow symbolic links. To edit content that a symlink points to, write to the target path.` +'get_file_contents' may return the target contents when a path is a symbolic link, but repository file writes use exact Git paths and do not follow symbolic links. To edit content that a symlink points to, write to the target path. 'create_or_update_file' rejects writes to existing symbolic links unless 'allow_symlink_write' is true; only opt in when intentionally changing the link target.` } func generateIssuesToolsetInstructions(_ *inventory.Inventory) string { diff --git a/pkg/github/toolset_instructions_test.go b/pkg/github/toolset_instructions_test.go index 5e7dbb4cdc..73dc070992 100644 --- a/pkg/github/toolset_instructions_test.go +++ b/pkg/github/toolset_instructions_test.go @@ -25,6 +25,7 @@ func TestRepositoryInstructionsExplainSymlinkWriteSemantics(t *testing.T) { assert.Contains(t, instructions, "## repository file writes") assert.Contains(t, instructions, "may return the target contents") assert.Contains(t, instructions, "do not follow symbolic links") + assert.Contains(t, instructions, "rejects writes to existing symbolic links") defaultInventory, err := inventory.NewBuilder(). SetTools([]inventory.ServerTool{ @@ -53,6 +54,8 @@ func TestFileWritePathsExplainSymlinkWriteSemantics(t *testing.T) { require.True(t, ok) createPath := createSchema.Properties["path"] require.NotNil(t, createPath) + createAllowSymlinkWrite := createSchema.Properties["allow_symlink_write"] + require.NotNil(t, createAllowSymlinkWrite) pushFiles := pushSchema.Properties["files"] require.NotNil(t, pushFiles) require.NotNil(t, pushFiles.Items) @@ -67,7 +70,7 @@ func TestFileWritePathsExplainSymlinkWriteSemantics(t *testing.T) { { name: "create_or_update_file", description: createPath.Description, - expectedBehavior: "rewrites the symbolic link's target path", + expectedBehavior: "changes the link target", }, { name: "push_files", @@ -83,4 +86,7 @@ func TestFileWritePathsExplainSymlinkWriteSemantics(t *testing.T) { assert.Contains(t, description, tool.expectedBehavior) }) } + + assert.Equal(t, "boolean", createAllowSymlinkWrite.Type) + assert.Contains(t, strings.ToLower(createAllowSymlinkWrite.Description), "intentionally change a symbolic link's target") } From 318dfd4321d29d2b85e56180bd3df008805a93b9 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 10:44:48 +0200 Subject: [PATCH 3/6] perf(repos): reduce symlink guard overhead Remove persistent repository and push-files guidance in favor of concise runtime recovery content. Resolve refs directly through Git Trees, skip inspection for explicit symlinks and opt-ins, and lock request counts in tests.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 4 +- .../__toolsnaps__/create_or_update_file.snap | 4 +- pkg/github/__toolsnaps__/push_files.snap | 2 +- pkg/github/repositories.go | 9 +- pkg/github/repositories_helper.go | 64 +++--- pkg/github/repositories_test.go | 192 +++++++++++++----- pkg/github/tools.go | 9 +- pkg/github/toolset_instructions.go | 6 - pkg/github/toolset_instructions_test.go | 92 --------- 9 files changed, 187 insertions(+), 195 deletions(-) delete mode 100644 pkg/github/toolset_instructions_test.go diff --git a/README.md b/README.md index 4bd100139a..67943bc5c4 100644 --- a/README.md +++ b/README.md @@ -1284,12 +1284,12 @@ The following sets of tools are available: - **create_or_update_file** - Create or update file - **Required OAuth Scopes**: `repo` - - `allow_symlink_write`: Set to true only to intentionally change a symbolic link's target. The content must be the new link target path. By default, writes to existing symbolic links are rejected to prevent replacing the link target with file contents returned by get_file_contents. (boolean, optional) + - `allow_symlink_write`: Set true to update a symbolic link itself; content must be its new target path. (boolean, optional) - `branch`: Branch to create/update the file in (string, required) - `content`: Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API. (string, required) - `message`: Commit message (string, required) - `owner`: Repository owner (username or organization) (string, required) - - `path`: Exact Git path to write. Writing to a symbolic link path changes the link target to the supplied content; it does not update the linked file. (string, required) + - `path`: Path where to create/update the file (string, required) - `repo`: Repository name (string, required) - `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional) diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index 5f31f4f030..37bf3b46bf 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -9,7 +9,7 @@ "properties": { "allow_symlink_write": { "default": false, - "description": "Set to true only to intentionally change a symbolic link's target. The content must be the new link target path. By default, writes to existing symbolic links are rejected to prevent replacing the link target with file contents returned by get_file_contents.", + "description": "Set true to update a symbolic link itself; content must be its new target path.", "type": "boolean" }, "branch": { @@ -29,7 +29,7 @@ "type": "string" }, "path": { - "description": "Exact Git path to write. Writing to a symbolic link path changes the link target to the supplied content; it does not update the linked file.", + "description": "Path where to create/update the file", "type": "string" }, "repo": { diff --git a/pkg/github/__toolsnaps__/push_files.snap b/pkg/github/__toolsnaps__/push_files.snap index 9981878c69..798ad18451 100644 --- a/pkg/github/__toolsnaps__/push_files.snap +++ b/pkg/github/__toolsnaps__/push_files.snap @@ -21,7 +21,7 @@ "type": "string" }, "path": { - "description": "Exact Git path to write. Writing to a symbolic link path replaces the link with a regular file; use the linked file's path to update its contents.", + "description": "path to the file", "type": "string" } }, diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 9e869e1791..c632a93aa7 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -433,7 +433,7 @@ SHA MUST be provided for existing file updates. }, "path": { Type: "string", - Description: "Exact Git path to write. Writing to a symbolic link path changes the link target to the supplied content; it does not update the linked file.", + Description: "Path where to create/update the file", }, "content": { Type: "string", @@ -453,7 +453,7 @@ SHA MUST be provided for existing file updates. }, "allow_symlink_write": { Type: "boolean", - Description: "Set to true only to intentionally change a symbolic link's target. The content must be the new link target path. By default, writes to existing symbolic links are rejected to prevent replacing the link target with file contents returned by get_file_contents.", + Description: "Set true to update a symbolic link itself; content must be its new target path.", Default: json.RawMessage("false"), }, }, @@ -552,6 +552,9 @@ SHA MUST be provided for existing file updates. sha, currentSHA, branch, path)), nil, nil } if !allowSymlinkWrite { + if existingFile.GetType() == "symlink" { + return newSymlinkWriteBlockedResult(path, existingFile.GetTarget()), nil, nil + } symlinkTarget, isSymlink, respTree, err := symlinkTargetAtPath(ctx, client, owner, repo, branch, path) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -1596,7 +1599,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Properties: map[string]*jsonschema.Schema{ "path": { Type: "string", - Description: "Exact Git path to write. Writing to a symbolic link path replaces the link with a regular file; use the linked file's path to update its contents.", + Description: "path to the file", }, "content": { Type: "string", diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 74826fa355..ac30911ba5 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" pathpkg "path" "strings" @@ -96,45 +97,40 @@ const gitSymlinkMode = "120000" type symlinkWriteBlockedError struct { Error string `json:"error"` Path string `json:"path"` - SymlinkTarget string `json:"symlink_target"` - ResolvedTargetPath string `json:"resolved_target_path,omitempty"` - Message string `json:"message"` + Target string `json:"target"` + ResolvedTargetPath string `json:"resolved_path,omitempty"` } func newSymlinkWriteBlockedResult(path, target string) *mcp.CallToolResult { resolvedTargetPath := resolveRepositorySymlinkTarget(path, target) - message := "The exact Git path is a symbolic link. get_file_contents may have returned the linked file's content and SHA, " + - "but create_or_update_file would write that content into the symlink itself. " - if resolvedTargetPath != "" { - message += fmt.Sprintf("Write to %q instead, or set allow_symlink_write to true only to intentionally change the link target.", resolvedTargetPath) - } else { - message += "The link target resolves outside this repository. Set allow_symlink_write to true only to intentionally change the link target." - } - payload, _ := json.Marshal(symlinkWriteBlockedError{ - Error: "symlink_write_requires_explicit_opt_in", + Error: "symlink_write_requires_opt_in", Path: path, - SymlinkTarget: target, + Target: target, ResolvedTargetPath: resolvedTargetPath, - Message: message, }) - return utils.NewToolResultError(string(payload)) + recovery := fmt.Sprintf( + `Target is outside this repository. Retarget link: allow_symlink_write=true. Replace with a file: push_files path=%q.`, + path, + ) + if resolvedTargetPath != "" { + recovery = fmt.Sprintf( + `Edit target: create_or_update_file path=%q. Retarget link: allow_symlink_write=true. Replace with a file: push_files path=%q.`, + resolvedTargetPath, + path, + ) + } + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: string(payload)}, + &mcp.TextContent{Text: recovery}, + }, + IsError: true, + } } func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (string, bool, *github.Response, error) { - ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+treeish) - if err != nil { - return "", false, resp, err - } - if resp != nil && resp.Body != nil { - _ = resp.Body.Close() - } - headSHA := ref.GetObject().GetSHA() - if headSHA == "" { - return "", false, nil, fmt.Errorf("branch %q has no commit SHA", treeish) - } - - entry, resp, err := getTreeEntry(ctx, client, owner, repo, headSHA, path) + entry, resp, err := getTreeEntry(ctx, client, owner, repo, treeish, path) if err != nil { return "", false, resp, err } @@ -157,6 +153,7 @@ func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo func getTreeEntry(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (*github.TreeEntry, *github.Response, error) { segments := strings.Split(pathpkg.Clean(strings.TrimPrefix(path, "/")), "/") + treeish = escapeGitTreeish(treeish) for i, segment := range segments { tree, resp, err := client.Git.GetTree(ctx, owner, repo, treeish, false) if err != nil { @@ -165,6 +162,9 @@ func getTreeEntry(ctx context.Context, client *github.Client, owner, repo, treei if resp != nil && resp.Body != nil { _ = resp.Body.Close() } + if tree.GetTruncated() { + return nil, resp, fmt.Errorf("git tree %q is truncated", treeish) + } var matched *github.TreeEntry for _, entry := range tree.Entries { @@ -187,6 +187,14 @@ func getTreeEntry(ctx context.Context, client *github.Client, owner, repo, treei return nil, nil, nil } +func escapeGitTreeish(treeish string) string { + segments := strings.Split(treeish, "/") + for i, segment := range segments { + segments[i] = url.PathEscape(segment) + } + return strings.Join(segments, "/") +} + func resolveRepositorySymlinkTarget(linkPath, target string) string { if pathpkg.IsAbs(target) { return "" diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index e32b05c2d7..c393139c2a 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -26,6 +26,16 @@ import ( "github.com/stretchr/testify/require" ) +type repositoryRequestCountingTransport struct { + inner http.RoundTripper + count int +} + +func (t *repositoryRequestCountingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.count++ + return t.inner.RoundTrip(req) +} + func Test_GetFileContents(t *testing.T) { // Verify tool definition once serverTool := GetFileContents(translations.NullTranslationHelper) @@ -1767,6 +1777,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { assert.Contains(t, schema.Properties, "message") assert.Contains(t, schema.Properties, "branch") assert.Contains(t, schema.Properties, "sha") + assert.Contains(t, schema.Properties, "allow_symlink_write") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "path", "content", "message", "branch"}) // Setup mock file content response @@ -1794,7 +1805,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { return func(w http.ResponseWriter, r *http.Request) { var tree *github.Tree switch { - case strings.HasSuffix(r.URL.Path, "/head-sha"): + case strings.HasSuffix(r.URL.Path, "/main"), strings.HasSuffix(r.URL.Path, "/release/#candidate"): tree = &github.Tree{ Entries: []*github.TreeEntry{ { @@ -1822,22 +1833,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { mockResponse(t, http.StatusOK, tree)(w, r) } } - mockBranchRef := mockResponse(t, http.StatusOK, &github.Reference{ - Ref: github.Ptr("refs/heads/main"), - Object: &github.GitObject{ - SHA: github.Ptr("head-sha"), - Type: github.Ptr("commit"), - }, - }) tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]any - expectError bool - expectedContent *github.RepositoryContentResponse - expectedErrMsg string - expectedErrMsgs []string + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedContent *github.RepositoryContentResponse + expectedErrMsg string + expectedErrMsgs []string + expectedRequestCount int }{ { name: "successful file creation", @@ -1865,13 +1870,13 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Add example file", "branch": "main", }, - expectError: false, - expectedContent: mockFileResponse, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 2, }, { name: "successful file update with SHA", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - GetReposGitRefByOwnerByRepoByRef: mockBranchRef, GetReposGitTreesByOwnerByRepoByTree: mockPathTree("100644"), "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), @@ -1907,8 +1912,9 @@ func Test_CreateOrUpdateFile(t *testing.T) { "branch": "main", "sha": "abc123def456", }, - expectError: false, - expectedContent: mockFileResponse, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 4, }, { name: "file creation fails", @@ -1930,13 +1936,13 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Invalid request", "branch": "nonexistent-branch", }, - expectError: true, - expectedErrMsg: "failed to create/update file", + expectError: true, + expectedErrMsg: "failed to create/update file", + expectedRequestCount: 2, }, { name: "sha validation - current sha matches", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - GetReposGitRefByOwnerByRepoByRef: mockBranchRef, GetReposGitTreesByOwnerByRepoByTree: mockPathTree("100644"), "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), @@ -1972,13 +1978,13 @@ func Test_CreateOrUpdateFile(t *testing.T) { "branch": "main", "sha": "abc123def456", }, - expectError: false, - expectedContent: mockFileResponse, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 4, }, { name: "rejects symbolic link update without explicit opt-in", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - GetReposGitRefByOwnerByRepoByRef: mockBranchRef, GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("other.md")) @@ -2002,24 +2008,22 @@ func Test_CreateOrUpdateFile(t *testing.T) { "sha": "abc123def456", }, expectError: true, - expectedErrMsg: `"error":"symlink_write_requires_explicit_opt_in"`, + expectedErrMsg: `"error":"symlink_write_requires_opt_in"`, expectedErrMsgs: []string{ - `"symlink_target":"other.md"`, - `"resolved_target_path":"docs/other.md"`, + `"target":"other.md"`, + `"resolved_path":"docs/other.md"`, + `create_or_update_file path="docs/other.md"`, + `allow_symlink_write=true`, + `push_files path="docs/example.md"`, }, + expectedRequestCount: 4, }, { - name: "resolves special-character branch before inspecting symlink", + name: "escapes special-character branch before inspecting symlink", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - GetReposGitRefByOwnerByRepoByRef: func(w http.ResponseWriter, r *http.Request) { - assert.Contains(t, r.URL.EscapedPath(), "/git/ref/heads/release%23candidate") - mockResponse(t, http.StatusOK, &github.Reference{ - Ref: github.Ptr("refs/heads/release#candidate"), - Object: &github.GitObject{ - SHA: github.Ptr("head-sha"), - Type: github.Ptr("commit"), - }, - })(w, r) + "GET /repos/owner/repo/git/trees/release/#candidate": func(w http.ResponseWriter, r *http.Request) { + assert.Contains(t, r.URL.EscapedPath(), "/git/trees/release/%23candidate") + mockPathTree("120000")(w, r) }, GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { @@ -2040,11 +2044,12 @@ func Test_CreateOrUpdateFile(t *testing.T) { "path": "docs/example.md", "content": "# Content returned by get_file_contents", "message": "Update linked file", - "branch": "release#candidate", + "branch": "release/#candidate", "sha": "abc123def456", }, - expectError: true, - expectedErrMsg: `"error":"symlink_write_requires_explicit_opt_in"`, + expectError: true, + expectedErrMsg: `"error":"symlink_write_requires_opt_in"`, + expectedRequestCount: 4, }, { name: "allows intentional symbolic link update with explicit opt-in", @@ -2084,8 +2089,69 @@ func Test_CreateOrUpdateFile(t *testing.T) { "sha": "abc123def456", "allow_symlink_write": true, }, - expectError: false, - expectedContent: mockFileResponse, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 2, + }, + { + name: "rejects explicit symbolic link response without tree inspection", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("symlink"), + Target: github.Ptr("../../outside"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("symlink"), + Target: github.Ptr("../../outside"), + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "docs/example.md", + "content": "new-target", + "message": "Update linked file", + "branch": "main", + "sha": "abc123def456", + }, + expectError: true, + expectedErrMsg: `"target":"../../outside"`, + expectedErrMsgs: []string{ + `"error":"symlink_write_requires_opt_in"`, + `Target is outside this repository`, + `push_files path="docs/example.md"`, + }, + expectedRequestCount: 1, + }, + { + name: "fails closed when git tree is truncated", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ + Truncated: github.Ptr(true), + }), + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "docs/example.md", + "content": "updated", + "message": "Update file", + "branch": "main", + "sha": "abc123def456", + }, + expectError: true, + expectedErrMsg: "failed to verify whether file path is a symbolic link", + expectedRequestCount: 2, }, { name: "sha validation - stale sha detected", @@ -2108,8 +2174,9 @@ func Test_CreateOrUpdateFile(t *testing.T) { "branch": "main", "sha": "oldsha123456", }, - expectError: true, - expectedErrMsg: "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888", + expectError: true, + expectedErrMsg: "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888", + expectedRequestCount: 1, }, { name: "sha validation - file doesn't exist (404), proceed with create", @@ -2146,8 +2213,9 @@ func Test_CreateOrUpdateFile(t *testing.T) { "branch": "main", "sha": "ignoredsha", }, - expectError: false, - expectedContent: mockFileResponse, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 2, }, { name: "no sha provided - file exists, rejects update", @@ -2169,8 +2237,9 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Update without SHA", "branch": "main", }, - expectError: true, - expectedErrMsg: "File already exists at docs/example.md", + expectError: true, + expectedErrMsg: "File already exists at docs/example.md", + expectedRequestCount: 1, }, { name: "no sha provided - file doesn't exist, no warning", @@ -2204,15 +2273,17 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Create new file", "branch": "main", }, - expectError: false, - expectedContent: mockFileResponse, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 2, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup client with mock - client := mustNewGHClient(t, tc.mockedClient) + requestCounter := &repositoryRequestCountingTransport{inner: tc.mockedClient.Transport} + client := mustNewGHClient(t, &http.Client{Transport: requestCounter}) deps := BaseDeps{ Client: client, } @@ -2223,15 +2294,24 @@ func Test_CreateOrUpdateFile(t *testing.T) { // Call handler result, err := handler(ContextWithDeps(context.Background(), deps), &request) + if tc.expectedRequestCount > 0 { + assert.Equal(t, tc.expectedRequestCount, requestCounter.count) + } // Verify results if tc.expectError { require.NoError(t, err) require.True(t, result.IsError) - errorContent := getErrorResult(t, result) - assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + var errorText strings.Builder + for _, content := range result.Content { + textContent, ok := content.(*mcp.TextContent) + require.True(t, ok, "expected error content to be TextContent") + errorText.WriteString(textContent.Text) + errorText.WriteByte('\n') + } + assert.Contains(t, errorText.String(), tc.expectedErrMsg) for _, expectedErrMsg := range tc.expectedErrMsgs { - assert.Contains(t, errorContent.Text, expectedErrMsg) + assert.Contains(t, errorText.String(), expectedErrMsg) } return } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index a720ef365a..f9b51159b5 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -38,11 +38,10 @@ var ( InstructionsFunc: generateContextToolsetInstructions, } ToolsetMetadataRepos = inventory.ToolsetMetadata{ - ID: "repos", - Description: "GitHub Repository related tools", - Default: true, - Icon: "repo", - InstructionsFunc: generateReposToolsetInstructions, + ID: "repos", + Description: "GitHub Repository related tools", + Default: true, + Icon: "repo", } ToolsetMetadataGit = inventory.ToolsetMetadata{ ID: "git", diff --git a/pkg/github/toolset_instructions.go b/pkg/github/toolset_instructions.go index 01a4cab848..3b3a54eadd 100644 --- a/pkg/github/toolset_instructions.go +++ b/pkg/github/toolset_instructions.go @@ -9,12 +9,6 @@ func generateContextToolsetInstructions(_ *inventory.Inventory) string { return "Always call 'get_me' first to understand current user permissions and context." } -func generateReposToolsetInstructions(_ *inventory.Inventory) string { - return `## Repository file writes - -'get_file_contents' may return the target contents when a path is a symbolic link, but repository file writes use exact Git paths and do not follow symbolic links. To edit content that a symlink points to, write to the target path. 'create_or_update_file' rejects writes to existing symbolic links unless 'allow_symlink_write' is true; only opt in when intentionally changing the link target.` -} - func generateIssuesToolsetInstructions(_ *inventory.Inventory) string { return `## Issues diff --git a/pkg/github/toolset_instructions_test.go b/pkg/github/toolset_instructions_test.go deleted file mode 100644 index 73dc070992..0000000000 --- a/pkg/github/toolset_instructions_test.go +++ /dev/null @@ -1,92 +0,0 @@ -package github - -import ( - "strings" - "testing" - - "github.com/github/github-mcp-server/pkg/inventory" - "github.com/github/github-mcp-server/pkg/translations" - "github.com/google/jsonschema-go/jsonschema" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestRepositoryInstructionsExplainSymlinkWriteSemantics(t *testing.T) { - t.Setenv("DISABLE_INSTRUCTIONS", "false") - - reposInventory, err := inventory.NewBuilder(). - SetTools([]inventory.ServerTool{{Toolset: ToolsetMetadataRepos}}). - WithToolsets([]string{"repos"}). - WithServerInstructions(). - Build() - require.NoError(t, err) - - instructions := strings.ToLower(reposInventory.Instructions()) - assert.Contains(t, instructions, "## repository file writes") - assert.Contains(t, instructions, "may return the target contents") - assert.Contains(t, instructions, "do not follow symbolic links") - assert.Contains(t, instructions, "rejects writes to existing symbolic links") - - defaultInventory, err := inventory.NewBuilder(). - SetTools([]inventory.ServerTool{ - {Toolset: ToolsetMetadataContext}, - {Toolset: ToolsetMetadataRepos}, - }). - WithToolsets([]string{"default"}). - WithServerInstructions(). - Build() - require.NoError(t, err) - assert.Contains(t, strings.ToLower(defaultInventory.Instructions()), "## repository file writes") - - contextInventory, err := inventory.NewBuilder(). - SetTools([]inventory.ServerTool{{Toolset: ToolsetMetadataContext}}). - WithToolsets([]string{"context"}). - WithServerInstructions(). - Build() - require.NoError(t, err) - assert.NotContains(t, strings.ToLower(contextInventory.Instructions()), "## repository file writes") -} - -func TestFileWritePathsExplainSymlinkWriteSemantics(t *testing.T) { - createSchema, ok := CreateOrUpdateFile(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok) - pushSchema, ok := PushFiles(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok) - createPath := createSchema.Properties["path"] - require.NotNil(t, createPath) - createAllowSymlinkWrite := createSchema.Properties["allow_symlink_write"] - require.NotNil(t, createAllowSymlinkWrite) - pushFiles := pushSchema.Properties["files"] - require.NotNil(t, pushFiles) - require.NotNil(t, pushFiles.Items) - pushPath := pushFiles.Items.Properties["path"] - require.NotNil(t, pushPath) - - tools := []struct { - name string - description string - expectedBehavior string - }{ - { - name: "create_or_update_file", - description: createPath.Description, - expectedBehavior: "changes the link target", - }, - { - name: "push_files", - description: pushPath.Description, - expectedBehavior: "replaces the link with a regular file", - }, - } - - for _, tool := range tools { - t.Run(tool.name, func(t *testing.T) { - description := strings.ToLower(tool.description) - assert.Contains(t, description, "exact git path") - assert.Contains(t, description, tool.expectedBehavior) - }) - } - - assert.Equal(t, "boolean", createAllowSymlinkWrite.Type) - assert.Contains(t, strings.ToLower(createAllowSymlinkWrite.Description), "intentionally change a symbolic link's target") -} From 9f92437419fc4b6c907835c96d88f31547b18d66 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 11:38:33 +0200 Subject: [PATCH 4/6] fix(repos): disclose dereferenced symlink reads Detect internal symlink dereferences from Git blob identity mismatches, disclose explicit links and submodules, and preserve requested-path resource output. Use bounded exact-path tree inspection only when inline content is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/github/repositories.go | 77 ++- pkg/github/repositories_helper.go | 209 +++++++- pkg/github/repositories_test.go | 766 +++++++++++++++++++++++++++++- 3 files changed, 1010 insertions(+), 42 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index c632a93aa7..1399207cdc 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1071,23 +1071,48 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if fallbackUsed { successNote = fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.", originalRef, rawOpts.Ref) } + const maxContentSize = 1024 * 1024 // 1MB + + inspection, respInspect, err := inspectRepositoryFile(ctx, client, owner, repo, ref, path, fileContent) + if err != nil { + if respInspect != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to inspect repository file", + respInspect, + err, + ), nil, nil + } + return utils.NewToolResultError(fmt.Sprintf("failed to inspect repository file: %s", err)), nil, nil + } + if inspection.Submodule != nil { + return attachIFC(utils.NewToolResultText(marshalRepositorySubmoduleMetadata(inspection.Submodule))), nil, nil + } + if inspection.Symlink != nil && + !inspection.ContentAvailable && + (inspection.Symlink.Explicit || fileSize < maxContentSize) { + return attachIFC(utils.NewToolResultText(marshalRepositorySymlinkMetadata( + inspection.Symlink, + unavailableSymlinkContents, + successNote, + ))), nil, nil + } - // Empty files (0 bytes) have no content to decode; return - // them directly as empty text to avoid errors from - // GetContent when the API returns null content with a - // base64 encoding field, and to avoid DetectContentType - // misclassifying them as binary. - if fileSize == 0 { + // Empty files are returned as empty text to avoid + // DetectContentType misclassifying them as binary. + if fileSize == 0 && inspection.ContentAvailable { result := &mcp.ResourceContents{ URI: resourceURI, Text: "", MIMEType: "text/plain", } - return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil + message := fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote) + if inspection.Symlink != nil { + message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + } + return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } // For files >= 1MB, return a ResourceLink instead of content - const maxContentSize = 1024 * 1024 // 1MB if fileSize >= maxContentSize { size := int64(fileSize) resourceLink := &mcp.ResourceLink{ @@ -1096,22 +1121,28 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool Title: fmt.Sprintf("File: %s", path), Size: &size, } + message := 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) + if inspection.Symlink != nil { + targetPath := inspection.Symlink.ResolvedTargetPath + if targetPath == "" { + targetPath = inspection.Symlink.Target + } + resourceLink.Title = fmt.Sprintf("Dereferenced target %s via symlink %s", targetPath, path) + message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + } 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), + message, resourceLink)), nil, nil } - - // For files < 1MB, get content directly from Contents API - content, err := fileContent.GetContent() - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to decode file content: %s", err)), nil, nil + if !inspection.ContentAvailable { + return utils.NewToolResultError(fmt.Sprintf("failed to inspect repository file: Contents API did not provide content for path %q", path)), nil, nil } // Detect content type from the actual content bytes, // mirroring the original approach of using the Content-Type header // from the raw API response. - contentBytes := []byte(content) + contentBytes := inspection.Content contentType := http.DetectContentType(contentBytes) // Determine if content is text or binary based on detected content type @@ -1124,10 +1155,14 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if isTextContent { result := &mcp.ResourceContents{ URI: resourceURI, - Text: content, + Text: string(contentBytes), MIMEType: contentType, } - return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil + message := fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote) + if inspection.Symlink != nil { + message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + } + return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } result := &mcp.ResourceContents{ @@ -1135,7 +1170,11 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool Blob: contentBytes, MIMEType: contentType, } - return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil + message := fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote) + if inspection.Symlink != nil { + message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + } + return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } else if dirContent != nil { // file content or file SHA is nil which means it's a directory filtered := false diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index ac30911ba5..2bc34db3dd 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -1,13 +1,17 @@ package github import ( + "bytes" "context" + "crypto/sha1" //nolint:gosec // Git object IDs are defined using SHA-1. + "encoding/hex" "encoding/json" "fmt" "net/http" "net/url" pathpkg "path" "strings" + "unicode/utf8" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/raw" @@ -92,7 +96,45 @@ func createReferenceFromDefaultBranch(ctx context.Context, client *github.Client return createdRef, nil } -const gitSymlinkMode = "120000" +const ( + gitSymlinkMode = "120000" + gitSubmoduleMode = "160000" + maxGitTreeTraversalDepth = 64 + dereferencedContentLabel = "dereferenced_target" + unavailableSymlinkContents = "not_returned" +) + +type repositorySymlink struct { + Path string + SHA string + Target string + ResolvedTargetPath string + Explicit bool +} + +type repositoryFileInspection struct { + Content []byte + ContentAvailable bool + Symlink *repositorySymlink + Submodule *repositorySubmoduleReadMetadata +} + +type repositorySymlinkReadMetadata struct { + Type string `json:"type"` + Path string `json:"path"` + SHA string `json:"sha,omitempty"` + Target string `json:"target"` + ResolvedTargetPath string `json:"resolved_path,omitempty"` + Content string `json:"content"` + Note string `json:"note,omitempty"` +} + +type repositorySubmoduleReadMetadata struct { + Type string `json:"type"` + Path string `json:"path"` + SHA string `json:"sha,omitempty"` + GitURL string `json:"git_url,omitempty"` +} type symlinkWriteBlockedError struct { Error string `json:"error"` @@ -129,6 +171,163 @@ func newSymlinkWriteBlockedResult(path, target string) *mcp.CallToolResult { } } +func inspectRepositoryFile(ctx context.Context, client *github.Client, owner, repo, treeish, path string, file *github.RepositoryContent) (*repositoryFileInspection, *github.Response, error) { + if file.GetType() == "symlink" { + content, available, err := suppliedRepositoryContent(file) + if err != nil { + return nil, nil, err + } + return &repositoryFileInspection{ + Content: content, + ContentAvailable: available, + Symlink: newRepositorySymlink(path, file.GetSHA(), file.GetTarget(), true), + }, nil, nil + } + + if file.GetType() == "submodule" || file.GetSubmoduleGitURL() != "" { + return &repositoryFileInspection{ + Submodule: &repositorySubmoduleReadMetadata{ + Type: "submodule", + Path: path, + SHA: file.GetSHA(), + GitURL: file.GetSubmoduleGitURL(), + }, + }, nil, nil + } + + content, available, err := suppliedRepositoryContent(file) + if err != nil { + return nil, nil, err + } + if available { + if !looksLikeSHA(file.GetSHA()) { + return nil, nil, fmt.Errorf("contents API returned malformed Git blob SHA %q for path %q", file.GetSHA(), path) + } + if strings.EqualFold(gitBlobSHA1(content), file.GetSHA()) { + return &repositoryFileInspection{Content: content, ContentAvailable: true}, nil, nil + } + + target, resp, err := symlinkTargetFromBlob(ctx, client, owner, repo, file.GetSHA()) + if err != nil { + return nil, resp, fmt.Errorf("contents API bytes did not match the reported Git blob and the path blob was not a valid symbolic link target: %w", err) + } + return &repositoryFileInspection{ + Content: content, + ContentAvailable: true, + Symlink: newRepositorySymlink(path, file.GetSHA(), target, false), + }, nil, nil + } + + entry, resp, err := getTreeEntry(ctx, client, owner, repo, treeish, path) + if err != nil { + return nil, resp, err + } + if entry == nil { + return nil, nil, fmt.Errorf("path %q exists according to the Contents API but was not found in the Git tree", path) + } + if !looksLikeSHA(file.GetSHA()) || !strings.EqualFold(file.GetSHA(), entry.GetSHA()) { + return nil, nil, fmt.Errorf("contents API blob SHA %q does not match Git tree blob SHA %q for path %q", file.GetSHA(), entry.GetSHA(), path) + } + + switch entry.GetMode() { + case gitSymlinkMode: + target, resp, err := symlinkTargetFromBlob(ctx, client, owner, repo, entry.GetSHA()) + if err != nil { + return nil, resp, err + } + return &repositoryFileInspection{ + Symlink: newRepositorySymlink(path, entry.GetSHA(), target, false), + }, nil, nil + case gitSubmoduleMode: + return &repositoryFileInspection{ + Submodule: &repositorySubmoduleReadMetadata{ + Type: "submodule", + Path: path, + SHA: entry.GetSHA(), + }, + }, nil, nil + default: + return &repositoryFileInspection{}, nil, nil + } +} + +func suppliedRepositoryContent(file *github.RepositoryContent) ([]byte, bool, error) { + if file.Content != nil { + content, err := file.GetContent() + if err != nil { + return nil, false, fmt.Errorf("failed to decode file content: %w", err) + } + return []byte(content), true, nil + } + if file.GetType() != "symlink" && file.GetSize() == 0 { + return []byte{}, true, nil + } + return nil, false, nil +} + +func gitBlobSHA1(content []byte) string { + hasher := sha1.New() //nolint:gosec // SHA-1 is required by the Git object ID format. + _, _ = fmt.Fprintf(hasher, "blob %d\x00", len(content)) + _, _ = hasher.Write(content) + return hex.EncodeToString(hasher.Sum(nil)) +} + +func symlinkTargetFromBlob(ctx context.Context, client *github.Client, owner, repo, sha string) (string, *github.Response, error) { + target, resp, err := gitBlobBytes(ctx, client, owner, repo, sha) + if err != nil { + return "", resp, err + } + if len(target) == 0 || !utf8.Valid(target) || bytes.IndexByte(target, 0) >= 0 { + return "", nil, fmt.Errorf("git blob %q is not a valid symbolic link target", sha) + } + return string(target), nil, nil +} + +func gitBlobBytes(ctx context.Context, client *github.Client, owner, repo, sha string) ([]byte, *github.Response, error) { + if !looksLikeSHA(sha) { + return nil, nil, fmt.Errorf("malformed Git blob SHA %q", sha) + } + content, resp, err := client.Git.GetBlobRaw(ctx, owner, repo, sha) + if err != nil { + return nil, resp, err + } + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + if !strings.EqualFold(gitBlobSHA1(content), sha) { + return nil, nil, fmt.Errorf("blob bytes returned by the Git Blobs API do not match SHA %q", sha) + } + return content, nil, nil +} + +func newRepositorySymlink(path, sha, target string, explicit bool) *repositorySymlink { + return &repositorySymlink{ + Path: path, + SHA: sha, + Target: target, + ResolvedTargetPath: resolveRepositorySymlinkTarget(path, target), + Explicit: explicit, + } +} + +func marshalRepositorySymlinkMetadata(link *repositorySymlink, content, note string) string { + payload, _ := json.Marshal(repositorySymlinkReadMetadata{ + Type: "symlink", + Path: link.Path, + SHA: link.SHA, + Target: link.Target, + ResolvedTargetPath: link.ResolvedTargetPath, + Content: content, + Note: strings.TrimSpace(note), + }) + return string(payload) +} + +func marshalRepositorySubmoduleMetadata(submodule *repositorySubmoduleReadMetadata) string { + payload, _ := json.Marshal(submodule) + return string(payload) +} + func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (string, bool, *github.Response, error) { entry, resp, err := getTreeEntry(ctx, client, owner, repo, treeish, path) if err != nil { @@ -141,18 +340,18 @@ func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo return "", false, nil, nil } - target, resp, err := client.Git.GetBlobRaw(ctx, owner, repo, entry.GetSHA()) + target, resp, err := gitBlobBytes(ctx, client, owner, repo, entry.GetSHA()) if err != nil { return "", false, resp, err } - if resp != nil && resp.Body != nil { - _ = resp.Body.Close() - } return string(target), true, nil, nil } func getTreeEntry(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (*github.TreeEntry, *github.Response, error) { segments := strings.Split(pathpkg.Clean(strings.TrimPrefix(path, "/")), "/") + if len(segments) > maxGitTreeTraversalDepth { + return nil, nil, fmt.Errorf("path %q exceeds the maximum Git tree traversal depth of %d", path, maxGitTreeTraversalDepth) + } treeish = escapeGitTreeish(treeish) for i, segment := range segments { tree, resp, err := client.Git.GetTree(ctx, owner, repo, treeish, false) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index c393139c2a..1602369b12 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/json" + "maps" "net/http" "net/url" "strings" @@ -36,6 +37,16 @@ func (t *repositoryRequestCountingTransport) RoundTrip(req *http.Request) (*http return t.inner.RoundTrip(req) } +func parseRepositoryPathMetadata(t *testing.T, result *mcp.CallToolResult) map[string]any { + t.Helper() + require.NotEmpty(t, result.Content) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected first result content to be TextContent") + var metadata map[string]any + require.NoError(t, json.Unmarshal([]byte(text.Text), &metadata)) + return metadata +} + func Test_GetFileContents(t *testing.T) { // Verify tool definition once serverTool := GetFileContents(translations.NullTranslationHelper) @@ -57,6 +68,9 @@ func Test_GetFileContents(t *testing.T) { // Mock response for raw content mockRawContent := []byte("# Test Repository\n\nThis is a test repository.") + mockPNGContent := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01") + mockPDFContent := []byte("%PDF-1.4 fake pdf content") + largeFileSHA := strings.Repeat("a", 40) // Setup mock directory content for success case mockDirContent := []*github.RepositoryContent{ @@ -97,7 +111,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr("abc123"), + SHA: github.Ptr(gitBlobSHA1(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(string(mockRawContent)), Size: github.Ptr(len(mockRawContent)), @@ -127,15 +141,14 @@ func Test_GetFileContents(t *testing.T) { GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) // PNG magic bytes followed by some data - pngContent := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01") - encodedContent := base64.StdEncoding.EncodeToString(pngContent) + encodedContent := base64.StdEncoding.EncodeToString(mockPNGContent) fileContent := &github.RepositoryContent{ Name: github.Ptr("test.png"), Path: github.Ptr("test.png"), - SHA: github.Ptr("def456"), + SHA: github.Ptr(gitBlobSHA1(mockPNGContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), - Size: github.Ptr(len(pngContent)), + Size: github.Ptr(len(mockPNGContent)), Encoding: github.Ptr("base64"), } contentBytes, _ := json.Marshal(fileContent) @@ -163,15 +176,14 @@ func Test_GetFileContents(t *testing.T) { GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) // PDF magic bytes - pdfContent := []byte("%PDF-1.4 fake pdf content") - encodedContent := base64.StdEncoding.EncodeToString(pdfContent) + encodedContent := base64.StdEncoding.EncodeToString(mockPDFContent) fileContent := &github.RepositoryContent{ Name: github.Ptr("document.pdf"), Path: github.Ptr("document.pdf"), - SHA: github.Ptr("pdf123"), + SHA: github.Ptr(gitBlobSHA1(mockPDFContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), - Size: github.Ptr(len(pdfContent)), + Size: github.Ptr(len(mockPDFContent)), Encoding: github.Ptr("base64"), } contentBytes, _ := json.Marshal(fileContent) @@ -223,7 +235,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr("abc123"), + SHA: github.Ptr(gitBlobSHA1(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -307,7 +319,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr("abc123"), + SHA: github.Ptr(gitBlobSHA1(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -336,13 +348,21 @@ func Test_GetFileContents(t *testing.T) { mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + "GET /repos/owner/repo/git/trees/refs/heads/main": mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("large-file.bin"), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + SHA: github.Ptr(largeFileSHA), + }}, + }), GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) // File larger than 1MB - Contents API returns metadata but no content fileContent := &github.RepositoryContent{ Name: github.Ptr("large-file.bin"), Path: github.Ptr("large-file.bin"), - SHA: github.Ptr("largesha123"), + SHA: github.Ptr(largeFileSHA), Type: github.Ptr("file"), Size: github.Ptr(2 * 1024 * 1024), // 2MB DownloadURL: github.Ptr("https://raw.githubusercontent.com/owner/repo/main/large-file.bin"), @@ -374,7 +394,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr(".gitkeep"), Path: github.Ptr(".gitkeep"), - SHA: github.Ptr("empty123"), + SHA: github.Ptr(gitBlobSHA1(nil)), Type: github.Ptr("file"), Content: nil, Size: github.Ptr(0), @@ -522,6 +542,660 @@ func Test_GetFileContents(t *testing.T) { } } +func Test_GetFileContents_SymlinkDisclosureRequestCounts(t *testing.T) { + serverTool := GetFileContents(translations.NullTranslationHelper) + commitSHA := strings.Repeat("c", 40) + textTargetContent := []byte("resolved text content") + binaryTargetContent := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR") + + tests := []struct { + name string + path string + handlers func(*testing.T) map[string]http.HandlerFunc + expectedRequestCount int + check func(*testing.T, *mcp.CallToolResult) + }{ + { + name: "normal small text file remains one request", + path: "README.md", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("README.md"), + Path: github.Ptr("README.md"), + SHA: github.Ptr(gitBlobSHA1(textTargetContent)), + Type: github.Ptr("file"), + Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), + Size: github.Ptr(len(textTargetContent)), + Encoding: github.Ptr("base64"), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + assert.Equal(t, string(textTargetContent), getResourceResult(t, result).Text) + assert.Contains(t, result.Content[0].(*mcp.TextContent).Text, "successfully downloaded text file") + }, + }, + { + name: "internal text symlink adds one blob request", + path: "docs/link.txt", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + target := "../target.txt" + linkSHA := gitBlobSHA1([]byte(target)) + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("link.txt"), + Path: github.Ptr("docs/link.txt"), + SHA: github.Ptr(linkSHA), + Type: github.Ptr("file"), + Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), + Size: github.Ptr(len(textTargetContent)), + Encoding: github.Ptr("base64"), + }), + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(target)) + }, + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "symlink", metadata["type"]) + assert.Equal(t, "docs/link.txt", metadata["path"]) + assert.Equal(t, "../target.txt", metadata["target"]) + assert.Equal(t, "target.txt", metadata["resolved_path"]) + assert.Equal(t, dereferencedContentLabel, metadata["content"]) + resource := getResourceResult(t, result) + assert.Equal(t, string(textTargetContent), resource.Text) + assert.Equal(t, "text/plain; charset=utf-8", resource.MIMEType) + assert.Contains(t, resource.URI, "/contents/docs/link.txt") + }, + }, + { + name: "internal binary symlink preserves blob and MIME type", + path: "assets/current.png", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + target := "images/logo.png" + linkSHA := gitBlobSHA1([]byte(target)) + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("current.png"), + Path: github.Ptr("assets/current.png"), + SHA: github.Ptr(linkSHA), + Type: github.Ptr("file"), + Content: github.Ptr(base64.StdEncoding.EncodeToString(binaryTargetContent)), + Size: github.Ptr(len(binaryTargetContent)), + Encoding: github.Ptr("base64"), + }), + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(target)) + }, + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "assets/images/logo.png", metadata["resolved_path"]) + resource := getResourceResult(t, result) + assert.Equal(t, binaryTargetContent, resource.Blob) + assert.Equal(t, "image/png", resource.MIMEType) + }, + }, + { + name: "explicit dangling symlink discloses without fetching", + path: "docs/missing", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + target := "missing.txt" + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("missing"), + Path: github.Ptr("docs/missing"), + SHA: github.Ptr(gitBlobSHA1([]byte(target))), + Type: github.Ptr("symlink"), + Target: github.Ptr(target), + Size: github.Ptr(0), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "symlink", metadata["type"]) + assert.Equal(t, "docs/missing.txt", metadata["resolved_path"]) + assert.Equal(t, unavailableSymlinkContents, metadata["content"]) + require.Len(t, result.Content, 1) + }, + }, + { + name: "explicit outside symlink omits repository resolved path", + path: "docs/outside", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + target := "../../outside" + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("outside"), + Path: github.Ptr("docs/outside"), + SHA: github.Ptr(gitBlobSHA1([]byte(target))), + Type: github.Ptr("symlink"), + Target: github.Ptr(target), + Size: github.Ptr(0), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "../../outside", metadata["target"]) + assert.NotContains(t, metadata, "resolved_path") + }, + }, + { + name: "explicit symlink returns content only when supplied", + path: "docs/explicit", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + target := "target.txt" + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("explicit"), + Path: github.Ptr("docs/explicit"), + SHA: github.Ptr(gitBlobSHA1([]byte(target))), + Type: github.Ptr("symlink"), + Target: github.Ptr(target), + Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), + Encoding: github.Ptr("base64"), + Size: github.Ptr(len(textTargetContent)), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, dereferencedContentLabel, metadata["content"]) + assert.Equal(t, string(textTargetContent), getResourceResult(t, result).Text) + }, + }, + { + name: "explicit symlink without content never fabricates a large resource", + path: "docs/pathological", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + target := "target.txt" + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("pathological"), + Path: github.Ptr("docs/pathological"), + SHA: github.Ptr(gitBlobSHA1([]byte(target))), + Type: github.Ptr("symlink"), + Target: github.Ptr(target), + Size: github.Ptr(2 * 1024 * 1024), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, unavailableSymlinkContents, metadata["content"]) + require.Len(t, result.Content, 1) + }, + }, + { + name: "submodule response is not classified as symlink", + path: "vendor/dependency", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("dependency"), + Path: github.Ptr("vendor/dependency"), + SHA: github.Ptr(strings.Repeat("d", 40)), + Type: github.Ptr("file"), + SubmoduleGitURL: github.Ptr("https://github.com/example/dependency.git"), + Size: github.Ptr(0), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "submodule", metadata["type"]) + assert.Equal(t, "vendor/dependency", metadata["path"]) + assert.Equal(t, "https://github.com/example/dependency.git", metadata["git_url"]) + assert.NotContains(t, metadata, "target") + }, + }, + { + name: "malformed SHA fails closed without another request", + path: "README.md", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("README.md"), + Path: github.Ptr("README.md"), + SHA: github.Ptr("not-a-git-object-id"), + Type: github.Ptr("file"), + Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), + Size: github.Ptr(len(textTargetContent)), + Encoding: github.Ptr("base64"), + }), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + assert.Contains(t, getErrorResult(t, result).Text, "malformed Git blob SHA") + }, + }, + { + name: "anomalous mismatch is not mislabeled as symlink", + path: "README.md", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + pathBlob := []byte("invalid\x00target") + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("README.md"), + Path: github.Ptr("README.md"), + SHA: github.Ptr(gitBlobSHA1(pathBlob)), + Type: github.Ptr("file"), + Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), + Size: github.Ptr(len(textTargetContent)), + Encoding: github.Ptr("base64"), + }), + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(pathBlob) + }, + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + text := getErrorResult(t, result).Text + assert.Contains(t, text, "not a valid symbolic link target") + assert.NotContains(t, text, `"type":"symlink"`) + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + handlers := tc.handlers(t) + if contentsHandler, ok := handlers[GetReposContentsByOwnerByRepoByPath]; ok { + handlers["GET /repos/{owner}/{repo}/contents/{path:.*}"] = contentsHandler + } + mockedClient := MockHTTPClientWithHandlers(handlers) + counter := &repositoryRequestCountingTransport{inner: mockedClient.Transport} + client := mustNewGHClient(t, &http.Client{Transport: counter}) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "path": tc.path, + "sha": commitSHA, + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.Equal(t, tc.expectedRequestCount, counter.count) + tc.check(t, result) + }) + } +} + +func Test_GetFileContents_ContentlessRequestCounts(t *testing.T) { + serverTool := GetFileContents(translations.NullTranslationHelper) + commitSHA := strings.Repeat("c", 40) + largeFileSHA := strings.Repeat("a", 40) + linkTarget := "../targets/large.bin" + linkSHA := gitBlobSHA1([]byte(linkTarget)) + const largeFileSize = 2 * 1024 * 1024 + + tests := []struct { + name string + path string + handlers func(*testing.T) map[string]http.HandlerFunc + expectedRequestCount int + check func(*testing.T, *mcp.CallToolResult) + }{ + { + name: "normal large file uses one exact-path tree request", + path: "large.bin", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("large.bin"), + Path: github.Ptr("large.bin"), + SHA: github.Ptr(largeFileSHA), + Type: github.Ptr("file"), + Size: github.Ptr(largeFileSize), + DownloadURL: github.Ptr("https://raw.example.com/large.bin"), + }), + GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("large.bin"), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + SHA: github.Ptr(largeFileSHA), + }}, + }), + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + require.Len(t, result.Content, 2) + link, ok := result.Content[1].(*mcp.ResourceLink) + require.True(t, ok) + assert.Equal(t, "File: large.bin", link.Title) + }, + }, + { + name: "internal large symlink uses bounded tree descent and blob request", + path: "docs/current.bin", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, commitSHA, r.URL.Query().Get("ref")) + mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("current.bin"), + Path: github.Ptr("docs/current.bin"), + SHA: github.Ptr(linkSHA), + Type: github.Ptr("file"), + Size: github.Ptr(largeFileSize), + DownloadURL: github.Ptr("https://raw.example.com/docs/current.bin"), + })(w, r) + }, + GetReposGitTreesByOwnerByRepoByTree: func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/"+commitSHA): + mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("docs"), + Mode: github.Ptr("040000"), + Type: github.Ptr("tree"), + SHA: github.Ptr("docs-tree"), + }}, + })(w, r) + case strings.HasSuffix(r.URL.Path, "/docs-tree"): + mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("current.bin"), + Mode: github.Ptr(gitSymlinkMode), + Type: github.Ptr("blob"), + SHA: github.Ptr(linkSHA), + }}, + })(w, r) + default: + require.FailNow(t, "unexpected tree request", r.URL.Path) + } + }, + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(linkTarget)) + }, + } + }, + expectedRequestCount: 4, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "symlink", metadata["type"]) + assert.Equal(t, "targets/large.bin", metadata["resolved_path"]) + assert.Equal(t, dereferencedContentLabel, metadata["content"]) + require.Len(t, result.Content, 2) + link, ok := result.Content[1].(*mcp.ResourceLink) + require.True(t, ok) + assert.Equal(t, "Dereferenced target targets/large.bin via symlink docs/current.bin", link.Title) + assert.Contains(t, link.URI, "/contents/docs/current.bin") + }, + }, + { + name: "truncated tree fails closed", + path: "large.bin", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("large.bin"), + Path: github.Ptr("large.bin"), + SHA: github.Ptr(largeFileSHA), + Type: github.Ptr("file"), + Size: github.Ptr(largeFileSize), + }), + GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ + Truncated: github.Ptr(true), + }), + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + assert.Contains(t, getErrorResult(t, result).Text, "is truncated") + }, + }, + { + name: "contentless SHA mismatch fails closed", + path: "large.bin", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("large.bin"), + Path: github.Ptr("large.bin"), + SHA: github.Ptr(largeFileSHA), + Type: github.Ptr("file"), + Size: github.Ptr(largeFileSize), + }), + GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("large.bin"), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + SHA: github.Ptr(strings.Repeat("b", 40)), + }}, + }), + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + assert.Contains(t, getErrorResult(t, result).Text, "does not match Git tree blob SHA") + }, + }, + { + name: "tree-mode submodule fallback is explicit", + path: "vendor/dependency", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + submoduleSHA := strings.Repeat("d", 40) + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("dependency"), + Path: github.Ptr("vendor/dependency"), + SHA: github.Ptr(submoduleSHA), + Type: github.Ptr("file"), + Size: github.Ptr(1), + }), + GetReposGitTreesByOwnerByRepoByTree: func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/"+commitSHA): + mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("vendor"), + Mode: github.Ptr("040000"), + Type: github.Ptr("tree"), + SHA: github.Ptr("vendor-tree"), + }}, + })(w, r) + case strings.HasSuffix(r.URL.Path, "/vendor-tree"): + mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("dependency"), + Mode: github.Ptr(gitSubmoduleMode), + Type: github.Ptr("commit"), + SHA: github.Ptr(submoduleSHA), + }}, + })(w, r) + default: + require.FailNow(t, "unexpected tree request", r.URL.Path) + } + }, + } + }, + expectedRequestCount: 3, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + metadata := parseRepositoryPathMetadata(t, result) + assert.Equal(t, "submodule", metadata["type"]) + assert.Equal(t, "vendor/dependency", metadata["path"]) + assert.NotContains(t, metadata, "target") + }, + }, + { + name: "directory adds no inspection request", + path: "docs", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, []*github.RepositoryContent{{ + Name: github.Ptr("README.md"), + Path: github.Ptr("docs/README.md"), + SHA: github.Ptr(largeFileSHA), + Type: github.Ptr("file"), + }}), + } + }, + expectedRequestCount: 1, + check: func(t *testing.T, result *mcp.CallToolResult) { + require.False(t, result.IsError) + var entries []*github.RepositoryContent + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &entries)) + require.Len(t, entries, 1) + }, + }, + { + name: "missing path retains one recursive fallback request", + path: "missing.txt", + handlers: func(t *testing.T) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), + GetReposGitTreesByOwnerByRepoByTree: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "1", r.URL.Query().Get("recursive")) + mockResponse(t, http.StatusOK, &github.Tree{Entries: []*github.TreeEntry{}})(w, r) + }, + } + }, + expectedRequestCount: 2, + check: func(t *testing.T, result *mcp.CallToolResult) { + assert.Contains(t, getErrorResult(t, result).Text, "Failed to get file contents") + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + handlers := tc.handlers(t) + if contentsHandler, ok := handlers[GetReposContentsByOwnerByRepoByPath]; ok { + handlers["GET /repos/{owner}/{repo}/contents/{path:.*}"] = contentsHandler + } + mockedClient := MockHTTPClientWithHandlers(handlers) + counter := &repositoryRequestCountingTransport{inner: mockedClient.Transport} + client := mustNewGHClient(t, &http.Client{Transport: counter}) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "path": tc.path, + "sha": commitSHA, + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.Equal(t, tc.expectedRequestCount, counter.count) + tc.check(t, result) + }) + } +} + +func Test_GetFileContents_ContentlessTreeUsesRequestedRevision(t *testing.T) { + serverTool := GetFileContents(translations.NullTranslationHelper) + blobSHA := strings.Repeat("a", 40) + const fileSize = 2 * 1024 * 1024 + + tests := []struct { + name string + args map[string]any + expectedRef string + treePath string + refHandler http.HandlerFunc + expectedRequestCount int + }{ + { + name: "commit SHA", + args: map[string]any{"sha": strings.Repeat("c", 40)}, + expectedRef: strings.Repeat("c", 40), + treePath: "/repos/owner/repo/git/trees/" + strings.Repeat("c", 40), + expectedRequestCount: 2, + }, + { + name: "fully qualified branch resolves to one commit", + args: map[string]any{"ref": "refs/heads/release"}, + expectedRef: strings.Repeat("d", 40), + treePath: "/repos/owner/repo/git/trees/" + strings.Repeat("d", 40), + refHandler: mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/release"), + Object: &github.GitObject{ + SHA: github.Ptr(strings.Repeat("d", 40)), + }, + }), + expectedRequestCount: 3, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + handlers := map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, tc.expectedRef, r.URL.Query().Get("ref")) + mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("large.bin"), + Path: github.Ptr("large.bin"), + SHA: github.Ptr(blobSHA), + Type: github.Ptr("file"), + Size: github.Ptr(fileSize), + })(w, r) + }, + "GET " + tc.treePath: mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: github.Ptr("large.bin"), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + SHA: github.Ptr(blobSHA), + }}, + }), + } + if tc.refHandler != nil { + handlers[GetReposGitRefByOwnerByRepoByRef] = tc.refHandler + } + mockedClient := MockHTTPClientWithHandlers(handlers) + counter := &repositoryRequestCountingTransport{inner: mockedClient.Transport} + client := mustNewGHClient(t, &http.Client{Transport: counter}) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + args := map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "large.bin", + } + maps.Copy(args, tc.args) + + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + assert.Equal(t, tc.expectedRequestCount, counter.count) + }) + } +} + func Test_GetFileContents_DirectoryFieldFiltering(t *testing.T) { mockDirContent := []*github.RepositoryContent{ { @@ -657,7 +1331,7 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr("abc123"), + SHA: github.Ptr(gitBlobSHA1(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -750,7 +1424,7 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr("abc123"), + SHA: github.Ptr(gitBlobSHA1(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -776,6 +1450,56 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails") } }) + + t.Run("detected symlink preserves ifc label", func(t *testing.T) { + target := "target.txt" + targetContent := []byte("target content") + client := mustNewGHClient(t, 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": false, + }), + GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ + Name: github.Ptr("link.txt"), + Path: github.Ptr("link.txt"), + SHA: github.Ptr(gitBlobSHA1([]byte(target))), + Type: github.Ptr("file"), + Content: github.Ptr(base64.StdEncoding.EncodeToString(targetContent)), + Size: github.Ptr(len(targetContent)), + Encoding: github.Ptr("base64"), + }), + GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(target)) + }, + })) + deps := BaseDeps{ + Client: client, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), + } + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "octocat", + "repo": "repo", + "path": "link.txt", + "ref": "refs/heads/main", + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + assert.Equal(t, "symlink", parseRepositoryPathMetadata(t, result)["type"]) + require.NotNil(t, result.Meta) + ifcLabel, ok := result.Meta["ifc"] + require.True(t, ok) + 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"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) + }) } // Test_GetCommit_IFC_FeatureFlag verifies that the IFC security label is only @@ -1801,6 +2525,8 @@ func Test_CreateOrUpdateFile(t *testing.T) { HTMLURL: github.Ptr("https://github.com/owner/repo/commit/def456abc789"), }, } + symlinkTarget := "other.md" + symlinkSHA := gitBlobSHA1([]byte(symlinkTarget)) mockPathTree := func(mode string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { var tree *github.Tree @@ -1817,13 +2543,17 @@ func Test_CreateOrUpdateFile(t *testing.T) { }, } case strings.HasSuffix(r.URL.Path, "/docs-tree"): + entrySHA := strings.Repeat("e", 40) + if mode == gitSymlinkMode { + entrySHA = symlinkSHA + } tree = &github.Tree{ Entries: []*github.TreeEntry{ { Path: github.Ptr("example.md"), Mode: github.Ptr(mode), Type: github.Ptr("blob"), - SHA: github.Ptr("example-sha"), + SHA: github.Ptr(entrySHA), }, }, } @@ -1987,7 +2717,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("other.md")) + _, _ = w.Write([]byte(symlinkTarget)) }, "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), @@ -2027,7 +2757,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { }, GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("other.md")) + _, _ = w.Write([]byte(symlinkTarget)) }, "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), From 783ff2876d3fab60ef2a211eeb88e80209d1f2c3 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 12:37:26 +0200 Subject: [PATCH 5/6] refactor(repos): simplify symlink read disclosure Keep the lazy blob-identity check and bounded tree fallback while consolidating metadata handling and request-count tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21420b11-5dae-49b6-ac77-965faec7f88b --- pkg/github/repositories.go | 60 +- pkg/github/repositories_helper.go | 216 +++---- pkg/github/repositories_test.go | 952 ++++++++---------------------- 3 files changed, 362 insertions(+), 866 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 1399207cdc..60937f49bf 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1073,41 +1073,34 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool } const maxContentSize = 1024 * 1024 // 1MB - inspection, respInspect, err := inspectRepositoryFile(ctx, client, owner, repo, ref, path, fileContent) + read, respInspect, err := inspectRepositoryFile(ctx, client, owner, repo, ref, path, fileContent) if err != nil { if respInspect != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to inspect repository file", - respInspect, - err, - ), nil, nil + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to inspect repository file", respInspect, err), nil, nil } return utils.NewToolResultError(fmt.Sprintf("failed to inspect repository file: %s", err)), nil, nil } - if inspection.Submodule != nil { - return attachIFC(utils.NewToolResultText(marshalRepositorySubmoduleMetadata(inspection.Submodule))), nil, nil + if read.Metadata != nil && read.Metadata.Type == "submodule" { + return attachIFC(utils.NewToolResultText(marshalRepositoryPathMetadata(read.Metadata, "", successNote))), nil, nil } - if inspection.Symlink != nil && - !inspection.ContentAvailable && - (inspection.Symlink.Explicit || fileSize < maxContentSize) { - return attachIFC(utils.NewToolResultText(marshalRepositorySymlinkMetadata( - inspection.Symlink, - unavailableSymlinkContents, - successNote, - ))), nil, nil + if read.Metadata != nil && fileContent.GetType() == "symlink" && !read.ContentAvailable { + return attachIFC(utils.NewToolResultText(marshalRepositoryPathMetadata(read.Metadata, "not_returned", successNote))), nil, nil } - // Empty files are returned as empty text to avoid - // DetectContentType misclassifying them as binary. - if fileSize == 0 && inspection.ContentAvailable { + // Empty files (0 bytes) have no content to decode; return + // them directly as empty text to avoid errors from + // GetContent when the API returns null content with a + // base64 encoding field, and to avoid DetectContentType + // misclassifying them as binary. + if fileSize == 0 && read.ContentAvailable { result := &mcp.ResourceContents{ URI: resourceURI, Text: "", MIMEType: "text/plain", } message := fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote) - if inspection.Symlink != nil { - message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + if read.Metadata != nil { + message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) } return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } @@ -1123,26 +1116,23 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool } message := 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) - if inspection.Symlink != nil { - targetPath := inspection.Symlink.ResolvedTargetPath - if targetPath == "" { - targetPath = inspection.Symlink.Target - } - resourceLink.Title = fmt.Sprintf("Dereferenced target %s via symlink %s", targetPath, path) - message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + if read.Metadata != nil { + message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) + resourceLink.Title = fmt.Sprintf("Dereferenced target %s via symlink %s", read.Metadata.ResolvedTargetPath, path) } return attachIFC(utils.NewToolResultResourceLink( message, resourceLink)), nil, nil } - if !inspection.ContentAvailable { - return utils.NewToolResultError(fmt.Sprintf("failed to inspect repository file: Contents API did not provide content for path %q", path)), nil, nil + + if !read.ContentAvailable { + return utils.NewToolResultError("failed to inspect repository file: content unavailable"), nil, nil } // Detect content type from the actual content bytes, // mirroring the original approach of using the Content-Type header // from the raw API response. - contentBytes := inspection.Content + contentBytes := read.Content contentType := http.DetectContentType(contentBytes) // Determine if content is text or binary based on detected content type @@ -1159,8 +1149,8 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool MIMEType: contentType, } message := fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote) - if inspection.Symlink != nil { - message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + if read.Metadata != nil { + message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) } return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } @@ -1171,8 +1161,8 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool MIMEType: contentType, } message := fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote) - if inspection.Symlink != nil { - message = marshalRepositorySymlinkMetadata(inspection.Symlink, dereferencedContentLabel, successNote) + if read.Metadata != nil { + message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) } return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } else if dirContent != nil { diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 2bc34db3dd..978471372a 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -1,9 +1,8 @@ package github import ( - "bytes" "context" - "crypto/sha1" //nolint:gosec // Git object IDs are defined using SHA-1. + "crypto/sha1" //nolint:gosec // Git object IDs use SHA-1 by definition. "encoding/hex" "encoding/json" "fmt" @@ -97,43 +96,25 @@ func createReferenceFromDefaultBranch(ctx context.Context, client *github.Client } const ( - gitSymlinkMode = "120000" - gitSubmoduleMode = "160000" - maxGitTreeTraversalDepth = 64 - dereferencedContentLabel = "dereferenced_target" - unavailableSymlinkContents = "not_returned" + gitSymlinkMode = "120000" + gitSubmoduleMode = "160000" ) -type repositorySymlink struct { - Path string - SHA string - Target string - ResolvedTargetPath string - Explicit bool -} - -type repositoryFileInspection struct { - Content []byte - ContentAvailable bool - Symlink *repositorySymlink - Submodule *repositorySubmoduleReadMetadata -} - -type repositorySymlinkReadMetadata struct { +type repositoryPathMetadata struct { Type string `json:"type"` Path string `json:"path"` SHA string `json:"sha,omitempty"` - Target string `json:"target"` + Target string `json:"target,omitempty"` ResolvedTargetPath string `json:"resolved_path,omitempty"` - Content string `json:"content"` + GitURL string `json:"git_url,omitempty"` + Content string `json:"content,omitempty"` Note string `json:"note,omitempty"` } -type repositorySubmoduleReadMetadata struct { - Type string `json:"type"` - Path string `json:"path"` - SHA string `json:"sha,omitempty"` - GitURL string `json:"git_url,omitempty"` +type repositoryFileRead struct { + Content []byte + ContentAvailable bool + Metadata *repositoryPathMetadata } type symlinkWriteBlockedError struct { @@ -171,160 +152,121 @@ func newSymlinkWriteBlockedResult(path, target string) *mcp.CallToolResult { } } -func inspectRepositoryFile(ctx context.Context, client *github.Client, owner, repo, treeish, path string, file *github.RepositoryContent) (*repositoryFileInspection, *github.Response, error) { - if file.GetType() == "symlink" { - content, available, err := suppliedRepositoryContent(file) - if err != nil { - return nil, nil, err - } - return &repositoryFileInspection{ - Content: content, - ContentAvailable: available, - Symlink: newRepositorySymlink(path, file.GetSHA(), file.GetTarget(), true), - }, nil, nil - } - +func inspectRepositoryFile(ctx context.Context, client *github.Client, owner, repo, ref, path string, file *github.RepositoryContent) (*repositoryFileRead, *github.Response, error) { if file.GetType() == "submodule" || file.GetSubmoduleGitURL() != "" { - return &repositoryFileInspection{ - Submodule: &repositorySubmoduleReadMetadata{ - Type: "submodule", - Path: path, - SHA: file.GetSHA(), - GitURL: file.GetSubmoduleGitURL(), - }, - }, nil, nil + return &repositoryFileRead{Metadata: &repositoryPathMetadata{ + Type: "submodule", Path: path, SHA: file.GetSHA(), GitURL: file.GetSubmoduleGitURL(), + }}, nil, nil } - content, available, err := suppliedRepositoryContent(file) + content, available, err := repositoryContentBytes(file) if err != nil { return nil, nil, err } + if file.GetType() == "symlink" { + return &repositoryFileRead{ + Content: content, ContentAvailable: available, + Metadata: newSymlinkReadMetadata(path, file.GetSHA(), file.GetTarget()), + }, nil, nil + } + if available { if !looksLikeSHA(file.GetSHA()) { - return nil, nil, fmt.Errorf("contents API returned malformed Git blob SHA %q for path %q", file.GetSHA(), path) + return nil, nil, fmt.Errorf("contents API returned malformed Git blob SHA %q", file.GetSHA()) } - if strings.EqualFold(gitBlobSHA1(content), file.GetSHA()) { - return &repositoryFileInspection{Content: content, ContentAvailable: true}, nil, nil + if strings.EqualFold(gitBlobSHA(content), file.GetSHA()) { + return &repositoryFileRead{Content: content, ContentAvailable: true}, nil, nil } - - target, resp, err := symlinkTargetFromBlob(ctx, client, owner, repo, file.GetSHA()) + target, resp, err := getVerifiedBlob(ctx, client, owner, repo, file.GetSHA()) if err != nil { - return nil, resp, fmt.Errorf("contents API bytes did not match the reported Git blob and the path blob was not a valid symbolic link target: %w", err) + return nil, resp, err + } + if !validInternalSymlinkTarget(path, target) { + return nil, nil, fmt.Errorf("blob %q is not a valid internal symbolic link target", file.GetSHA()) } - return &repositoryFileInspection{ - Content: content, - ContentAvailable: true, - Symlink: newRepositorySymlink(path, file.GetSHA(), target, false), + return &repositoryFileRead{ + Content: content, ContentAvailable: true, + Metadata: newSymlinkReadMetadata(path, file.GetSHA(), string(target)), }, nil, nil } - entry, resp, err := getTreeEntry(ctx, client, owner, repo, treeish, path) + entry, resp, err := getTreeEntry(ctx, client, owner, repo, ref, path) if err != nil { return nil, resp, err } - if entry == nil { - return nil, nil, fmt.Errorf("path %q exists according to the Contents API but was not found in the Git tree", path) - } - if !looksLikeSHA(file.GetSHA()) || !strings.EqualFold(file.GetSHA(), entry.GetSHA()) { - return nil, nil, fmt.Errorf("contents API blob SHA %q does not match Git tree blob SHA %q for path %q", file.GetSHA(), entry.GetSHA(), path) + if entry == nil || !looksLikeSHA(file.GetSHA()) || !strings.EqualFold(entry.GetSHA(), file.GetSHA()) { + return nil, nil, fmt.Errorf("contents API metadata does not match the Git tree for path %q", path) } - switch entry.GetMode() { case gitSymlinkMode: - target, resp, err := symlinkTargetFromBlob(ctx, client, owner, repo, entry.GetSHA()) + target, resp, err := getVerifiedBlob(ctx, client, owner, repo, entry.GetSHA()) if err != nil { return nil, resp, err } - return &repositoryFileInspection{ - Symlink: newRepositorySymlink(path, entry.GetSHA(), target, false), - }, nil, nil + return &repositoryFileRead{Metadata: newSymlinkReadMetadata(path, entry.GetSHA(), string(target))}, nil, nil case gitSubmoduleMode: - return &repositoryFileInspection{ - Submodule: &repositorySubmoduleReadMetadata{ - Type: "submodule", - Path: path, - SHA: entry.GetSHA(), - }, - }, nil, nil + return &repositoryFileRead{Metadata: &repositoryPathMetadata{ + Type: "submodule", Path: path, SHA: entry.GetSHA(), + }}, nil, nil default: - return &repositoryFileInspection{}, nil, nil + return &repositoryFileRead{}, nil, nil } } -func suppliedRepositoryContent(file *github.RepositoryContent) ([]byte, bool, error) { - if file.Content != nil { - content, err := file.GetContent() - if err != nil { - return nil, false, fmt.Errorf("failed to decode file content: %w", err) - } - return []byte(content), true, nil +func repositoryContentBytes(file *github.RepositoryContent) ([]byte, bool, error) { + if file.Content == nil { + return []byte{}, file.GetType() != "symlink" && file.GetSize() == 0, nil } - if file.GetType() != "symlink" && file.GetSize() == 0 { - return []byte{}, true, nil + content, err := file.GetContent() + if err != nil { + return nil, false, fmt.Errorf("failed to decode file content: %w", err) } - return nil, false, nil + return []byte(content), true, nil } -func gitBlobSHA1(content []byte) string { - hasher := sha1.New() //nolint:gosec // SHA-1 is required by the Git object ID format. - _, _ = fmt.Fprintf(hasher, "blob %d\x00", len(content)) - _, _ = hasher.Write(content) - return hex.EncodeToString(hasher.Sum(nil)) +func gitBlobSHA(content []byte) string { + hash := sha1.New() //nolint:gosec // Git object IDs use SHA-1 by definition. + _, _ = fmt.Fprintf(hash, "blob %d\x00", len(content)) + _, _ = hash.Write(content) + return hex.EncodeToString(hash.Sum(nil)) } -func symlinkTargetFromBlob(ctx context.Context, client *github.Client, owner, repo, sha string) (string, *github.Response, error) { - target, resp, err := gitBlobBytes(ctx, client, owner, repo, sha) - if err != nil { - return "", resp, err - } - if len(target) == 0 || !utf8.Valid(target) || bytes.IndexByte(target, 0) >= 0 { - return "", nil, fmt.Errorf("git blob %q is not a valid symbolic link target", sha) - } - return string(target), nil, nil -} - -func gitBlobBytes(ctx context.Context, client *github.Client, owner, repo, sha string) ([]byte, *github.Response, error) { +func getVerifiedBlob(ctx context.Context, client *github.Client, owner, repo, sha string) ([]byte, *github.Response, error) { if !looksLikeSHA(sha) { return nil, nil, fmt.Errorf("malformed Git blob SHA %q", sha) } content, resp, err := client.Git.GetBlobRaw(ctx, owner, repo, sha) - if err != nil { - return nil, resp, err - } if resp != nil && resp.Body != nil { _ = resp.Body.Close() } - if !strings.EqualFold(gitBlobSHA1(content), sha) { - return nil, nil, fmt.Errorf("blob bytes returned by the Git Blobs API do not match SHA %q", sha) + if err != nil { + return nil, resp, err + } + if !strings.EqualFold(gitBlobSHA(content), sha) { + return nil, nil, fmt.Errorf("blob returned by the Git Blobs API does not match SHA %q", sha) } return content, nil, nil } -func newRepositorySymlink(path, sha, target string, explicit bool) *repositorySymlink { - return &repositorySymlink{ - Path: path, - SHA: sha, - Target: target, - ResolvedTargetPath: resolveRepositorySymlinkTarget(path, target), - Explicit: explicit, - } +func validInternalSymlinkTarget(path string, target []byte) bool { + return len(target) > 0 && + utf8.Valid(target) && + !strings.ContainsAny(string(target), "\x00\r\n") && + resolveRepositorySymlinkTarget(path, string(target)) != "" } -func marshalRepositorySymlinkMetadata(link *repositorySymlink, content, note string) string { - payload, _ := json.Marshal(repositorySymlinkReadMetadata{ - Type: "symlink", - Path: link.Path, - SHA: link.SHA, - Target: link.Target, - ResolvedTargetPath: link.ResolvedTargetPath, - Content: content, - Note: strings.TrimSpace(note), - }) - return string(payload) +func newSymlinkReadMetadata(path, sha, target string) *repositoryPathMetadata { + return &repositoryPathMetadata{ + Type: "symlink", Path: path, SHA: sha, Target: target, + ResolvedTargetPath: resolveRepositorySymlinkTarget(path, target), + } } -func marshalRepositorySubmoduleMetadata(submodule *repositorySubmoduleReadMetadata) string { - payload, _ := json.Marshal(submodule) +func marshalRepositoryPathMetadata(metadata *repositoryPathMetadata, content, note string) string { + result := *metadata + result.Content = content + result.Note = strings.TrimSpace(note) + payload, _ := json.Marshal(result) return string(payload) } @@ -340,7 +282,7 @@ func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo return "", false, nil, nil } - target, resp, err := gitBlobBytes(ctx, client, owner, repo, entry.GetSHA()) + target, resp, err := getVerifiedBlob(ctx, client, owner, repo, entry.GetSHA()) if err != nil { return "", false, resp, err } @@ -349,8 +291,8 @@ func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo func getTreeEntry(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (*github.TreeEntry, *github.Response, error) { segments := strings.Split(pathpkg.Clean(strings.TrimPrefix(path, "/")), "/") - if len(segments) > maxGitTreeTraversalDepth { - return nil, nil, fmt.Errorf("path %q exceeds the maximum Git tree traversal depth of %d", path, maxGitTreeTraversalDepth) + if len(segments) > 64 { + return nil, nil, fmt.Errorf("path %q exceeds Git tree traversal limit", path) } treeish = escapeGitTreeish(treeish) for i, segment := range segments { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 1602369b12..26e0978e37 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -4,7 +4,6 @@ import ( "context" "encoding/base64" "encoding/json" - "maps" "net/http" "net/url" "strings" @@ -37,12 +36,55 @@ func (t *repositoryRequestCountingTransport) RoundTrip(req *http.Request) (*http return t.inner.RoundTrip(req) } -func parseRepositoryPathMetadata(t *testing.T, result *mcp.CallToolResult) map[string]any { +type repositoryReadFixture struct { + contents any + blobs map[string][]byte + trees map[string]*github.Tree + inspect func(*http.Request) +} + +func runRepositoryReadFixture(t *testing.T, fixture repositoryReadFixture, args map[string]any) (*mcp.CallToolResult, int) { + t.Helper() + backend := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "": func(w http.ResponseWriter, r *http.Request) { + if fixture.inspect != nil { + fixture.inspect(r) + } + switch { + case strings.Contains(r.URL.Path, "/contents/"): + mockResponse(t, http.StatusOK, fixture.contents)(w, r) + case strings.Contains(r.URL.Path, "/git/blobs/"): + sha := r.URL.Path[strings.LastIndex(r.URL.Path, "/")+1:] + content, ok := fixture.blobs[sha] + require.True(t, ok, "unexpected blob request: %s", sha) + _, _ = w.Write(content) + case strings.Contains(r.URL.Path, "/git/trees/"): + treeish := r.URL.Path[strings.Index(r.URL.Path, "/git/trees/")+len("/git/trees/"):] + tree, ok := fixture.trees[treeish] + require.True(t, ok, "unexpected tree request: %s", treeish) + mockResponse(t, http.StatusOK, tree)(w, r) + default: + http.NotFound(w, r) + } + }, + }) + counter := &repositoryRequestCountingTransport{inner: backend.Transport} + client := mustNewGHClient(t, &http.Client{Transport: counter}) + deps := BaseDeps{Client: client} + tool := GetFileContents(translations.NullTranslationHelper) + handler := tool.Handler(deps) + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + return result, counter.count +} + +func repositoryPathMetadataFromResult(t *testing.T, result *mcp.CallToolResult) repositoryPathMetadata { t.Helper() require.NotEmpty(t, result.Content) text, ok := result.Content[0].(*mcp.TextContent) - require.True(t, ok, "expected first result content to be TextContent") - var metadata map[string]any + require.True(t, ok) + var metadata repositoryPathMetadata require.NoError(t, json.Unmarshal([]byte(text.Text), &metadata)) return metadata } @@ -68,9 +110,6 @@ func Test_GetFileContents(t *testing.T) { // Mock response for raw content mockRawContent := []byte("# Test Repository\n\nThis is a test repository.") - mockPNGContent := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01") - mockPDFContent := []byte("%PDF-1.4 fake pdf content") - largeFileSHA := strings.Repeat("a", 40) // Setup mock directory content for success case mockDirContent := []*github.RepositoryContent{ @@ -78,7 +117,7 @@ func Test_GetFileContents(t *testing.T) { Type: github.Ptr("file"), Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr("abc123"), + SHA: github.Ptr(gitBlobSHA(mockRawContent)), Size: github.Ptr(42), HTMLURL: github.Ptr("https://github.com/owner/repo/blob/main/README.md"), }, @@ -111,7 +150,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(mockRawContent)), + SHA: github.Ptr(gitBlobSHA(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(string(mockRawContent)), Size: github.Ptr(len(mockRawContent)), @@ -141,14 +180,15 @@ func Test_GetFileContents(t *testing.T) { GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) // PNG magic bytes followed by some data - encodedContent := base64.StdEncoding.EncodeToString(mockPNGContent) + pngContent := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01") + encodedContent := base64.StdEncoding.EncodeToString(pngContent) fileContent := &github.RepositoryContent{ Name: github.Ptr("test.png"), Path: github.Ptr("test.png"), - SHA: github.Ptr(gitBlobSHA1(mockPNGContent)), + SHA: github.Ptr(gitBlobSHA(pngContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), - Size: github.Ptr(len(mockPNGContent)), + Size: github.Ptr(len(pngContent)), Encoding: github.Ptr("base64"), } contentBytes, _ := json.Marshal(fileContent) @@ -176,14 +216,15 @@ func Test_GetFileContents(t *testing.T) { GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) // PDF magic bytes - encodedContent := base64.StdEncoding.EncodeToString(mockPDFContent) + pdfContent := []byte("%PDF-1.4 fake pdf content") + encodedContent := base64.StdEncoding.EncodeToString(pdfContent) fileContent := &github.RepositoryContent{ Name: github.Ptr("document.pdf"), Path: github.Ptr("document.pdf"), - SHA: github.Ptr(gitBlobSHA1(mockPDFContent)), + SHA: github.Ptr(gitBlobSHA(pdfContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), - Size: github.Ptr(len(mockPDFContent)), + Size: github.Ptr(len(pdfContent)), Encoding: github.Ptr("base64"), } contentBytes, _ := json.Marshal(fileContent) @@ -235,7 +276,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(mockRawContent)), + SHA: github.Ptr(gitBlobSHA(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -319,7 +360,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(mockRawContent)), + SHA: github.Ptr(gitBlobSHA(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -353,7 +394,7 @@ func Test_GetFileContents(t *testing.T) { Path: github.Ptr("large-file.bin"), Mode: github.Ptr("100644"), Type: github.Ptr("blob"), - SHA: github.Ptr(largeFileSHA), + SHA: github.Ptr(strings.Repeat("a", 40)), }}, }), GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { @@ -362,7 +403,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("large-file.bin"), Path: github.Ptr("large-file.bin"), - SHA: github.Ptr(largeFileSHA), + SHA: github.Ptr(strings.Repeat("a", 40)), Type: github.Ptr("file"), Size: github.Ptr(2 * 1024 * 1024), // 2MB DownloadURL: github.Ptr("https://raw.githubusercontent.com/owner/repo/main/large-file.bin"), @@ -394,7 +435,7 @@ func Test_GetFileContents(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr(".gitkeep"), Path: github.Ptr(".gitkeep"), - SHA: github.Ptr(gitBlobSHA1(nil)), + SHA: github.Ptr(gitBlobSHA(nil)), Type: github.Ptr("file"), Content: nil, Size: github.Ptr(0), @@ -542,658 +583,205 @@ func Test_GetFileContents(t *testing.T) { } } -func Test_GetFileContents_SymlinkDisclosureRequestCounts(t *testing.T) { - serverTool := GetFileContents(translations.NullTranslationHelper) +func Test_GetFileContents_SymlinkDisclosure(t *testing.T) { commitSHA := strings.Repeat("c", 40) - textTargetContent := []byte("resolved text content") - binaryTargetContent := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR") + args := func(path string) map[string]any { + return map[string]any{"owner": "owner", "repo": "repo", "path": path, "sha": commitSHA} + } - tests := []struct { - name string - path string - handlers func(*testing.T) map[string]http.HandlerFunc - expectedRequestCount int - check func(*testing.T, *mcp.CallToolResult) - }{ - { - name: "normal small text file remains one request", - path: "README.md", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("README.md"), - Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(textTargetContent)), - Type: github.Ptr("file"), - Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), - Size: github.Ptr(len(textTargetContent)), - Encoding: github.Ptr("base64"), - }), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - assert.Equal(t, string(textTargetContent), getResourceResult(t, result).Text) - assert.Contains(t, result.Content[0].(*mcp.TextContent).Text, "successfully downloaded text file") - }, - }, - { - name: "internal text symlink adds one blob request", - path: "docs/link.txt", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - target := "../target.txt" - linkSHA := gitBlobSHA1([]byte(target)) - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("link.txt"), - Path: github.Ptr("docs/link.txt"), - SHA: github.Ptr(linkSHA), - Type: github.Ptr("file"), - Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), - Size: github.Ptr(len(textTargetContent)), - Encoding: github.Ptr("base64"), - }), - GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(target)) - }, - } - }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "symlink", metadata["type"]) - assert.Equal(t, "docs/link.txt", metadata["path"]) - assert.Equal(t, "../target.txt", metadata["target"]) - assert.Equal(t, "target.txt", metadata["resolved_path"]) - assert.Equal(t, dereferencedContentLabel, metadata["content"]) - resource := getResourceResult(t, result) - assert.Equal(t, string(textTargetContent), resource.Text) - assert.Equal(t, "text/plain; charset=utf-8", resource.MIMEType) - assert.Contains(t, resource.URI, "/contents/docs/link.txt") - }, - }, - { - name: "internal binary symlink preserves blob and MIME type", - path: "assets/current.png", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - target := "images/logo.png" - linkSHA := gitBlobSHA1([]byte(target)) - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("current.png"), - Path: github.Ptr("assets/current.png"), - SHA: github.Ptr(linkSHA), - Type: github.Ptr("file"), - Content: github.Ptr(base64.StdEncoding.EncodeToString(binaryTargetContent)), - Size: github.Ptr(len(binaryTargetContent)), - Encoding: github.Ptr("base64"), - }), - GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(target)) - }, - } - }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "assets/images/logo.png", metadata["resolved_path"]) - resource := getResourceResult(t, result) - assert.Equal(t, binaryTargetContent, resource.Blob) - assert.Equal(t, "image/png", resource.MIMEType) - }, - }, - { - name: "explicit dangling symlink discloses without fetching", - path: "docs/missing", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - target := "missing.txt" - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("missing"), - Path: github.Ptr("docs/missing"), - SHA: github.Ptr(gitBlobSHA1([]byte(target))), - Type: github.Ptr("symlink"), - Target: github.Ptr(target), - Size: github.Ptr(0), - }), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "symlink", metadata["type"]) - assert.Equal(t, "docs/missing.txt", metadata["resolved_path"]) - assert.Equal(t, unavailableSymlinkContents, metadata["content"]) - require.Len(t, result.Content, 1) - }, - }, - { - name: "explicit outside symlink omits repository resolved path", - path: "docs/outside", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - target := "../../outside" - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("outside"), - Path: github.Ptr("docs/outside"), - SHA: github.Ptr(gitBlobSHA1([]byte(target))), - Type: github.Ptr("symlink"), - Target: github.Ptr(target), - Size: github.Ptr(0), - }), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "../../outside", metadata["target"]) - assert.NotContains(t, metadata, "resolved_path") - }, - }, - { - name: "explicit symlink returns content only when supplied", - path: "docs/explicit", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - target := "target.txt" - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("explicit"), - Path: github.Ptr("docs/explicit"), - SHA: github.Ptr(gitBlobSHA1([]byte(target))), - Type: github.Ptr("symlink"), - Target: github.Ptr(target), - Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), - Encoding: github.Ptr("base64"), - Size: github.Ptr(len(textTargetContent)), - }), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, dereferencedContentLabel, metadata["content"]) - assert.Equal(t, string(textTargetContent), getResourceResult(t, result).Text) - }, - }, - { - name: "explicit symlink without content never fabricates a large resource", - path: "docs/pathological", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - target := "target.txt" - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("pathological"), - Path: github.Ptr("docs/pathological"), - SHA: github.Ptr(gitBlobSHA1([]byte(target))), - Type: github.Ptr("symlink"), - Target: github.Ptr(target), - Size: github.Ptr(2 * 1024 * 1024), - }), - } + t.Run("normal file remains one request", func(t *testing.T) { + content := []byte("ordinary content") + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("README.md"), SHA: github.Ptr(gitBlobSHA(content)), + Content: github.Ptr(string(content)), Size: github.Ptr(len(content)), }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, unavailableSymlinkContents, metadata["content"]) - require.Len(t, result.Content, 1) - }, - }, - { - name: "submodule response is not classified as symlink", - path: "vendor/dependency", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("dependency"), - Path: github.Ptr("vendor/dependency"), - SHA: github.Ptr(strings.Repeat("d", 40)), - Type: github.Ptr("file"), - SubmoduleGitURL: github.Ptr("https://github.com/example/dependency.git"), - Size: github.Ptr(0), - }), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "submodule", metadata["type"]) - assert.Equal(t, "vendor/dependency", metadata["path"]) - assert.Equal(t, "https://github.com/example/dependency.git", metadata["git_url"]) - assert.NotContains(t, metadata, "target") - }, - }, - { - name: "malformed SHA fails closed without another request", - path: "README.md", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("README.md"), - Path: github.Ptr("README.md"), - SHA: github.Ptr("not-a-git-object-id"), - Type: github.Ptr("file"), - Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), - Size: github.Ptr(len(textTargetContent)), - Encoding: github.Ptr("base64"), - }), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - assert.Contains(t, getErrorResult(t, result).Text, "malformed Git blob SHA") - }, - }, - { - name: "anomalous mismatch is not mislabeled as symlink", - path: "README.md", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - pathBlob := []byte("invalid\x00target") - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("README.md"), - Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(pathBlob)), - Type: github.Ptr("file"), - Content: github.Ptr(base64.StdEncoding.EncodeToString(textTargetContent)), - Size: github.Ptr(len(textTargetContent)), - Encoding: github.Ptr("base64"), - }), - GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write(pathBlob) - }, - } - }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - text := getErrorResult(t, result).Text - assert.Contains(t, text, "not a valid symbolic link target") - assert.NotContains(t, text, `"type":"symlink"`) - }, - }, - } + }, args("README.md")) + require.False(t, result.IsError) + assert.Equal(t, 1, requests) + assert.Equal(t, string(content), getResourceResult(t, result).Text) + }) - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - handlers := tc.handlers(t) - if contentsHandler, ok := handlers[GetReposContentsByOwnerByRepoByPath]; ok { - handlers["GET /repos/{owner}/{repo}/contents/{path:.*}"] = contentsHandler + for _, tc := range []struct { + name string + content []byte + mime string + }{ + {name: "text", content: []byte("resolved text"), mime: "text/plain; charset=utf-8"}, + {name: "binary", content: []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR"), mime: "image/png"}, + } { + t.Run("internal "+tc.name, func(t *testing.T) { + target := "../target/" + tc.name + linkSHA := gitBlobSHA([]byte(target)) + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("docs/link"), SHA: github.Ptr(linkSHA), + Content: github.Ptr(base64.StdEncoding.EncodeToString(tc.content)), + Encoding: github.Ptr("base64"), Size: github.Ptr(len(tc.content)), + }, + blobs: map[string][]byte{linkSHA: []byte(target)}, + }, args("docs/link")) + require.False(t, result.IsError) + assert.Equal(t, 2, requests) + metadata := repositoryPathMetadataFromResult(t, result) + assert.Equal(t, "symlink", metadata.Type) + assert.Equal(t, "docs/link", metadata.Path) + assert.Equal(t, target, metadata.Target) + assert.Equal(t, "target/"+tc.name, metadata.ResolvedTargetPath) + assert.Equal(t, "dereferenced_target", metadata.Content) + resource := getResourceResult(t, result) + assert.Equal(t, tc.mime, resource.MIMEType) + if tc.name == "text" { + assert.Equal(t, string(tc.content), resource.Text) + } else { + assert.Equal(t, tc.content, resource.Blob) } - mockedClient := MockHTTPClientWithHandlers(handlers) - counter := &repositoryRequestCountingTransport{inner: mockedClient.Transport} - client := mustNewGHClient(t, &http.Client{Transport: counter}) - deps := BaseDeps{Client: client} - handler := serverTool.Handler(deps) - request := createMCPRequest(map[string]any{ - "owner": "owner", - "repo": "repo", - "path": tc.path, - "sha": commitSHA, - }) - - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.Equal(t, tc.expectedRequestCount, counter.count) - tc.check(t, result) }) } -} - -func Test_GetFileContents_ContentlessRequestCounts(t *testing.T) { - serverTool := GetFileContents(translations.NullTranslationHelper) - commitSHA := strings.Repeat("c", 40) - largeFileSHA := strings.Repeat("a", 40) - linkTarget := "../targets/large.bin" - linkSHA := gitBlobSHA1([]byte(linkTarget)) - const largeFileSize = 2 * 1024 * 1024 - tests := []struct { - name string - path string - handlers func(*testing.T) map[string]http.HandlerFunc - expectedRequestCount int - check func(*testing.T, *mcp.CallToolResult) - }{ - { - name: "normal large file uses one exact-path tree request", - path: "large.bin", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("large.bin"), - Path: github.Ptr("large.bin"), - SHA: github.Ptr(largeFileSHA), - Type: github.Ptr("file"), - Size: github.Ptr(largeFileSize), - DownloadURL: github.Ptr("https://raw.example.com/large.bin"), - }), - GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("large.bin"), - Mode: github.Ptr("100644"), - Type: github.Ptr("blob"), - SHA: github.Ptr(largeFileSHA), - }}, - }), - } + t.Run("explicit dangling or outside link", func(t *testing.T) { + target := "../../outside" + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("symlink"), Path: github.Ptr("docs/link"), + SHA: github.Ptr(gitBlobSHA([]byte(target))), Target: github.Ptr(target), }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - require.Len(t, result.Content, 2) - link, ok := result.Content[1].(*mcp.ResourceLink) - require.True(t, ok) - assert.Equal(t, "File: large.bin", link.Title) - }, - }, - { - name: "internal large symlink uses bounded tree descent and blob request", - path: "docs/current.bin", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, commitSHA, r.URL.Query().Get("ref")) - mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("current.bin"), - Path: github.Ptr("docs/current.bin"), - SHA: github.Ptr(linkSHA), - Type: github.Ptr("file"), - Size: github.Ptr(largeFileSize), - DownloadURL: github.Ptr("https://raw.example.com/docs/current.bin"), - })(w, r) - }, - GetReposGitTreesByOwnerByRepoByTree: func(w http.ResponseWriter, r *http.Request) { - switch { - case strings.HasSuffix(r.URL.Path, "/"+commitSHA): - mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("docs"), - Mode: github.Ptr("040000"), - Type: github.Ptr("tree"), - SHA: github.Ptr("docs-tree"), - }}, - })(w, r) - case strings.HasSuffix(r.URL.Path, "/docs-tree"): - mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("current.bin"), - Mode: github.Ptr(gitSymlinkMode), - Type: github.Ptr("blob"), - SHA: github.Ptr(linkSHA), - }}, - })(w, r) - default: - require.FailNow(t, "unexpected tree request", r.URL.Path) - } - }, - GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(linkTarget)) - }, - } - }, - expectedRequestCount: 4, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "symlink", metadata["type"]) - assert.Equal(t, "targets/large.bin", metadata["resolved_path"]) - assert.Equal(t, dereferencedContentLabel, metadata["content"]) - require.Len(t, result.Content, 2) - link, ok := result.Content[1].(*mcp.ResourceLink) - require.True(t, ok) - assert.Equal(t, "Dereferenced target targets/large.bin via symlink docs/current.bin", link.Title) - assert.Contains(t, link.URI, "/contents/docs/current.bin") - }, - }, - { - name: "truncated tree fails closed", - path: "large.bin", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("large.bin"), - Path: github.Ptr("large.bin"), - SHA: github.Ptr(largeFileSHA), - Type: github.Ptr("file"), - Size: github.Ptr(largeFileSize), - }), - GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ - Truncated: github.Ptr(true), - }), - } - }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - assert.Contains(t, getErrorResult(t, result).Text, "is truncated") - }, - }, - { - name: "contentless SHA mismatch fails closed", - path: "large.bin", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("large.bin"), - Path: github.Ptr("large.bin"), - SHA: github.Ptr(largeFileSHA), - Type: github.Ptr("file"), - Size: github.Ptr(largeFileSize), - }), - GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("large.bin"), - Mode: github.Ptr("100644"), - Type: github.Ptr("blob"), - SHA: github.Ptr(strings.Repeat("b", 40)), - }}, - }), - } - }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - assert.Contains(t, getErrorResult(t, result).Text, "does not match Git tree blob SHA") - }, - }, - { - name: "tree-mode submodule fallback is explicit", - path: "vendor/dependency", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - submoduleSHA := strings.Repeat("d", 40) - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("dependency"), - Path: github.Ptr("vendor/dependency"), - SHA: github.Ptr(submoduleSHA), - Type: github.Ptr("file"), - Size: github.Ptr(1), - }), - GetReposGitTreesByOwnerByRepoByTree: func(w http.ResponseWriter, r *http.Request) { - switch { - case strings.HasSuffix(r.URL.Path, "/"+commitSHA): - mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("vendor"), - Mode: github.Ptr("040000"), - Type: github.Ptr("tree"), - SHA: github.Ptr("vendor-tree"), - }}, - })(w, r) - case strings.HasSuffix(r.URL.Path, "/vendor-tree"): - mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("dependency"), - Mode: github.Ptr(gitSubmoduleMode), - Type: github.Ptr("commit"), - SHA: github.Ptr(submoduleSHA), - }}, - })(w, r) - default: - require.FailNow(t, "unexpected tree request", r.URL.Path) - } - }, - } - }, - expectedRequestCount: 3, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - metadata := parseRepositoryPathMetadata(t, result) - assert.Equal(t, "submodule", metadata["type"]) - assert.Equal(t, "vendor/dependency", metadata["path"]) - assert.NotContains(t, metadata, "target") - }, - }, - { - name: "directory adds no inspection request", - path: "docs", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, []*github.RepositoryContent{{ - Name: github.Ptr("README.md"), - Path: github.Ptr("docs/README.md"), - SHA: github.Ptr(largeFileSHA), - Type: github.Ptr("file"), - }}), - } - }, - expectedRequestCount: 1, - check: func(t *testing.T, result *mcp.CallToolResult) { - require.False(t, result.IsError) - var entries []*github.RepositoryContent - require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &entries)) - require.Len(t, entries, 1) - }, - }, - { - name: "missing path retains one recursive fallback request", - path: "missing.txt", - handlers: func(t *testing.T) map[string]http.HandlerFunc { - return map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), - GetReposGitTreesByOwnerByRepoByTree: func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "1", r.URL.Query().Get("recursive")) - mockResponse(t, http.StatusOK, &github.Tree{Entries: []*github.TreeEntry{}})(w, r) - }, - } + }, args("docs/link")) + require.False(t, result.IsError) + assert.Equal(t, 1, requests) + metadata := repositoryPathMetadataFromResult(t, result) + assert.Equal(t, target, metadata.Target) + assert.Empty(t, metadata.ResolvedTargetPath) + assert.Equal(t, "not_returned", metadata.Content) + require.Len(t, result.Content, 1) + }) + + t.Run("explicit link returns supplied content", func(t *testing.T) { + target, content := "target.txt", []byte("resolved") + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("symlink"), Path: github.Ptr("docs/link"), + SHA: github.Ptr(gitBlobSHA([]byte(target))), Target: github.Ptr(target), + Content: github.Ptr(string(content)), Size: github.Ptr(len(content)), }, - expectedRequestCount: 2, - check: func(t *testing.T, result *mcp.CallToolResult) { - assert.Contains(t, getErrorResult(t, result).Text, "Failed to get file contents") + }, args("docs/link")) + require.False(t, result.IsError) + assert.Equal(t, 1, requests) + assert.Equal(t, "dereferenced_target", repositoryPathMetadataFromResult(t, result).Content) + assert.Equal(t, string(content), getResourceResult(t, result).Text) + }) + + t.Run("submodule is explicit", func(t *testing.T) { + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("submodule"), Path: github.Ptr("vendor/module"), + SHA: github.Ptr(strings.Repeat("d", 40)), SubmoduleGitURL: github.Ptr("https://example.com/module.git"), }, - }, - } + }, args("vendor/module")) + require.False(t, result.IsError) + assert.Equal(t, 1, requests) + metadata := repositoryPathMetadataFromResult(t, result) + assert.Equal(t, "submodule", metadata.Type) + assert.Equal(t, "https://example.com/module.git", metadata.GitURL) + }) - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - handlers := tc.handlers(t) - if contentsHandler, ok := handlers[GetReposContentsByOwnerByRepoByPath]; ok { - handlers["GET /repos/{owner}/{repo}/contents/{path:.*}"] = contentsHandler - } - mockedClient := MockHTTPClientWithHandlers(handlers) - counter := &repositoryRequestCountingTransport{inner: mockedClient.Transport} - client := mustNewGHClient(t, &http.Client{Transport: counter}) - deps := BaseDeps{Client: client} - handler := serverTool.Handler(deps) - request := createMCPRequest(map[string]any{ - "owner": "owner", - "repo": "repo", - "path": tc.path, - "sha": commitSHA, - }) + t.Run("malformed and anomalous mismatches fail closed", func(t *testing.T) { + content := []byte("resolved") + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("bad"), SHA: github.Ptr("bad-sha"), + Content: github.Ptr(string(content)), Size: github.Ptr(len(content)), + }, + }, args("bad")) + assert.Equal(t, 1, requests) + assert.Contains(t, getErrorResult(t, result).Text, "malformed Git blob SHA") + + pathBlob := []byte("not\na\ntarget") + pathSHA := gitBlobSHA(pathBlob) + result, requests = runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("bad"), SHA: github.Ptr(pathSHA), + Content: github.Ptr(string(content)), Size: github.Ptr(len(content)), + }, + blobs: map[string][]byte{pathSHA: pathBlob}, + }, args("bad")) + assert.Equal(t, 2, requests) + assert.Contains(t, getErrorResult(t, result).Text, "not a valid internal symbolic link target") + }) +} - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.Equal(t, tc.expectedRequestCount, counter.count) - tc.check(t, result) - }) +func Test_GetFileContents_ContentlessRequestCounts(t *testing.T) { + commitSHA := strings.Repeat("c", 40) + fileSHA := strings.Repeat("a", 40) + const largeSize = 2 * 1024 * 1024 + args := func(path string) map[string]any { + return map[string]any{"owner": "owner", "repo": "repo", "path": path, "sha": commitSHA} } -} -func Test_GetFileContents_ContentlessTreeUsesRequestedRevision(t *testing.T) { - serverTool := GetFileContents(translations.NullTranslationHelper) - blobSHA := strings.Repeat("a", 40) - const fileSize = 2 * 1024 * 1024 + t.Run("normal large file", func(t *testing.T) { + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("large.bin"), SHA: github.Ptr(fileSHA), Size: github.Ptr(largeSize), + }, + trees: map[string]*github.Tree{commitSHA: {Entries: []*github.TreeEntry{ + {Path: github.Ptr("large.bin"), Mode: github.Ptr("100644"), Type: github.Ptr("blob"), SHA: github.Ptr(fileSHA)}, + }}}, + }, args("large.bin")) + require.False(t, result.IsError) + assert.Equal(t, 2, requests) + _, ok := result.Content[1].(*mcp.ResourceLink) + require.True(t, ok) + }) - tests := []struct { - name string - args map[string]any - expectedRef string - treePath string - refHandler http.HandlerFunc - expectedRequestCount int - }{ - { - name: "commit SHA", - args: map[string]any{"sha": strings.Repeat("c", 40)}, - expectedRef: strings.Repeat("c", 40), - treePath: "/repos/owner/repo/git/trees/" + strings.Repeat("c", 40), - expectedRequestCount: 2, - }, - { - name: "fully qualified branch resolves to one commit", - args: map[string]any{"ref": "refs/heads/release"}, - expectedRef: strings.Repeat("d", 40), - treePath: "/repos/owner/repo/git/trees/" + strings.Repeat("d", 40), - refHandler: mockResponse(t, http.StatusOK, &github.Reference{ - Ref: github.Ptr("refs/heads/release"), - Object: &github.GitObject{ - SHA: github.Ptr(strings.Repeat("d", 40)), - }, - }), - expectedRequestCount: 3, - }, - } + t.Run("internal large link uses exact-path tree descent", func(t *testing.T) { + target := "../target.bin" + linkSHA := gitBlobSHA([]byte(target)) + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("docs/link"), SHA: github.Ptr(linkSHA), Size: github.Ptr(largeSize), + }, + blobs: map[string][]byte{linkSHA: []byte(target)}, + trees: map[string]*github.Tree{ + commitSHA: {Entries: []*github.TreeEntry{ + {Path: github.Ptr("docs"), Mode: github.Ptr("040000"), Type: github.Ptr("tree"), SHA: github.Ptr("docs-tree")}, + }}, + "docs-tree": {Entries: []*github.TreeEntry{ + {Path: github.Ptr("link"), Mode: github.Ptr(gitSymlinkMode), Type: github.Ptr("blob"), SHA: github.Ptr(linkSHA)}, + }}, + }, + inspect: func(r *http.Request) { + if strings.Contains(r.URL.Path, "/contents/") { + assert.Equal(t, commitSHA, r.URL.Query().Get("ref")) + } + }, + }, args("docs/link")) + require.False(t, result.IsError) + assert.Equal(t, 4, requests) + metadata := repositoryPathMetadataFromResult(t, result) + assert.Equal(t, "target.bin", metadata.ResolvedTargetPath) + assert.Equal(t, "dereferenced_target", metadata.Content) + }) - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - handlers := map[string]http.HandlerFunc{ - GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, tc.expectedRef, r.URL.Query().Get("ref")) - mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("large.bin"), - Path: github.Ptr("large.bin"), - SHA: github.Ptr(blobSHA), - Type: github.Ptr("file"), - Size: github.Ptr(fileSize), - })(w, r) - }, - "GET " + tc.treePath: mockResponse(t, http.StatusOK, &github.Tree{ - Entries: []*github.TreeEntry{{ - Path: github.Ptr("large.bin"), - Mode: github.Ptr("100644"), - Type: github.Ptr("blob"), - SHA: github.Ptr(blobSHA), - }}, - }), - } - if tc.refHandler != nil { - handlers[GetReposGitRefByOwnerByRepoByRef] = tc.refHandler - } - mockedClient := MockHTTPClientWithHandlers(handlers) - counter := &repositoryRequestCountingTransport{inner: mockedClient.Transport} - client := mustNewGHClient(t, &http.Client{Transport: counter}) - deps := BaseDeps{Client: client} - handler := serverTool.Handler(deps) - args := map[string]any{ - "owner": "owner", - "repo": "repo", - "path": "large.bin", - } - maps.Copy(args, tc.args) + t.Run("truncated tree fails closed", func(t *testing.T) { + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: &github.RepositoryContent{ + Type: github.Ptr("file"), Path: github.Ptr("large.bin"), SHA: github.Ptr(fileSHA), Size: github.Ptr(largeSize), + }, + trees: map[string]*github.Tree{commitSHA: {Truncated: github.Ptr(true)}}, + }, args("large.bin")) + assert.Equal(t, 2, requests) + assert.Contains(t, getErrorResult(t, result).Text, "truncated") + }) - request := createMCPRequest(args) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - require.False(t, result.IsError) - assert.Equal(t, tc.expectedRequestCount, counter.count) - }) - } + t.Run("directory remains one request", func(t *testing.T) { + result, requests := runRepositoryReadFixture(t, repositoryReadFixture{ + contents: []*github.RepositoryContent{{ + Type: github.Ptr("file"), Path: github.Ptr("docs/readme"), SHA: github.Ptr(fileSHA), + }}, + }, args("docs")) + require.False(t, result.IsError) + assert.Equal(t, 1, requests) + }) } func Test_GetFileContents_DirectoryFieldFiltering(t *testing.T) { @@ -1331,7 +919,7 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(mockRawContent)), + SHA: github.Ptr(gitBlobSHA(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -1424,7 +1012,7 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { fileContent := &github.RepositoryContent{ Name: github.Ptr("README.md"), Path: github.Ptr("README.md"), - SHA: github.Ptr(gitBlobSHA1(mockRawContent)), + SHA: github.Ptr(gitBlobSHA(mockRawContent)), Type: github.Ptr("file"), Content: github.Ptr(encodedContent), Size: github.Ptr(len(mockRawContent)), @@ -1452,53 +1040,29 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { }) t.Run("detected symlink preserves ifc label", func(t *testing.T) { - target := "target.txt" - targetContent := []byte("target content") + target, content := []byte("target.txt"), []byte("resolved") + linkSHA := gitBlobSHA(target) client := mustNewGHClient(t, 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": false, - }), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, map[string]any{"private": false}), GetReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, &github.RepositoryContent{ - Name: github.Ptr("link.txt"), - Path: github.Ptr("link.txt"), - SHA: github.Ptr(gitBlobSHA1([]byte(target))), - Type: github.Ptr("file"), - Content: github.Ptr(base64.StdEncoding.EncodeToString(targetContent)), - Size: github.Ptr(len(targetContent)), - Encoding: github.Ptr("base64"), + Type: github.Ptr("file"), Path: github.Ptr("link"), SHA: github.Ptr(linkSHA), + Content: github.Ptr(string(content)), Size: github.Ptr(len(content)), }), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(target)) + _, _ = w.Write(target) }, })) - deps := BaseDeps{ - Client: client, - featureChecker: featureCheckerFor(FeatureFlagIFCLabels), - } + deps := BaseDeps{Client: client, featureChecker: featureCheckerFor(FeatureFlagIFCLabels)} handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{ - "owner": "octocat", - "repo": "repo", - "path": "link.txt", - "ref": "refs/heads/main", + "owner": "octocat", "repo": "repo", "path": "link", "ref": "refs/heads/main", }) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) require.NoError(t, err) require.False(t, result.IsError) - assert.Equal(t, "symlink", parseRepositoryPathMetadata(t, result)["type"]) - require.NotNil(t, result.Meta) - ifcLabel, ok := result.Meta["ifc"] - require.True(t, ok) - 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"]) - assert.Equal(t, "public", ifcMap["confidentiality"]) + assert.Equal(t, "symlink", repositoryPathMetadataFromResult(t, result).Type) + require.Contains(t, result.Meta, "ifc") }) } @@ -2525,8 +2089,8 @@ func Test_CreateOrUpdateFile(t *testing.T) { HTMLURL: github.Ptr("https://github.com/owner/repo/commit/def456abc789"), }, } - symlinkTarget := "other.md" - symlinkSHA := gitBlobSHA1([]byte(symlinkTarget)) + symlinkTarget := []byte("other.md") + symlinkSHA := gitBlobSHA(symlinkTarget) mockPathTree := func(mode string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { var tree *github.Tree @@ -2717,7 +2281,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(symlinkTarget)) + _, _ = w.Write(symlinkTarget) }, "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), @@ -2757,7 +2321,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { }, GetReposGitTreesByOwnerByRepoByTree: mockPathTree("120000"), GetReposGitBlobsByOwnerByRepoByFileSHA: func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(symlinkTarget)) + _, _ = w.Write(symlinkTarget) }, "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ SHA: github.Ptr("abc123def456"), From d4dcee2988b7af86aaf2f44ed580b617164530f1 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 12:51:18 +0200 Subject: [PATCH 6/6] fix(repos): label deferred symlink content accurately Report contentless large symlink targets as not returned while preserving their ResourceLink and requested-path identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21420b11-5dae-49b6-ac77-965faec7f88b --- pkg/github/repositories.go | 16 +++++----------- pkg/github/repositories_helper.go | 11 +++++++++++ pkg/github/repositories_test.go | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 60937f49bf..468f8da92e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1092,16 +1092,14 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool // GetContent when the API returns null content with a // base64 encoding field, and to avoid DetectContentType // misclassifying them as binary. - if fileSize == 0 && read.ContentAvailable { + if read.ContentAvailable && len(read.Content) == 0 { result := &mcp.ResourceContents{ URI: resourceURI, Text: "", MIMEType: "text/plain", } message := fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote) - if read.Metadata != nil { - message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) - } + message = repositoryReadMessage(read, message, successNote) return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } @@ -1117,9 +1115,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool message := 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) if read.Metadata != nil { - message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) resourceLink.Title = fmt.Sprintf("Dereferenced target %s via symlink %s", read.Metadata.ResolvedTargetPath, path) } + message = repositoryReadMessage(read, message, successNote) return attachIFC(utils.NewToolResultResourceLink( message, resourceLink)), nil, nil @@ -1149,9 +1147,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool MIMEType: contentType, } message := fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote) - if read.Metadata != nil { - message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) - } + message = repositoryReadMessage(read, message, successNote) return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } @@ -1161,9 +1157,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool MIMEType: contentType, } message := fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote) - if read.Metadata != nil { - message = marshalRepositoryPathMetadata(read.Metadata, "dereferenced_target", successNote) - } + message = repositoryReadMessage(read, message, successNote) return attachIFC(utils.NewToolResultResource(message, result)), nil, nil } else if dirContent != nil { // file content or file SHA is nil which means it's a directory diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 978471372a..8859b4a2ba 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -270,6 +270,17 @@ func marshalRepositoryPathMetadata(metadata *repositoryPathMetadata, content, no return string(payload) } +func repositoryReadMessage(read *repositoryFileRead, fallback, note string) string { + if read.Metadata == nil { + return fallback + } + content := "dereferenced_target" + if !read.ContentAvailable { + content = "not_returned" + } + return marshalRepositoryPathMetadata(read.Metadata, content, note) +} + func symlinkTargetAtPath(ctx context.Context, client *github.Client, owner, repo, treeish, path string) (string, bool, *github.Response, error) { entry, resp, err := getTreeEntry(ctx, client, owner, repo, treeish, path) if err != nil { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 26e0978e37..abfa78b30b 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -759,7 +759,7 @@ func Test_GetFileContents_ContentlessRequestCounts(t *testing.T) { assert.Equal(t, 4, requests) metadata := repositoryPathMetadataFromResult(t, result) assert.Equal(t, "target.bin", metadata.ResolvedTargetPath) - assert.Equal(t, "dereferenced_target", metadata.Content) + assert.Equal(t, "not_returned", metadata.Content) }) t.Run("truncated tree fails closed", func(t *testing.T) {