Skip to content

Commit 660bd4f

Browse files
committed
fix(scopes): allow public_repo scope for public repository write tools
Several repository write tools declared the broad `repo` scope, which hid them from tokens limited to `public_repo` and forced public-only OAuth deployments to request private-repository access. Lower the required scope to public_repo for tools that only operate on repositories the token can already reach: - add_issue_comment - issue_write - create_branch - push_files - create_pull_request - fork_repository Because RequiredScopes are expanded through the scope hierarchy, a full repo token remains accepted for every tool. GitHub continues to enforce actual per-repository permissions at the API layer. Fixes #3136
1 parent febc329 commit 660bd4f

7 files changed

Lines changed: 114 additions & 11 deletions

File tree

pkg/github/issues.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1403,7 +1403,7 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
14031403
Required: []string{"owner", "repo", "issue_number"},
14041404
},
14051405
},
1406-
scopes.RequireAll(scopes.Repo),
1406+
scopes.RequireAll(scopes.PublicRepo),
14071407
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
14081408
owner, err := RequiredParam[string](args, "owner")
14091409
if err != nil {
@@ -2524,7 +2524,7 @@ Options are:
25242524
Required: []string{"method", "owner", "repo"},
25252525
},
25262526
},
2527-
scopes.RequireAll(scopes.Repo),
2527+
scopes.RequireAll(scopes.PublicRepo),
25282528
func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
25292529
method, err := RequiredParam[string](args, "method")
25302530
if err != nil {
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
package github
2+
3+
import (
4+
"testing"
5+
6+
"github.com/github/github-mcp-server/pkg/inventory"
7+
"github.com/github/github-mcp-server/pkg/scopes"
8+
"github.com/github/github-mcp-server/pkg/translations"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// TestPublicRepoWriteToolsDeclareLeastPrivilegeScopes asserts that the public
14+
// repository write tools advertise `public_repo` as their required scope while
15+
// still accepting a full `repo` token (via the scope hierarchy). This enables
16+
// least-privilege, public-only OAuth deployments instead of forcing the broad
17+
// `repo` scope, which also grants private-repository access.
18+
// See https://github.com/github/github-mcp-server/issues/3136
19+
func TestPublicRepoWriteToolsDeclareLeastPrivilegeScopes(t *testing.T) {
20+
t.Parallel()
21+
22+
builders := map[string]func(translations.TranslationHelperFunc) *inventory.ServerTool{
23+
"add_issue_comment": wrapToolBuilder(AddIssueComment),
24+
"issue_write": wrapToolBuilder(IssueWrite),
25+
"create_branch": wrapToolBuilder(CreateBranch),
26+
"push_files": wrapToolBuilder(PushFiles),
27+
"create_pull_request": wrapToolBuilder(CreatePullRequest),
28+
"fork_repository": wrapToolBuilder(ForkRepository),
29+
}
30+
31+
for name, build := range builders {
32+
t.Run(name, func(t *testing.T) {
33+
t.Parallel()
34+
35+
st := build(translations.NullTranslationHelper)
36+
37+
require.NotNil(t, st.ScopeAccess.Challenge)
38+
args := map[string]any(nil)
39+
wantScopes := []string{string(scopes.PublicRepo)}
40+
if name == "push_files" {
41+
args = map[string]any{"files": []any{map[string]any{"path": "README.md"}}}
42+
wantScopes = append(wantScopes, string(scopes.Workflow))
43+
}
44+
assert.Equal(t, wantScopes, st.ScopeAccess.Scopes,
45+
"%s should expose the least-privilege scope upper bound", name)
46+
assert.Empty(t, st.ScopeAccess.Challenge(args, []string{string(scopes.PublicRepo)}),
47+
"%s should accept a public_repo token", name)
48+
assert.Empty(t, st.ScopeAccess.Challenge(args, []string{string(scopes.Repo)}),
49+
"%s should accept a parent repo token", name)
50+
if name == "push_files" {
51+
workflowArgs := map[string]any{"files": []any{map[string]any{"path": ".github/workflows/ci.yml"}}}
52+
assert.Equal(t, []string{string(scopes.PublicRepo), string(scopes.Workflow)},
53+
st.ScopeAccess.Challenge(workflowArgs, []string{string(scopes.PublicRepo)}),
54+
"push_files should additionally challenge for workflow when needed")
55+
}
56+
})
57+
}
58+
}
59+
60+
// TestPublicRepoWriteToolsVisibleToPublicRepoToken asserts the PAT tool filter
61+
// shows these tools to a public_repo-only token and keeps them visible for a
62+
// full repo token.
63+
func TestPublicRepoWriteToolsVisibleToPublicRepoToken(t *testing.T) {
64+
t.Parallel()
65+
66+
publicRepoToken := []string{string(scopes.PublicRepo)}
67+
repoToken := []string{string(scopes.Repo)}
68+
69+
filterForPublicToken := CreateToolScopeFilter(publicRepoToken)
70+
filterForRepoToken := CreateToolScopeFilter(repoToken)
71+
72+
builders := map[string]func(translations.TranslationHelperFunc) *inventory.ServerTool{
73+
"add_issue_comment": wrapToolBuilder(AddIssueComment),
74+
"issue_write": wrapToolBuilder(IssueWrite),
75+
"create_branch": wrapToolBuilder(CreateBranch),
76+
"push_files": wrapToolBuilder(PushFiles),
77+
"create_pull_request": wrapToolBuilder(CreatePullRequest),
78+
"fork_repository": wrapToolBuilder(ForkRepository),
79+
}
80+
81+
for name, build := range builders {
82+
t.Run(name, func(t *testing.T) {
83+
t.Parallel()
84+
85+
st := build(translations.NullTranslationHelper)
86+
87+
allowed, err := filterForPublicToken(t.Context(), st)
88+
require.NoError(t, err)
89+
assert.True(t, allowed, "%s should be visible with a public_repo-only token", name)
90+
91+
allowed, err = filterForRepoToken(t.Context(), st)
92+
require.NoError(t, err)
93+
assert.True(t, allowed, "%s should remain visible with a full repo token", name)
94+
})
95+
}
96+
}
97+
98+
func wrapToolBuilder(fn func(translations.TranslationHelperFunc) inventory.ServerTool) func(translations.TranslationHelperFunc) *inventory.ServerTool {
99+
return func(t translations.TranslationHelperFunc) *inventory.ServerTool {
100+
st := fn(t)
101+
return &st
102+
}
103+
}

pkg/github/pullrequests.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
706706
Required: []string{"owner", "repo", "title", "head", "base"},
707707
},
708708
},
709-
scopes.RequireAll(scopes.Repo),
709+
scopes.RequireAll(scopes.PublicRepo),
710710
func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
711711
owner, err := RequiredParam[string](args, "owner")
712712
if err != nil {

pkg/github/repositories.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,7 +1229,7 @@ func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool {
12291229
Required: []string{"owner", "repo"},
12301230
},
12311231
},
1232-
scopes.RequireAll(scopes.Repo),
1232+
scopes.RequireAll(scopes.PublicRepo),
12331233
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
12341234
owner, err := RequiredParam[string](args, "owner")
12351235
if err != nil {
@@ -1526,7 +1526,7 @@ func CreateBranch(t translations.TranslationHelperFunc) inventory.ServerTool {
15261526
Required: []string{"owner", "repo", "branch"},
15271527
},
15281528
},
1529-
scopes.RequireAll(scopes.Repo),
1529+
scopes.RequireAll(scopes.PublicRepo),
15301530
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
15311531
owner, err := RequiredParam[string](args, "owner")
15321532
if err != nil {
@@ -1658,7 +1658,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
16581658
Required: []string{"owner", "repo", "branch", "files", "message"},
16591659
},
16601660
},
1661-
scopes.RequireAll(scopes.Repo),
1661+
scopes.RequireAll(scopes.PublicRepo),
16621662
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
16631663
owner, err := RequiredParam[string](args, "owner")
16641664
if err != nil {
@@ -1840,7 +1840,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
18401840
},
18411841
)
18421842
tool.ScopeAccess = scopes.DynamicChallenge(
1843-
[]scopes.Scope{scopes.Repo, scopes.Workflow},
1843+
[]scopes.Scope{scopes.PublicRepo, scopes.Workflow},
18441844
tool.ScopeAccess.Visible,
18451845
workflowScopeChallengeForFiles,
18461846
)

pkg/github/repository_path.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func workflowScopeChallengeForFiles(arguments map[string]any, activeScopes []str
7676
}
7777
}
7878
if containsWorkflow {
79-
return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.Workflow)
79+
return scopes.ChallengeAll(activeScopes, scopes.PublicRepo, scopes.Workflow)
8080
}
81-
return scopes.ChallengeAll(activeScopes, scopes.Repo)
81+
return scopes.ChallengeAll(activeScopes, scopes.PublicRepo)
8282
}

pkg/github/repository_path_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func TestFileWriteWorkflowScopeChallenges(t *testing.T) {
8484
map[string]any{"path": "README.md"},
8585
map[string]any{"path": ".github/workflows/ci.yml"},
8686
}},
87-
want: []string{"repo", "workflow"},
87+
want: []string{"public_repo", "workflow"},
8888
},
8989
{
9090
name: "push validates entries after workflow",

pkg/github/tool_scopes_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ func TestDynamicToolScopeMetadataIsExhaustive(t *testing.T) {
105105
}{
106106
{tool: CreateOrUpdateFile(translations.NullTranslationHelper), maxScopes: []string{"repo", "workflow"}},
107107
{tool: DeleteFile(translations.NullTranslationHelper), maxScopes: []string{"repo", "workflow"}},
108-
{tool: PushFiles(translations.NullTranslationHelper), maxScopes: []string{"repo", "workflow"}},
108+
{tool: PushFiles(translations.NullTranslationHelper), maxScopes: []string{"public_repo", "workflow"}},
109109
{tool: ListIssueFields(translations.NullTranslationHelper), maxScopes: []string{"repo", "read:org"}},
110110
{tool: ListIssueTypes(translations.NullTranslationHelper), maxScopes: []string{"repo", "read:org"}},
111111
{tool: UIGet(translations.NullTranslationHelper), maxScopes: []string{"repo", "read:org"}},

0 commit comments

Comments
 (0)