From 840096462d21b76c6a39b84629d89a5994486feb Mon Sep 17 00:00:00 2001 From: JoannaaKL Date: Fri, 21 Nov 2025 11:55:39 +0100 Subject: [PATCH 1/4] Dont filter content from trusted bots --- pkg/lockdown/lockdown.go | 46 ++++++++++++++++++++++++++--------- pkg/lockdown/lockdown_test.go | 8 ++++-- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index 4c3500440..75407944f 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -15,17 +15,19 @@ 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 + ttl time.Duration + logger *slog.Logger + trustedBotLogins map[string]struct{} } type repoAccessCacheEntry struct { isPrivate bool knownUsers map[string]bool // normalized login -> has push access viewerLogin string + viewerType string } // RepoAccessInfo captures repository metadata needed for lockdown decisions. @@ -33,6 +35,7 @@ type RepoAccessInfo struct { IsPrivate bool HasPushAccess bool ViewerLogin string + ViewerType string } const ( @@ -85,6 +88,12 @@ func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessC client: client, cache: cache2go.Cache(defaultRepoAccessCacheKey), ttl: defaultRepoAccessTTL, + trustedBotLogins: map[string]struct{}{ + "dependabot[bot]": {}, + "dependabot-preview[bot]": {}, + "github-actions[bot]": {}, + "github-copilot[bot]": {}, + }, } for _, opt := range opts { if opt != nil { @@ -115,7 +124,8 @@ func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, re 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 { + + if c.isTrustedBot(username, repoInfo.ViewerType) || repoInfo.IsPrivate || repoInfo.ViewerLogin == strings.ToLower(username) { return true, nil } return repoInfo.HasPushAccess, nil @@ -150,12 +160,14 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner } entry.knownUsers[userKey] = info.HasPushAccess entry.viewerLogin = info.ViewerLogin + entry.viewerType = info.ViewerType entry.isPrivate = info.IsPrivate c.cache.Add(key, c.ttl, entry) return RepoAccessInfo{ IsPrivate: entry.isPrivate, HasPushAccess: entry.knownUsers[userKey], ViewerLogin: entry.viewerLogin, + ViewerType: entry.viewerType, }, nil } @@ -171,6 +183,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner knownUsers: map[string]bool{userKey: info.HasPushAccess}, isPrivate: info.IsPrivate, viewerLogin: info.ViewerLogin, + viewerType: info.ViewerType, } c.cache.Add(key, c.ttl, entry) @@ -178,6 +191,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner IsPrivate: entry.isPrivate, HasPushAccess: entry.knownUsers[userKey], ViewerLogin: entry.viewerLogin, + ViewerType: entry.viewerType, }, nil } @@ -188,7 +202,8 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own var query struct { Viewer struct { - Login githubv4.String + Typename string `graphql:"__typename"` + Login githubv4.String } Repository struct { IsPrivate githubv4.Boolean @@ -227,15 +242,24 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own IsPrivate: bool(query.Repository.IsPrivate), HasPushAccess: hasPush, ViewerLogin: string(query.Viewer.Login), + ViewerType: query.Viewer.Typename, }, nil } -func cacheKey(owner, repo string) string { - return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo)) -} - func (c *RepoAccessCache) logDebug(msg string, args ...any) { if c != nil && c.logger != nil { c.logger.Debug(msg, args...) } } + +func (c *RepoAccessCache) isTrustedBot(username string, viewerType string) bool { + if viewerType != "Bot" { + return false + } + _, ok := c.trustedBotLogins[strings.ToLower(username)] + return ok +} + +func cacheKey(owner, repo string) string { + return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo)) +} diff --git a/pkg/lockdown/lockdown_test.go b/pkg/lockdown/lockdown_test.go index c1cf5e86b..d9d0b88f9 100644 --- a/pkg/lockdown/lockdown_test.go +++ b/pkg/lockdown/lockdown_test.go @@ -19,7 +19,8 @@ const ( type repoAccessQuery struct { Viewer struct { - Login githubv4.String + Typename string `graphql:"__typename"` + Login githubv4.String } Repository struct { IsPrivate githubv4.Boolean @@ -66,7 +67,8 @@ func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache, response := githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ - "login": testUser, + "__typename": "User", + "login": testUser, }, "repository": map[string]any{ "isPrivate": false, @@ -99,6 +101,7 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) require.NoError(t, err) require.Equal(t, testUser, info.ViewerLogin) + require.Equal(t, "User", info.ViewerType) require.True(t, info.HasPushAccess) require.EqualValues(t, 1, transport.CallCount()) @@ -107,6 +110,7 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) require.NoError(t, err) require.Equal(t, testUser, info.ViewerLogin) + require.Equal(t, "User", info.ViewerType) require.True(t, info.HasPushAccess) require.EqualValues(t, 2, transport.CallCount()) } From 8ea409b761fb040321cdb70566dee8b6ed02a503 Mon Sep 17 00:00:00 2001 From: JoannaaKL Date: Fri, 21 Nov 2025 17:15:33 +0100 Subject: [PATCH 2/4] Final changes --- internal/ghmcp/server.go | 7 +++-- pkg/lockdown/lockdown.go | 54 +++++++++++++++++++---------------- pkg/lockdown/lockdown_test.go | 8 ++---- 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 15b1efc10..970d230ab 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -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) @@ -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...) @@ -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) } diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index 75407944f..e352c2d5c 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -27,7 +27,6 @@ type repoAccessCacheEntry struct { isPrivate bool knownUsers map[string]bool // normalized login -> has push access viewerLogin string - viewerType string } // RepoAccessInfo captures repository metadata needed for lockdown decisions. @@ -35,7 +34,6 @@ type RepoAccessInfo struct { IsPrivate bool HasPushAccess bool ViewerLogin string - ViewerType string } const ( @@ -89,10 +87,7 @@ func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessC cache: cache2go.Cache(defaultRepoAccessCacheKey), ttl: defaultRepoAccessTTL, trustedBotLogins: map[string]struct{}{ - "dependabot[bot]": {}, - "dependabot-preview[bot]": {}, - "github-actions[bot]": {}, - "github-copilot[bot]": {}, + "copilot": {}, }, } for _, opt := range opts { @@ -121,11 +116,13 @@ type CacheStats struct { 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 c.isTrustedBot(username, repoInfo.ViewerType) || repoInfo.IsPrivate || repoInfo.ViewerLogin == strings.ToLower(username) { + c.logInfo(ctx, fmt.Sprintf("evaluated repo access fur 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 @@ -146,32 +143,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, "repo access cache hit") 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") + info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo) if queryErr != nil { return RepoAccessInfo{}, queryErr } + entry.knownUsers[userKey] = info.HasPushAccess entry.viewerLogin = info.ViewerLogin - entry.viewerType = info.ViewerType entry.isPrivate = info.IsPrivate c.cache.Add(key, c.ttl, entry) + return RepoAccessInfo{ IsPrivate: entry.isPrivate, HasPushAccess: entry.knownUsers[userKey], ViewerLogin: entry.viewerLogin, - ViewerType: entry.viewerType, }, nil } - c.logDebug("repo access cache miss", "owner", owner, "repo", repo, "user", username) + c.logDebug(ctx, "repo access cache miss") info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo) if queryErr != nil { @@ -183,7 +182,6 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner knownUsers: map[string]bool{userKey: info.HasPushAccess}, isPrivate: info.IsPrivate, viewerLogin: info.ViewerLogin, - viewerType: info.ViewerType, } c.cache.Add(key, c.ttl, entry) @@ -191,7 +189,6 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner IsPrivate: entry.isPrivate, HasPushAccess: entry.knownUsers[userKey], ViewerLogin: entry.viewerLogin, - ViewerType: entry.viewerType, }, nil } @@ -202,8 +199,7 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own var query struct { Viewer struct { - Typename string `graphql:"__typename"` - Login githubv4.String + Login githubv4.String } Repository struct { IsPrivate githubv4.Boolean @@ -242,20 +238,28 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own IsPrivate: bool(query.Repository.IsPrivate), HasPushAccess: hasPush, ViewerLogin: string(query.Viewer.Login), - ViewerType: query.Viewer.Typename, }, nil } -func (c *RepoAccessCache) logDebug(msg string, args ...any) { - if c != nil && c.logger != nil { - c.logger.Debug(msg, args...) +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) isTrustedBot(username string, viewerType string) bool { - if viewerType != "Bot" { - return false - } +func (c *RepoAccessCache) logDebug(ctx context.Context, msg string, attrs ...slog.Attr) { + c.log(ctx, slog.LevelDebug, msg, attrs...) +} + +func (c *RepoAccessCache) logInfo(ctx context.Context, msg string, attrs ...slog.Attr) { + c.log(ctx, slog.LevelInfo, msg, attrs...) +} + +func (c *RepoAccessCache) isTrustedBot(username string) bool { _, ok := c.trustedBotLogins[strings.ToLower(username)] return ok } diff --git a/pkg/lockdown/lockdown_test.go b/pkg/lockdown/lockdown_test.go index d9d0b88f9..c1cf5e86b 100644 --- a/pkg/lockdown/lockdown_test.go +++ b/pkg/lockdown/lockdown_test.go @@ -19,8 +19,7 @@ const ( type repoAccessQuery struct { Viewer struct { - Typename string `graphql:"__typename"` - Login githubv4.String + Login githubv4.String } Repository struct { IsPrivate githubv4.Boolean @@ -67,8 +66,7 @@ func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache, response := githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ - "__typename": "User", - "login": testUser, + "login": testUser, }, "repository": map[string]any{ "isPrivate": false, @@ -101,7 +99,6 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) require.NoError(t, err) require.Equal(t, testUser, info.ViewerLogin) - require.Equal(t, "User", info.ViewerType) require.True(t, info.HasPushAccess) require.EqualValues(t, 1, transport.CallCount()) @@ -110,7 +107,6 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) require.NoError(t, err) require.Equal(t, testUser, info.ViewerLogin) - require.Equal(t, "User", info.ViewerType) require.True(t, info.HasPushAccess) require.EqualValues(t, 2, transport.CallCount()) } From 881dc82fa4c52c8d0e71cb16e02936d56b916086 Mon Sep 17 00:00:00 2001 From: JoannaaKL Date: Mon, 24 Nov 2025 10:35:59 +0100 Subject: [PATCH 3/4] Use only debug level --- pkg/lockdown/lockdown.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index e352c2d5c..17f7d9232 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -119,7 +119,7 @@ func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, re return false, err } - c.logInfo(ctx, fmt.Sprintf("evaluated repo access fur user %s to %s/%s for content filtering, result: hasPushAccess=%t, isPrivate=%t", + c.logDebug(ctx, fmt.Sprintf("evaluated repo access fur 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) { @@ -255,10 +255,6 @@ func (c *RepoAccessCache) logDebug(ctx context.Context, msg string, attrs ...slo c.log(ctx, slog.LevelDebug, msg, attrs...) } -func (c *RepoAccessCache) logInfo(ctx context.Context, msg string, attrs ...slog.Attr) { - c.log(ctx, slog.LevelInfo, msg, attrs...) -} - func (c *RepoAccessCache) isTrustedBot(username string) bool { _, ok := c.trustedBotLogins[strings.ToLower(username)] return ok From c5041413d910c23e85706ae523d2367d1fb4df45 Mon Sep 17 00:00:00 2001 From: JoannaaKL Date: Mon, 24 Nov 2025 11:15:05 +0100 Subject: [PATCH 4/4] Add logs and comments --- pkg/lockdown/lockdown.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index 17f7d9232..80eca07f8 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -113,13 +113,19 @@ 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. func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, repo string) (bool, error) { repoInfo, err := c.getRepoAccessInfo(ctx, username, owner, repo) if err != nil { return false, err } - c.logDebug(ctx, fmt.Sprintf("evaluated repo access fur user %s to %s/%s for content filtering, result: hasPushAccess=%t, isPrivate=%t", + 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) { @@ -143,7 +149,7 @@ 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(ctx, "repo access cache hit") + 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, @@ -151,7 +157,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner }, nil } - c.logDebug(ctx, "known users cache miss") + c.logDebug(ctx, "known users cache miss, fetching from graphql API") info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo) if queryErr != nil { @@ -170,7 +176,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner }, nil } - c.logDebug(ctx, "repo access cache miss") + 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 { @@ -234,6 +240,9 @@ 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,