Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions internal/ghmcp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type MCPServerConfig struct {

const stdioServerLogPrefix = "stdioserver"

func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
func NewMCPServer(cfg MCPServerConfig, logger *slog.Logger) (*server.MCPServer, error) {
apiHost, err := parseAPIHost(cfg.Host)
if err != nil {
return nil, fmt.Errorf("failed to parse API host: %w", err)
Expand All @@ -88,6 +88,9 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
if cfg.RepoAccessTTL != nil {
repoAccessOpts = append(repoAccessOpts, lockdown.WithTTL(*cfg.RepoAccessTTL))
}

repoAccessLogger := logger.With("component", "lockdown")
repoAccessOpts = append(repoAccessOpts, lockdown.WithLogger(repoAccessLogger))
var repoAccessCache *lockdown.RepoAccessCache
if cfg.LockdownMode {
repoAccessCache = lockdown.GetInstance(gqlClient, repoAccessOpts...)
Expand Down Expand Up @@ -273,7 +276,7 @@ func RunStdioServer(cfg StdioServerConfig) error {
ContentWindowSize: cfg.ContentWindowSize,
LockdownMode: cfg.LockdownMode,
RepoAccessTTL: cfg.RepoAccessCacheTTL,
})
}, logger)
if err != nil {
return fmt.Errorf("failed to create MCP server: %w", err)
}
Expand Down
65 changes: 49 additions & 16 deletions pkg/lockdown/lockdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ import (
// RepoAccessCache caches repository metadata related to lockdown checks so that
// multiple tools can reuse the same access information safely across goroutines.
type RepoAccessCache struct {
client *githubv4.Client
mu sync.Mutex
cache *cache2go.CacheTable
ttl time.Duration
logger *slog.Logger
client *githubv4.Client
mu sync.Mutex
cache *cache2go.CacheTable
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible if it's expensive enough we might want to use redis or memcached on remote server, do you have a solution in mind for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking of that, or even creating an adapter for the local server. Generally using Redis in the remote server would be the next logical step.
Two main aspects that will guide enhancing this mode are

  1. lockdown mode becomes popular and
  2. if the memory burden on maintaining this cache with TTL is too high (but I need to analyse usage patterns to be 100% sure, my gut feeling is that typical user won't be scanning thousands of repos)

ttl time.Duration
logger *slog.Logger
trustedBotLogins map[string]struct{}
}

type repoAccessCacheEntry struct {
Expand Down Expand Up @@ -85,6 +86,9 @@ func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessC
client: client,
cache: cache2go.Cache(defaultRepoAccessCacheKey),
ttl: defaultRepoAccessTTL,
trustedBotLogins: map[string]struct{}{
"copilot": {},
},
}
for _, opt := range opts {
if opt != nil {
Expand All @@ -109,13 +113,22 @@ type CacheStats struct {
Evictions int64
}

// IsSafeContent determines if the specified user can safely access the requested repository content.
// Safe access applies when any of the following is true:
// - the content was created by a trusted bot;
// - the author currently has push access to the repository;
// - the repository is private;
// - the content was created by the viewer.
Comment on lines +119 to +121
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The documentation lists the conditions in a different order than they are checked in the code. The comment states:

  1. trusted bot
  2. push access
  3. private repo
  4. viewer matches username

But the code checks:

  1. trusted bot
  2. private repo
  3. viewer matches username
  4. push access

For clarity and maintainability, the documentation should match the code order. Consider updating the comment to:

// IsSafeContent determines if the specified user can safely access the requested repository content.
// Safe access applies when any of the following is true:
// - the content was created by a trusted bot;
// - the repository is private;
// - the content was created by the viewer;
// - the author currently has push access to the repository.
Suggested change
// - the author currently has push access to the repository;
// - the repository is private;
// - the content was created by the viewer.
// - the repository is private;
// - the content was created by the viewer;
// - the author currently has push access to the repository.

Copilot uses AI. Check for mistakes.
func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, repo string) (bool, error) {
repoInfo, err := c.getRepoAccessInfo(ctx, username, owner, repo)
if err != nil {
c.logDebug("error checking repo access info for content filtering", "owner", owner, "repo", repo, "user", username, "error", err)
return false, err
}
if repoInfo.IsPrivate || repoInfo.ViewerLogin == username {

c.logDebug(ctx, fmt.Sprintf("evaluated repo access for user %s to %s/%s for content filtering, result: hasPushAccess=%t, isPrivate=%t",
username, owner, repo, repoInfo.HasPushAccess, repoInfo.IsPrivate))

if c.isTrustedBot(username) || repoInfo.IsPrivate || repoInfo.ViewerLogin == strings.ToLower(username) {
return true, nil
}
return repoInfo.HasPushAccess, nil
Comment on lines 122 to 134
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The trusted bot check should be performed before calling getRepoAccessInfo to avoid an unnecessary API query for trusted bots. Currently, the function fetches repo access information first (which may involve a GraphQL API call), then checks if the user is a trusted bot. This wastes API quota and adds latency for bot accounts.

Consider moving the trusted bot check to the beginning:

func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, repo string) (bool, error) {
	if c.isTrustedBot(username) {
		return true, nil
	}
	
	repoInfo, err := c.getRepoAccessInfo(ctx, username, owner, repo)
	if err != nil {
		return false, err
	}
	// ... rest of the logic
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds reasonable

Expand All @@ -136,30 +149,34 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
if err == nil {
entry := cacheItem.Data().(*repoAccessCacheEntry)
if cachedHasPush, known := entry.knownUsers[userKey]; known {
c.logDebug("repo access cache hit", "owner", owner, "repo", repo, "user", username)
c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo))
return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: cachedHasPush,
ViewerLogin: entry.viewerLogin,
}, nil
}
c.logDebug("known users cache miss", "owner", owner, "repo", repo, "user", username)

c.logDebug(ctx, "known users cache miss, fetching from graphql API")

info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo)
if queryErr != nil {
return RepoAccessInfo{}, queryErr
}

entry.knownUsers[userKey] = info.HasPushAccess
entry.viewerLogin = info.ViewerLogin
entry.isPrivate = info.IsPrivate
c.cache.Add(key, c.ttl, entry)

return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: entry.knownUsers[userKey],
ViewerLogin: entry.viewerLogin,
}, nil
}

c.logDebug("repo access cache miss", "owner", owner, "repo", repo, "user", username)
Copy link
Contributor

Choose a reason for hiding this comment

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

why you decided to remove attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to be too verbose but this is good point, I re-added them. It will make debugging easier
CleanShot 2025-11-24 at 11 16 06@2x

c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo))

info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo)
if queryErr != nil {
Expand Down Expand Up @@ -223,19 +240,35 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own
}
}

c.logDebug(ctx, fmt.Sprintf("queried repo access info for user %s to %s/%s: isPrivate=%t, hasPushAccess=%t, viewerLogin=%s",
username, owner, repo, bool(query.Repository.IsPrivate), hasPush, query.Viewer.Login))

return RepoAccessInfo{
IsPrivate: bool(query.Repository.IsPrivate),
HasPushAccess: hasPush,
ViewerLogin: string(query.Viewer.Login),
}, nil
}

func cacheKey(owner, repo string) string {
return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo))
func (c *RepoAccessCache) log(ctx context.Context, level slog.Level, msg string, attrs ...slog.Attr) {
if c == nil || c.logger == nil {
return
}
if !c.logger.Enabled(ctx, level) {
return
}
c.logger.LogAttrs(ctx, level, msg, attrs...)
}

func (c *RepoAccessCache) logDebug(msg string, args ...any) {
if c != nil && c.logger != nil {
c.logger.Debug(msg, args...)
}
func (c *RepoAccessCache) logDebug(ctx context.Context, msg string, attrs ...slog.Attr) {
c.log(ctx, slog.LevelDebug, msg, attrs...)
}

func (c *RepoAccessCache) isTrustedBot(username string) bool {
_, ok := c.trustedBotLogins[strings.ToLower(username)]
return ok
}
Comment on lines +267 to +270
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The new trusted bot functionality lacks test coverage. Since other functions in this package have tests (e.g., TestRepoAccessCacheEvictsAfterTTL), the new behavior should also be tested.

Consider adding test cases for:

  1. Verifying that "copilot" (case-insensitive) is recognized as a trusted bot
  2. Ensuring IsSafeContent returns true for trusted bots without querying the API
  3. Testing that non-trusted usernames still trigger the normal access checks

Copilot uses AI. Check for mistakes.

func cacheKey(owner, repo string) string {
return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo))
}
Loading