From cd1642ae8e864aa3f6e6e849d3e73b3eeeea271a Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 14:16:18 +0200 Subject: [PATCH] fix(lockdown): harden pull_request_read get_commits handling pull_request_read's get_commits method previously returned commit messages without any lockdown check, unlike get_diff and get_files which restrict the whole result when the PR author lacks push access. Commit content is part of the same untrusted head branch as the diff and file list, so GetPullRequestCommits now reuses enforcePullRequestLockdown for consistent, fail-closed behavior without adding a per-commit permission lookup. Also updates the lockdown documentation in README.md and docs/server-configuration.md to: - list pull_request_read:get_diff, get_files, and get_commits among the tools that error when the PR author lacks push access (get_diff and get_files were already implemented this way but undocumented) - clarify that lockdown mode is a best-effort content filter to reduce prompt-injection risk, not an authorization boundary - document the existing intentional trusted-bot exception (github-actions[bot], copilot) accurately Fixes #3105 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 7 +++ docs/server-configuration.md | 4 ++ pkg/github/pullrequests.go | 14 ++++- pkg/github/pullrequests_test.go | 93 ++++++++++++++++++++++++++++++++- 4 files changed, 114 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 67943bc5c4..6aafea5590 100644 --- a/README.md +++ b/README.md @@ -1602,6 +1602,10 @@ docker run -i --rm \ Lockdown mode limits the content that the server will surface from public repositories. When enabled, the server checks whether the author of each item has push access to the repository. Private repositories are unaffected, and collaborators keep full access to their own content. +Lockdown mode is a best-effort content filter intended to reduce the risk of prompt injection from untrusted repository content (issues, pull requests, comments, commits, etc.). It is **not** an authorization boundary: it does not change what the underlying GitHub credential can read or write, and content withheld from a filtered tool response may still be reachable through other tools or direct GitHub API access with the same credential. + +As an intentional exception, content authored by a small set of trusted bot accounts (currently `github-actions[bot]` and `copilot`) is always treated as safe, regardless of push access. This avoids filtering routine automation output (e.g. CI-generated commits or comments) that would otherwise be withheld under lockdown mode. + ```bash ./github-mcp-server --lockdown-mode ``` @@ -1621,6 +1625,9 @@ Following tools will return an error when the author lacks the push access: - `issue_read:get` - `pull_request_read:get` +- `pull_request_read:get_diff` +- `pull_request_read:get_files` +- `pull_request_read:get_commits` Following tools will filter out content from users lacking the push access: diff --git a/docs/server-configuration.md b/docs/server-configuration.md index 500c4bb868..20e745cab2 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -292,6 +292,10 @@ When active, this mode will disable all tools that are not read-only even if the Lockdown mode ensures the server only surfaces content in public repositories from users with push access to that repository. Private repositories are unaffected, and collaborators retain full access to their own content. +Lockdown mode is a best-effort content filter meant to reduce prompt-injection risk from untrusted repository content; it is not an authorization boundary. It does not restrict what the underlying credential can otherwise read or write, and content withheld from a filtered tool response may still be reachable through other tools or direct GitHub API access with the same credential. + +As an intentional exception, content authored by trusted bot accounts (currently `github-actions[bot]` and `copilot`) is always treated as safe, regardless of push access, so routine automation output isn't filtered. + **Example:** diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index a86b699f7f..3907ebc0ec 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -132,7 +132,7 @@ Possible options: result, err := GetPullRequestFiles(ctx, client, deps, owner, repo, pullNumber, pagination) return attachIFC(result), nil, err case "get_commits": - result, err := GetPullRequestCommits(ctx, client, owner, repo, pullNumber, pagination) + result, err := GetPullRequestCommits(ctx, client, deps, owner, repo, pullNumber, pagination) return attachIFC(result), nil, err case "get_review_comments": gqlClient, err := deps.GetGQLClient(ctx) @@ -412,7 +412,17 @@ func GetPullRequestFiles(ctx context.Context, client *github.Client, deps ToolDe return MarshalledTextResult(minimalFiles), nil } -func GetPullRequestCommits(ctx context.Context, client *github.Client, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { +// GetPullRequestCommits returns the commits on a pull request. Commit messages +// are user-authored content like the PR diff and files, so under lockdown mode +// this applies the same PR-author check as GetPullRequestDiff/GetPullRequestFiles +// rather than filtering individual commits: all commits on a pull request are +// part of the same untrusted head branch, so a single check on the PR author is +// sufficient and avoids an extra permission lookup per commit. +func GetPullRequestCommits(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { + if restricted, err := enforcePullRequestLockdown(ctx, client, deps, owner, repo, pullNumber); restricted != nil || err != nil { + return restricted, err + } + opts := &github.ListOptions{ PerPage: pagination.PerPage, Page: pagination.Page, diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 1edf16e7b7..ec8fa8e86f 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -1434,6 +1434,8 @@ func Test_GetPullRequestCommits(t *testing.T) { expectError bool expectedCommits []*github.RepositoryCommit expectedErrMsg string + lockdownEnabled bool + restPermission string }{ { name: "successful commits fetch", @@ -1497,16 +1499,103 @@ func Test_GetPullRequestCommits(t *testing.T) { expectError: true, expectedErrMsg: "failed to get pull request commits", }, + { + name: "lockdown enabled - author lacks push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, &github.PullRequest{ + Number: github.Ptr(42), + User: &github.User{Login: github.Ptr("reader")}, + }), + }), + requestArgs: map[string]any{ + "method": "get_commits", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + lockdownEnabled: true, + restPermission: "read", + expectError: true, + expectedErrMsg: "access to pull request is restricted by lockdown mode", + }, + { + name: "lockdown enabled - author has push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, &github.PullRequest{ + Number: github.Ptr(42), + User: &github.User{Login: github.Ptr("writer")}, + }), + GetReposPullsCommitsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockCommits), + }), + requestArgs: map[string]any{ + "method": "get_commits", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + lockdownEnabled: true, + restPermission: "write", + expectError: false, + expectedCommits: mockCommits, + }, + { + // Trusted bot logins (e.g. github-actions[bot], copilot) are treated as + // safe content sources regardless of push access, matching the + // intentional exception documented for lockdown mode. + name: "lockdown enabled - trusted bot author lacks push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, &github.PullRequest{ + Number: github.Ptr(42), + User: &github.User{Login: github.Ptr("github-actions[bot]")}, + }), + GetReposPullsCommitsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockCommits), + }), + requestArgs: map[string]any{ + "method": "get_commits", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + lockdownEnabled: true, + restPermission: "read", + expectError: false, + expectedCommits: mockCommits, + }, + { + name: "lockdown enabled - pull request fetch fails", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + }), + requestArgs: map[string]any{ + "method": "get_commits", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(999), + }, + lockdownEnabled: true, + restPermission: "read", + expectError: true, + expectedErrMsg: "failed to get pull request", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { client := mustNewGHClient(t, tc.mockedClient) serverTool := PullRequestRead(translations.NullTranslationHelper) + + var restClient *github.Client + if tc.lockdownEnabled { + restClient = mockRESTPermissionServer(t, tc.restPermission, nil) + } + deps := BaseDeps{ Client: client, - RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), - Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + RepoAccessCache: stubRepoAccessCache(restClient, 5*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}), } handler := serverTool.Handler(deps) request := createMCPRequest(tc.requestArgs)
Remote ServerLocal Server