Skip to content

Lockdown mode: scope RepoAccessCache per request#2571

Merged
SamMorrowDrums merged 2 commits into
mainfrom
kerobbi/lockdown-singleton-fix
May 29, 2026
Merged

Lockdown mode: scope RepoAccessCache per request#2571
SamMorrowDrums merged 2 commits into
mainfrom
kerobbi/lockdown-singleton-fix

Conversation

@kerobbi
Copy link
Copy Markdown
Contributor

@kerobbi kerobbi commented May 29, 2026

Summary

Scope RepoAccessCache per request so viewer identity is no longer reused across callers via the process-shared cache table.

Lockdown's trust model is unchanged.

Reported in GHSA-pjp5-fpmr-3349 by @hewei-gikaku and @matte1782.

Why

lockdown.GetInstance returned a process-wide *RepoAccessCache whose entries cached the viewer login of the first request to populate them. Under HTTP mode with multiple callers, subsequent requests could observe a viewer identity carried over from an earlier request, causing IsSafeContent's viewer-self check to evaluate against the wrong identity.

What changed

  • Replaced lockdown.GetInstance with per-request lockdown.NewRepoAccessCache instances bound to the caller's GraphQL/REST clients
  • Kept the cache2go table process-shared since entries now hold only viewer-independent fields (isPrivate, knownUsers)
  • Moved viewer login onto *RepoAccessCache behind sync.MutexviewerLoginFor lazy-queries (rejecting empty responses), setViewerLogin piggy-backs on the repo-metadata query
  • Deferred viewer-login resolution in IsSafeContent until both private and push-access branches miss
  • Published cache entries copy-on-write so readers on one instance can't observe partial mutations from another
  • Removed dead bits (SetLogger, RepoAccessInfo.ViewerLogin)
  • Updated existing tests and added new ones to cover viewer isolation, transient-error retry, and empty-response rejection

Impact

  • Scoped to HTTP deployments serving more than one user with lockdown enabled
  • No auth bypass — each request's API calls still run under its own token
  • No confidentiality impact — the cached fields (isPrivate, knownUsers) don't leak across callers
  • Stdio/single-user deployments unaffected

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
  • Data exposure, filtering, or token/size limits considered

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 requested a review from a team as a code owner May 29, 2026 09:49
Copilot AI review requested due to automatic review settings May 29, 2026 09:49
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 scopes lockdown repository access caching so viewer identity is held per RepoAccessCache instance while keeping repository metadata in the shared cache table.

Changes:

  • Replaces process-wide lockdown.GetInstance usage with NewRepoAccessCache.
  • Moves viewer login caching onto the cache instance and defers viewer lookup until needed.
  • Updates lockdown and GitHub tests for per-instance viewer isolation and viewer-login retry behavior.
Show a summary per file
File Description
pkg/lockdown/lockdown.go Refactors lockdown cache construction, viewer login handling, and shared cache entry contents.
pkg/lockdown/lockdown_test.go Adds tests for viewer isolation, transient retry, and empty viewer login handling.
pkg/github/issues_test.go Adjusts GraphQL mock responses for separate viewer/repository queries.
pkg/github/dependencies.go Creates a new repo access cache from request dependencies.
internal/ghmcp/server.go Uses the new cache constructor for stdio server setup.

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 3

Comment thread pkg/lockdown/lockdown.go
Comment thread pkg/lockdown/lockdown.go
Comment thread pkg/lockdown/lockdown.go
@SamMorrowDrums SamMorrowDrums merged commit b5397f6 into main May 29, 2026
19 checks passed
@SamMorrowDrums SamMorrowDrums deleted the kerobbi/lockdown-singleton-fix branch May 29, 2026 10:24
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.

3 participants