Python: scope under-specified approve-for-session permission decisions - #7607
Conversation
PermissionDecisionApproveForSession carries an optional `approval` (tool
prompts) and an optional `domain` (URL prompts), so it can be constructed
with neither. A bare PermissionDecisionApproveForSession() serializes to
{"kind": "approve-for-session"}, which the Copilot CLI cannot interpret: it
dereferences the absent approval and crashes the CLI process with "Cannot
read properties of undefined (reading 'commandIdentifiers')", taking the
whole run down rather than failing a single tool call.
Wrap the resolved permission handler so such decisions are scoped using the
request that triggered them: shell prompts become an approval for that
prompt's command identifiers, MCP prompts an approval for that server and
tool, URL prompts an approval for that URL's domain, and so on.
The decision is only ever narrowed, never widened. When the prompt reports
can_offer_session_approval=False, or the request kind has no session-scoped
approval (such as a hook prompt), the decision is downgraded to a single-use
approval and a warning is logged. Decisions that already specify a scope are
forwarded unchanged, and handler exceptions still propagate so the SDK's
deny-on-error behavior is preserved.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Scopes bare session-approval decisions from their triggering permission request to prevent Copilot CLI crashes.
Changes:
- Normalizes under-specified session approvals.
- Downgrades unsupported scopes to one-time approval.
- Adds regression tests and usage documentation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
_agent.py |
Implements permission-scope normalization. |
test_github_copilot_agent.py |
Adds regression coverage. |
README.md |
Documents automatic scoping behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Agent Framework Review — Iteration 1
Completed passes: 5 | Result: No high-severity findings
Scope: full PR (1 commit(s)): 1182e35102cb
Review passes
- Correctness (
gpt-5.6-sol) — No issues found in this pass. - Security Reliability (
claude-opus-4.8) — No issues found in this pass. - Test Coverage (
gpt-5.6-sol) — No issues found in this pass. - Failure Modes (
claude-opus-4.8) — No issues found in this pass. - Design Approach (
claude-opus-4.8) — No issues found in this pass.
The permission-handler wrapper returned PermissionHandlerType (the sync-or-async union), so awaiting its result in tests was rejected by the stricter CI type checkers (pyrefly, ty, zuban). Give the wrapper a dedicated AsyncPermissionHandlerType return type, and narrow the awaited result with an isinstance assert before accessing its scope in the async-handler test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
GitHub Actions
Active
Cover the two previously-untested branches of _derive_session_approval: extension-management preserves the request operation, and extension-permission-access preserves the extension name. Both assert the serialized approval payload as well. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
Agent Framework Review — Iteration 2
Completed passes: 5 | Result: No high-severity findings
Scope: 2 net-new commit(s): c0dfe303854e, c67cc9fae52e
Review passes
- Correctness (
gpt-5.6-sol) — No issues found in this pass. - Security Reliability (
claude-opus-4.8) — No issues found in this pass. - Test Coverage (
gpt-5.6-sol) — No issues found in this pass. - Failure Modes (
claude-opus-4.8) — No issues found in this pass. - Design Approach (
claude-opus-4.8) — No issues found in this pass.
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 3
Result: Findings reported
Scope: full PR (3 commit(s)): 1182e35102cb, c0dfe303854e, c67cc9fae52e
Model: gpt-5.6-sol
Overview
The PR safely normalizes bare session approvals across the SDK's permission variants, preserves explicitly scoped decisions, and falls back to one-time approval when no representable session scope exists. Its focused tests cover synchronous and asynchronous handlers, serialization, unsupported prompts, and session wiring. One authorization-boundary risk remains: Python and the Node-based CLI can derive different hosts from the same parser-ambiguous URL, causing session approval to be persisted for an unrelated domain.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 high) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/github_copilot/agent_framework_github_copilot/_agent.py
GitHub Actions
Active
The URL branch derived the persisted domain with Python's urlparse, but the Copilot CLI parses URLs with WHATWG semantics. The two disagree on crafted authorities -- e.g. a backslash before the '@' in 'https://example.com<backslash>@evil.com' resolves to example.com under the CLI but evil.com under urlparse -- so trusting urlparse could persist a session-wide approval for an unrelated, attacker-chosen domain, widening authorization. Add _derive_url_session_domain, which returns a domain only when the URL contains none of the characters WHATWG and urlparse handle differently (backslash, tab, newline, carriage return); any ambiguity (or a URL with no host) narrows the decision to a single-use PermissionDecisionApproveOnce. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
MAF Automated Review — Iteration 4
Result: No findings
Scope: 12 net-new commit(s): db979b616ae7, 0d7536533187, 6fff2c9b1f6f, 8a0731ad9256, 4ca093371ebb, e85b3c8ba8c5, 30996433ac86, 5e52c6a7183a, 27d82b1567a6, 56d13bce4eb2, 148df855414f, b54852b056fc
Model: gpt-5.6-sol
Overview
The latest change addresses the existing URL-parser mismatch by refusing to persist a domain when the request URL contains characters with unsafe or deceptive parsing behavior, while retaining normal session approval for ordinary URLs. The normalizer remains fail-closed to a single-use approval, preserves explicitly scoped decisions, and is covered by focused regression tests for backslash and control-character inputs. No new Critical, High, or Medium defect remained after reconciling the conservative fallback behavior, existing feedback, and authoritative diff scope.
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
Motivation & Context
A permission handler that returns a bare
PermissionDecisionApproveForSession()— anatural way to express "approve this for the rest of the session" — crashes the whole
run.
PermissionDecisionApproveForSessioncarries an optionalapproval(tool prompts)and an optional
domain(URL prompts), so it can be constructed with neither. Thatserializes to
{"kind": "approve-for-session"}, which the Copilot CLI cannot interpret:it dereferences the absent
approvaland throwsCannot read properties of undefined (reading 'commandIdentifiers'). Because the crash is inside the CLI process rather thanin Python, it takes down the entire run instead of failing a single tool call.
The scope is never actually ambiguous — the permission request that triggered the prompt
already describes what is being approved — so the framework can reconstruct it.
Description & Review Guide
What are the major changes?
_agent.py: the resolvedon_permission_requesthandler is now wrapped in_build_session_kwargsso its decisions are normalized before reaching the SDK. Anunder-specified
approve-for-sessiondecision (bothapprovalanddomainunset) isscoped from the request that triggered it: shell → an approval for that prompt's
command identifiers; read/write/memory → the matching approval variant; mcp → server +
tool; custom-tool → tool name; extension prompts → operation / extension name; url →
domainfrom the URL's hostname.can_offer_session_approval=False, or therequest kind has no session-scoped approval (such as a
hookprompt), the decision isdowngraded to a single-use
PermissionDecisionApproveOnce()and a warning is logged.still propagate so the SDK's deny-on-error behavior is preserved.
What is the impact of these changes?
changes is the one that currently kills the CLI process, so nothing that works today
regresses. The public API, the handler type, and
_permission_handler(which stillstores the raw handler) are unchanged; wrapping happens only when building session
kwargs.
What do you want reviewers to focus on?
downgrading to a single-use approval. The inferred scope matches what the CLI's own
interactive "approve for session" would grant, and the narrow-never-widen rule ensures
we never grant more than was requested.
Related Issue
Fixes #7553
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.