Python: narrow unscoped Copilot URL session approvals

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`
 

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<backslash>@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()
 

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: