Challenge workflow file writes for OAuth scope - #3092
Conversation
Sorry, something went wrong.
Add per-call OAuth scope resolution for workflow paths and reject unsafe repository-relative paths before file writes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds per-call OAuth scope challenges for workflow-file writes while preserving repo-only authorization for other repository writes.
Changes:
- Adds traversal-safe path validation and conditional workflow scope resolution.
- Extends scope middleware and inventory metadata for argument-dependent scopes.
- Projects top-level paths into MCP headers and adds regression tests.
| File | Description |
|---|---|
| pkg/scopes/scopes.go | Removes unused Codespace scope metadata. |
| pkg/scopes/scopes_test.go | Updates scope catalog expectations. |
| pkg/scopes/map.go | Resolves conditional scopes per invocation. |
| pkg/scopes/map_test.go | Tests conditional scope resolution. |
| pkg/inventory/server_tool.go | Adds scope resolvers and path header projection. |
| pkg/inventory/server_tool_test.go | Tests path header annotations. |
| pkg/http/oauth/oauth_test.go | Updates advertised scope expectations. |
| pkg/http/middleware/scope_challenge.go | Challenges only for missing resolved scopes. |
| pkg/http/middleware/scope_challenge_test.go | Tests body- and context-derived challenges. |
| pkg/github/repository_path.go | Adds safe path validation and workflow detection. |
| pkg/github/repository_path_test.go | Covers traversal and workflow scope behavior. |
| pkg/github/repositories.go | Integrates validation and resolvers into write tools. |
| pkg/github/header_params_test.go | Verifies top-level versus nested path projection. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 13/13 changed files
- Comments generated: 0
- Review effort level: Balanced
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| } | ||
| if len(resolved.RequiredScopeGroups) == 0 { |
There was a problem hiding this comment.
This fallback silently converts "any of" into "all of".
When RequiredScopeGroups is empty, a multi-entry RequiredScopes means any of — generate_docs.go renders exactly that (Required OAuth Scopes (any of)), and HasAcceptedScope falls through to the AcceptedScopes union. Giving each entry its own group flips every alternative into an independently mandatory requirement.
No tool hits this today (all three resolvers sit on tools with a single repo scope), but it's a quiet landmine for whoever adds the next resolver. I probed it against a list_issue_fields-shaped tool:
base := &ToolScopeInfo{
RequiredScopes: []string{"repo", "read:org"},
AcceptedScopes: ExpandScopes(Repo, ReadOrg),
ScopeResolver: func(map[string]any) []string { return []string{"workflow"} },
}
base.HasAcceptedScope("repo") // true — repo alone satisfies any-of
resolved := base.Resolve(map[string]any{})
// groups = [[repo] [admin:org read:org write:org] [workflow]]
resolved.HasAcceptedScope("repo", "workflow") // false ← now demands read:org too
resolved.MissingScopes("repo") // [read:org workflow]A token holding repo + workflow would get challenged for read:org it never needed, and the challenge would ask for it by name.
Collapsing the base into a single group preserves the any-of semantics:
if len(resolved.RequiredScopeGroups) == 0 && len(resolved.AcceptedScopes) > 0 {
resolved.RequiredScopeGroups = [][]string{resolved.AcceptedScopes}
}Though note that also breaks the positional groups[i] -> RequiredScopes[i] assumption MissingScopes relies on, so that pairing probably wants to become explicit rather than positional. Happy with a comment documenting the single-scope precondition instead if you'd rather keep this narrow for now — mainly want the constraint written down somewhere.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Summary
Conditionally require the workflow OAuth scope when file-write tools target .github/workflows/**, while preserving normal repo-only writes elsewhere.
Why
GitHub requires the additional workflow scope for workflow-file updates, but statically requiring it would over-scope every file write. The remote server needs a per-call 403 challenge before handlers execute.
Fixes # N/A
What changed
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
Docs