From cfe080ffabca963e8efaf8b4022779321f3c7b7a Mon Sep 17 00:00:00 2001 From: Nate Berkopec Date: Sun, 4 Oct 2026 18:31:59 +0900 Subject: [PATCH 1/3] fix: preserve executable modes and support explicit regular-file modes Refs #2578 --- README.md | 2 +- pkg/github/__toolsnaps__/push_files.snap | 10 +++- pkg/github/repositories.go | 47 +++++++++++---- pkg/github/repositories_helper.go | 76 +++++++++++++++++------- pkg/github/repositories_test.go | 25 +++++++- 5 files changed, 123 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index c1f9857d1a..bef42408e3 100644 --- a/README.md +++ b/README.md @@ -1482,7 +1482,7 @@ The following sets of tools are available: - **push_files** - Push files to repository - **OAuth Challenge Scopes**: `repo`, `workflow` - `branch`: Branch to push to (string, required) - - `files`: Array of file objects to push, each object with path (string) and content (string) (object[], required) + - `files`: Array of file objects to push, each with path, content, and optional mode (100644 or 100755). Omitted mode preserves existing regular file permissions; new files default to 100644. Only regular files are supported. (object[], required) - `message`: Commit message (string, required) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) diff --git a/pkg/github/__toolsnaps__/push_files.snap b/pkg/github/__toolsnaps__/push_files.snap index 798ad18451..0f902d4a80 100644 --- a/pkg/github/__toolsnaps__/push_files.snap +++ b/pkg/github/__toolsnaps__/push_files.snap @@ -12,7 +12,7 @@ "type": "string" }, "files": { - "description": "Array of file objects to push, each object with path (string) and content (string)", + "description": "Array of file objects to push, each with path, content, and optional mode (100644 or 100755). Omitted mode preserves existing regular file permissions; new files default to 100644. Only regular files are supported.", "items": { "additionalProperties": false, "properties": { @@ -20,6 +20,14 @@ "description": "file content", "type": "string" }, + "mode": { + "description": "File mode: 100644 (ordinary) or 100755 (executable). Omit to preserve an existing regular file's mode or default a new file to 100644.", + "enum": [ + "100644", + "100755" + ], + "type": "string" + }, "path": { "description": "path to the file", "type": "string" diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 4b7e84cb9a..20f15ac5f6 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1636,7 +1636,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { }, "files": { Type: "array", - Description: "Array of file objects to push, each object with path (string) and content (string)", + Description: "Array of file objects to push, each with path, content, and optional mode (100644 or 100755). Omitted mode preserves existing regular file permissions; new files default to 100644. Only regular files are supported.", Items: &jsonschema.Schema{ Type: "object", AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, @@ -1649,6 +1649,11 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "string", Description: "file content", }, + "mode": { + Type: "string", + Description: "File mode: 100644 (ordinary) or 100755 (executable). Omit to preserve an existing regular file's mode or default a new file to 100644.", + Enum: []any{"100644", "100755"}, + }, }, Required: []string{"path", "content"}, }, @@ -1707,9 +1712,18 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError("each file must have content"), nil, nil } + var mode *string + if value, supplied := fileMap["mode"]; supplied { + fileMode, ok := value.(string) + if !ok || (fileMode != "100644" && fileMode != "100755") { + return utils.NewToolResultError("file mode must be a string with value 100644 or 100755"), nil, nil + } + mode = new(fileMode) + } + entries = append(entries, &github.TreeEntry{ Path: new(filePath), - Mode: new("100644"), + Mode: mode, Type: new("blob"), Content: new(content), }) @@ -1750,7 +1764,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { var baseCommit *github.Commit if !repositoryIsEmpty { if branchNotFound { - ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) + ref, err = resolveDefaultBranch(ctx, client, owner, repo) if err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil } @@ -1777,17 +1791,28 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { } defaultBranch := strings.TrimPrefix(*ref.Ref, "refs/heads/") - if branch != defaultBranch { - // Create the requested branch from the default branch - ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil - } - } - + branchNotFound = branch != defaultBranch baseCommit = base } + if err := resolvePushFilesModes(ctx, client, owner, repo, baseCommit.Tree.GetSHA(), entries); err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to resolve file modes: %v", err)), nil, nil + } + + // Validate against the pinned default-branch tree before creating a missing branch. + if branchNotFound { + ref, resp, err = client.Git.CreateRef(ctx, owner, repo, github.CreateRef{ + Ref: "refs/heads/" + branch, + SHA: baseCommit.GetSHA(), + }) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create branch from default", resp, err), nil, nil + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + } + // Create a new tree with the file entries (baseCommit is now guaranteed to exist) newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, entries) if err != nil { diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 519385fc8c..0466693fcc 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -71,28 +71,60 @@ func initializeRepository(ctx context.Context, client *github.Client, owner, rep return ref, baseCommit, nil } -// createReferenceFromDefaultBranch creates a new branch reference from the repository's default branch -func createReferenceFromDefaultBranch(ctx context.Context, client *github.Client, owner, repo, branch string) (*github.Reference, error) { - defaultRef, err := resolveDefaultBranch(ctx, client, owner, repo) - if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to resolve default branch", nil, err) - return nil, fmt.Errorf("failed to resolve default branch: %w", err) - } - - // Create the new branch reference - createdRef, resp, err := client.Git.CreateRef(ctx, owner, repo, github.CreateRef{ - Ref: "refs/heads/" + branch, - SHA: *defaultRef.Object.SHA, - }) - if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to create new branch reference", resp, err) - return nil, fmt.Errorf("failed to create new branch reference: %w", err) +// resolvePushFilesModes reads only the pinned base tree, without following symlinks. +// Cache nonrecursive trees so shared parent directories require only one lookup. +func resolvePushFilesModes(ctx context.Context, client *github.Client, owner, repo, baseTree string, entries []*github.TreeEntry) error { + if baseTree == "" { + return fmt.Errorf("base commit has no tree SHA") } - if resp != nil && resp.Body != nil { - defer func() { _ = resp.Body.Close() }() + trees := make(map[string]map[string]*github.TreeEntry) + for _, entry := range entries { + treeSHA := baseTree + parts := strings.Split(entry.GetPath(), "/") + mode := "100644" + for i, part := range parts { + treeEntries, cached := trees[treeSHA] + if !cached { + tree, resp, err := client.Git.GetTree(ctx, owner, repo, treeSHA, false) + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + if err != nil { + return fmt.Errorf("failed to get tree for %q: %w", entry.GetPath(), err) + } + if tree == nil || tree.GetTruncated() { + return fmt.Errorf("incomplete tree for %q", entry.GetPath()) + } + treeEntries = make(map[string]*github.TreeEntry, len(tree.Entries)) + for _, existing := range tree.Entries { + if existing == nil || existing.GetPath() == "" { + return fmt.Errorf("invalid tree entry for %q", entry.GetPath()) + } + treeEntries[existing.GetPath()] = existing + } + trees[treeSHA] = treeEntries + } + existing, found := treeEntries[part] + if !found { + break + } + if i < len(parts)-1 { + if existing.GetType() != "tree" || existing.GetMode() != "040000" || existing.GetSHA() == "" { + return fmt.Errorf("cannot write %q: ancestor %q is not a directory with a tree SHA", entry.GetPath(), strings.Join(parts[:i+1], "/")) + } + treeSHA = existing.GetSHA() + continue + } + if existing.GetType() != "blob" || (existing.GetMode() != "100644" && existing.GetMode() != "100755") { + return fmt.Errorf("cannot write %q: existing entry is not a regular file (type %q, mode %q)", entry.GetPath(), existing.GetType(), existing.GetMode()) + } + mode = existing.GetMode() + } + if entry.Mode == nil { + entry.Mode = new(mode) + } } - - return createdRef, nil + return nil } const ( @@ -133,12 +165,12 @@ func newSymlinkWriteBlockedResult(path, target string) *mcp.CallToolResult { ResolvedTargetPath: resolvedTargetPath, }) recovery := fmt.Sprintf( - `Target is outside this repository. Retarget link: allow_symlink_write=true. Replace with a file: push_files path=%q.`, + `Target is outside this repository. Retarget link: allow_symlink_write=true. push_files only writes regular files; remove the symlink at %q separately before replacing it.`, 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.`, + `Edit target: create_or_update_file path=%q. Retarget link: allow_symlink_write=true. push_files only writes regular files; remove the symlink at %q separately before replacing it.`, resolvedTargetPath, path, ) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 0c930ae8c0..4f39ef1332 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2315,7 +2315,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { `"resolved_path":"docs/other.md"`, `create_or_update_file path="docs/other.md"`, `allow_symlink_write=true`, - `push_files path="docs/example.md"`, + `push_files only writes regular files; remove the symlink at "docs/example.md" separately before replacing it.`, }, expectedRequestCount: 4, }, @@ -2422,7 +2422,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedErrMsgs: []string{ `"error":"symlink_write_requires_opt_in"`, `Target is outside this repository`, - `push_files path="docs/example.md"`, + `push_files only writes regular files; remove the symlink at "docs/example.md" separately before replacing it.`, }, expectedRequestCount: 1, }, @@ -2993,6 +2993,10 @@ func Test_PushFiles(t *testing.T) { GetReposGitCommitsByOwnerByRepoByCommitSHA, mockCommit, ), + WithRequestMatch( + GetReposGitTreesByOwnerByRepoByTree, + &github.Tree{SHA: new("def456")}, + ), // Create tree WithRequestMatchHandler( PostReposGitTreesByOwnerByRepo, @@ -3085,6 +3089,10 @@ func Test_PushFiles(t *testing.T) { GetReposGitCommitsByOwnerByRepoByCommitSHA, mockCommit, ), + WithRequestMatch( + GetReposGitTreesByOwnerByRepoByTree, + &github.Tree{SHA: new("def456")}, + ), ), requestArgs: map[string]any{ "owner": "owner", @@ -3113,6 +3121,10 @@ func Test_PushFiles(t *testing.T) { GetReposGitCommitsByOwnerByRepoByCommitSHA, mockCommit, ), + WithRequestMatch( + GetReposGitTreesByOwnerByRepoByTree, + &github.Tree{SHA: new("def456")}, + ), ), requestArgs: map[string]any{ "owner": "owner", @@ -3199,6 +3211,10 @@ func Test_PushFiles(t *testing.T) { GetReposGitCommitsByOwnerByRepoByCommitSHA, mockCommit, ), + WithRequestMatch( + GetReposGitTreesByOwnerByRepoByTree, + &github.Tree{SHA: new("def456")}, + ), // Fail to create tree WithRequestMatchHandler( PostReposGitTreesByOwnerByRepo, @@ -3275,6 +3291,10 @@ func Test_PushFiles(t *testing.T) { GetReposGitCommitsByOwnerByRepoByCommitSHA, mockCommit, ), + WithRequestMatch( + GetReposGitTreesByOwnerByRepoByTree, + &github.Tree{SHA: new("def456")}, + ), // Create tree WithRequestMatch( PostReposGitTreesByOwnerByRepo, @@ -3386,6 +3406,7 @@ func Test_PushFiles(t *testing.T) { _, _ = w.Write(b) }), ), + WithRequestMatch(GetReposGitTreesByOwnerByRepoByTree, &github.Tree{SHA: new("tree456")}), // Create tree with all user files WithRequestMatchHandler( PostReposGitTreesByOwnerByRepo, From 965edf4b06564d16fb4f3b0a9295c777af0affd0 Mon Sep 17 00:00:00 2001 From: Nate Berkopec Date: Sun, 4 Oct 2026 18:34:26 +0900 Subject: [PATCH 2/3] test: cover mode preservation and fail-closed tree lookup Refs #2578 --- pkg/github/push_files_test.go | 233 ++++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 pkg/github/push_files_test.go diff --git a/pkg/github/push_files_test.go b/pkg/github/push_files_test.go new file mode 100644 index 0000000000..be1901e7d1 --- /dev/null +++ b/pkg/github/push_files_test.go @@ -0,0 +1,233 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v92/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" +) + +type pushFilesFixture struct { + trees map[string]*github.Tree + lookupStatus int + missingBranch bool + emptyRepository bool + writes []string + treeReads map[string]int + requests int + entries []*github.TreeEntry +} + +func (f *pushFilesFixture) run(t *testing.T, files []any) *mcp.CallToolResult { + t.Helper() + f.treeReads = map[string]int{} + backend := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{"": func(w http.ResponseWriter, r *http.Request) { + f.requests++ + path := strings.TrimPrefix(r.URL.Path, "/repos/owner/repo") + if r.Method != http.MethodGet { + f.writes = append(f.writes, r.Method+" "+path) + } + ref := &github.Reference{Ref: new("refs/heads/main"), Object: &github.GitObject{SHA: new("base-commit")}} + switch { + case r.Method == http.MethodGet && path == "/git/ref/heads/main": + switch { + case f.emptyRepository: + mockResponse(t, http.StatusConflict, map[string]any{"message": "Git Repository is empty."})(w, r) + case f.missingBranch: + mockResponse(t, 404, nil)(w, r) + default: + mockResponse(t, 200, ref)(w, r) + } + case r.Method == http.MethodGet && path == "": + mockResponse(t, 200, &github.Repository{DefaultBranch: new("default")})(w, r) + case r.Method == http.MethodGet && path == "/git/ref/heads/default": + mockResponse(t, 200, &github.Reference{Ref: new("refs/heads/default"), Object: ref.Object})(w, r) + case r.Method == http.MethodPut && path == "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/contents/README.md": + mockResponse(t, 201, &github.RepositoryContentResponse{Commit: github.Commit{SHA: new("base-commit")}})(w, r) + case r.Method == http.MethodGet && path == "/git/commits/base-commit": + mockResponse(t, 200, &github.Commit{SHA: new("base-commit"), Tree: &github.Tree{SHA: new("root")}})(w, r) + case r.Method == http.MethodGet && strings.HasPrefix(path, "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/git/trees/"): + require.Empty(t, r.URL.Query().Get("recursive")) + sha := strings.TrimPrefix(path, "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/git/trees/") + f.treeReads[sha]++ + if f.lookupStatus != 0 { + mockResponse(t, f.lookupStatus, nil)(w, r) + return + } + tree, ok := f.trees[sha] + require.True(t, ok, "unexpected tree lookup: %s", sha) + mockResponse(t, 200, tree)(w, r) + case r.Method == http.MethodPost && path == "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/git/trees": + var body struct { + BaseTree string `json:"base_tree"` + Entries []*github.TreeEntry `json:"tree"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + require.Equal(t, "root", body.BaseTree) + f.entries = body.Entries + mockResponse(t, 201, &github.Tree{SHA: new("new-tree")})(w, r) + case r.Method == http.MethodPost && path == "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/git/commits": + var body map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + require.Equal(t, []any{"base-commit"}, body["parents"]) + require.Equal(t, "new-tree", body["tree"]) + mockResponse(t, 201, &github.Commit{SHA: new("new-commit")})(w, r) + case r.Method == http.MethodPost && path == "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/git/refs": + var body map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + require.Equal(t, "base-commit", body["sha"]) + require.Equal(t, "refs/heads/main", body["ref"]) + mockResponse(t, 201, ref)(w, r) + case r.Method == http.MethodPatch && path == "/git/refs/heads/main": + mockResponse(t, 200, ref)(w, r) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + http.NotFound(w, r) + } + }}) + deps := BaseDeps{Client: mustNewGHClient(t, backend)} + tool := PushFiles(translations.NullTranslationHelper) + request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo", "branch": "main", "message": "update", "files": files}) + result, err := tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + return result +} + +func pushFilesTreeEntry(path, mode, kind string) *github.TreeEntry { + return &github.TreeEntry{Path: new(path), Mode: new(mode), Type: new(kind), SHA: new(path + "-sha")} +} + +func Test_PushFiles_FileModes(t *testing.T) { + for _, content := range []string{"changed content", "same content"} { + t.Run("preserve executable "+content, func(t *testing.T) { + existing := pushFilesTreeEntry("run.sh", "100755", "blob") + existing.SHA = new(gitBlobSHA([]byte("same content"))) + f := pushFilesFixture{trees: map[string]*github.Tree{"root": {Entries: []*github.TreeEntry{existing}}}} + result := f.run(t, []any{map[string]any{"path": "run.sh", "content": content}}) + require.False(t, result.IsError) + require.Len(t, f.entries, 1) + require.Equal(t, "100755", f.entries[0].GetMode()) + require.Equal(t, content, f.entries[0].GetContent()) + }) + } + t.Run("mixed defaults explicit modes and nested cache", func(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{ + "root": {Entries: []*github.TreeEntry{pushFilesTreeEntry("ordinary", "100644", "blob"), pushFilesTreeEntry("make-exec", "100644", "blob"), pushFilesTreeEntry("exec", "100755", "blob"), pushFilesTreeEntry("untouched-link", "120000", "blob"), pushFilesTreeEntry("untouched-module", "160000", "commit"), {Path: new("dir"), Mode: new("040000"), Type: new("tree"), SHA: new("nested")}}}, + "nested": {Entries: []*github.TreeEntry{pushFilesTreeEntry("run", "100755", "blob")}}, + }} + files := []any{ + map[string]any{"path": "ordinary", "content": "a"}, + map[string]any{"path": "new", "content": "b"}, + map[string]any{"path": "exec", "content": "c", "mode": "100644"}, + map[string]any{"path": "make-exec", "content": "d", "mode": "100755"}, + map[string]any{"path": "new-exec", "content": "e", "mode": "100755"}, + map[string]any{"path": "new-ordinary", "content": "f", "mode": "100644"}, + map[string]any{"path": "dir/run", "content": "g"}, + map[string]any{"path": "dir/new", "content": "h"}, + map[string]any{"path": "absent/deep/new", "content": "i"}, + } + result := f.run(t, files) + require.False(t, result.IsError) + require.Len(t, f.entries, len(files)) + for i, mode := range []string{"100644", "100644", "100644", "100755", "100755", "100644", "100755", "100644", "100644"} { + require.Equal(t, mode, f.entries[i].GetMode()) + require.Equal(t, "blob", f.entries[i].GetType()) + } + require.Equal(t, map[string]int{"root": 1, "nested": 1}, f.treeReads) + }) +} + +func Test_PushFiles_RejectUnsupportedEntries(t *testing.T) { + for _, entry := range []*github.TreeEntry{pushFilesTreeEntry("target", "120000", "blob"), pushFilesTreeEntry("target", "160000", "commit"), pushFilesTreeEntry("target", "040000", "tree"), pushFilesTreeEntry("target", "100644", "tree")} { + for _, path := range []string{"target", "target/child"} { + // A real directory ancestor is supported; test unsupported final directory only. + if path == "target/child" && entry.GetMode() == "040000" { + continue + } + for _, explicit := range []bool{false, true} { + t.Run(entry.GetMode()+entry.GetType()+path+map[bool]string{false: " omitted", true: " explicit"}[explicit], func(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{"root": {Entries: []*github.TreeEntry{entry}}}, missingBranch: true} + file := map[string]any{"path": path, "content": "new"} + if explicit { + file["mode"] = "100755" + } + result := f.run(t, []any{map[string]any{"path": "safe", "content": "safe"}, file}) + require.True(t, result.IsError) + require.Empty(t, f.writes) + }) + } + } + } +} + +func Test_PushFiles_LookupFailsClosed(t *testing.T) { + for _, tc := range []struct { + name string + status int + tree *github.Tree + }{ + {name: "HTTP error", status: 500}, + {name: "truncated", tree: &github.Tree{Truncated: new(true)}}, + {name: "missing directory SHA", tree: &github.Tree{Entries: []*github.TreeEntry{{Path: new("dir"), Mode: new("040000"), Type: new("tree")}}}}, + } { + t.Run(tc.name, func(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{"root": tc.tree}, lookupStatus: tc.status, missingBranch: true} + result := f.run(t, []any{map[string]any{"path": "dir/run", "content": "x", "mode": "100755"}}) + require.True(t, result.IsError) + require.Empty(t, f.writes) + }) + } +} + +func Test_PushFiles_InvalidModesBeforeRequests(t *testing.T) { + for _, mode := range []any{nil, 123, true, "", "100600", "120000", "040000", "160000", []any{"100755"}} { + name, _ := json.Marshal(mode) + t.Run(string(name), func(t *testing.T) { + f := pushFilesFixture{} + result := f.run(t, []any{map[string]any{"path": "safe", "content": "safe"}, map[string]any{"path": "run", "content": "x", "mode": mode}}) + require.True(t, result.IsError) + require.Zero(t, f.requests) + require.Empty(t, f.writes) + }) + } +} + +func Test_PushFiles_NestedLookupFailsClosed(t *testing.T) { + for _, nested := range []*github.Tree{ + {Truncated: new(true)}, + {Entries: []*github.TreeEntry{pushFilesTreeEntry("link", "120000", "blob")}}, + } { + t.Run(map[bool]string{true: "truncated", false: "symlink ancestor"}[nested.GetTruncated()], func(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{ + "root": {Entries: []*github.TreeEntry{{Path: new("dir"), Mode: new("040000"), Type: new("tree"), SHA: new("nested")}}}, + "nested": nested, + }, missingBranch: true} + result := f.run(t, []any{map[string]any{"path": "dir/link/run", "content": "x"}}) + require.True(t, result.IsError) + require.Empty(t, f.writes) + require.Equal(t, map[string]int{"root": 1, "nested": 1}, f.treeReads) + }) + } +} + +func Test_PushFiles_EmptyRepositoryNewBranch(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{"root": {}}, emptyRepository: true} + result := f.run(t, []any{map[string]any{"path": "run", "content": "x", "mode": "100755"}}) + require.False(t, result.IsError) + require.Equal(t, "100755", f.entries[0].GetMode()) + require.Equal(t, []string{"PUT /contents/README.md", "POST /git/refs", "POST /git/trees", "POST /git/commits", "PATCH /git/refs/heads/main"}, f.writes) +} + +func Test_PushFiles_NewBranchPreservesModes(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{"root": {Entries: []*github.TreeEntry{pushFilesTreeEntry("run", "100755", "blob")}}}, missingBranch: true} + result := f.run(t, []any{map[string]any{"path": "run", "content": "x"}}) + require.False(t, result.IsError) + require.Equal(t, "100755", f.entries[0].GetMode()) + require.Equal(t, []string{"POST /git/refs", "POST /git/trees", "POST /git/commits", "PATCH /git/refs/heads/main"}, f.writes) +} From f91c211322cf5217cf568b7939d05807db311233 Mon Sep 17 00:00:00 2001 From: Nate Berkopec Date: Mon, 5 Oct 2026 06:52:47 +0900 Subject: [PATCH 3/3] fix: bound push_files tree traversal to 64 components Refs #2578 --- pkg/github/push_files_test.go | 33 +++++++++++++++++++++++++++++++ pkg/github/repositories_helper.go | 3 +++ 2 files changed, 36 insertions(+) diff --git a/pkg/github/push_files_test.go b/pkg/github/push_files_test.go index be1901e7d1..9a92ea93d9 100644 --- a/pkg/github/push_files_test.go +++ b/pkg/github/push_files_test.go @@ -231,3 +231,36 @@ func Test_PushFiles_NewBranchPreservesModes(t *testing.T) { require.Equal(t, "100755", f.entries[0].GetMode()) require.Equal(t, []string{"POST /git/refs", "POST /git/trees", "POST /git/commits", "PATCH /git/refs/heads/main"}, f.writes) } + +func Test_PushFiles_TreeTraversalLimit(t *testing.T) { + for _, explicit := range []bool{false, true} { + for _, depth := range []int{64, 65} { + name := strings.Repeat("dir/", depth-1) + "run" + t.Run(name+map[bool]string{false: " omitted", true: " explicit"}[explicit], func(t *testing.T) { + f := pushFilesFixture{trees: map[string]*github.Tree{}, missingBranch: true} + sha := "root" + for i := 1; i < depth; i++ { + next := sha + "-child" + f.trees[sha] = &github.Tree{Entries: []*github.TreeEntry{{Path: new("dir"), Mode: new("040000"), Type: new("tree"), SHA: new(next)}}} + sha = next + } + f.trees[sha] = &github.Tree{Entries: []*github.TreeEntry{pushFilesTreeEntry("run", "100755", "blob")}} + file := map[string]any{"path": name, "content": "x"} + if explicit { + file["mode"] = "100755" + } + result := f.run(t, []any{file}) + if depth > 64 { + require.True(t, result.IsError) + require.Contains(t, result.Content[0].(*mcp.TextContent).Text, "exceeds Git tree traversal limit") + require.Empty(t, f.treeReads) + require.Empty(t, f.writes) + } else { + require.False(t, result.IsError) + require.Len(t, f.treeReads, depth) + require.Equal(t, "100755", f.entries[0].GetMode()) + } + }) + } + } +} diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 0466693fcc..c946b41c8f 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -81,6 +81,9 @@ func resolvePushFilesModes(ctx context.Context, client *github.Client, owner, re for _, entry := range entries { treeSHA := baseTree parts := strings.Split(entry.GetPath(), "/") + if len(parts) > 64 { + return fmt.Errorf("path %q exceeds Git tree traversal limit", entry.GetPath()) + } mode := "100644" for i, part := range parts { treeEntries, cached := trees[treeSHA]