pFad - Phone/Frame/Anonymizer/Declutterfier! Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

URL: http://github.com/github/github-mcp-server/commit/4bded57e02e9346cb1fd595156e07dea51f0fb29

s://github.githubassets.com/assets/global-d18f184ea1a06a2c.css" /> Fix lockdown mode permission check (#2361) · github/github-mcp-server@4bded57 · GitHub
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)
pFad - Phonifier reborn

Pfad - The Proxy pFad © 2024 Your Company Name. All rights reserved.




Check this box to remove all script contents from the fetched content.



Check this box to remove all images from the fetched content.


Check this box to remove all CSS styles from the fetched content.


Check this box to keep images inefficiently compressed and original size.

Note: This service is not intended for secure transactions such as banking, social media, email, or purchasing. Use at your own risk. We assume no liability whatsoever for broken pages.


Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy