diff --git a/python/packages/declarative/agent_framework_declarative/_workflows/_http_handler.py b/python/packages/declarative/agent_framework_declarative/_workflows/_http_handler.py index 3dff14d17d6..e73cb860223 100644 --- a/python/packages/declarative/agent_framework_declarative/_workflows/_http_handler.py +++ b/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) diff --git a/python/packages/declarative/tests/test_default_http_request_handler.py b/python/packages/declarative/tests/test_default_http_request_handler.py index 3db80fd943e..7d4391fa43f 100644 --- a/python/packages/declarative/tests/test_default_http_request_handler.py +++ b/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,6 +873,7 @@ 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) @@ -733,7 +881,7 @@ def respond(request: httpx.Request) -> httpx.Response: 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: diff --git a/python/packages/declarative/tests/test_http_request_yaml_integration.py b/python/packages/declarative/tests/test_http_request_yaml_integration.py index 1887b6787db..c35cc9c23f8 100644 --- a/python/packages/declarative/tests/test_http_request_yaml_integration.py +++ b/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."""