Fix OAuth scope satisfaction to AND-of-ORs (required means required)#2778
Open
SamMorrowDrums wants to merge 3 commits into
Open
Fix OAuth scope satisfaction to AND-of-ORs (required means required)#2778SamMorrowDrums wants to merge 3 commits into
SamMorrowDrums wants to merge 3 commits into
Conversation
Tool scope requirements conflate two relationships: conjunction (a tool needs
several different capabilities, AND) and hierarchy/substitution (a requirement
is met by itself or a higher scope, OR). Previously both collapsed into one OR
list checked against the expanded AcceptedScopes, so a token holding only repo
was treated as satisfying ui_get (which also needs read:org): the PAT filter
showed it and no scope challenge was issued, then it failed at runtime.
Make RequiredScopes the single source of truth and evaluate satisfaction as
AND-of-ORs: expand the token downward through the hierarchy and require every
declared scope to be present. AcceptedScopes becomes display-only metadata.
- HasRequiredScopes: AND semantics over requiredScopes.
- ToolScopeInfo.Satisfies (was HasAcceptedScope) + precise MissingScopes.
- scope_filter: filter on RequiredScopes; keep read-only repo-only special case.
- scope_challenge: challenge with only the missing required scopes.
- generate-docs: drop "(any of)" rendering; all required scopes are AND.
Affects ui_get, list_issue_types, and list_issue_fields (all {repo, read:org}).
Filtering stays gated to classic ghp_ PATs and fails open; fine-grained/OAuth/
app tokens are unaffected. Split out of the PR #2751 review discussion.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes OAuth scope satisfaction for classic PAT tool filtering and OAuth scope challenges by treating multiple required scopes as AND requirements, while still allowing hierarchy-based substitution (a higher scope satisfies a lower one). This aligns runtime behavior, scope challenges, and generated documentation with the intended “required means required” semantics.
Changes:
- Update scope satisfaction logic to AND-of-ORs by expanding the token’s scopes downward through the scope hierarchy and requiring all declared
RequiredScopes. - Make OAuth scope challenges request only the missing required scopes, merged with the user’s existing scopes for the
scope=parameter. - Update generated docs to remove the misleading “(any of)” phrasing and add a short preamble clarifying AND semantics for flagged/insiders tool listings.
Show a summary per file
| File | Description |
|---|---|
| README.md | Removes “(any of)” wording so multiple required scopes are rendered as AND. |
| pkg/scopes/scopes.go | Changes HasRequiredScopes semantics to require all declared required scopes via downward token expansion. |
| pkg/scopes/scopes_test.go | Updates/extends tests to cover AND behavior and hierarchy satisfaction. |
| pkg/scopes/map.go | Updates ToolScopeInfo satisfaction/missing-scope logic to use required scopes with hierarchy expansion. |
| pkg/scopes/map_test.go | Updates tests for ToolScopeInfo satisfaction and precise missing-scope computation. |
| pkg/http/middleware/scope_challenge.go | Scope challenge now requests only missing required scopes and describes them in the error. |
| pkg/github/scope_filter.go | PAT tool filtering now uses RequiredScopes as authoritative and enforces AND semantics. |
| pkg/github/scope_filter_test.go | Updates filter tests to model {repo, read:org} as true AND requirements. |
| docs/insiders-features.md | Removes “(any of)” and adds a preamble clarifying AND + hierarchy substitution. |
| docs/feature-flags.md | Removes “(any of)” and adds a preamble clarifying AND + hierarchy substitution. |
| cmd/github-mcp-server/generate_docs.go | Updates tool doc rendering to always label required scopes as required (AND). |
| cmd/github-mcp-server/feature_flag_docs.go | Prepends scope semantics clarification to feature-flag/insiders generated sections. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Low
Comment on lines
+82
to
84
| func (t *ToolScopeInfo) Satisfies(userScopes ...string) bool { | ||
| if t == nil || len(t.RequiredScopes) == 0 { | ||
| return true // No scopes required |
Correct the over-claim that the scope hierarchy covers all OR cases. The
hierarchy only models ancestor substitution, not sibling alternatives, so it
cannot represent every way a tool may legitimately be satisfied.
- Reword HasRequiredScopes doc: AND-of-ORs is ancestor-only; note the
best-effort/fail-open posture and make the future CNF extension concrete using
code scanning as the motivating example (security_events OR public_repo OR repo).
- Note in CreateToolScopeFilter that filtering is a best-effort UX filter, not an
authorization boundary (gated to ghp_ PATs, skipped when scopes can't be fetched).
- Verify security tools ({security_events}) stay behavior-neutral under AND: a
repo token still satisfies them; a public_repo-only token was already hidden
before. Add explicit neutrality tests at the scope and filter layers.
- docs/scope-filtering.md: add a Limitations and Fail-Open Posture section
(sibling scopes, org roles, repo visibility) and a best-effort intro note.
No tool declarations or scopes changed; no CNF structure built.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Flesh out the Limitations section: public-vs-private repo ambiguity (code scanning public_repo vs private security_events/repo), sibling-OR alternatives naming the four security tools, org roles invisible to scopes, and other token types skipped (gated to ghp_) as fail-open by design. Note the OR-groups extension point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Tool scope requirements conflate two different relationships and collapse both into a single OR list, which makes PAT tool‑filtering and OAuth scope‑challenges over‑permissive:
ui_getreads labels (repo) and teams (read:org).read:orgis satisfied byread:org | write:org | admin:org.These compose as AND‑of‑ORs: satisfy every required scope, where each required scope can be met by itself or an ancestor scope.
Previously
AcceptedScopes = ExpandScopes(required)was checked with OR, so a token holding onlyrepowas treated as satisfyingui_get(which also needsread:org): the PAT filter showed the tool and no scope challenge was issued, then it failed at runtime on the org call. The generated docs also rendered multiple required scopes asRequired OAuth Scopes (any of), which is wrong for AND requirements.Fix
RequiredScopesis the single source of truth for all allow/challenge decisions. Required means required.repo → public_repo, security_events;admin:org → write:org, read:org; …). A tool is satisfied iff, for every required scope, the expanded token set contains it.AcceptedScopesis now display‑only metadata and no longer drives any filter/challenge/missing decision.scope=parameter).(any of)rendering — all listed required scopes are AND.Affected tools
Exactly 3 tools declare multiple required scopes, all
{repo, read:org}, all genuinely AND:ui_get(pkg/github/ui_tools.go)list_issue_types(pkg/github/issues.go)list_issue_fields(pkg/github/issue_fields.go)Security tools are behavior‑neutral
All security tools (
code_scanning.go,secret_scanning.go,dependabot.go,security_advisories.go) declare a single{security_events}scope. A single required scope has nothing to conjoin, andrepoalready expands tosecurity_eventsvia the hierarchy, so the AND change is behavior‑neutral for them: arepotoken still satisfies them, and apublic_repo‑only token was already hidden before this change. Explicit neutrality tests were added at both the scope and filter layers.Scope filtering is best‑effort, not an authorization boundary (fail‑open)
This is the guiding posture, now encoded in comments and
docs/scope-filtering.md: scope filtering is a best‑effort UX nicety for classicghp_PATs only, not an authorization boundary — the GitHub API is the source of truth. The server fails open: it only hides (or challenges) when confident the token cannot work, and prefers showing the tool when access is plausible. Filtering stays gated toghp_classic PATs and is skipped when scopes can't be fetched. Fine‑grained / OAuth / GitHub App tokens are unaffected.Documented limitations (pre‑existing; intentionally not solved here)
The scope hierarchy only models ancestor substitution, so it does not cover every way a tool can legitimately be satisfied:
public_repo, a sibling of the declaredsecurity_events(both children ofrepo) — token expansion can't bridge siblings, so apublic_repo‑only PAT gets security tools hidden even though it could read public alerts.A faithful model would make
RequiredScopesa CNF list of OR‑groups (groups AND‑ed, members within a group OR‑ed), with code scanning as the motivating example:security_eventsORpublic_repoORrepo. That structure is deliberately not built in this PR (YAGNI); the fail‑open posture keeps these cases from being wrongly hidden.No tool declarations or scopes were changed; tool input schemas are untouched (toolsnaps unchanged).
Validation
script/lint— 0 issuesUPDATE_TOOLSNAPS=true go test ./...thengo test -race ./...— green, no.snapchangesscript/generate-docs— regeneratedREADME.md,docs/feature-flags.md,docs/insiders-features.md; hand‑updateddocs/scope-filtering.mdscript/test— greenSplit out of the PR #2751 review discussion.