diff --git a/CHANGES b/CHANGES index 66bc5f17..61794d12 100644 --- a/CHANGES +++ b/CHANGES @@ -10,6 +10,12 @@ * Fixed `fragment_identifier_matcher` treating opaque fragments (those without ``=``, e.g. ``/users/5``) as always equal, so a required fragment matched a different one or none at all. See #806 +* Fixed `responses` mutating the live `PreparedRequest` object that `requests` + threads back to the caller (e.g. attached to a raised exception's + ``.request``, or the outer ``Response.request``) with internal-use + ``params``/``req_kwargs`` attributes. These attributes are now only + attached to the request object exposed via ``responses.calls[i].request``. + See #738 0.26.2 ------ diff --git a/README.rst b/README.rst index a5a8bb0e..2dd3ce41 100644 --- a/README.rst +++ b/README.rst @@ -368,7 +368,10 @@ deprecated argument. constructed_url = r"http://example.com/test?I+am=a+big+test&hello=world" assert resp.url == constructed_url assert resp.request.url == constructed_url - assert resp.request.params == params + # ``params`` is only available on ``responses.calls[i].request``, not on the + # actual request/response objects returned to the caller, so that mocked + # requests don't expose responses-internal attributes to production code. + assert responses.calls[0].request.params == params By default, matcher will validate that all parameters match strictly. To validate that only parameters specified in the matcher are present in original request diff --git a/responses/__init__.py b/responses/__init__.py index 41f5fde8..dca64eeb 100644 --- a/responses/__init__.py +++ b/responses/__init__.py @@ -1,3 +1,4 @@ +import copy import inspect import json as json_module import logging @@ -1104,6 +1105,16 @@ def _on_request( match, match_failed_reasons = self._find_match(request) resp_callback = self.response_callback + # `request` is the same live PreparedRequest object that `requests` threads + # back to the caller (e.g. via a raised exception's `.request`, or the + # outer `Response.request`), so it must not carry responses-internal + # attributes. Keep a copy with those attributes attached for use only in + # the internal call log (`responses.calls`), and strip them off the + # object that flows back to the caller. See GH #738. + call_request = copy.copy(request) + del request.params # type: ignore[attr-defined] + del request.req_kwargs # type: ignore[attr-defined] + if match is None: if any( [ @@ -1136,7 +1147,7 @@ def _on_request( response = ConnectionError(error_msg) response.request = request - self._calls.add(request, response) + self._calls.add(call_request, response) raise response if match.passthrough: @@ -1148,14 +1159,14 @@ def _on_request( request, match.get_response(request) ) except BaseException as response: - call = Call(request, response) + call = Call(call_request, response) self._calls.add_call(call) match.calls.add_call(call) raise if resp_callback: response = resp_callback(response) # type: ignore[misc] - call = Call(request, response) # type: ignore[misc] + call = Call(call_request, response) # type: ignore[misc] self._calls.add_call(call) match.calls.add_call(call) diff --git a/responses/tests/test_matchers.py b/responses/tests/test_matchers.py index 8eefce69..f89cfeaa 100644 --- a/responses/tests/test_matchers.py +++ b/responses/tests/test_matchers.py @@ -583,8 +583,12 @@ def run(): assert resp.url == constructed_url assert resp.request.url == constructed_url - resp_params = getattr(resp.request, "params") - assert resp_params == params + # `params` is a responses-internal attribute and is only exposed via + # `responses.calls[i].request`, not on the live request/response + # objects returned to the caller. See GH #738. + assert not hasattr(resp.request, "params") + call_params = getattr(responses.calls[0].request, "params") + assert call_params == params run() assert_reset() diff --git a/responses/tests/test_responses.py b/responses/tests/test_responses.py index b3740ed9..a6543b97 100644 --- a/responses/tests/test_responses.py +++ b/responses/tests/test_responses.py @@ -48,11 +48,19 @@ def assert_response( def assert_params(resp, expected): + # NOTE: `params`/`req_kwargs` are responses-internal attributes and are + # intentionally only exposed via `responses.calls[i].request`, not on the + # actual request/response objects returned to the caller. See GH #738. assert hasattr(resp, "request"), "Missing request" - assert hasattr( + assert not hasattr( resp.request, "params" - ), "Missing params on request that responses should add" - assert getattr(resp.request, "params") == expected, "Incorrect parameters" + ), "params leaked onto the live request object returned to the caller" + assert len(responses.calls) >= 1, "Missing calls" + call_request = responses.calls[-1].request + assert hasattr( + call_request, "params" + ), "Missing params on responses.calls[-1].request that responses should add" + assert getattr(call_request, "params") == expected, "Incorrect parameters" def test_response(): @@ -354,6 +362,60 @@ def run(): assert_reset() +def test_response_params_not_leaked_to_caller_request(): + """The live ``PreparedRequest`` object that `requests` threads back to the + caller (e.g. via a raised exception's ``.request``) must not carry + responses-internal ``params``/``req_kwargs`` attributes -- those exist + only on ``responses.calls[i].request`` as documented. See GH #738. + """ + + @responses.activate + def run(): + url = "http://example.com/test" + params = {"hello": "world"} + responses.get(url, status=403) + + with pytest.raises(HTTPError) as exc_info: + resp = requests.get(url, params=params) + resp.raise_for_status() + + caught_request = exc_info.value.request + assert not hasattr(caught_request, "params") + assert not hasattr(caught_request, "req_kwargs") + + # meanwhile, the documented convenience attributes are still available + # on the internal call log. + assert len(responses.calls) == 1 + assert responses.calls[0].request.params == params + assert isinstance(responses.calls[0].request.req_kwargs, dict) + + run() + assert_reset() + + +def test_connection_error_request_params_not_leaked(): + """Same guarantee as above, but for the ``ConnectionError`` raised when a + request doesn't match any registered mock. + """ + + @responses.activate + def run(): + responses.add(responses.GET, "http://example.com") + + with pytest.raises(ConnectionError) as exc_info: + requests.get("http://example.com/foo", params={"hello": "world"}) + + caught_request = exc_info.value.request + assert not hasattr(caught_request, "params") + assert not hasattr(caught_request, "req_kwargs") + + assert len(responses.calls) == 1 + assert responses.calls[0].request.params == {"hello": "world"} + + run() + assert_reset() + + def test_match_querystring(): @responses.activate def run():