From 2784d7b15847d3e12a6c46eb8332afbdde4b30e9 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:09:01 +0000 Subject: [PATCH 1/2] fix: never unpickle binary request bodies unless explicitly asked handle_binary_req now takes a keyword-only `loads` (bytes -> mapping of inputs) and raises at decoration time when it is not given. Unpickling is still available as an explicit opt-in, `loads=unsafe_pickle_loads`, for services whose clients are all trusted. The decoded body must be a mapping. Refs #18 (item 1, server side). Co-Authored-By: Claude Opus 5 --- py2http/decorators.py | 57 ++++++++++++++++++++++---- py2http/tests/test_binary_req.py | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 7 deletions(-) create mode 100644 py2http/tests/test_binary_req.py diff --git a/py2http/decorators.py b/py2http/decorators.py index aefb8c6..ff7f149 100644 --- a/py2http/decorators.py +++ b/py2http/decorators.py @@ -1007,7 +1007,7 @@ def _validate_and_invoke_mapper(func, inputs): return func(**inputs) -def _handle_req(func, content_type): +def _handle_req(func, content_type, *, binary_loads=None): func.request_schema = mk_input_schema_from_func(func) func.content_type = content_type @@ -1017,7 +1017,7 @@ def input_mapper(req): if content_type not in req.content_type: raise RuntimeError(f"The incoming request's content is of type \ {req.content_type}, when {content_type} is expected.") - inputs = _get_inputs_from_request(req, content_type) + inputs = _get_inputs_from_request(req, content_type, binary_loads=binary_loads) return _validate_and_invoke_mapper(func, inputs) return input_mapper @@ -1027,8 +1027,43 @@ def handle_json_req(func): return _handle_req(func, JSON_CONTENT_TYPE) -def handle_binary_req(func): - return _handle_req(func, BINARY_CONTENT_TYPE) +def unsafe_pickle_loads(data: bytes): + """Unpickle ``data``. UNSAFE on anything a client can send. + + Unpickling runs code chosen by whoever produced the bytes, so this must only + be used when every caller of the endpoint is fully trusted (for example, a + service reachable only by your own processes). It exists so that opting into + pickled request bodies is explicit and visible at the call site:: + + handle_binary_req(func, loads=unsafe_pickle_loads) + """ + return pickle.loads(data) + + +def handle_binary_req(func, *, loads: Callable[[bytes], Mapping] = None): + """Make an input mapper that decodes a binary (octet-stream) request body. + + ``loads`` turns the raw body bytes into the mapping of keyword arguments for + ``func``. There is deliberately no default: request bodies used to be + unpickled implicitly, which lets any client run code on the server. Pass a + safe decoder of your own, or ``loads=unsafe_pickle_loads`` if (and only if) + every client is trusted. + + >>> handle_binary_req(lambda x: x) + Traceback (most recent call last): + ... + TypeError: handle_binary_req needs an explicit loads=... (bytes -> dict of inputs). ... + """ + if loads is None: + raise TypeError( + "handle_binary_req needs an explicit loads=... (bytes -> dict of " + "inputs). Request bodies are no longer unpickled by default, since " + "unpickling client data lets the client run code on the server. " + "Use loads=unsafe_pickle_loads only if every client is trusted." + ) + if not callable(loads): + raise TypeError(f"loads must be callable, got {loads!r}") + return _handle_req(func, BINARY_CONTENT_TYPE, binary_loads=loads) def handle_form_req(func): @@ -1170,7 +1205,7 @@ def decorator(func): return decorator -def _get_inputs_from_request(request, content_type): +def _get_inputs_from_request(request, content_type, *, binary_loads=None): defaults = getattr(request, "defaults", {}) if request.method == "POST": if content_type == JSON_CONTENT_TYPE: @@ -1179,8 +1214,16 @@ def _get_inputs_from_request(request, content_type): data = request.body.read().decode("utf-8") inputs = json.loads(data) elif content_type == BINARY_CONTENT_TYPE: - data = request.body.read() - inputs = pickle.loads(data) + if binary_loads is None: + raise TypeError( + "No decoder given for a binary request body; see handle_binary_req." + ) + inputs = binary_loads(request.body.read()) + if not isinstance(inputs, Mapping): + raise TypeError( + f"The binary request body decoded to a {type(inputs).__name__}, " + "not a mapping of inputs." + ) elif content_type == FORM_CONTENT_TYPE: fields = json.loads( request.files.pop("__fields").file.read().decode("utf-8") diff --git a/py2http/tests/test_binary_req.py b/py2http/tests/test_binary_req.py new file mode 100644 index 0000000..ffd50f2 --- /dev/null +++ b/py2http/tests/test_binary_req.py @@ -0,0 +1,70 @@ +"""Binary (octet-stream) request bodies are never unpickled unless explicitly asked.""" + +import io +import json +import pickle + +import pytest + +from py2http.constants import BINARY_CONTENT_TYPE +from py2http.decorators import ( + _get_inputs_from_request, + handle_binary_req, + unsafe_pickle_loads, +) + +_calls = [] + + +def _record(*args): + _calls.append(args) + return {} + + +class _RecordsWhenUnpickled: + """Unpickling this calls ``_record``: a harmless stand-in for side effects.""" + + def __reduce__(self): + return (_record, ("unpickled",)) + + +class _FakeRequest: + def __init__(self, body: bytes, *, content_type=BINARY_CONTENT_TYPE): + self.method = "POST" + self.content_type = content_type + self.body = io.BytesIO(body) + + +def add(x: int, y: int = 1): + return x + y + + +def test_binary_req_requires_explicit_loads(): + with pytest.raises(TypeError, match="explicit loads"): + handle_binary_req(add) + with pytest.raises(TypeError, match="callable"): + handle_binary_req(add, loads="pickle") + + +def test_binary_body_not_unpickled_by_default(): + _calls.clear() + payload = pickle.dumps(_RecordsWhenUnpickled()) + with pytest.raises(TypeError): + _get_inputs_from_request(_FakeRequest(payload), BINARY_CONTENT_TYPE) + assert _calls == [] + + +def test_binary_req_with_safe_loads(): + mapper = handle_binary_req(add, loads=json.loads) + assert mapper(_FakeRequest(json.dumps({"x": 2, "y": 3}).encode())) == 5 + + +def test_binary_req_rejects_non_mapping(): + mapper = handle_binary_req(add, loads=json.loads) + with pytest.raises(TypeError, match="not a mapping"): + mapper(_FakeRequest(b"[1, 2]")) + + +def test_binary_req_explicit_unsafe_opt_in_still_works(): + mapper = handle_binary_req(add, loads=unsafe_pickle_loads) + assert mapper(_FakeRequest(pickle.dumps({"x": 2}))) == 3 From afab7e7c7e453c2ef6a0d452bdcb560c9c5c72ac Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:12:11 +0000 Subject: [PATCH 2/2] review: warn on bare pickle.loads, end-to-end test, doctest flags Co-Authored-By: Claude Opus 5 --- py2http/decorators.py | 17 ++++++++-- py2http/tests/test_binary_req.py | 56 ++++++++++++++++++++++++++++---- 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/py2http/decorators.py b/py2http/decorators.py index ff7f149..4708bad 100644 --- a/py2http/decorators.py +++ b/py2http/decorators.py @@ -10,7 +10,8 @@ import inspect import json import pickle -from typing import Iterable, Callable, Union, Mapping +import warnings +from typing import Iterable, Callable, Union, Mapping, Optional from functools import lru_cache, partial, wraps, update_wrapper from json import JSONEncoder, dumps from aiohttp import web @@ -1040,7 +1041,7 @@ def unsafe_pickle_loads(data: bytes): return pickle.loads(data) -def handle_binary_req(func, *, loads: Callable[[bytes], Mapping] = None): +def handle_binary_req(func, *, loads: Optional[Callable[[bytes], Mapping]] = None): """Make an input mapper that decodes a binary (octet-stream) request body. ``loads`` turns the raw body bytes into the mapping of keyword arguments for @@ -1049,7 +1050,10 @@ def handle_binary_req(func, *, loads: Callable[[bytes], Mapping] = None): safe decoder of your own, or ``loads=unsafe_pickle_loads`` if (and only if) every client is trusted. - >>> handle_binary_req(lambda x: x) + Note that ``http2py`` clients encode binary request bodies with pickle, so + they only work against endpoints that opted into ``unsafe_pickle_loads``. + + >>> handle_binary_req(lambda x: x) # doctest: +ELLIPSIS Traceback (most recent call last): ... TypeError: handle_binary_req needs an explicit loads=... (bytes -> dict of inputs). ... @@ -1063,6 +1067,13 @@ def handle_binary_req(func, *, loads: Callable[[bytes], Mapping] = None): ) if not callable(loads): raise TypeError(f"loads must be callable, got {loads!r}") + if loads is pickle.loads: + warnings.warn( + "handle_binary_req(loads=pickle.loads) unpickles client data, which lets " + "clients run code on the server. Use loads=unsafe_pickle_loads to make " + "that choice explicit, and only if every client is trusted.", + stacklevel=2, + ) return _handle_req(func, BINARY_CONTENT_TYPE, binary_loads=loads) diff --git a/py2http/tests/test_binary_req.py b/py2http/tests/test_binary_req.py index ffd50f2..61c21d1 100644 --- a/py2http/tests/test_binary_req.py +++ b/py2http/tests/test_binary_req.py @@ -3,10 +3,12 @@ import io import json import pickle +from wsgiref.util import setup_testing_defaults import pytest -from py2http.constants import BINARY_CONTENT_TYPE +from py2http import mk_app +from py2http.constants import BINARY_CONTENT_TYPE, JSON_CONTENT_TYPE from py2http.decorators import ( _get_inputs_from_request, handle_binary_req, @@ -28,6 +30,13 @@ def __reduce__(self): return (_record, ("unpickled",)) +@pytest.fixture +def calls(): + _calls.clear() + yield _calls + _calls.clear() + + class _FakeRequest: def __init__(self, body: bytes, *, content_type=BINARY_CONTENT_TYPE): self.method = "POST" @@ -39,6 +48,22 @@ def add(x: int, y: int = 1): return x + y +def _post(app, path, body: bytes, content_type: str): + """Call a WSGI app directly; return (status, body).""" + environ = {} + setup_testing_defaults(environ) + environ.update( + REQUEST_METHOD="POST", + PATH_INFO=path, + CONTENT_TYPE=content_type, + CONTENT_LENGTH=str(len(body)), + ) + environ["wsgi.input"] = io.BytesIO(body) + statuses = [] + out = b"".join(app(environ, lambda status, headers, *a: statuses.append(status))) + return statuses[0], out + + def test_binary_req_requires_explicit_loads(): with pytest.raises(TypeError, match="explicit loads"): handle_binary_req(add) @@ -46,17 +71,24 @@ def test_binary_req_requires_explicit_loads(): handle_binary_req(add, loads="pickle") -def test_binary_body_not_unpickled_by_default(): - _calls.clear() +def test_bare_pickle_loads_warns(): + with pytest.warns(UserWarning, match="unsafe_pickle_loads"): + handle_binary_req(add, loads=pickle.loads) + + +def test_binary_body_not_unpickled_by_default(calls): payload = pickle.dumps(_RecordsWhenUnpickled()) - with pytest.raises(TypeError): + with pytest.raises(TypeError, match="No decoder"): _get_inputs_from_request(_FakeRequest(payload), BINARY_CONTENT_TYPE) - assert _calls == [] + assert calls == [] -def test_binary_req_with_safe_loads(): +def test_binary_req_with_safe_loads(calls): mapper = handle_binary_req(add, loads=json.loads) assert mapper(_FakeRequest(json.dumps({"x": 2, "y": 3}).encode())) == 5 + with pytest.raises(ValueError): + mapper(_FakeRequest(pickle.dumps(_RecordsWhenUnpickled()))) + assert calls == [] def test_binary_req_rejects_non_mapping(): @@ -68,3 +100,15 @@ def test_binary_req_rejects_non_mapping(): def test_binary_req_explicit_unsafe_opt_in_still_works(): mapper = handle_binary_req(add, loads=unsafe_pickle_loads) assert mapper(_FakeRequest(pickle.dumps({"x": 2}))) == 3 + + +def test_default_service_does_not_unpickle_octet_stream_bodies(calls): + app = mk_app([add]) + status, _ = _post( + app, "/add", pickle.dumps(_RecordsWhenUnpickled()), BINARY_CONTENT_TYPE + ) + assert not status.startswith("2") + assert calls == [] + # the normal JSON path still works + status, body = _post(app, "/add", json.dumps({"x": 2}).encode(), JSON_CONTENT_TYPE) + assert status.startswith("200") and json.loads(body) == 3