diff --git a/py2http/decorators.py b/py2http/decorators.py index aefb8c6..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 @@ -1007,7 +1008,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 +1018,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 +1028,53 @@ 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: 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 + ``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. + + 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). ... + """ + 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}") + 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) def handle_form_req(func): @@ -1170,7 +1216,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 +1225,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..61c21d1 --- /dev/null +++ b/py2http/tests/test_binary_req.py @@ -0,0 +1,114 @@ +"""Binary (octet-stream) request bodies are never unpickled unless explicitly asked.""" + +import io +import json +import pickle +from wsgiref.util import setup_testing_defaults + +import pytest + +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, + 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",)) + + +@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" + self.content_type = content_type + self.body = io.BytesIO(body) + + +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) + with pytest.raises(TypeError, match="callable"): + handle_binary_req(add, loads="pickle") + + +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, match="No decoder"): + _get_inputs_from_request(_FakeRequest(payload), BINARY_CONTENT_TYPE) + assert calls == [] + + +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(): + 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 + + +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