Skip to content

Fix lockdown mode permission check#2361

Open
kerobbi wants to merge 5 commits intomainfrom
kerobbi/lockdown-fix
Open

Fix lockdown mode permission check#2361
kerobbi wants to merge 5 commits intomainfrom
kerobbi/lockdown-fix

Conversation

@kerobbi
Copy link
Copy Markdown
Contributor

@kerobbi kerobbi commented Apr 21, 2026

Summary

Replaces the GraphQL repository.collaborators permission check in lockdown mode with the REST GetPermissionLevel endpoint, and add github-actions[bot] to the trusted bots list.

Lockdown filtering behaviour is preserved; same trust model (push access = trusted), same fail-closed error handling.

Why

Lockdown mode filters issue and PR content on public repos based on whether the author has push access to the repository. It does this using the GraphQL repository.collaborators field, but this field requires the calling token to have admin/write access on the repo to enumerate collaborators. GITHUB_TOKEN and GitHub App installation tokens typically don't have this, so the query silently returns empty results (no error), and lockdown ends up treating every author as untrusted, blocking all content.

The REST GetPermissionLevel endpoint works with any authenticated token and returns the exact permission level.

What changed

  • Replaced GraphQL collaborators query with REST GetPermissionLevel for permission checks (kept GraphQL for repo metadata only: viewer.login, isPrivate)
  • Added restClient as a positional parameter on GetInstance and wired it through
  • Optimised cache-hit path: when repo metadata is already cached for a repo, new user permission lookups skip GraphQL and go straight to REST
  • Added NewRepoAccessCache constructor for test isolation
  • Added github-actions[bot] to trustedBotLogins
  • Moved isTrustedBot check before API calls in IsSafeContent to skip unnecessary lookups for trusted bots
  • Updated all lockdown tests to reflect the new REST-based flow

MCP impact

  • No tool or API changes
  • Tool schema or behavior changed
  • New tool added

Security / limits

  • No security or limits impact
  • Auth / permissions considered - REST GetPermissionLevel requires only metadata: read (included by default on all token types as far as I know) and works with any authenticated token on public repos.
  • Data exposure, filtering, or token/size limits considered - all error paths remain fail-closed (content blocked). The REST endpoint is publicly callable by any authenticated user, so lockdown doesn't expose new information.

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
    • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR

Lint & tests

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed
  • Updated (README / docs / examples)

@kerobbi kerobbi marked this pull request as ready for review April 22, 2026 09:14
@kerobbi kerobbi requested a review from a team as a code owner April 22, 2026 09:14
Copilot AI review requested due to automatic review settings April 22, 2026 09:14
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes lockdown-mode permission checks by switching from a GraphQL collaborators-based approach (which can silently return empty results under limited tokens) to the REST Repositories.GetPermissionLevel endpoint, and expands the trusted bot list to avoid unnecessary lookups for known-safe actors.

Changes:

  • Replace GraphQL collaborators permission checks with REST GetPermissionLevel, keeping GraphQL only for repo metadata (viewer.login, isPrivate).
  • Thread a REST client into the lockdown RepoAccessCache (singleton + new constructor for test isolation) and optimize cache-hit flows to skip GraphQL.
  • Update lockdown-related tests and add github-actions[bot] to the trusted bot list.
Show a summary per file
File Description
pkg/lockdown/lockdown.go Switches permission lookup to REST and adjusts cache construction / trusted-bot behavior.
pkg/lockdown/lockdown_test.go Updates cache tests to mock REST permission responses.
pkg/github/server_test.go Refactors test helpers to build a RepoAccessCache with an optional REST client + REST permission mock.
pkg/github/pullrequests_test.go Updates PR lockdown tests to use REST permission mocking.
pkg/github/issues_test.go Updates issue lockdown tests to use REST permission mocking and revised GraphQL metadata mock.
pkg/github/dependencies.go Wires REST client into lockdown.GetInstance during request dependency construction.
internal/ghmcp/server.go Wires REST client into lockdown.GetInstance for stdio server initialization.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 4

Comment thread pkg/github/pullrequests_test.go
Comment thread pkg/lockdown/lockdown.go
Comment thread pkg/lockdown/lockdown.go
Comment thread pkg/github/issues_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants