Python: [BREAKING] Normalize workflow HTTP request URLs

python/packages/declarative/agent_framework_declarative/_workflows/_http_handler.py

@@ -22,10 +22,10 @@
 
 import asyncio
 from collections.abc import Awaitable, Callable, Mapping
-from dataclasses import dataclass, field
+from dataclasses import dataclass, field, replace
 from http.cookiejar import CookieJar, DefaultCookiePolicy
 from typing import Any, Protocol, runtime_checkable
-from urllib.parse import parse_qsl, quote, urlencode, urlsplit, urlunsplit
+from urllib.parse import parse_qsl, quote, urlencode, urlsplit
 
 import httpx
 
@@ -131,6 +131,12 @@ class DefaultHttpRequestHandler:
        provider's clients retain their cookie behavior and are not closed by :meth:`aclose`.
        Returning ``None`` falls back to ``client``, if supplied, then to the owned client.
 
+    The provider receives a new :class:`HttpRequestInfo` with an absolute HTTP(S) URL
+    normalized by ``httpx`` and ``query_parameters`` already appended to ``url``.
+    The snapshot's ``query_parameters`` is empty; the original request is not modified.
+    Client-level query defaults are applied after selection, and supplied clients retain
+    their redirect behavior. The provider is not called again for automatic redirects.
+
     Applications requiring persistent cookies must supply a client through ``client``
     or ``client_provider`` scoped to one authenticated principal and manage its lifetime.
     Explicit outbound ``Cookie`` headers and response ``Set-Cookie`` headers are preserved;
@@ -162,6 +168,22 @@ async def send(self, info: HttpRequestInfo) -> HttpRequestResult:
         if not info.method:
             raise ValueError("HttpRequestInfo.method must be a non-empty string.")
 
+        url = httpx.URL(info.url)
+        if not url.is_absolute_url or url.scheme not in {"http", "https"}:
+            raise ValueError("HttpRequestInfo.url must be an absolute HTTP or HTTPS URL.")
+
+        # Preserve the authored query bytes, appending rather than replacing duplicate
+        # keys. QueryParams would rewrite escapes, bare flags and duplicate ordering.
+        query = urlsplit(info.url).query
+        explicit_pairs = [(key, value) for key, value in info.query_parameters.items() if key]
+        if explicit_pairs:
+            query += ("&" if query else "") + urlencode(explicit_pairs, quote_via=quote)
+        raw_path = url.raw_path.split(b"?", 1)[0]
+        if query:
+            raw_path += b"?" + _encode_query(query)
+        url = url.copy_with(raw_path=raw_path)
+
+        info = replace(info, url=str(url), headers=dict(info.headers), query_parameters={})
         client = await self._resolve_client(info)
 
         timeout: httpx.Timeout | object
@@ -181,54 +203,26 @@ async def send(self, info: HttpRequestInfo) -> HttpRequestResult:
                 # callers (not just the YAML executor) get sensible defaults.
                 headers["Content-Type"] = info.body_content_type or "text/plain"
 
-        # Compose the query as raw bytes rather than routing it through ``params=``.
-        # Anything that reaches ``httpx.QueryParams`` -- ``params=`` or the client's own
-        # ``AsyncClient.params`` merge -- is decoded and re-encoded. Measured against
-        # httpx 0.28.1, that rewrites ``%20`` in a value as ``+``, expands a bare
-        # ``download`` into ``download=``, and reorders interleaved duplicates
-        # (``x=1&y=2&x=3`` becomes ``x=1&x=3&y=2``), all of it even when nothing needed
-        # merging. Base64 and ``%3A`` round-trip unchanged, so a signature is not
-        # rewritten by itself, but any scheme that signs a value holding an encoded
-        # space is, and a server that distinguishes ``download`` from ``download=`` or
-        # reads repeated keys positionally sees a different request either way.
-        # Building the query here and writing it back over the request's ``raw_path``
-        # keeps the caller's bytes exactly as given.
-        raw = urlsplit(info.url)
-
-        # ``query_parameters`` append rather than replace, so a URL carrying
-        # ``filter=region&filter=status`` plus ``{"filter": "tenant"}`` sends all three,
-        # matching the .NET handler. Client-level params are defaults only: they apply
-        # for a key absent from both request-level sources.
-        explicit_pairs = [(key, value) for key, value in info.query_parameters.items() if key]
-        request_keys = {key for key, _ in parse_qsl(raw.query, keep_blank_values=True)}
-        request_keys.update(key for key, _ in explicit_pairs)
+        # Selected-client params remain defaults for keys absent from the workflow's
+        # composed query. They are host configuration, unavailable before selection.
+        request_keys = {key for key, _ in parse_qsl(url.query.decode("ascii"), keep_blank_values=True)}
         client_defaults = [(key, value) for key, value in client.params.multi_items() if key not in request_keys]
-
-        query_segments: list[str] = []
-        if raw.query:
-            query_segments.append(raw.query)
-        # ``quote_via=quote`` keeps spaces as ``%20`` instead of ``+``, so appended
-        # parameters are encoded the same way the preserved URL query is.
-        for pairs in (explicit_pairs, client_defaults):
-            if pairs:
-                query_segments.append(urlencode(pairs, quote_via=quote))
-        query = "&".join(segment for segment in query_segments if segment)
+        if client_defaults:
+            encoded_query = url.query + (b"&" if url.query else b"")
+            encoded_query += urlencode(client_defaults, quote_via=quote).encode("ascii")
+            url = url.copy_with(query=encoded_query)
 
         # Build without the query so the client's params merge has nothing to clobber --
         # it drops the URL's query outright -- then overwrite the query httpx composed
         # with ours. The fragment stays on the built URL; httpx does not send it.
         request = client.build_request(
             method=info.method,
-            url=urlunsplit((raw.scheme, raw.netloc, raw.path, "", raw.fragment)),
+            url=url.copy_with(query=None),
             headers=headers or None,
             content=content,
             timeout=timeout,
         )
-        # Reuse httpx's own path encoding and replace only the query part of ``raw_path``.
-        raw_path = request.url.raw_path.split(b"?", 1)[0]
-        if query:
-            raw_path += b"?" + _encode_query(query)
-        request.url = request.url.copy_with(raw_path=raw_path)
+        request.url = url
 
         response = await client.send(request)
 

python/packages/declarative/tests/test_default_http_request_handler.py

@@ -15,7 +15,8 @@
 import sys
 from collections.abc import AsyncIterator
 from typing import Any
-from unittest.mock import patch
+from unittest.mock import AsyncMock, patch
+from urllib.parse import urlsplit
 
 import httpx
 import pytest
@@ -538,6 +539,133 @@ async def send(index: int) -> None:
 
 
 class TestClientProvider:
+    @pytest.mark.parametrize(
+        ("query_parameters", "expected_query"),
+        [
+            ({}, "term=a%20b&download&x=1&y=2&x=3"),
+            ({"term": "c d", "q": "a&b"}, "term=a%20b&download&x=1&y=2&x=3&term=c%20d&q=a%26b"),
+            ({"q": "caf\u00e9", "": "ignored"}, "term=a%20b&download&x=1&y=2&x=3&q=caf%C3%A9"),
+        ],
+    )
+    async def test_client_provider_receives_canonical_composed_url(
+        self, query_parameters: dict[str, str], expected_query: str
+    ) -> None:
+        provider_urls: list[str] = []
+        requests: list[httpx.Request] = []
+
+        def respond(request: httpx.Request) -> httpx.Response:
+            requests.append(request)
+            return httpx.Response(200, text="ok")
+
+        info = HttpRequestInfo(
+            method="POST",
+            url="https://API.EXAMPLE.TEST:443/items/../settings?term=a%20b&download&x=1&y=2&x=3#details",
+            headers={"X-Request": "value"},
+            query_parameters=dict(query_parameters),
+            body="request body",
+            body_content_type="text/plain",
+            timeout_ms=1500,
+            connection_name="example-connection",
+        )
+        original_url = info.url
+        async with httpx.AsyncClient(transport=httpx.MockTransport(respond)) as client:
+
+            async def provider(request_info: HttpRequestInfo) -> httpx.AsyncClient:
+                provider_urls.append(request_info.url)
+                assert request_info is not info
+                assert request_info.query_parameters == {}
+                assert request_info.headers == info.headers
+                assert request_info.headers is not info.headers
+                assert request_info.method == info.method
+                assert request_info.body == info.body
+                assert request_info.body_content_type == info.body_content_type
+                assert request_info.timeout_ms == info.timeout_ms
+                assert request_info.connection_name == info.connection_name
+                return client
+
+            async with DefaultHttpRequestHandler(client_provider=provider) as handler:
+                await handler.send(info)
+
+        expected_url = f"https://api.example.test/settings?{expected_query}#details"
+        assert [str(request.url) for request in requests] == [expected_url]
+        assert provider_urls == [expected_url]
+        assert info.url == original_url
+        assert info.query_parameters == query_parameters
+        assert info.headers == {"X-Request": "value"}
+        assert requests[0].method == "POST"
+        assert requests[0].content == b"request body"
+
+    @pytest.mark.parametrize(
+        ("path", "expected_status"),
+        [
+            pytest.param("items/list", 200, id="allowed-path"),
+            pytest.param("settings/update", 403, id="restricted-path"),
+            pytest.param("items/../settings/update", 403, id="normalized-restricted-path"),
+        ],
+    )
+    async def test_client_provider_credentials_match_canonical_path(self, path: str, expected_status: int) -> None:
+        requests: list[httpx.Request] = []
+        protected_actions: list[str] = []
+        credential = "Bearer synthetic-test-credential"
+
+        def respond(request: httpx.Request) -> httpx.Response:
+            requests.append(request)
+            authenticated = request.headers.get("Authorization") == credential
+            if request.url.path == "/settings/update" and authenticated:
+                protected_actions.append(request.url.path)
+            return httpx.Response(200 if authenticated else 403)
+
+        async with (
+            httpx.AsyncClient(
+                headers={"Authorization": credential}, transport=httpx.MockTransport(respond)
+            ) as authenticated_client,
+            httpx.AsyncClient(transport=httpx.MockTransport(respond)) as anonymous_client,
+        ):
+
+            async def provider(info: HttpRequestInfo) -> httpx.AsyncClient:
+                url = urlsplit(info.url)
+                if url.scheme == "https" and url.netloc == "api.example.test" and url.path.startswith("/items/"):
+                    return authenticated_client
+                return anonymous_client
+
+            async with DefaultHttpRequestHandler(client_provider=provider) as handler:
+                result = await handler.send(HttpRequestInfo(method="POST", url=f"https://api.example.test/{path}"))
+
+        assert result.status_code == expected_status
+        assert len(requests) == 1
+        assert requests[0].headers.get("Authorization") == (credential if expected_status == 200 else None)
+        assert protected_actions == []
+
+    @pytest.mark.parametrize("follow_redirects", [False, True])
+    async def test_provider_client_retains_redirect_behavior(self, follow_redirects: bool) -> None:
+        provider_urls: list[str] = []
+        request_paths: list[str] = []
+
+        def respond(request: httpx.Request) -> httpx.Response:
+            request_paths.append(request.url.path)
+            if request.url.path == "/start":
+                return httpx.Response(307, headers={"Location": "/next"})
+            return httpx.Response(200, text="ok")
+
+        async with httpx.AsyncClient(
+            follow_redirects=follow_redirects, transport=httpx.MockTransport(respond)
+        ) as client:
+
+            async def provider(info: HttpRequestInfo) -> httpx.AsyncClient:
+                provider_urls.append(info.url)
+                return client
+
+            async with DefaultHttpRequestHandler(client_provider=provider) as handler:
+                result = await handler.send(
+                    HttpRequestInfo(method="GET", url="https://api.example.test/items/../start")
+                )
+            assert client.follow_redirects is follow_redirects
+            assert not client.is_closed
+
+        assert result.status_code == (200 if follow_redirects else 307)
+        assert request_paths == (["/start", "/next"] if follow_redirects else ["/start"])
+        assert provider_urls == ["https://api.example.test/start"]
+
     @pytest.mark.asyncio
     async def test_client_provider_overrides_default(self) -> None:
         captured: dict[str, str] = {}
@@ -590,6 +718,25 @@ async def provider(info: HttpRequestInfo) -> httpx.AsyncClient | None:
 
 
 class TestValidation:
+    @pytest.mark.parametrize(
+        ("url", "error_type"),
+        [
+            ("/items", ValueError),
+            ("//api.example.test/items", ValueError),
+            ("https:///items", ValueError),
+            ("ftp://api.example.test/items", ValueError),
+            ("https://api.example.test:invalid/items", httpx.InvalidURL),
+            ("https://api.example.test/items\n", httpx.InvalidURL),
+        ],
+    )
+    async def test_invalid_url_rejected_before_client_selection(self, url: str, error_type: type[Exception]) -> None:
+        provider = AsyncMock(return_value=None)
+        async with DefaultHttpRequestHandler(client_provider=provider) as handler:
+            with pytest.raises(error_type):
+                await handler.send(HttpRequestInfo(method="GET", url=url))
+            assert handler._owned_client is None
+        provider.assert_not_awaited()
+
     @pytest.mark.asyncio
     async def test_empty_url_raises(self) -> None:
         handler = DefaultHttpRequestHandler()
@@ -726,14 +873,15 @@ def respond(request: httpx.Request) -> httpx.Response:
     async def test_client_headers_cookies_auth_and_timeout_survive(self, client_source: str) -> None:
         """Building through the client keeps its configuration, params merge bypassed."""
         requests: list[httpx.Request] = []
+        provider_urls: list[str] = []
 
         def respond(request: httpx.Request) -> httpx.Response:
             requests.append(request)
             return httpx.Response(200, text="ok", headers={"Set-Cookie": "response=retained; Path=/; Secure"})
 
         auth = httpx.BasicAuth("user", "pass")
         client = httpx.AsyncClient(
-            params={"api-version": "2024-01-01"},
+            params={"api-version": "2024-01-01", "keep": "client", "extra": "client"},
             headers={"X-Client-Header": "client-value"},
             cookies={"session": "cookie-value"},
             auth=auth,
@@ -743,6 +891,7 @@ def respond(request: httpx.Request) -> httpx.Response:
         )
 
         async def provider(info: HttpRequestInfo) -> httpx.AsyncClient | None:
+            provider_urls.append(info.url)
             return client if client_source == "provider" else None
 
         handler = DefaultHttpRequestHandler(
@@ -753,7 +902,8 @@ async def provider(info: HttpRequestInfo) -> httpx.AsyncClient | None:
             await handler.send(
                 HttpRequestInfo(
                     method="GET",
-                    url="https://api.example.test/s?keep=me",
+                    url="https://api.example.test/items/../s?keep=me",
+                    query_parameters={"extra": "a b"},
                     timeout_ms=4500,
                 )
             )
@@ -784,7 +934,12 @@ async def provider(info: HttpRequestInfo) -> httpx.AsyncClient | None:
             "write": 4.5,
             "pool": 4.5,
         }
-        assert request.url.raw_path == b"/s?keep=me&api-version=2024-01-01"
+        assert request.url.raw_path == b"/s?keep=me&extra=a%20b&api-version=2024-01-01"
+        assert provider_urls == (
+            []
+            if client_source == "caller"
+            else ["https://api.example.test/s?keep=me&extra=a%20b", "https://api.example.test/s"]
+        )
 
     @pytest.mark.asyncio
     async def test_non_ascii_url_query_is_percent_encoded_not_rejected(self) -> None:

python/packages/declarative/tests/test_http_request_yaml_integration.py

@@ -144,6 +144,43 @@ def create_client(**kwargs: Any) -> httpx.AsyncClient:
     assert all("Cookie" not in request.headers for request in requests)
 
 
+async def test_http_request_yaml_provider_receives_canonical_input_url() -> None:
+    provider_urls: list[str] = []
+    request_urls: list[str] = []
+
+    def respond(request: httpx.Request) -> httpx.Response:
+        request_urls.append(str(request.url))
+        return httpx.Response(200, text="ok")
+
+    definition = """
+kind: Workflow
+trigger:
+  kind: OnConversationStart
+  id: canonical_url_test
+  actions:
+    - kind: HttpRequestAction
+      id: request
+      method: GET
+      url: '=Concatenate("https://api.example.test/", inputs.path)'
+      queryParameters:
+        term: =inputs.term
+      response: Local.Response
+"""
+    async with httpx.AsyncClient(transport=httpx.MockTransport(respond)) as client:
+
+        async def provider(info: HttpRequestInfo) -> httpx.AsyncClient:
+            provider_urls.append(info.url)
+            return client
+
+        async with DefaultHttpRequestHandler(client_provider=provider) as handler:
+            workflow = WorkflowFactory(http_request_handler=handler).create_workflow_from_yaml(definition)
+            await workflow.run({"path": "items/../settings?keep=a%20b", "term": "c d"})
+
+    expected_url = "https://api.example.test/settings?keep=a%20b&term=c%20d"
+    assert provider_urls == [expected_url]
+    assert request_urls == [expected_url]
+
+
 @pytest.mark.asyncio
 async def test_http_request_yaml_missing_handler_fails_at_build_time() -> None:
     """Without an http_request_handler, building the workflow must raise."""