Skip to content

Commit 4bded57

Browse files
kerobbiSamMorrowDrums
kerobbi
and
SamMorrowDrums
authored
Fix lockdown mode permission check (#2361)
* use REST API for permission checks * update tests * skip API call for bots and add github-action[bot] to trusted logins * improve tests * add nil guard to IsSafeContent * add comment clarifying maintain mapping --------- Co-authored-by: Sam Morrow <info@sam-morrow.com>
1 parent 3a6a6f6 commit 4bded57

7 files changed

Lines changed: 168 additions & 203 deletions

File tree

‎internal/ghmcp/server.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
9191
if cfg.RepoAccessTTL != nil {
9292
opts = append(opts, lockdown.WithTTL(*cfg.RepoAccessTTL))
9393
}
94-
repoAccessCache = lockdown.GetInstance(gqlClient, opts...)
94+
repoAccessCache = lockdown.GetInstance(gqlClient, restClient, opts...)
9595
}
9696

9797
return &githubClients{

‎pkg/github/dependencies.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,8 +386,13 @@ func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAcc
386386
return nil, err
387387
}
388388

389+
restClient, err := d.GetClient(ctx)
390+
if err != nil {
391+
return nil, err
392+
}
393+
389394
// Create repo access cache
390-
instance := lockdown.GetInstance(gqlClient, d.RepoAccessOpts...)
395+
instance := lockdown.GetInstance(gqlClient, restClient, d.RepoAccessOpts...)
391396
return instance, nil
392397
}
393398

‎pkg/github/issues_test.go‎

Lines changed: 28 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313

1414
"github.com/github/github-mcp-server/internal/githubv4mock"
1515
"github.com/github/github-mcp-server/internal/toolsnaps"
16-
"github.com/github/github-mcp-server/pkg/lockdown"
1716
"github.com/github/github-mcp-server/pkg/translations"
1817
"github.com/google/go-github/v82/github"
1918
"github.com/google/jsonschema-go/jsonschema"
@@ -23,17 +22,14 @@ import (
2322
)
2423

2524
var defaultGQLClient *githubv4.Client = githubv4.NewClient(newRepoAccessHTTPClient())
26-
var repoAccessCache *lockdown.RepoAccessCache = stubRepoAccessCache(defaultGQLClient, 15*time.Minute)
2725

2826
type repoAccessKey struct {
29-
owner string
30-
repo string
31-
username string
27+
owner string
28+
repo string
3229
}
3330

3431
type repoAccessValue struct {
35-
isPrivate bool
36-
permission string
32+
isPrivate bool
3733
}
3834

3935
type repoAccessMockTransport struct {
@@ -42,8 +38,8 @@ type repoAccessMockTransport struct {
4238

4339
func newRepoAccessHTTPClient() *http.Client {
4440
responses := map[repoAccessKey]repoAccessValue{
45-
{owner: "owner2", repo: "repo2", username: "testuser2"}: {isPrivate: true},
46-
{owner: "owner", repo: "repo", username: "testuser"}: {isPrivate: false, permission: "READ"},
41+
{owner: "owner2", repo: "repo2"}: {isPrivate: true},
42+
{owner: "owner", repo: "repo"}: {isPrivate: false},
4743
}
4844

4945
return &http.Client{Transport: &repoAccessMockTransport{responses: responses}}
@@ -66,30 +62,19 @@ func (rt *repoAccessMockTransport) RoundTrip(req *http.Request) (*http.Response,
6662

6763
owner := toString(payload.Variables["owner"])
6864
repo := toString(payload.Variables["name"])
69-
username := toString(payload.Variables["username"])
7065

71-
value, ok := rt.responses[repoAccessKey{owner: owner, repo: repo, username: username}]
66+
value, ok := rt.responses[repoAccessKey{owner: owner, repo: repo}]
7267
if !ok {
73-
value = repoAccessValue{isPrivate: false, permission: "WRITE"}
74-
}
75-
76-
edges := []any{}
77-
if value.permission != "" {
78-
edges = append(edges, map[string]any{
79-
"permission": value.permission,
80-
"node": map[string]any{
81-
"login": username,
82-
},
83-
})
68+
value = repoAccessValue{isPrivate: false}
8469
}
8570

8671
responseBody, err := json.Marshal(map[string]any{
8772
"data": map[string]any{
73+
"viewer": map[string]any{
74+
"login": "test-viewer",
75+
},
8876
"repository": map[string]any{
8977
"isPrivate": value.isPrivate,
90-
"collaborators": map[string]any{
91-
"edges": edges,
92-
},
9378
},
9479
},
9580
})
@@ -170,13 +155,13 @@ func Test_GetIssue(t *testing.T) {
170155
tests := []struct {
171156
name string
172157
mockedClient *http.Client
173-
gqlHTTPClient *http.Client
174158
requestArgs map[string]any
175159
expectHandlerError bool
176160
expectResultError bool
177161
expectedIssue *github.Issue
178162
expectedErrMsg string
179163
lockdownEnabled bool
164+
restPermission string
180165
}{
181166
{
182167
name: "successful issue retrieval",
@@ -210,36 +195,6 @@ func Test_GetIssue(t *testing.T) {
210195
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
211196
GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue2),
212197
}),
213-
gqlHTTPClient: githubv4mock.NewMockedHTTPClient(
214-
githubv4mock.NewQueryMatcher(
215-
struct {
216-
Repository struct {
217-
IsPrivate githubv4.Boolean
218-
Collaborators struct {
219-
Edges []struct {
220-
Permission githubv4.String
221-
Node struct {
222-
Login githubv4.String
223-
}
224-
}
225-
} `graphql:"collaborators(query: $username, first: 1)"`
226-
} `graphql:"repository(owner: $owner, name: $name)"`
227-
}{},
228-
map[string]any{
229-
"owner": githubv4.String("owner2"),
230-
"name": githubv4.String("repo2"),
231-
"username": githubv4.String("testuser2"),
232-
},
233-
githubv4mock.DataResponse(map[string]any{
234-
"repository": map[string]any{
235-
"isPrivate": true,
236-
"collaborators": map[string]any{
237-
"edges": []any{},
238-
},
239-
},
240-
}),
241-
),
242-
),
243198
requestArgs: map[string]any{
244199
"method": "get",
245200
"owner": "owner2",
@@ -248,49 +203,13 @@ func Test_GetIssue(t *testing.T) {
248203
},
249204
expectedIssue: mockIssue2,
250205
lockdownEnabled: true,
206+
restPermission: "none",
251207
},
252208
{
253209
name: "lockdown enabled - user lacks push access",
254210
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
255211
GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue),
256212
}),
257-
gqlHTTPClient: githubv4mock.NewMockedHTTPClient(
258-
githubv4mock.NewQueryMatcher(
259-
struct {
260-
Repository struct {
261-
IsPrivate githubv4.Boolean
262-
Collaborators struct {
263-
Edges []struct {
264-
Permission githubv4.String
265-
Node struct {
266-
Login githubv4.String
267-
}
268-
}
269-
} `graphql:"collaborators(query: $username, first: 1)"`
270-
} `graphql:"repository(owner: $owner, name: $name)"`
271-
}{},
272-
map[string]any{
273-
"owner": githubv4.String("owner"),
274-
"name": githubv4.String("repo"),
275-
"username": githubv4.String("testuser"),
276-
},
277-
githubv4mock.DataResponse(map[string]any{
278-
"repository": map[string]any{
279-
"isPrivate": false,
280-
"collaborators": map[string]any{
281-
"edges": []any{
282-
map[string]any{
283-
"permission": "READ",
284-
"node": map[string]any{
285-
"login": "testuser",
286-
},
287-
},
288-
},
289-
},
290-
},
291-
}),
292-
),
293-
),
294213
requestArgs: map[string]any{
295214
"method": "get",
296215
"owner": "owner",
@@ -300,26 +219,24 @@ func Test_GetIssue(t *testing.T) {
300219
expectResultError: true,
301220
expectedErrMsg: "access to issue details is restricted by lockdown mode",
302221
lockdownEnabled: true,
222+
restPermission: "read",
303223
},
304224
}
305225

306226
for _, tc := range tests {
307227
t.Run(tc.name, func(t *testing.T) {
308228
client := github.NewClient(tc.mockedClient)
309229

310-
var gqlClient *githubv4.Client
311-
cache := repoAccessCache
312-
if tc.gqlHTTPClient != nil {
313-
gqlClient = githubv4.NewClient(tc.gqlHTTPClient)
314-
cache = stubRepoAccessCache(gqlClient, 15*time.Minute)
315-
} else {
316-
gqlClient = githubv4.NewClient(nil)
230+
var restClient *github.Client
231+
if tc.restPermission != "" {
232+
restClient = mockRESTPermissionServer(t, tc.restPermission, nil)
317233
}
234+
cache := stubRepoAccessCache(restClient, 15*time.Minute)
318235

319236
flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
320237
deps := BaseDeps{
321238
Client: client,
322-
GQLClient: gqlClient,
239+
GQLClient: defaultGQLClient,
323240
RepoAccessCache: cache,
324241
Flags: flags,
325242
}
@@ -1997,7 +1914,6 @@ func Test_GetIssueComments(t *testing.T) {
19971914
tests := []struct {
19981915
name string
19991916
mockedClient *http.Client
2000-
gqlHTTPClient *http.Client
20011917
requestArgs map[string]any
20021918
expectError bool
20031919
expectedComments []*github.IssueComment
@@ -2069,7 +1985,6 @@ func Test_GetIssueComments(t *testing.T) {
20691985
},
20701986
}),
20711987
}),
2072-
gqlHTTPClient: newRepoAccessHTTPClient(),
20731988
requestArgs: map[string]any{
20741989
"method": "get_comments",
20751990
"owner": "owner",
@@ -2092,17 +2007,18 @@ func Test_GetIssueComments(t *testing.T) {
20922007
t.Run(tc.name, func(t *testing.T) {
20932008
// Setup client with mock
20942009
client := github.NewClient(tc.mockedClient)
2095-
var gqlClient *githubv4.Client
2096-
if tc.gqlHTTPClient != nil {
2097-
gqlClient = githubv4.NewClient(tc.gqlHTTPClient)
2098-
} else {
2099-
gqlClient = githubv4.NewClient(nil)
2010+
var restClient *github.Client
2011+
if tc.lockdownEnabled {
2012+
restClient = mockRESTPermissionServer(t, "read", map[string]string{
2013+
"maintainer": "write",
2014+
"testuser": "read",
2015+
})
21002016
}
2101-
cache := stubRepoAccessCache(gqlClient, 15*time.Minute)
2017+
cache := stubRepoAccessCache(restClient, 15*time.Minute)
21022018
flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
21032019
deps := BaseDeps{
21042020
Client: client,
2105-
GQLClient: gqlClient,
2021+
GQLClient: defaultGQLClient,
21062022
RepoAccessCache: cache,
21072023
Flags: flags,
21082024
}
@@ -2223,7 +2139,7 @@ func Test_GetIssueLabels(t *testing.T) {
22232139
deps := BaseDeps{
22242140
Client: client,
22252141
GQLClient: gqlClient,
2226-
RepoAccessCache: stubRepoAccessCache(gqlClient, 15*time.Minute),
2142+
RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute),
22272143
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
22282144
}
22292145
handler := serverTool.Handler(deps)
@@ -2652,7 +2568,7 @@ func Test_GetSubIssues(t *testing.T) {
26522568
deps := BaseDeps{
26532569
Client: client,
26542570
GQLClient: gqlClient,
2655-
RepoAccessCache: stubRepoAccessCache(gqlClient, 15*time.Minute),
2571+
RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute),
26562572
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
26572573
}
26582574
handler := serverTool.Handler(deps)

‎pkg/github/pullrequests_test.go‎

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
"github.com/github/github-mcp-server/internal/githubv4mock"
1111
"github.com/github/github-mcp-server/internal/toolsnaps"
12-
"github.com/github/github-mcp-server/pkg/lockdown"
1312
"github.com/github/github-mcp-server/pkg/translations"
1413
"github.com/google/go-github/v82/github"
1514
"github.com/google/jsonschema-go/jsonschema"
@@ -101,7 +100,7 @@ func Test_GetPullRequest(t *testing.T) {
101100
deps := BaseDeps{
102101
Client: client,
103102
GQLClient: gqlClient,
104-
RepoAccessCache: stubRepoAccessCache(gqlClient, 5*time.Minute),
103+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
105104
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
106105
}
107106
handler := serverTool.Handler(deps)
@@ -1202,7 +1201,7 @@ func Test_GetPullRequestFiles(t *testing.T) {
12021201
serverTool := PullRequestRead(translations.NullTranslationHelper)
12031202
deps := BaseDeps{
12041203
Client: client,
1205-
RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute),
1204+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
12061205
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
12071206
}
12081207
handler := serverTool.Handler(deps)
@@ -1362,7 +1361,7 @@ func Test_GetPullRequestStatus(t *testing.T) {
13621361
serverTool := PullRequestRead(translations.NullTranslationHelper)
13631362
deps := BaseDeps{
13641363
Client: client,
1365-
RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute),
1364+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
13661365
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
13671366
}
13681367
handler := serverTool.Handler(deps)
@@ -1518,7 +1517,7 @@ func Test_GetPullRequestCheckRuns(t *testing.T) {
15181517
serverTool := PullRequestRead(translations.NullTranslationHelper)
15191518
deps := BaseDeps{
15201519
Client: client,
1521-
RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute),
1520+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
15221521
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
15231522
}
15241523
handler := serverTool.Handler(deps)
@@ -1937,12 +1936,15 @@ func Test_GetPullRequestComments(t *testing.T) {
19371936
}
19381937

19391938
// Setup cache for lockdown mode
1940-
var cache *lockdown.RepoAccessCache
1939+
var restClient *github.Client
19411940
if tc.lockdownEnabled {
1942-
cache = stubRepoAccessCache(githubv4.NewClient(newRepoAccessHTTPClient()), 5*time.Minute)
1943-
} else {
1944-
cache = stubRepoAccessCache(gqlClient, 5*time.Minute)
1941+
restClient = mockRESTPermissionServer(t, "read", map[string]string{
1942+
"maintainer": "write",
1943+
"external-user": "read",
1944+
"testuser": "read",
1945+
})
19451946
}
1947+
cache := stubRepoAccessCache(restClient, 5*time.Minute)
19461948

19471949
flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
19481950
serverTool := PullRequestRead(translations.NullTranslationHelper)
@@ -2083,7 +2085,6 @@ func Test_GetPullRequestReviews(t *testing.T) {
20832085
},
20842086
}),
20852087
}),
2086-
gqlHTTPClient: newRepoAccessHTTPClient(),
20872088
requestArgs: map[string]any{
20882089
"method": "get_reviews",
20892090
"owner": "owner",
@@ -2107,13 +2108,14 @@ func Test_GetPullRequestReviews(t *testing.T) {
21072108
t.Run(tc.name, func(t *testing.T) {
21082109
// Setup client with mock
21092110
client := github.NewClient(tc.mockedClient)
2110-
var gqlClient *githubv4.Client
2111-
if tc.gqlHTTPClient != nil {
2112-
gqlClient = githubv4.NewClient(tc.gqlHTTPClient)
2113-
} else {
2114-
gqlClient = githubv4.NewClient(nil)
2111+
var restClient *github.Client
2112+
if tc.lockdownEnabled {
2113+
restClient = mockRESTPermissionServer(t, "read", map[string]string{
2114+
"maintainer": "write",
2115+
"testuser": "read",
2116+
})
21152117
}
2116-
cache := stubRepoAccessCache(gqlClient, 5*time.Minute)
2118+
cache := stubRepoAccessCache(restClient, 5*time.Minute)
21172119
flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
21182120
serverTool := PullRequestRead(translations.NullTranslationHelper)
21192121
deps := BaseDeps{
@@ -3348,7 +3350,7 @@ index 5d6e7b2..8a4f5c3 100644
33483350
serverTool := PullRequestRead(translations.NullTranslationHelper)
33493351
deps := BaseDeps{
33503352
Client: client,
3351-
RepoAccessCache: stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute),
3353+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
33523354
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
33533355
}
33543356
handler := serverTool.Handler(deps)

0 commit comments

Comments
 (0)