fix: never unpickle binary request bodies unless explicitly asked

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")

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