Skip to content

Commit dd55836

Browse files
fix(auth): validate reviewer scope arguments
Defer OAuth challenges to normal handler validation when ui_get reviewer calls omit or malform the repository argument. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 8de98b3 commit dd55836

2 files changed

Lines changed: 23 additions & 2 deletions

File tree

pkg/github/tool_scopes.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,10 @@ func uiGetScopeAccess() inventory.ScopeAccess {
4646
return scopes.ChallengeAll(activeScopes, scopes.ReadOrg)
4747
}
4848
if method == "reviewers" {
49-
return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.ReadOrg)
49+
if repo, ok := arguments["repo"].(string); ok && repo != "" {
50+
return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.ReadOrg)
51+
}
52+
return nil
5053
}
5154
switch method {
5255
case "labels", "assignees", "milestones", "branches", "issue_fields":

pkg/github/tool_scopes_test.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,20 @@ func TestConditionalToolScopeChecks(t *testing.T) {
6666
allowed: []string{"repo", "read:org"},
6767
disallowed: []string{"repo"},
6868
},
69+
{
70+
name: "ui reviewers method without repository defers to validation",
71+
tool: UIGet(translations.NullTranslationHelper),
72+
arguments: map[string]any{"method": "reviewers", "owner": "octo"},
73+
allowed: nil,
74+
disallowed: nil,
75+
},
76+
{
77+
name: "ui reviewers method with malformed repository defers to validation",
78+
tool: UIGet(translations.NullTranslationHelper),
79+
arguments: map[string]any{"method": "reviewers", "owner": "octo", "repo": 123},
80+
allowed: nil,
81+
disallowed: nil,
82+
},
6983
}
7084

7185
for _, tt := range tests {
@@ -74,7 +88,11 @@ func TestConditionalToolScopeChecks(t *testing.T) {
7488
assert.True(t, tt.tool.ScopeAccess.Dynamic)
7589
assert.Equal(t, []string{"repo", "read:org"}, tt.tool.ScopeAccess.Scopes)
7690
assert.Empty(t, tt.tool.ScopeAccess.Challenge(tt.arguments, tt.allowed))
77-
assert.NotEmpty(t, tt.tool.ScopeAccess.Challenge(tt.arguments, tt.disallowed))
91+
if tt.disallowed == nil {
92+
assert.Empty(t, tt.tool.ScopeAccess.Challenge(tt.arguments, nil))
93+
} else {
94+
assert.NotEmpty(t, tt.tool.ScopeAccess.Challenge(tt.arguments, tt.disallowed))
95+
}
7896
assert.True(t, tt.tool.ScopeAccess.Visible(nil))
7997
})
8098
}

0 commit comments

Comments
 (0)