diff --git a/python/packages/github_copilot/README.md b/python/packages/github_copilot/README.md index 6642421d3a1..04352d65582 100644 --- a/python/packages/github_copilot/README.md +++ b/python/packages/github_copilot/README.md @@ -69,10 +69,9 @@ prompts) or a `domain` (URL prompts). Both are optional, so a bare `PermissionDecisionApproveForSession()` carries no scope at all and the Copilot CLI cannot interpret it. -`GitHubCopilotAgent` therefore scopes such a decision automatically, using the request that -triggered it — a shell prompt becomes an approval for that prompt's command identifiers, an -MCP prompt an approval for that server and tool, a URL prompt an approval for that URL's -domain, and so on: +`GitHubCopilotAgent` therefore scopes such a decision automatically when the request provides +a structured session approval — a shell prompt becomes an approval for that prompt's command +identifiers, an MCP prompt an approval for that server and tool, and so on: ```python from copilot.generated.rpc import PermissionDecisionApproveForSession @@ -83,12 +82,13 @@ def on_permission_request(request, invocation): return PermissionDecisionApproveForSession() ``` -The decision is only ever narrowed, never widened. When the prompt reports that it cannot -offer session-scoped approval (`can_offer_session_approval=False`), or the request kind has -no session-scoped approval at all (such as a `hook` prompt), the decision is downgraded to a -single-use approval and a warning is logged. Pass an explicit `approval=` or `domain=` when -you want to approve something other than the request being handled — decisions that already -specify a scope are forwarded unchanged. +The decision is only ever narrowed, never widened. A bare decision for a URL prompt is +downgraded to a single-use approval because the framework cannot safely infer the Copilot +CLI's exact URL permission scope. The same downgrade occurs when the prompt reports that it +cannot offer session-scoped approval (`can_offer_session_approval=False`), or the request +kind has no session-scoped approval at all (such as a `hook` prompt). A warning is logged in +each case. Pass an explicit `approval=` or `domain=` when you want to approve a specific +scope for the session — decisions that already specify a scope are forwarded unchanged. ### Deprecated: `on_function_approval` diff --git a/python/packages/github_copilot/agent_framework_github_copilot/_agent.py b/python/packages/github_copilot/agent_framework_github_copilot/_agent.py index a0583126606..4295475b092 100644 --- a/python/packages/github_copilot/agent_framework_github_copilot/_agent.py +++ b/python/packages/github_copilot/agent_framework_github_copilot/_agent.py @@ -22,7 +22,6 @@ ) from pathlib import Path from typing import Any, ClassVar, Generic, Literal, Self, TypedDict, cast, overload -from urllib.parse import urlparse from agent_framework import ( AgentMiddlewareLayer, @@ -268,41 +267,6 @@ def _derive_session_approval(request: PermissionRequest) -> PermissionDecisionAp return None -# Characters the WHATWG URL parser (used by the Copilot CLI) treats specially for -# special-scheme URLs in ways that can move the authority boundary: backslashes are -# normalized to forward slashes, and tabs/newlines/carriage returns are stripped before -# parsing. Python's ``urlparse`` does none of this, so a URL containing any of them may -# resolve to a different host than the CLI actually contacts. -_WHATWG_AMBIGUOUS_URL_CHARS = ("\\", "\t", "\n", "\r") - - -def _derive_url_session_domain(url: str) -> str | None: - """Return the domain to persist for a URL prompt, or ``None`` when it is unsafe to. - - The persisted domain must match the host the Copilot CLI actually contacts, but the CLI - parses URLs with WHATWG semantics while this runs on Python's ``urlparse``. The two - disagree on crafted authorities -- a backslash before the ``@`` in - ``https://example.com@evil.com`` resolves to ``example.com`` under the CLI - but ``evil.com`` under ``urlparse`` -- so trusting ``urlparse`` here could persist a - session-wide approval for an unrelated, attacker-chosen domain. - - To keep the "narrow, never widen" guarantee, the domain is only returned when the URL - contains none of the characters the two parsers handle differently; any ambiguity (or a - URL with no derivable host) yields ``None`` so the caller can approve the single request - without persisting a domain. - - Args: - url: The URL from the permission request. - - Returns: - The lower-cased host to approve for the session, or ``None`` when the URL is - parser-ambiguous or has no host. - """ - if any(char in url for char in _WHATWG_AMBIGUOUS_URL_CHARS): - return None - return urlparse(url).hostname or None - - def _normalize_permission_decision( decision: PermissionRequestResult, request: PermissionRequest, @@ -314,11 +278,12 @@ def _normalize_permission_decision( is constructible with neither. That serializes to ``{"kind": "approve-for-session"}``, which the Copilot CLI cannot interpret -- it crashes with ``Cannot read properties of undefined (reading 'commandIdentifiers')``, taking the whole run down with it. This - reconstructs the intended scope from ``request``. + reconstructs structured tool scopes from ``request`` and narrows to a single request + when an exact session scope cannot be safely inferred. - The decision is only ever narrowed, never widened: when the prompt does not offer - session-scoped approval, or the request kind has no session approval representation, - the decision is downgraded to a single-use approval. + The decision is only ever narrowed, never widened: URL prompts, prompts that do not + offer session-scoped approval, and request kinds with no session approval representation + are downgraded to a single-use approval. Args: decision: The decision returned by the caller's permission handler. @@ -336,14 +301,11 @@ def _normalize_permission_decision( try: if isinstance(request, PermissionRequestUrl): - domain = _derive_url_session_domain(request.url) - if domain: - return PermissionDecisionApproveForSession(domain=domain) logger.warning( "Permission handler returned an unscoped 'approve-for-session' decision for a URL prompt, " - "but no unambiguous domain could be derived from '%s'. Approving this request only. Return " + "but Agent Framework cannot safely derive the Copilot CLI's exact URL permission scope. " + "Approving this request only. Return " "PermissionDecisionApproveForSession(domain=...) to approve a domain for the session.", - request.url, ) return PermissionDecisionApproveOnce() diff --git a/python/packages/github_copilot/tests/test_github_copilot_agent.py b/python/packages/github_copilot/tests/test_github_copilot_agent.py index cf29cd9eb4f..b0d6ad71f33 100644 --- a/python/packages/github_copilot/tests/test_github_copilot_agent.py +++ b/python/packages/github_copilot/tests/test_github_copilot_agent.py @@ -3488,20 +3488,32 @@ async def test_extension_permission_access_request_preserves_extension_name(self assert result.approval.extension_name == "my-ext" assert result.to_dict()["approval"] == {"kind": "extension-permission-access", "extensionName": "my-ext"} - async def test_url_request_derives_domain_instead_of_approval(self) -> None: - """A URL prompt is scoped by ``domain``; URL prompts have no ``approval``.""" - from copilot.generated.rpc import PermissionDecisionApproveForSession + @pytest.mark.parametrize( + "url", + [ + pytest.param("http://example.com/a", id="http"), + pytest.param("https://example.com/a", id="https"), + pytest.param("http://example.com:80/a", id="http-default-port"), + pytest.param("https://example.com:443/a", id="https-default-port"), + pytest.param("https://example.com:8443/a", id="https-non-default-port"), + pytest.param("http://127.0.0.1/a", id="ipv4"), + pytest.param("http://[::1]/a", id="ipv6"), + pytest.param("https://xn--bcher-kva.example/a", id="idna"), + pytest.param("https://example.com./a", id="trailing-dot"), + ], + ) + async def test_url_request_without_explicit_domain_narrows_to_approve_once(self, url: str) -> None: + """A URL prompt without an explicit scope must not fabricate a session approval.""" + from copilot.generated.rpc import PermissionDecisionApproveForSession, PermissionDecisionApproveOnce from copilot.session_events import PermissionRequestUrl - request = PermissionRequestUrl(intention="fetch", url="https://example.com/some/path?q=1") + request = PermissionRequestUrl(intention="fetch", url=url) result = await self.normalize(request, PermissionDecisionApproveForSession()) - assert isinstance(result, PermissionDecisionApproveForSession) - assert result.domain == "example.com" - assert result.approval is None + assert isinstance(result, PermissionDecisionApproveOnce) - async def test_url_request_without_derivable_domain_falls_back_to_approve_once(self) -> None: - """A URL with no host cannot be scoped, so the decision narrows to a single approval.""" + async def test_malformed_url_falls_back_to_approve_once(self) -> None: + """A malformed URL must not receive a framework-derived session scope.""" from copilot.generated.rpc import PermissionDecisionApproveForSession, PermissionDecisionApproveOnce from copilot.session_events import PermissionRequestUrl @@ -3520,12 +3532,7 @@ async def test_url_request_without_derivable_domain_falls_back_to_approve_once(s ], ) async def test_parser_ambiguous_url_falls_back_to_approve_once(self, url: str) -> None: - """A URL whose host Python and the CLI parse differently must not persist a domain. - - The CLI (WHATWG) contacts ``example.com`` for these URLs while ``urlparse`` derives - ``evil.com``. Persisting ``evil.com`` would widen approval to an unrelated, - attacker-chosen domain, so the decision must narrow to a single-use approval. - """ + """A parser-ambiguous URL must not persist a framework-derived session scope.""" from copilot.generated.rpc import PermissionDecisionApproveForSession, PermissionDecisionApproveOnce from copilot.session_events import PermissionRequestUrl @@ -3534,16 +3541,15 @@ async def test_parser_ambiguous_url_falls_back_to_approve_once(self, url: str) - assert isinstance(result, PermissionDecisionApproveOnce) - async def test_unambiguous_url_with_userinfo_derives_real_host(self) -> None: - """Userinfo without ambiguous characters is safe; the real host is persisted.""" - from copilot.generated.rpc import PermissionDecisionApproveForSession + async def test_url_with_userinfo_falls_back_to_approve_once(self) -> None: + """Userinfo must not be discarded while deriving a session scope.""" + from copilot.generated.rpc import PermissionDecisionApproveForSession, PermissionDecisionApproveOnce from copilot.session_events import PermissionRequestUrl request = PermissionRequestUrl(intention="fetch", url="https://user:pass@example.com/a") result = await self.normalize(request, PermissionDecisionApproveForSession()) - assert isinstance(result, PermissionDecisionApproveForSession) - assert result.domain == "example.com" + assert isinstance(result, PermissionDecisionApproveOnce) async def test_shell_request_that_cannot_offer_session_approval_narrows_to_approve_once(self) -> None: """Never fabricate a session approval the prompt said it could not offer.""" @@ -3585,17 +3591,18 @@ async def test_fully_specified_approval_is_passed_through_untouched(self) -> Non assert result is decision - async def test_explicit_domain_is_passed_through_untouched(self) -> None: + @pytest.mark.parametrize("domain", ["contoso.com", "http://example.com", "https://example.com:8443"]) + async def test_explicit_domain_is_passed_through_untouched(self, domain: str) -> None: """A decision scoped by ``domain`` must not be rewritten either.""" from copilot.generated.rpc import PermissionDecisionApproveForSession from copilot.session_events import PermissionRequestUrl - decision = PermissionDecisionApproveForSession(domain="contoso.com") + decision = PermissionDecisionApproveForSession(domain=domain) request = PermissionRequestUrl(intention="fetch", url="https://example.com/a") result = await self.normalize(request, decision) assert result is decision - assert result.domain == "contoso.com" + assert result.domain == domain @pytest.mark.parametrize("decision_name", ["PermissionDecisionApproveOnce", "PermissionDecisionUserNotAvailable"]) async def test_other_decision_kinds_are_passed_through_untouched(self, decision_name: str) -> None: