diff --git a/src/pact/_server.py b/src/pact/_server.py index e05c9dfe4..f1f1d48de 100644 --- a/src/pact/_server.py +++ b/src/pact/_server.py @@ -282,23 +282,58 @@ def do_POST(self) -> None: self.send_error(400, "Bad Request") return - self.send_response(200, "OK") - - message = self.server.handler(description, data) + # The handler must be called before the response line is sent. Any + # exception it raises has to be reported as a 500, and not leave the + # client with a truncated 200 response. + try: + message = self.server.handler(description, data) + except Exception as e: + logger.exception("Message handler for %s raised an exception.", description) + self.send_error( + 500, + "Message handler failed", + f"{type(e).__name__}: {e}", + ) + return - metadata = message.get("metadata") or {} - if content_type := message.get("content_type"): - self.send_header("Content-Type", content_type) - if "contentType" not in metadata: + # The response is serialised in full before the response line is sent, + # for the same reason: a malformed message must be reported as a 500, + # and not leave the client with a truncated 200 response. + try: + contents = message.get("contents") or b"" + if not isinstance(contents, (bytes, bytearray)): + msg = ( + f"Message handler for {description!r} returned contents of " + f"type {type(contents).__name__}, expected bytes." + ) + raise TypeError(msg) # noqa: TRY301 + + metadata = message.get("metadata") or {} + content_type = message.get("content_type") + if content_type and "contentType" not in metadata: metadata["contentType"] = content_type - - if metadata: - self.send_header( - "Pact-Message-Metadata", - base64.b64encode(json.dumps(metadata).encode()).decode(), + encoded_metadata = ( + base64.b64encode(json.dumps(metadata).encode()).decode() + if metadata + else None + ) + except Exception as e: + logger.exception( + "Message from handler for %s could not be serialised.", + description, ) + self.send_error( + 500, + "Message handler failed", + f"{type(e).__name__}: {e}", + ) + return - contents = message.get("contents", b"") + self.send_response(200, "OK") + if content_type: + self.send_header("Content-Type", content_type) + if encoded_metadata: + self.send_header("Pact-Message-Metadata", encoded_metadata) self.send_header("Content-Length", str(len(contents))) self.end_headers() self.wfile.write(contents) @@ -491,7 +526,17 @@ def do_POST(self) -> None: self.send_error(400, "Bad Request") return - self.server.handler(state, action, params) + try: + self.server.handler(state, action, params) + except Exception as e: + logger.exception("State handler for %s raised an exception.", state) + self.send_error( + 500, + "State handler failed", + f"{type(e).__name__}: {e}", + ) + return + self.send_response(200, "OK") self.end_headers() diff --git a/src/pact/verifier.py b/src/pact/verifier.py index c706c71c3..3f3bbc672 100644 --- a/src/pact/verifier.py +++ b/src/pact/verifier.py @@ -111,6 +111,65 @@ logger = logging.getLogger(__name__) +def _missing_contents_msg(name: str) -> str: + """ + Error message for a message handler dictionary without a `contents` key. + + Args: + name: + The name of the message whose handler is invalid. + """ + return ( + f"Message handler for {name!r} is missing the 'contents' key. Dictionary " + "values must be Message envelopes, such as {'contents': b'...', " + "'content_type': 'application/json'}, and not the raw payload." + ) + + +def _invalid_value_msg(name: str, value: object) -> str: + """ + Error message for a message handler value of an unsupported type. + + Args: + name: + The name of the message whose handler is invalid. + + value: + The offending value. + """ + return ( + f"Invalid message handler value for {name!r}: expected a callable, bytes, " + f"or a Message dictionary, got {type(value).__name__}." + ) + + +def _validate_message_handlers( + handler: dict[str, Callable[..., Message] | Message | bytes], +) -> None: + """ + Check that every value of a message handler dictionary is usable. + + The handler is only called during verification, at which point a failure is + reported by the underlying FFI as a failed interaction and its cause is + easily missed. + + Args: + handler: + The dictionary mapping message names to handler values. + + Raises: + TypeError: + If any value is neither a callable, bytes, nor a Message dictionary. + """ + for name, value in handler.items(): + if callable(value) or isinstance(value, bytes): + continue + if not isinstance(value, dict): + raise TypeError(_invalid_value_msg(name, value)) + if "contents" not in value: + raise TypeError(_missing_contents_msg(name)) + + class _ProviderTransport(TypedDict): """ Provider transport information. @@ -385,6 +444,12 @@ def message_handler( Raises: TypeError: If the handler or its values are invalid. + + KeyError: + If a message is requested which is not present in the + dictionary. As the handler is called during verification, this + is raised within the message relay server and surfaces as a + failed interaction. """ logger.debug( "Setting message handler for verifier", @@ -414,12 +479,19 @@ def _handler( return self if isinstance(handler, dict): + _validate_message_handlers(handler) def _handler( name: str, metadata: dict[str, Any] | None, ) -> Message: logger.info("Internal message produced called.") + if name not in handler: + msg = ( + f"No message handler for {name!r}. " + f"Known messages: {', '.join(sorted(handler))}" + ) + raise KeyError(msg) val = handler[name] if callable(val): @@ -430,14 +502,15 @@ def _handler( if isinstance(val, bytes): return Message(contents=val, metadata=None, content_type=None) if isinstance(val, dict): + if "contents" not in val: + raise TypeError(_missing_contents_msg(name)) return Message( contents=val["contents"], metadata=val.get("metadata"), content_type=val.get("content_type"), ) - msg = "Invalid message handler value" - raise TypeError(msg) + raise TypeError(_invalid_value_msg(name, val)) self._message_producer = MessageProducer(_handler) self.add_transport( diff --git a/tests/test_server.py b/tests/test_server.py index 3526ac1d5..2764569f6 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -5,6 +5,7 @@ from __future__ import annotations import json +from typing import Any from unittest.mock import MagicMock import aiohttp @@ -73,6 +74,71 @@ async def test_message_post_http() -> None: assert handler.call_args.args == ("A simple message", {}) +@pytest.mark.asyncio +async def test_message_post_handler_raises() -> None: + """ + A failing handler must produce a 500, not a truncated 200. + + The response line used to be sent before the handler was called, so any + exception left the client with a half-written response and the Pact core + reported it as `error sending request for url`. See #1665. + """ + handler = MagicMock(side_effect=RuntimeError("handler is broken")) + server = MessageProducer(handler) + + with server: + async with aiohttp.ClientSession() as session: + async with session.post( + server.url, + data=json.dumps({"description": "A simple message"}), + ) as response: + assert response.status == 500 + assert "handler is broken" in await response.text() + + handler.assert_called_once() + + +@pytest.mark.parametrize( + ("message", "match"), + [ + pytest.param( + {"contents": "not bytes", "metadata": None, "content_type": None}, + "expected bytes", + id="str_contents", + ), + pytest.param( + {"contents": b"", "metadata": {"key": object()}, "content_type": None}, + "TypeError", + id="unserialisable_metadata", + ), + ], +) +@pytest.mark.asyncio +async def test_message_post_unserialisable_message( + message: dict[str, Any], + match: str, +) -> None: + """ + A message which cannot be serialised must produce a 500. + + The handler itself succeeds here; it is the response which cannot be built, + and that must not leave the client with a truncated 200 either. + """ + handler = MagicMock(return_value=message) + server = MessageProducer(handler) + + with server: + async with aiohttp.ClientSession() as session: + async with session.post( + server.url, + data=json.dumps({"description": "A simple message"}), + ) as response: + assert response.status == 500 + assert match in await response.text() + + handler.assert_called_once() + + def test_callback_default_init() -> None: handler = MagicMock() server = StateCallback(handler) @@ -132,3 +198,21 @@ async def test_callback_post() -> None: "setup", {"id": 123}, ) + + +@pytest.mark.asyncio +async def test_callback_post_handler_raises() -> None: + """A failing state handler must produce a 500.""" + handler = MagicMock(side_effect=RuntimeError("state setup is broken")) + server = StateCallback(handler) + + with server: + async with aiohttp.ClientSession() as session: + async with session.post( + server.url, + json={"state": "user exists", "action": "setup", "params": {}}, + ) as response: + assert response.status == 500 + assert "state setup is broken" in await response.text() + + handler.assert_called_once() diff --git a/tests/test_verifier.py b/tests/test_verifier.py index b249d2958..196defbe4 100644 --- a/tests/test_verifier.py +++ b/tests/test_verifier.py @@ -10,6 +10,7 @@ import json import re +from contextlib import nullcontext from pathlib import Path from typing import Any from unittest.mock import patch @@ -238,6 +239,47 @@ def test_verify_message_only(verifier: Verifier) -> None: mock_add_transport.assert_not_called() +@pytest.mark.parametrize( + ("handler", "match"), + [ + pytest.param( + {"a-message": {"field": "value"}}, + r"missing the 'contents' key", + id="dict_without_contents", + ), + pytest.param( + {"a-message": 42}, + r"expected a callable, bytes, or a Message dictionary, got int", + id="unsupported_value_type", + ), + ], +) +def test_message_handler_invalid_dict_value( + verifier: Verifier, + handler: dict[str, Any], + match: str, +) -> None: + """ + Invalid handler values must be rejected when the handler is set. + + Deferring the error to verification time hides the cause behind a failed + interaction. See #1665. + """ + with pytest.raises(TypeError, match=match): + verifier.message_handler(handler) + + +def test_message_handler_unknown_message(verifier: Verifier) -> None: + """A message with no handler must name the messages which do have one.""" + verifier.message_handler({"a-message": b"", "b-message": b""}) + producer = verifier._message_producer # noqa: SLF001 + assert not isinstance(producer, nullcontext) + handler = producer._handler # noqa: SLF001 + + with pytest.raises(KeyError, match=r"Known messages: a-message, b-message"): + handler("c-message", None) + + def test_logs(verifier: Verifier) -> None: logs = verifier.logs assert logs == ""