From 3109915a50d2739d99f7392fbfc7668b74b27e0e Mon Sep 17 00:00:00 2001 From: kerobbi Date: Mon, 20 Apr 2026 21:47:22 +0100 Subject: [PATCH 1/4] use REST API for permission checks --- internal/ghmcp/server.go | 2 +- pkg/github/dependencies.go | 7 ++- pkg/lockdown/lockdown.go | 98 +++++++++++++++++++++----------------- 3 files changed, 62 insertions(+), 45 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 5dfaf596c6..8bcb7c815c 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -91,7 +91,7 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv if cfg.RepoAccessTTL != nil { opts = append(opts, lockdown.WithTTL(*cfg.RepoAccessTTL)) } - repoAccessCache = lockdown.GetInstance(gqlClient, opts...) + repoAccessCache = lockdown.GetInstance(gqlClient, restClient, opts...) } return &githubClients{ diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 57c6133a8a..aad213e4e5 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -386,8 +386,13 @@ func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAcc return nil, err } + restClient, err := d.GetClient(ctx) + if err != nil { + return nil, err + } + // Create repo access cache - instance := lockdown.GetInstance(gqlClient, d.RepoAccessOpts...) + instance := lockdown.GetInstance(gqlClient, restClient, d.RepoAccessOpts...) return instance, nil } diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index 2dceac8aa6..c3ed524d5e 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -8,6 +8,7 @@ import ( "sync" "time" + "github.com/google/go-github/v82/github" "github.com/muesli/cache2go" "github.com/shurcooL/githubv4" ) @@ -16,6 +17,7 @@ import ( // multiple tools can reuse the same access information safely across goroutines. type RepoAccessCache struct { client *githubv4.Client + restClient *github.Client mu sync.Mutex cache *cache2go.CacheTable ttl time.Duration @@ -78,27 +80,38 @@ func WithCacheName(name string) RepoAccessOption { // It initializes the instance on first call with the provided client and options. // Subsequent calls ignore the client and options parameters and return the existing instance. // This is the preferred way to access the cache in production code. -func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessCache { +func GetInstance(client *githubv4.Client, restClient *github.Client, opts ...RepoAccessOption) *RepoAccessCache { instanceMu.Lock() defer instanceMu.Unlock() if instance == nil { - instance = &RepoAccessCache{ - client: client, - cache: cache2go.Cache(defaultRepoAccessCacheKey), - ttl: defaultRepoAccessTTL, - trustedBotLogins: map[string]struct{}{ - "copilot": {}, - }, - } - for _, opt := range opts { - if opt != nil { - opt(instance) - } - } + instance = newRepoAccessCache(client, restClient, opts...) } return instance } +// NewRepoAccessCache creates a standalone cache instance, used for tests. +func NewRepoAccessCache(client *githubv4.Client, restClient *github.Client, opts ...RepoAccessOption) *RepoAccessCache { + return newRepoAccessCache(client, restClient, opts...) +} + +func newRepoAccessCache(client *githubv4.Client, restClient *github.Client, opts ...RepoAccessOption) *RepoAccessCache { + c := &RepoAccessCache{ + client: client, + restClient: restClient, + cache: cache2go.Cache(defaultRepoAccessCacheKey), + ttl: defaultRepoAccessTTL, + trustedBotLogins: map[string]struct{}{ + "copilot": {}, + }, + } + for _, opt := range opts { + if opt != nil { + opt(c) + } + } + return c +} + // SetLogger updates the logger used for cache diagnostics. func (c *RepoAccessCache) SetLogger(logger *slog.Logger) { c.mu.Lock() @@ -157,21 +170,19 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner }, nil } - c.logDebug(ctx, "known users cache miss, fetching from graphql API") + c.logDebug(ctx, "known users cache miss, fetching permission") - info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo) - if queryErr != nil { - return RepoAccessInfo{}, queryErr + hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo) + if pushErr != nil { + return RepoAccessInfo{}, pushErr } - entry.knownUsers[userKey] = info.HasPushAccess - entry.viewerLogin = info.ViewerLogin - entry.isPrivate = info.IsPrivate + entry.knownUsers[userKey] = hasPush c.cache.Add(key, c.ttl, entry) return RepoAccessInfo{ IsPrivate: entry.isPrivate, - HasPushAccess: entry.knownUsers[userKey], + HasPushAccess: hasPush, ViewerLogin: entry.viewerLogin, }, nil } @@ -208,36 +219,22 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own Login githubv4.String } Repository struct { - IsPrivate githubv4.Boolean - Collaborators struct { - Edges []struct { - Permission githubv4.String - Node struct { - Login githubv4.String - } - } - } `graphql:"collaborators(query: $username, first: 1)"` + IsPrivate githubv4.Boolean } `graphql:"repository(owner: $owner, name: $name)"` } variables := map[string]any{ - "owner": githubv4.String(owner), - "name": githubv4.String(repo), - "username": githubv4.String(username), + "owner": githubv4.String(owner), + "name": githubv4.String(repo), } if err := c.client.Query(ctx, &query, variables); err != nil { - return RepoAccessInfo{}, fmt.Errorf("failed to query repository access info: %w", err) + return RepoAccessInfo{}, fmt.Errorf("failed to query repository metadata: %w", err) } - hasPush := false - for _, edge := range query.Repository.Collaborators.Edges { - login := string(edge.Node.Login) - if strings.EqualFold(login, username) { - permission := string(edge.Permission) - hasPush = permission == "WRITE" || permission == "ADMIN" || permission == "MAINTAIN" - break - } + hasPush, err := c.checkPushAccess(ctx, username, owner, repo) + if err != nil { + return RepoAccessInfo{}, err } c.logDebug(ctx, fmt.Sprintf("queried repo access info for user %s to %s/%s: isPrivate=%t, hasPushAccess=%t, viewerLogin=%s", @@ -250,6 +247,21 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own }, nil } +// checkPushAccess checks if the user has push access to the repository via the REST permission endpoint. +func (c *RepoAccessCache) checkPushAccess(ctx context.Context, username, owner, repo string) (bool, error) { + if c.restClient == nil { + return false, fmt.Errorf("nil REST client") + } + + permLevel, _, err := c.restClient.Repositories.GetPermissionLevel(ctx, owner, repo, username) + if err != nil { + return false, fmt.Errorf("failed to get user permission level: %w", err) + } + + permission := permLevel.GetPermission() + return permission == "admin" || permission == "write", nil +} + func (c *RepoAccessCache) log(ctx context.Context, level slog.Level, msg string, attrs ...slog.Attr) { if c == nil || c.logger == nil { return From 836c41e4d44331789ec2fb408fb0fe308e46756f Mon Sep 17 00:00:00 2001 From: kerobbi Date: Mon, 20 Apr 2026 21:47:39 +0100 Subject: [PATCH 2/4] update tests --- pkg/github/issues_test.go | 133 +++++++------------------------- pkg/github/pullrequests_test.go | 21 +++-- pkg/github/server_test.go | 31 +++++++- pkg/lockdown/lockdown_test.go | 47 +++++------ 4 files changed, 95 insertions(+), 137 deletions(-) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index d06721be72..4d89fda09f 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -23,17 +23,14 @@ import ( ) var defaultGQLClient *githubv4.Client = githubv4.NewClient(newRepoAccessHTTPClient()) -var repoAccessCache *lockdown.RepoAccessCache = stubRepoAccessCache(defaultGQLClient, 15*time.Minute) type repoAccessKey struct { - owner string - repo string - username string + owner string + repo string } type repoAccessValue struct { - isPrivate bool - permission string + isPrivate bool } type repoAccessMockTransport struct { @@ -42,8 +39,8 @@ type repoAccessMockTransport struct { func newRepoAccessHTTPClient() *http.Client { responses := map[repoAccessKey]repoAccessValue{ - {owner: "owner2", repo: "repo2", username: "testuser2"}: {isPrivate: true}, - {owner: "owner", repo: "repo", username: "testuser"}: {isPrivate: false, permission: "READ"}, + {owner: "owner2", repo: "repo2"}: {isPrivate: true}, + {owner: "owner", repo: "repo"}: {isPrivate: false}, } return &http.Client{Transport: &repoAccessMockTransport{responses: responses}} @@ -66,30 +63,19 @@ func (rt *repoAccessMockTransport) RoundTrip(req *http.Request) (*http.Response, owner := toString(payload.Variables["owner"]) repo := toString(payload.Variables["name"]) - username := toString(payload.Variables["username"]) - value, ok := rt.responses[repoAccessKey{owner: owner, repo: repo, username: username}] + value, ok := rt.responses[repoAccessKey{owner: owner, repo: repo}] if !ok { - value = repoAccessValue{isPrivate: false, permission: "WRITE"} - } - - edges := []any{} - if value.permission != "" { - edges = append(edges, map[string]any{ - "permission": value.permission, - "node": map[string]any{ - "login": username, - }, - }) + value = repoAccessValue{isPrivate: false} } responseBody, err := json.Marshal(map[string]any{ "data": map[string]any{ + "viewer": map[string]any{ + "login": "test-viewer", + }, "repository": map[string]any{ "isPrivate": value.isPrivate, - "collaborators": map[string]any{ - "edges": edges, - }, }, }, }) @@ -170,13 +156,13 @@ func Test_GetIssue(t *testing.T) { tests := []struct { name string mockedClient *http.Client - gqlHTTPClient *http.Client requestArgs map[string]any expectHandlerError bool expectResultError bool expectedIssue *github.Issue expectedErrMsg string lockdownEnabled bool + restPermission string }{ { name: "successful issue retrieval", @@ -210,36 +196,6 @@ func Test_GetIssue(t *testing.T) { mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue2), }), - gqlHTTPClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - struct { - Repository struct { - IsPrivate githubv4.Boolean - Collaborators struct { - Edges []struct { - Permission githubv4.String - Node struct { - Login githubv4.String - } - } - } `graphql:"collaborators(query: $username, first: 1)"` - } `graphql:"repository(owner: $owner, name: $name)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner2"), - "name": githubv4.String("repo2"), - "username": githubv4.String("testuser2"), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "isPrivate": true, - "collaborators": map[string]any{ - "edges": []any{}, - }, - }, - }), - ), - ), requestArgs: map[string]any{ "method": "get", "owner": "owner2", @@ -248,49 +204,13 @@ func Test_GetIssue(t *testing.T) { }, expectedIssue: mockIssue2, lockdownEnabled: true, + restPermission: "none", }, { name: "lockdown enabled - user lacks push access", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue), }), - gqlHTTPClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - struct { - Repository struct { - IsPrivate githubv4.Boolean - Collaborators struct { - Edges []struct { - Permission githubv4.String - Node struct { - Login githubv4.String - } - } - } `graphql:"collaborators(query: $username, first: 1)"` - } `graphql:"repository(owner: $owner, name: $name)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "name": githubv4.String("repo"), - "username": githubv4.String("testuser"), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "isPrivate": false, - "collaborators": map[string]any{ - "edges": []any{ - map[string]any{ - "permission": "READ", - "node": map[string]any{ - "login": "testuser", - }, - }, - }, - }, - }, - }), - ), - ), requestArgs: map[string]any{ "method": "get", "owner": "owner", @@ -300,6 +220,7 @@ func Test_GetIssue(t *testing.T) { expectResultError: true, expectedErrMsg: "access to issue details is restricted by lockdown mode", lockdownEnabled: true, + restPermission: "read", }, } @@ -307,13 +228,13 @@ func Test_GetIssue(t *testing.T) { t.Run(tc.name, func(t *testing.T) { client := github.NewClient(tc.mockedClient) - var gqlClient *githubv4.Client - cache := repoAccessCache - if tc.gqlHTTPClient != nil { - gqlClient = githubv4.NewClient(tc.gqlHTTPClient) - cache = stubRepoAccessCache(gqlClient, 15*time.Minute) + gqlClient := defaultGQLClient + var cache *lockdown.RepoAccessCache + if tc.restPermission != "" { + restClient := mockRESTPermissionServer(t, tc.restPermission, nil) + cache = stubLockdownCache(t, restClient, 15*time.Minute) } else { - gqlClient = githubv4.NewClient(nil) + cache = stubRepoAccessCache(gqlClient, 15*time.Minute) } flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) @@ -1997,7 +1918,6 @@ func Test_GetIssueComments(t *testing.T) { tests := []struct { name string mockedClient *http.Client - gqlHTTPClient *http.Client requestArgs map[string]any expectError bool expectedComments []*github.IssueComment @@ -2069,7 +1989,6 @@ func Test_GetIssueComments(t *testing.T) { }, }), }), - gqlHTTPClient: newRepoAccessHTTPClient(), requestArgs: map[string]any{ "method": "get_comments", "owner": "owner", @@ -2092,13 +2011,17 @@ func Test_GetIssueComments(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - var gqlClient *githubv4.Client - if tc.gqlHTTPClient != nil { - gqlClient = githubv4.NewClient(tc.gqlHTTPClient) + gqlClient := defaultGQLClient + var cache *lockdown.RepoAccessCache + if tc.lockdownEnabled { + restClient := mockRESTPermissionServer(t, "read", map[string]string{ + "maintainer": "write", + "testuser": "read", + }) + cache = stubLockdownCache(t, restClient, 15*time.Minute) } else { - gqlClient = githubv4.NewClient(nil) + cache = stubRepoAccessCache(gqlClient, 15*time.Minute) } - cache := stubRepoAccessCache(gqlClient, 15*time.Minute) flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) deps := BaseDeps{ Client: client, diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 801122dca8..cd2982e95b 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -1939,7 +1939,12 @@ func Test_GetPullRequestComments(t *testing.T) { // Setup cache for lockdown mode var cache *lockdown.RepoAccessCache if tc.lockdownEnabled { - cache = stubRepoAccessCache(githubv4.NewClient(newRepoAccessHTTPClient()), 5*time.Minute) + restClient := mockRESTPermissionServer(t, "read", map[string]string{ + "maintainer": "write", + "external-user": "read", + "testuser": "read", + }) + cache = stubLockdownCache(t, restClient, 5*time.Minute) } else { cache = stubRepoAccessCache(gqlClient, 5*time.Minute) } @@ -2083,7 +2088,6 @@ func Test_GetPullRequestReviews(t *testing.T) { }, }), }), - gqlHTTPClient: newRepoAccessHTTPClient(), requestArgs: map[string]any{ "method": "get_reviews", "owner": "owner", @@ -2107,13 +2111,20 @@ func Test_GetPullRequestReviews(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - var gqlClient *githubv4.Client + gqlClient := defaultGQLClient if tc.gqlHTTPClient != nil { gqlClient = githubv4.NewClient(tc.gqlHTTPClient) + } + var cache *lockdown.RepoAccessCache + if tc.lockdownEnabled { + restClient := mockRESTPermissionServer(t, "read", map[string]string{ + "maintainer": "write", + "testuser": "read", + }) + cache = stubLockdownCache(t, restClient, 5*time.Minute) } else { - gqlClient = githubv4.NewClient(nil) + cache = stubRepoAccessCache(gqlClient, 5*time.Minute) } - cache := stubRepoAccessCache(gqlClient, 5*time.Minute) flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) serverTool := PullRequestRead(translations.NullTranslationHelper) deps := BaseDeps{ diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index bf29ed1329..caabc4cb1b 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -7,6 +7,7 @@ import ( "fmt" "log/slog" "net/http" + "strings" "testing" "time" @@ -99,7 +100,35 @@ func stubGQLClientFnErr(errMsg string) func(context.Context) (*githubv4.Client, func stubRepoAccessCache(client *githubv4.Client, ttl time.Duration) *lockdown.RepoAccessCache { cacheName := fmt.Sprintf("repo-access-cache-test-%d", time.Now().UnixNano()) - return lockdown.GetInstance(client, lockdown.WithTTL(ttl), lockdown.WithCacheName(cacheName)) + return lockdown.NewRepoAccessCache(client, nil, lockdown.WithTTL(ttl), lockdown.WithCacheName(cacheName)) +} + +func stubLockdownCache(t *testing.T, restClient *gogithub.Client, ttl time.Duration) *lockdown.RepoAccessCache { + t.Helper() + cacheName := fmt.Sprintf("repo-access-cache-test-%d", time.Now().UnixNano()) + return lockdown.NewRepoAccessCache( + githubv4.NewClient(newRepoAccessHTTPClient()), + restClient, + lockdown.WithTTL(ttl), + lockdown.WithCacheName(cacheName), + ) +} + +func mockRESTPermissionServer(t *testing.T, defaultPerm string, overrides map[string]string) *gogithub.Client { + t.Helper() + return gogithub.NewClient(MockHTTPClientWithHandler(func(w http.ResponseWriter, r *http.Request) { + perm := defaultPerm + for user, p := range overrides { + if strings.Contains(r.URL.Path, "/collaborators/"+user+"/") { + perm = p + break + } + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(gogithub.RepositoryPermissionLevel{ + Permission: gogithub.Ptr(perm), + }) + })) } func stubFeatureFlags(enabledFlags map[string]bool) FeatureFlags { diff --git a/pkg/lockdown/lockdown_test.go b/pkg/lockdown/lockdown_test.go index c1cf5e86b8..55e755a3ec 100644 --- a/pkg/lockdown/lockdown_test.go +++ b/pkg/lockdown/lockdown_test.go @@ -1,12 +1,16 @@ package lockdown import ( + "encoding/json" "net/http" + "net/http/httptest" + "net/url" "sync" "testing" "time" "github.com/github/github-mcp-server/internal/githubv4mock" + gogithub "github.com/google/go-github/v82/github" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/require" ) @@ -17,20 +21,12 @@ const ( testUser = "octocat" ) -type repoAccessQuery struct { +type repoMetadataQuery struct { Viewer struct { Login githubv4.String } Repository struct { - IsPrivate githubv4.Boolean - Collaborators struct { - Edges []struct { - Permission githubv4.String - Node struct { - Login githubv4.String - } - } - } `graphql:"collaborators(query: $username, first: 1)"` + IsPrivate githubv4.Boolean } `graphql:"repository(owner: $owner, name: $name)"` } @@ -56,12 +52,11 @@ func (c *countingTransport) CallCount() int { func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache, *countingTransport) { t.Helper() - var query repoAccessQuery + var query repoMetadataQuery variables := map[string]any{ - "owner": githubv4.String(testOwner), - "name": githubv4.String(testRepo), - "username": githubv4.String(testUser), + "owner": githubv4.String(testOwner), + "name": githubv4.String(testRepo), } response := githubv4mock.DataResponse(map[string]any{ @@ -70,26 +65,26 @@ func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache, }, "repository": map[string]any{ "isPrivate": false, - "collaborators": map[string]any{ - "edges": []any{ - map[string]any{ - "permission": "WRITE", - "node": map[string]any{ - "login": testUser, - }, - }, - }, - }, }, }) httpClient := githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(query, variables, response)) counting := &countingTransport{next: httpClient.Transport} httpClient.Transport = counting - gqlClient := githubv4.NewClient(httpClient) - return GetInstance(gqlClient, WithTTL(ttl)), counting + restServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + resp := gogithub.RepositoryPermissionLevel{ + Permission: gogithub.Ptr("write"), + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) + })) + t.Cleanup(restServer.Close) + restClient := gogithub.NewClient(nil) + restClient.BaseURL, _ = url.Parse(restServer.URL + "/") + + return NewRepoAccessCache(gqlClient, restClient, WithTTL(ttl)), counting } func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { From 227f73e9ff72c67bea30ed8f9bb43fccb8dd74a0 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Mon, 20 Apr 2026 21:53:47 +0100 Subject: [PATCH 3/4] skip API call for bots and add github-action[bot] to trusted logins --- pkg/lockdown/lockdown.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index c3ed524d5e..66bfdf511b 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -101,7 +101,8 @@ func newRepoAccessCache(client *githubv4.Client, restClient *github.Client, opts cache: cache2go.Cache(defaultRepoAccessCacheKey), ttl: defaultRepoAccessTTL, trustedBotLogins: map[string]struct{}{ - "copilot": {}, + "copilot": {}, + "github-actions[bot]": {}, }, } for _, opt := range opts { @@ -133,6 +134,10 @@ type CacheStats struct { // - the repository is private; // - the content was created by the viewer. 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 @@ -141,7 +146,7 @@ func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, re 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) { + if repoInfo.IsPrivate || repoInfo.ViewerLogin == strings.ToLower(username) { return true, nil } return repoInfo.HasPushAccess, nil From 4dfc946776cca05dab630e659c327523dc29ee6b Mon Sep 17 00:00:00 2001 From: kerobbi Date: Tue, 21 Apr 2026 13:07:54 +0100 Subject: [PATCH 4/4] improve tests --- pkg/github/issues_test.go | 27 ++++++++++----------------- pkg/github/pullrequests_test.go | 31 +++++++++++-------------------- pkg/github/server_test.go | 15 +++++---------- 3 files changed, 26 insertions(+), 47 deletions(-) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 4d89fda09f..9c20824746 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -13,7 +13,6 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" - "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v82/github" "github.com/google/jsonschema-go/jsonschema" @@ -228,19 +227,16 @@ func Test_GetIssue(t *testing.T) { t.Run(tc.name, func(t *testing.T) { client := github.NewClient(tc.mockedClient) - gqlClient := defaultGQLClient - var cache *lockdown.RepoAccessCache + var restClient *github.Client if tc.restPermission != "" { - restClient := mockRESTPermissionServer(t, tc.restPermission, nil) - cache = stubLockdownCache(t, restClient, 15*time.Minute) - } else { - cache = stubRepoAccessCache(gqlClient, 15*time.Minute) + restClient = mockRESTPermissionServer(t, tc.restPermission, nil) } + cache := stubRepoAccessCache(restClient, 15*time.Minute) flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) deps := BaseDeps{ Client: client, - GQLClient: gqlClient, + GQLClient: defaultGQLClient, RepoAccessCache: cache, Flags: flags, } @@ -2011,21 +2007,18 @@ func Test_GetIssueComments(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - gqlClient := defaultGQLClient - var cache *lockdown.RepoAccessCache + var restClient *github.Client if tc.lockdownEnabled { - restClient := mockRESTPermissionServer(t, "read", map[string]string{ + restClient = mockRESTPermissionServer(t, "read", map[string]string{ "maintainer": "write", "testuser": "read", }) - cache = stubLockdownCache(t, restClient, 15*time.Minute) - } else { - cache = stubRepoAccessCache(gqlClient, 15*time.Minute) } + cache := stubRepoAccessCache(restClient, 15*time.Minute) flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) deps := BaseDeps{ Client: client, - GQLClient: gqlClient, + GQLClient: defaultGQLClient, RepoAccessCache: cache, Flags: flags, } @@ -2146,7 +2139,7 @@ func Test_GetIssueLabels(t *testing.T) { deps := BaseDeps{ Client: client, GQLClient: gqlClient, - RepoAccessCache: stubRepoAccessCache(gqlClient, 15*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) @@ -2575,7 +2568,7 @@ func Test_GetSubIssues(t *testing.T) { deps := BaseDeps{ Client: client, GQLClient: gqlClient, - RepoAccessCache: stubRepoAccessCache(gqlClient, 15*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index cd2982e95b..4f0ec9493b 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -9,7 +9,6 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" - "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v82/github" "github.com/google/jsonschema-go/jsonschema" @@ -101,7 +100,7 @@ func Test_GetPullRequest(t *testing.T) { deps := BaseDeps{ Client: client, GQLClient: gqlClient, - RepoAccessCache: stubRepoAccessCache(gqlClient, 5*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) @@ -1202,7 +1201,7 @@ func Test_GetPullRequestFiles(t *testing.T) { serverTool := PullRequestRead(translations.NullTranslationHelper) deps := BaseDeps{ Client: client, - RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) @@ -1362,7 +1361,7 @@ func Test_GetPullRequestStatus(t *testing.T) { serverTool := PullRequestRead(translations.NullTranslationHelper) deps := BaseDeps{ Client: client, - RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) @@ -1518,7 +1517,7 @@ func Test_GetPullRequestCheckRuns(t *testing.T) { serverTool := PullRequestRead(translations.NullTranslationHelper) deps := BaseDeps{ Client: client, - RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) @@ -1937,17 +1936,15 @@ func Test_GetPullRequestComments(t *testing.T) { } // Setup cache for lockdown mode - var cache *lockdown.RepoAccessCache + var restClient *github.Client if tc.lockdownEnabled { - restClient := mockRESTPermissionServer(t, "read", map[string]string{ + restClient = mockRESTPermissionServer(t, "read", map[string]string{ "maintainer": "write", "external-user": "read", "testuser": "read", }) - cache = stubLockdownCache(t, restClient, 5*time.Minute) - } else { - cache = stubRepoAccessCache(gqlClient, 5*time.Minute) } + cache := stubRepoAccessCache(restClient, 5*time.Minute) flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) serverTool := PullRequestRead(translations.NullTranslationHelper) @@ -2111,20 +2108,14 @@ func Test_GetPullRequestReviews(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - gqlClient := defaultGQLClient - if tc.gqlHTTPClient != nil { - gqlClient = githubv4.NewClient(tc.gqlHTTPClient) - } - var cache *lockdown.RepoAccessCache + var restClient *github.Client if tc.lockdownEnabled { - restClient := mockRESTPermissionServer(t, "read", map[string]string{ + restClient = mockRESTPermissionServer(t, "read", map[string]string{ "maintainer": "write", "testuser": "read", }) - cache = stubLockdownCache(t, restClient, 5*time.Minute) - } else { - cache = stubRepoAccessCache(gqlClient, 5*time.Minute) } + cache := stubRepoAccessCache(restClient, 5*time.Minute) flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) serverTool := PullRequestRead(translations.NullTranslationHelper) deps := BaseDeps{ @@ -3359,7 +3350,7 @@ index 5d6e7b2..8a4f5c3 100644 serverTool := PullRequestRead(translations.NullTranslationHelper) deps := BaseDeps{ Client: client, - RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), } handler := serverTool.Handler(deps) diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index caabc4cb1b..264ffa50fe 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -98,13 +98,7 @@ func stubGQLClientFnErr(errMsg string) func(context.Context) (*githubv4.Client, } } -func stubRepoAccessCache(client *githubv4.Client, ttl time.Duration) *lockdown.RepoAccessCache { - cacheName := fmt.Sprintf("repo-access-cache-test-%d", time.Now().UnixNano()) - return lockdown.NewRepoAccessCache(client, nil, lockdown.WithTTL(ttl), lockdown.WithCacheName(cacheName)) -} - -func stubLockdownCache(t *testing.T, restClient *gogithub.Client, ttl time.Duration) *lockdown.RepoAccessCache { - t.Helper() +func stubRepoAccessCache(restClient *gogithub.Client, ttl time.Duration) *lockdown.RepoAccessCache { cacheName := fmt.Sprintf("repo-access-cache-test-%d", time.Now().UnixNano()) return lockdown.NewRepoAccessCache( githubv4.NewClient(newRepoAccessHTTPClient()), @@ -124,10 +118,11 @@ func mockRESTPermissionServer(t *testing.T, defaultPerm string, overrides map[st break } } - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(gogithub.RepositoryPermissionLevel{ + resp := gogithub.RepositoryPermissionLevel{ Permission: gogithub.Ptr(perm), - }) + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) })) }