From 7322cb28c33ae8c7ee037700bb9ee8d621126914 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 06:43:01 +0000 Subject: [PATCH 1/5] test: cover response decoding, check_loads, binary request bodies and URL encoding Characterisation tests on the unchanged code. Co-Authored-By: Claude Opus 5.5 --- tests/test_response_decoding.py | 100 ++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 tests/test_response_decoding.py diff --git a/tests/test_response_decoding.py b/tests/test_response_decoding.py new file mode 100644 index 0000000..7ec8ab3 --- /dev/null +++ b/tests/test_response_decoding.py @@ -0,0 +1,100 @@ +"""How responses are decoded (``http2py.decorators``) and how a client's requests are +built from a spec it may have fetched from a server (``mk_request_function``).""" + +import pickle +import warnings + +import pytest + +from http2py.decorators import ( + BinaryResponseNotDecoded, + check_loads, + handle_binary_resp, + unsafe_pickle_loads, +) +from http2py.py2request import mk_request_function + + +class Resp: + def __init__(self, content=b"", *, status_code=200, text="", reason="OK"): + self.content, self.status_code, self.text, self.reason = ( + content, + status_code, + text, + reason, + ) + + +def _identity(x): + return x + + +def test_binary_response_needs_an_explicit_decoder(): + with pytest.raises(BinaryResponseNotDecoded): + handle_binary_resp(_identity)(Resp(pickle.dumps(1))) + assert handle_binary_resp(_identity, loads=bytes)(Resp(b"raw")) == b"raw" + trusted = handle_binary_resp(_identity, loads=unsafe_pickle_loads) + assert trusted(Resp(pickle.dumps({"a": 1}))) == {"a": 1} + + +def test_check_loads(): + check_loads(None) + check_loads(bytes) + with pytest.raises(TypeError): + check_loads("pickle.loads") + with pytest.warns(UserWarning, match="unsafe_pickle_loads"): + check_loads(pickle.loads) + with warnings.catch_warnings(): + warnings.simplefilter("error") + check_loads(unsafe_pickle_loads) # the explicit opt-in does not warn + + +def test_non_200_binary_response_raises_the_mapped_error(): + from i2.errors import NotFoundError + + with pytest.raises(NotFoundError): + handle_binary_resp(_identity, loads=bytes)(Resp(status_code=404, text="no")) + + +def _captured_request(method_spec, **call_kwargs): + """The (method, url, request kwargs) a request function would dispatch.""" + calls = [] + + def dispatch(method, url, **kwargs): + calls.append((method, url, kwargs)) + return Resp(b"{}") + + func = mk_request_function(method_spec, dispatch=dispatch) + func(None, **call_kwargs) + return calls[0] + + +def test_binary_request_bodies_use_binary_dumps(): + spec = { + "method": "POST", + "url_template": "http://h/f", + "body_arg_names": ["x"], + "content_type": "application/octet-stream", + "output_trans": _identity, + } + _, _, kwargs = _captured_request(spec, x=1) + assert pickle.loads(kwargs["data"]) == {"x": 1} + calls = [] + func = mk_request_function( + spec, + dispatch=lambda m, u, **kw: calls.append(kw) or Resp(), + binary_dumps=lambda d: b"custom", + ) + func(None, x=1) + assert calls[0]["data"] == b"custom" + + +def test_path_args_are_url_encoded(): + spec = { + "method": "GET", + "url_template": "http://h/u/{uid}", + "path_arg_names": ["uid"], + "output_trans": _identity, + } + _, url, _ = _captured_request(spec, uid="a/b c") + assert url == "http://h/u/a%2Fb%20c" From c0e0fe5c10b5682de53929ef45d906d218e2db33 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 06:43:46 +0000 Subject: [PATCH 2/5] test: a second route of the same name replaces the first, with a warning Co-Authored-By: Claude Opus 5.5 --- tests/test_route_namespace.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_route_namespace.py b/tests/test_route_namespace.py index 325d9c6..c460ad0 100644 --- a/tests/test_route_namespace.py +++ b/tests/test_route_namespace.py @@ -83,6 +83,7 @@ def test_cli_calls_the_route_not_the_client_attribute(monkeypatch): spec = _spec_naming_plain("ensure_login") with pytest.warns(UserWarning): route = _client(spec).routes["ensure_login"] + class Sent(Exception): pass @@ -162,3 +163,14 @@ def test_unusable_operation_id_falls_back_to_method_and_path(operation_id): spec = _spec_without_names() spec["paths"]["/plain"]["get"]["operationId"] = operation_id assert "get_plain" in _client(spec).routes + + +def test_a_second_route_of_the_same_name_replaces_the_first_with_a_warning(): + spec = copy.deepcopy(SPEC) + other = copy.deepcopy(spec["paths"]["/plain"]) + spec["paths"]["/plain2"] = other + with pytest.warns(UserWarning, match="Two routes are named"): + client = _client(spec) + name = other["get"]["x-method_name"] + assert client.routes[name].method_spec["url_template"].endswith("/plain2") + assert getattr(client, name) is client.routes[name] From 6f73ac7eb4a1011f8c2ee60b12412239bb21570d Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 06:43:46 +0000 Subject: [PATCH 3/5] fix: URL templates from a spec may only name arguments A request function fills its url_template with str.format(**arguments), and the template comes from the spec, which HttpClient may fetch from the server. A placeholder such as {x.__class__} or {x.f.__globals__[KEY]} would read attributes of the caller's own arguments (with pickled object arguments, module globals) into the URL sent to that server. mk_request_function now refuses any placeholder that is not a plain name, so a client built from such a spec fails at construction. Co-Authored-By: Claude Opus 5.5 --- http2py/py2request.py | 26 ++++++++++++++++++++++++++ tests/test_response_decoding.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/http2py/py2request.py b/http2py/py2request.py index 2a7caf1..4b7679a 100644 --- a/http2py/py2request.py +++ b/http2py/py2request.py @@ -200,6 +200,8 @@ def mk_request_function( original_spec = method_spec method_spec = method_spec.copy() # make a copy + if "url_template" in method_spec: + check_url_template(method_spec["url_template"]) method_spec["input_trans"] = method_spec.get("input_trans", None) or {} request_kwargs = method_spec.get("request_kwargs", {}).copy() @@ -502,6 +504,30 @@ def _difference_conserving_order(a, b): return [x for x in a if x not in b] +def check_url_template(url_template: str) -> str: + """Return ``url_template`` if every placeholder in it is a plain name. + + A template is filled with ``str.format(**arguments)``, and it may come from a spec + fetched from a server. ``{a.__class__}`` or ``{a[0]}`` would read attributes or + items of the caller's arguments into the URL, so they are refused. + + >>> check_url_template('http://h/u/{uid}?q={q}') + 'http://h/u/{uid}?q={q}' + >>> check_url_template('http://h/{a.__init__.__globals__}') + Traceback (most recent call last): + ... + ValueError: URL template placeholder '{a.__init__.__globals__}' is not a plain name + """ + for _, field_name, format_spec, _ in str_formatter.parse(url_template): + if field_name is None: + continue + if not field_name.isidentifier() or "{" in (format_spec or ""): + raise ValueError( + f"URL template placeholder {'{' + field_name + '}'!r} is not a plain name" + ) + return url_template + + def extract_format_names(string_template): """ Extracts the names of the fields in a string template, returning a list of diff --git a/tests/test_response_decoding.py b/tests/test_response_decoding.py index 7ec8ab3..2cd6727 100644 --- a/tests/test_response_decoding.py +++ b/tests/test_response_decoding.py @@ -98,3 +98,36 @@ def test_path_args_are_url_encoded(): } _, url, _ = _captured_request(spec, uid="a/b c") assert url == "http://h/u/a%2Fb%20c" + + +@pytest.mark.parametrize( + "template", + [ + "http://h/{x.__class__}", + "http://h/{x[0]}", + "http://h/{x:{x.__class__}}", + "http://h/{0}", + ], +) +def test_url_templates_may_only_name_arguments(template): + """A spec served by a remote must not read attributes of the caller's + arguments into the URL.""" + spec = {"method": "GET", "url_template": template, "path_arg_names": ["x"]} + with pytest.raises(ValueError, match="not a plain name"): + mk_request_function(spec) + + +def test_spec_with_attribute_placeholder_is_refused_by_the_client(): + import copy + import sys + from pathlib import Path + + sys.path.insert(0, str(Path(__file__).parent)) + from spec_fixture import SPEC + + from http2py import HttpClient + + spec = copy.deepcopy(SPEC) + spec["paths"]["/x/{uid.__class__.__mro__}"] = spec["paths"].pop("/u/{uid}/p/{pid}") + with pytest.raises(ValueError, match="not a plain name"): + HttpClient(spec, session_state={"session": object(), "refresh_inputs": {}}) From 627a5d8b5cbbcf77cd595bedd62f2b04e095adaf Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 06:44:17 +0000 Subject: [PATCH 4/5] fix: multipart form requests send their fields with any method, not only GET The form branch of request_func hung off 'if method != "GET"' as an elif, so a POST (or PUT...) with content type multipart/form-data sent no body at all. Co-Authored-By: Claude Opus 5.5 --- http2py/py2request.py | 2 +- tests/test_response_decoding.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/http2py/py2request.py b/http2py/py2request.py index 4b7679a..cb6275c 100644 --- a/http2py/py2request.py +++ b/http2py/py2request.py @@ -287,7 +287,7 @@ def request_func(*args, **kwargs): _request_kwargs["data"] = json.dumps(json_kwargs) elif content_type == BINARY_CONTENT_TYPE: _request_kwargs["data"] = binary_dumps(kwargs) - elif content_type == FORM_CONTENT_TYPE: + if content_type == FORM_CONTENT_TYPE: # (it used to be sent with GET only) fields = json.dumps(json_kwargs).encode("utf-8") binaries = {k: (f"{k}.bin", v) for k, v in binary_kwargs.items()} files_data = dict(binaries, __fields=fields) diff --git a/tests/test_response_decoding.py b/tests/test_response_decoding.py index 2cd6727..ac78702 100644 --- a/tests/test_response_decoding.py +++ b/tests/test_response_decoding.py @@ -131,3 +131,16 @@ def test_spec_with_attribute_placeholder_is_refused_by_the_client(): spec["paths"]["/x/{uid.__class__.__mro__}"] = spec["paths"].pop("/u/{uid}/p/{pid}") with pytest.raises(ValueError, match="not a plain name"): HttpClient(spec, session_state={"session": object(), "refresh_inputs": {}}) + + +@pytest.mark.parametrize("method", ["GET", "POST"]) +def test_form_requests_send_their_fields_whatever_the_method(method): + spec = { + "method": method, + "url_template": "http://h/f", + "body_arg_names": ["x", "blob"], + "content_type": "multipart/form-data", + "output_trans": _identity, + } + _, _, kwargs = _captured_request(spec, x=1, blob=b"\x00") + assert set(kwargs["files"]) == {"blob", "__fields"} From 08f13bfdb741284f77638d5e79abc26e0bf85688 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 06:47:47 +0000 Subject: [PATCH 5/5] fix: refuse only placeholders that read into arguments; check query_arg_names too From the adversarial review of this PR: - hyphenated OpenAPI placeholders ({user-id}) worked on master and were refused, taking the whole client down with them. Only '.' and '[' (attribute and item lookups) and nested fields in a format spec are refused now; - query_arg_names, appended as ?name={name} at call time, bypassed the check. Co-Authored-By: Claude Opus 5.5 --- http2py/py2request.py | 41 +++++++++++++++++++++++---------- tests/test_response_decoding.py | 26 ++++++++++++++++++--- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/http2py/py2request.py b/http2py/py2request.py index cb6275c..eb4c47e 100644 --- a/http2py/py2request.py +++ b/http2py/py2request.py @@ -201,7 +201,10 @@ def mk_request_function( original_spec = method_spec method_spec = method_spec.copy() # make a copy if "url_template" in method_spec: - check_url_template(method_spec["url_template"]) + check_url_template( + method_spec["url_template"], + query_arg_names=_ensure_list(method_spec.get("query_arg_names", [])), + ) method_spec["input_trans"] = method_spec.get("input_trans", None) or {} request_kwargs = method_spec.get("request_kwargs", {}).copy() @@ -504,26 +507,40 @@ def _difference_conserving_order(a, b): return [x for x in a if x not in b] -def check_url_template(url_template: str) -> str: - """Return ``url_template`` if every placeholder in it is a plain name. +def _reads_into_arguments(field_name: str) -> bool: + """Whether a ``str.format`` field looks up an attribute or item of an argument; + any other name (``user-id``, ``0``...) only selects an argument.""" + return "." in field_name or "[" in field_name + + +def check_url_template(url_template: str, *, query_arg_names=()) -> str: + """Return ``url_template`` if no placeholder in it, nor any of the + ``query_arg_names`` (appended as ``?name={name}`` when calling), reads into an + argument. A template is filled with ``str.format(**arguments)``, and it may come from a spec fetched from a server. ``{a.__class__}`` or ``{a[0]}`` would read attributes or - items of the caller's arguments into the URL, so they are refused. + items of the caller's arguments into the URL, so they are refused. Other names, + such as OpenAPI's hyphenated ``{user-id}``, are fine. - >>> check_url_template('http://h/u/{uid}?q={q}') - 'http://h/u/{uid}?q={q}' + >>> check_url_template('http://h/u/{user-id}?q={q}') + 'http://h/u/{user-id}?q={q}' >>> check_url_template('http://h/{a.__init__.__globals__}') Traceback (most recent call last): ... - ValueError: URL template placeholder '{a.__init__.__globals__}' is not a plain name + ValueError: URL template placeholder '{a.__init__.__globals__}' reads into an argument """ - for _, field_name, format_spec, _ in str_formatter.parse(url_template): - if field_name is None: - continue - if not field_name.isidentifier() or "{" in (format_spec or ""): + fields = [ + (field_name, format_spec) + for _, field_name, format_spec, _ in str_formatter.parse(url_template) + if field_name is not None + ] + fields += [(name, "") for name in query_arg_names] + for field_name, format_spec in fields: + if _reads_into_arguments(field_name) or "{" in (format_spec or ""): raise ValueError( - f"URL template placeholder {'{' + field_name + '}'!r} is not a plain name" + f"URL template placeholder {'{' + field_name + '}'!r} reads into an " + "argument" ) return url_template diff --git a/tests/test_response_decoding.py b/tests/test_response_decoding.py index ac78702..5b460ee 100644 --- a/tests/test_response_decoding.py +++ b/tests/test_response_decoding.py @@ -106,17 +106,37 @@ def test_path_args_are_url_encoded(): "http://h/{x.__class__}", "http://h/{x[0]}", "http://h/{x:{x.__class__}}", - "http://h/{0}", + "http://h/{x!r:{y}}", ], ) def test_url_templates_may_only_name_arguments(template): """A spec served by a remote must not read attributes of the caller's arguments into the URL.""" spec = {"method": "GET", "url_template": template, "path_arg_names": ["x"]} - with pytest.raises(ValueError, match="not a plain name"): + with pytest.raises(ValueError, match="reads into an argument"): mk_request_function(spec) +def test_query_arg_names_may_only_name_arguments(): + spec = { + "method": "GET", + "url_template": "http://h/x", + "query_arg_names": ["obj", "obj.token"], + } + with pytest.raises(ValueError, match="reads into an argument"): + mk_request_function(spec) + + +def test_hyphenated_placeholders_still_work(): + spec = {"method": "GET", "url_template": "http://h/u/{user-id}"} + calls = [] + func = mk_request_function( + spec, dispatch=lambda m, u, **kw: calls.append(u) or Resp(b"") + ) + func(None, **{"user-id": "42"}) + assert calls == ["http://h/u/42"] + + def test_spec_with_attribute_placeholder_is_refused_by_the_client(): import copy import sys @@ -129,7 +149,7 @@ def test_spec_with_attribute_placeholder_is_refused_by_the_client(): spec = copy.deepcopy(SPEC) spec["paths"]["/x/{uid.__class__.__mro__}"] = spec["paths"].pop("/u/{uid}/p/{pid}") - with pytest.raises(ValueError, match="not a plain name"): + with pytest.raises(ValueError, match="reads into an argument"): HttpClient(spec, session_state={"session": object(), "refresh_inputs": {}})