diff --git a/CHANGELOG.md b/CHANGELOG.md index c101390a..4a441e85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ * (Major): The API token is passed as `api_key=""`. An `Authorization` header in `HttpOptions.headers` raises `ValueError`. * (Major): The deprecated `environment` setting has been removed. The `environment` field of the evaluation context and the impact metrics `environment` label come from the API token, or are `"default"` when the token doesn't name one. * (Major): The `UnleashClient` and `AsyncUnleashClient` constructors raise `ValueError` if `url` is not an absolute http(s) URL with a host, for every client. +* (Major): The constructors raise `ValueError` if `api_key`, `app_name` or `IdentityOptions.instance_id` contains a line break, such as an API token read from a file with a trailing newline, and `TypeError` if one isn't a str. These values are sent as headers. +* (Major): `initialize_client()` no longer raises when registration fails. A failed registration is logged and the client keeps running, the same way for both clients. * (Major): Upgraded to yggdrasil-engine 3, new engine built with PyO3 for better performance. * (Minor): New `FETCH_COMPLETED` event, emitted after every feature fetch with how it ended: `CHANGED`, `NOT_CHANGED`, `UNAUTHORIZED`, `RATE_LIMITED`, `UNAVAILABLE` or `INVALID_RESPONSE`. * (Minor): `feature_definitions()` reports whether each feature is enabled in the environment. diff --git a/UnleashClient/_async_unleash_api_client.py b/UnleashClient/_async_unleash_api_client.py index 2b34961d..d7127fa6 100644 --- a/UnleashClient/_async_unleash_api_client.py +++ b/UnleashClient/_async_unleash_api_client.py @@ -36,8 +36,6 @@ RETRYABLE_ERRORS = (aiohttp.ClientConnectionError, asyncio.TimeoutError) -FATAL_URL_ERRORS = (aiohttp.InvalidURL, aiohttp.NonHttpUrlClientError) - class _AsyncUnleashApiClient: """The asyncio twin of :class:`UnleashClient._unleash_api_client._UnleashApiClient`.""" @@ -187,9 +185,9 @@ async def register(self, payload: Dict[str, Any]) -> bool: """ Register this client with the server. - Returns True on 200 or 202, False on any other status and on a general - ``ClientError``. The two errors aiohttp uses for a url it cannot make a - request against at all are re-raised rather than swallowed. + Returns True on 200 or 202, False on any other status and when the + request fails: any exception from the transport is logged and reported + as False. :param payload: as built by :func:`UnleashClient._payloads._build_register_payload`. @@ -198,48 +196,41 @@ async def register(self, payload: Dict[str, Any]) -> bool: self._raise_if_closed() config = self._config + LOGGER.info("Registering unleash client with unleash @ %s", config.url) + LOGGER.info("Registration request information: %s", payload) + + request = Request( + method="POST", + url=_normalized_url(config.url, REGISTER_URL), + headers=self._headers.base(), + body=json.dumps(payload).encode("utf-8"), + ) try: - LOGGER.info("Registering unleash client with unleash @ %s", config.url) - LOGGER.info("Registration request information: %s", payload) - - resp = await self._transport.execute( - Request( - method="POST", - url=_normalized_url(config.url, REGISTER_URL), - headers=self._headers.base(), - body=json.dumps(payload).encode("utf-8"), - ) - ) - - if resp.status not in {200, 202}: - _log_resp_info(resp) - LOGGER.warning( - "Unleash Client registration failed due to unexpected HTTP status code: %s", - resp.status, - ) - return False - - LOGGER.info("Unleash Client successfully registered!") - - return True - except FATAL_URL_ERRORS as exc: - LOGGER.exception( - "Unleash Client registration failed fatally due to exception: %s", exc - ) - raise exc - except aiohttp.ClientError as exc: + resp = await self._transport.execute(request) + except Exception as exc: # pylint: disable=broad-except LOGGER.exception( "Unleash Client registration failed due to exception: %s", exc ) + return False + + if resp.status not in {200, 202}: + _log_resp_info(resp) + LOGGER.warning( + "Unleash Client registration failed due to unexpected HTTP status code: %s", + resp.status, + ) + return False - return False + LOGGER.info("Unleash Client successfully registered!") + + return True async def send_metrics(self, payload: Dict[str, Any]) -> bool: """ Send one metrics bucket. - Returns True only on 202. Every other status, a ``ClientError`` and a - timeout all return False. + Returns True only on 202. Every other status and any exception from the + transport return False. :param payload: the metrics request body. :raises AlreadyClosedError: if the API client has been closed. @@ -248,38 +239,37 @@ async def send_metrics(self, payload: Dict[str, Any]) -> bool: self._raise_if_closed() config = self._config + LOGGER.info("Sending messages to with unleash @ %s", config.url) + LOGGER.info("unleash metrics information: %s", payload) + + request = Request( + method="POST", + url=_normalized_url(config.url, METRICS_URL), + headers=self._headers.metrics(), + body=json.dumps(payload).encode("utf-8"), + ) try: - LOGGER.info("Sending messages to with unleash @ %s", config.url) - LOGGER.info("unleash metrics information: %s", payload) - - resp = await self._transport.execute( - Request( - method="POST", - url=_normalized_url(config.url, METRICS_URL), - headers=self._headers.metrics(), - body=json.dumps(payload).encode("utf-8"), - ) + resp = await self._transport.execute(request) + except asyncio.CancelledError: + LOGGER.warning("Unleash Client metrics submission was cancelled.") + raise + except asyncio.TimeoutError: + LOGGER.warning("Unleash Client metrics submission timed out.") + return False + except Exception as exc: # pylint: disable=broad-except + LOGGER.warning( + "Unleash Client metrics submission failed due to exception: %s", exc ) + return False - if resp.status != 202: - _log_resp_info(resp) - LOGGER.warning( - "Unleash Client metrics submission due to unexpected HTTP status code: %s", - resp.status, - ) - return False - - LOGGER.info("Unleash Client metrics successfully sent!") - - return True - except aiohttp.ClientError as exc: + if resp.status != 202: + _log_resp_info(resp) LOGGER.warning( - "Unleash Client metrics submission failed due to exception: %s", exc + "Unleash Client metrics submission due to unexpected HTTP status code: %s", + resp.status, ) - except asyncio.TimeoutError: - LOGGER.warning("Unleash Client metrics submission timed out.") - except asyncio.CancelledError: - LOGGER.warning("Unleash Client metrics submission was cancelled.") - raise + return False + + LOGGER.info("Unleash Client metrics successfully sent!") - return False + return True diff --git a/UnleashClient/_unleash_api_client.py b/UnleashClient/_unleash_api_client.py index 3bf99eb0..81a5bdbc 100644 --- a/UnleashClient/_unleash_api_client.py +++ b/UnleashClient/_unleash_api_client.py @@ -2,9 +2,6 @@ import uuid from typing import Any, Dict, NamedTuple, Optional, Tuple -import requests -from requests.exceptions import InvalidHeader, InvalidSchema, InvalidURL, MissingSchema - from UnleashClient._event_dispatcher import _EventDispatcher from UnleashClient._headers import _HeaderFactory from UnleashClient._transport import Request, RequestsTransport, Response, Transport @@ -206,87 +203,79 @@ def register(self, payload: Dict[str, Any]) -> bool: """ Register this client with the Unleash server. - Malformed-URL errors (``MissingSchema``, ``InvalidSchema``, - ``InvalidHeader``, ``InvalidURL``) propagate, so ``initialize_client()`` - fails loudly instead of starting a client that can never reach the - server. + Any exception from the transport is a failed request: it is logged and + False is returned. :param payload: Registration request body. :return: True on 200 or 202; False on any other status and on a request error. """ config = self._config + LOGGER.info("Registering unleash client with unleash @ %s", config.url) + LOGGER.info("Registration request information: %s", payload) + + request = Request( + method="POST", + url=_normalized_url(config.url, REGISTER_URL), + headers=self._headers.base(), + body=json.dumps(payload).encode("utf-8"), + ) try: - LOGGER.info("Registering unleash client with unleash @ %s", config.url) - LOGGER.info("Registration request information: %s", payload) - - resp = self._transport.execute( - Request( - method="POST", - url=_normalized_url(config.url, REGISTER_URL), - headers=self._headers.base(), - body=json.dumps(payload).encode("utf-8"), - ) - ) - - if resp.status not in {200, 202}: - _log_resp_info(resp) - LOGGER.warning( - "Unleash Client registration failed due to unexpected HTTP status code: %s", - resp.status, - ) - return False - - LOGGER.info("Unleash Client successfully registered!") - - return True - except (MissingSchema, InvalidSchema, InvalidHeader, InvalidURL) as exc: - LOGGER.exception( - "Unleash Client registration failed fatally due to exception: %s", exc - ) - raise exc - except requests.RequestException as exc: + resp = self._transport.execute(request) + except Exception as exc: # pylint: disable=broad-except LOGGER.exception( "Unleash Client registration failed due to exception: %s", exc ) + return False + + if resp.status not in {200, 202}: + _log_resp_info(resp) + LOGGER.warning( + "Unleash Client registration failed due to unexpected HTTP status code: %s", + resp.status, + ) + return False + + LOGGER.info("Unleash Client successfully registered!") - return False + return True def send_metrics(self, payload: Dict[str, Any]) -> bool: """ Send one metrics bucket to the Unleash server. + Any exception from the transport is a failed request: it is logged and + False is returned. + :param payload: Metrics request body. :return: True on 202; False on any other status and on a request error. """ config = self._config + LOGGER.info("Sending messages to with unleash @ %s", config.url) + LOGGER.info("unleash metrics information: %s", payload) + + request = Request( + method="POST", + url=_normalized_url(config.url, METRICS_URL), + headers=self._headers.metrics(), + body=json.dumps(payload).encode("utf-8"), + ) try: - LOGGER.info("Sending messages to with unleash @ %s", config.url) - LOGGER.info("unleash metrics information: %s", payload) - - resp = self._transport.execute( - Request( - method="POST", - url=_normalized_url(config.url, METRICS_URL), - headers=self._headers.metrics(), - body=json.dumps(payload).encode("utf-8"), - ) + resp = self._transport.execute(request) + except Exception as exc: # pylint: disable=broad-except + LOGGER.warning( + "Unleash Client metrics submission failed due to exception: %s", exc ) + return False - if resp.status != 202: - _log_resp_info(resp) - LOGGER.warning( - "Unleash Client metrics submission due to unexpected HTTP status code: %s", - resp.status, - ) - return False - - LOGGER.info("Unleash Client metrics successfully sent!") - - return True - except requests.RequestException as exc: + if resp.status != 202: + _log_resp_info(resp) LOGGER.warning( - "Unleash Client metrics submission failed due to exception: %s", exc + "Unleash Client metrics submission due to unexpected HTTP status code: %s", + resp.status, ) + return False + + LOGGER.info("Unleash Client metrics successfully sent!") - return False + return True diff --git a/UnleashClient/clients/async_unleash_client.py b/UnleashClient/clients/async_unleash_client.py index fe7e1917..a3e3073f 100644 --- a/UnleashClient/clients/async_unleash_client.py +++ b/UnleashClient/clients/async_unleash_client.py @@ -37,7 +37,7 @@ from UnleashClient.constants import ETAG, METRIC_LAST_SENT_TIME from UnleashClient.events import BaseEvent from UnleashClient.impact_metrics import ImpactMetrics -from UnleashClient.utils import LOGGER, InstanceAllowType, _validate_url +from UnleashClient.utils import LOGGER, InstanceAllowType class AsyncUnleashClient: @@ -308,9 +308,6 @@ async def initialize_client(self, fetch_toggles: bool = True) -> None: :param fetch_toggles: Accepted for parity with :class:`UnleashClient`. It has no effect: the client always polls. :raises ValueError: If a custom strategy is invalid. - :raises aiohttp.InvalidURL: If registration is enabled and aiohttp - rejects a URL that passed the constructor's - check. """ if self._closed or self._starting or self._run_state > _RunState.UNINITIALIZED: warnings.warn( @@ -320,8 +317,6 @@ async def initialize_client(self, fetch_toggles: bool = True) -> None: self._starting = True try: - _validate_url(self._config.url) - self._cache.mset( {METRIC_LAST_SENT_TIME: datetime.now(timezone.utc), ETAG: ""} ) diff --git a/UnleashClient/clients/unleash_client.py b/UnleashClient/clients/unleash_client.py index 51aaeb62..b62af5cb 100644 --- a/UnleashClient/clients/unleash_client.py +++ b/UnleashClient/clients/unleash_client.py @@ -46,11 +46,7 @@ UnleashReadyEvent, ) from UnleashClient.impact_metrics import ImpactMetrics -from UnleashClient.utils import ( - LOGGER, - InstanceAllowType, - _validate_url, -) +from UnleashClient.utils import LOGGER, InstanceAllowType INSTANCES = _get_instance_registry() @@ -290,8 +286,6 @@ def initialize_client(self, fetch_toggles: bool = True) -> None: try: start_scheduler = False - _validate_url(self._config.url) - if self._custom_strategies: self._engine.register_custom_strategies(self._custom_strategies) diff --git a/UnleashClient/config.py b/UnleashClient/config.py index e62f0817..e61393a2 100644 --- a/UnleashClient/config.py +++ b/UnleashClient/config.py @@ -8,6 +8,7 @@ from UnleashClient.constants import REQUEST_RETRIES, REQUEST_TIMEOUT from UnleashClient.utils import ( LOGGER, + _validate_header_value, _validate_url, extract_environment_from_api_key, ) @@ -128,7 +129,12 @@ class UnleashConfig: never talks to the server: deciding which settings make requests would need updating with every new server call. - :raises ValueError: if ``url`` is not an absolute http(s) URL with a host. + ``api_key``, ``app_name``, ``identity.instance_id`` and ``connection_id`` + are sent as headers, so they must be str without a line break. + + :raises ValueError: if ``url`` is not an absolute http(s) URL with a host, + or a value sent as a header contains a line break. + :raises TypeError: if a value sent as a header is not a str. ``api_key`` is optional because a client that only loads from a backup never talks to the server. """ @@ -153,6 +159,14 @@ def __post_init__(self) -> None: _validate_url(self.url) if self.identity is None: object.__setattr__(self, "identity", IdentityOptions()) + for name, value in ( + ("Authorization", self.api_key), + ("unleash-appname", self.app_name), + ("unleash-instanceid", self.identity.instance_id), + ("unleash-connection-id", self.connection_id), + ): + if value is not None: + _validate_header_value(name, value) if self.http is None: object.__setattr__(self, "http", HttpOptions()) if self.mode is None: diff --git a/UnleashClient/utils.py b/UnleashClient/utils.py index ed33c099..83a44698 100644 --- a/UnleashClient/utils.py +++ b/UnleashClient/utils.py @@ -62,3 +62,20 @@ def _validate_url(url: str) -> None: ) if not parts.hostname: raise ValueError(f"Unleash URL has no host: {url!r}") + + +def _validate_header_value(name: str, value: str) -> None: + """ + Checks a value the client sends as the ``name`` header. + + :raises TypeError: if ``value`` is not a str. + :raises ValueError: if ``value`` contains a line break. + """ + if not isinstance(value, str): + raise TypeError(f"The {name} header must be a str.") + + if "\r" in value or "\n" in value: + raise ValueError( + f"The {name} header contains a line break. If the value was read " + "from a file or an environment variable, strip it first." + ) diff --git a/tests/unit_tests/test_async_unleash_api_client.py b/tests/unit_tests/test_async_unleash_api_client.py index 5d5be8bc..3a56c09f 100644 --- a/tests/unit_tests/test_async_unleash_api_client.py +++ b/tests/unit_tests/test_async_unleash_api_client.py @@ -23,6 +23,7 @@ REQUEST_TIMEOUT, URL, ) +from UnleashClient._aiohttp_transport import AiohttpTransport from UnleashClient._async_unleash_api_client import _AsyncUnleashApiClient from UnleashClient._headers import _HeaderFactory from UnleashClient._transport import Request, Response @@ -272,7 +273,7 @@ async def test_register_sends_the_payload_it_is_given(server, api_client): def skip_url_validation(monkeypatch): """ UnleashConfig rejects these urls on construction, so register() only sees - one that slipped past that check. Skip it to reach the re-raise. + one that slipped past that check. Skip it to reach aiohttp's own error. """ monkeypatch.setattr("UnleashClient.config._validate_url", lambda url: None) @@ -282,24 +283,14 @@ def skip_url_validation(monkeypatch): @mark.parametrize( "url", ("thisisnotavalidurl", "localhost:4242/api"), ids=("no-scheme", "bad-scheme") ) -async def test_register_reraises_an_unusable_url(server, build_api_client, url): - api_client = build_api_client(url=url) - - with pytest.raises(aiohttp.ClientError): - await api_client.register({}) +async def test_register_reports_a_url_aiohttp_rejects_as_a_failure( + server, build_api_client, url +): + assert await build_api_client(url=url).register({}) is False assert server.requests == [] -@mark.asyncio -@mark.usefixtures("skip_url_validation") -async def test_register_reraises_a_missing_scheme_as_a_value_error(build_api_client): - api_client = build_api_client(url="thisisnotavalidurl") - - with pytest.raises(ValueError): - await api_client.register({}) - - @mark.asyncio @mark.parametrize( "reply,expected", @@ -509,13 +500,15 @@ async def aclose(self) -> None: self.closed = True -def api_client_over(transport: RecordingTransport) -> _AsyncUnleashApiClient: +def api_client_over(transport, events=None) -> _AsyncUnleashApiClient: config = UnleashConfig( url=URL, app_name=APP_NAME, identity=IdentityOptions(instance_id=INSTANCE_ID), ) - return _AsyncUnleashApiClient(config, _HeaderFactory(config), transport=transport) + return _AsyncUnleashApiClient( + config, _HeaderFactory(config), events, transport=transport + ) @mark.asyncio @@ -578,3 +571,68 @@ async def test_aclose_closes_the_default_transport(server, api_client): await api_client.aclose() assert session.closed + + +class RaisingTransport: + """Raises the same exception for every request.""" + + def __init__(self, exc: Exception) -> None: + self.exc = exc + + async def execute(self, request: Request) -> Response: + raise self.exc + + +class CustomTransportError(Exception): + pass + + +@mark.asyncio +@mark.parametrize( + "exc", + (CustomTransportError("boom"), OSError("refused"), ValueError("bad")), + ids=("foreign", "os-error", "value-error"), +) +async def test_any_transport_exception_is_a_failed_request(exc): + events = RecordingEvents() + api_client = api_client_over(RaisingTransport(exc), events) + + result = await api_client.fetch_features(options=PollingOptions()) + + assert result == (None, "", False) + assert fetch_outcomes(events) == [FetchStatus.UNAVAILABLE] + assert await api_client.register({}) is False + assert await api_client.send_metrics(MOCK_METRICS_REQUEST) is False + + +@mark.asyncio +async def test_a_transport_closed_by_its_creator_gives_failed_requests(): + transport = AiohttpTransport() + await transport.aclose() + events = RecordingEvents() + api_client = api_client_over(transport, events) + + result = await api_client.fetch_features(options=PollingOptions()) + + assert result == (None, "", False) + assert fetch_outcomes(events) == [FetchStatus.UNAVAILABLE] + assert await api_client.register({}) is False + assert await api_client.send_metrics(MOCK_METRICS_REQUEST) is False + + +@mark.asyncio +@mark.parametrize( + "call", + ( + param(lambda api_client: api_client.register, id="register"), + param(lambda api_client: api_client.send_metrics, id="send_metrics"), + ), +) +async def test_a_payload_that_cannot_be_serialised_still_raises(call): + # Only the transport's exceptions are failed requests: this is an SDK bug. + transport = RecordingTransport(Response(status=202, headers={}, body=b"")) + + with pytest.raises(TypeError): + await call(api_client_over(transport))({"not-json": object()}) + + assert transport.requests == [] diff --git a/tests/unit_tests/test_config.py b/tests/unit_tests/test_config.py index 3357eb99..3f4055c9 100644 --- a/tests/unit_tests/test_config.py +++ b/tests/unit_tests/test_config.py @@ -179,6 +179,34 @@ def test_an_authorization_header_is_rejected(name): HttpOptions(headers={name: "project:environment.hash"}) +@pytest.mark.parametrize( + "kwargs", + [ + {"api_key": "project:environment.hash\n"}, + {"app_name": "my\napp"}, + {"identity": IdentityOptions(instance_id="instance\r1")}, + {"connection_id": "id\n"}, + ], + ids=["api_key", "app_name", "instance_id", "connection_id"], +) +def test_a_value_sent_as_a_header_with_a_line_break_is_rejected(kwargs): + config = {"url": URL, "app_name": APP_NAME, **kwargs} + + with pytest.raises(ValueError, match="line break"): + UnleashConfig(**config) + + +def test_no_api_key_is_fine(): + assert UnleashConfig(URL, APP_NAME).api_key is None + + +def test_a_rejected_api_key_is_kept_out_of_the_message(): + with pytest.raises(ValueError) as error: + UnleashConfig(URL, APP_NAME, api_key="project:environment.secret\n") + + assert "secret" not in str(error.value) + + def test_an_authorization_header_cannot_be_added_later(): http = HttpOptions(headers={"X-Custom": "header"}) diff --git a/tests/unit_tests/test_unleash_api_client.py b/tests/unit_tests/test_unleash_api_client.py index 3375b7e5..4e80353e 100644 --- a/tests/unit_tests/test_unleash_api_client.py +++ b/tests/unit_tests/test_unleash_api_client.py @@ -4,7 +4,6 @@ import responses from pytest import mark, param from requests import ConnectionError -from requests.exceptions import InvalidSchema, MissingSchema from tests.utilities.mocks.mock_features import ( MOCK_FEATURE_RESPONSE, @@ -238,39 +237,28 @@ def test_register_sends_the_payload_it_is_given(api_client): def skip_url_validation(monkeypatch): """ UnleashConfig rejects these urls on construction, so register() only sees - one that slipped past that check. Skip it to reach the re-raise. + one that slipped past that check. Skip it to reach requests' own error. """ monkeypatch.setattr("UnleashClient.config._validate_url", lambda url: None) @responses.activate @pytest.mark.usefixtures("skip_url_validation") -def test_register_reraises_a_missing_schema(build_api_client): - api_client = build_api_client(url="thisisnotavalidurl") - - # These four are re-raised rather than reported as False, so a url - # requests rejects still fails initialize_client() loudly. - with pytest.raises(MissingSchema): - api_client.register({}) - - -@responses.activate -@pytest.mark.usefixtures("skip_url_validation") -def test_register_reraises_an_invalid_schema(build_api_client): - api_client = build_api_client(url="localhost:4242/api") - - with pytest.raises(InvalidSchema): - api_client.register({}) +@mark.parametrize( + "url", ("thisisnotavalidurl", "localhost:4242/api"), ids=("no-scheme", "bad-scheme") +) +def test_register_reports_a_url_requests_rejects_as_a_failure(build_api_client, url): + assert build_api_client(url=url).register({}) is False @responses.activate -def test_register_lets_a_bad_custom_option_escape(build_api_client): +def test_register_reports_a_bad_custom_option_as_a_failure(build_api_client): responses.add(responses.POST, FULL_REGISTER_URL, json={}, status=202) - # A TypeError is not a RequestException, so unlike fetch_features this - # propagates and fails initialize_client(). - with pytest.raises(TypeError): - build_api_client(http=HttpOptions(options={"timeout": 5})).register({}) + api_client = build_api_client(http=HttpOptions(options={"timeout": 5})) + + assert api_client.register({}) is False + assert len(responses.calls) == 0 # send_metrics @@ -305,11 +293,13 @@ def test_send_metrics(api_client, payload, status, expected): @responses.activate -def test_send_metrics_lets_a_bad_custom_option_escape(build_api_client): +def test_send_metrics_reports_a_bad_custom_option_as_a_failure(build_api_client): responses.add(responses.POST, FULL_METRICS_URL, json={}, status=202) - with pytest.raises(TypeError): - build_api_client(http=HttpOptions(options={"timeout": 5})).send_metrics({}) + api_client = build_api_client(http=HttpOptions(options={"timeout": 5})) + + assert api_client.send_metrics({}) is False + assert len(responses.calls) == 0 @responses.activate @@ -479,3 +469,55 @@ def test_send_metrics_posts_its_payload_through_the_transport(): assert request.method == "POST" assert request.url == FULL_METRICS_URL assert json.loads(request.body) == {"appName": APP_NAME} + + +class RaisingTransport: + """Raises the same exception for every request.""" + + def __init__(self, exc: Exception) -> None: + self.exc = exc + + def execute(self, request: Request) -> Response: + raise self.exc + + +def api_client_over(transport, events=None) -> _UnleashApiClient: + config = UnleashConfig(URL, APP_NAME) + return _UnleashApiClient( + config, _HeaderFactory(config), events, transport=transport + ) + + +class CustomTransportError(Exception): + pass + + +@mark.parametrize( + "exc", + (CustomTransportError("boom"), OSError("refused"), ValueError("bad")), + ids=("foreign", "os-error", "value-error"), +) +def test_any_transport_exception_is_a_failed_request(exc): + events = RecordingEvents() + api_client = api_client_over(RaisingTransport(exc), events) + + assert api_client.fetch_features(options=PollingOptions()) == (None, "", False) + assert fetch_outcomes(events) == [FetchStatus.UNAVAILABLE] + assert api_client.register({}) is False + assert api_client.send_metrics({}) is False + + +@mark.parametrize( + "call", + ( + param(lambda api_client: api_client.register, id="register"), + param(lambda api_client: api_client.send_metrics, id="send_metrics"), + ), +) +def test_a_payload_that_cannot_be_serialised_still_raises(call): + transport = RecordingTransport(Response(status=202, headers={}, body=b"")) + + with pytest.raises(TypeError): + call(api_client_over(transport))({"not-json": object()}) + + assert transport.requests == [] diff --git a/tests/unit_tests/test_utils.py b/tests/unit_tests/test_utils.py index ddf4eaa3..5d9bc119 100644 --- a/tests/unit_tests/test_utils.py +++ b/tests/unit_tests/test_utils.py @@ -1,6 +1,10 @@ import pytest -from UnleashClient.utils import _validate_url, extract_environment_from_api_key +from UnleashClient.utils import ( + _validate_header_value, + _validate_url, + extract_environment_from_api_key, +) def test_valid_api_key(): @@ -50,3 +54,34 @@ def test_validate_url_accepts_http_urls_with_a_host(url): def test_validate_url_rejects_malformed_urls(url, message): with pytest.raises(ValueError, match=message): _validate_url(url) + + +@pytest.mark.parametrize( + "value", + [ + "value", + "", + " leading-space", + "nul\0byte", + ], +) +def test_validate_header_value_accepts_values_some_library_sends(value): + _validate_header_value("X-Custom", value) + + +@pytest.mark.parametrize("value", ["a\nb", "a\rb", "trailing-newline\n"]) +def test_validate_header_value_rejects_a_line_break(value): + with pytest.raises(ValueError, match="line break"): + _validate_header_value("X-Custom", value) + + +def test_validate_header_value_rejects_a_value_that_is_not_a_str(): + with pytest.raises(TypeError, match="str"): + _validate_header_value("X-Custom", 1) + + +def test_validate_header_value_keeps_the_value_out_of_the_message(): + with pytest.raises(ValueError) as error: + _validate_header_value("Authorization", "project:env.secret\n") + + assert "secret" not in str(error.value) diff --git a/v7_MIGRATION_GUIDE.md b/v7_MIGRATION_GUIDE.md index 7d75b2fd..f354da86 100644 --- a/v7_MIGRATION_GUIDE.md +++ b/v7_MIGRATION_GUIDE.md @@ -98,6 +98,16 @@ client.initialize_client(fetch_toggles=False) In v6 a malformed URL only raised when the client registered. With registration disabled, every fetch and metrics send failed and was only logged. +## Values sent as headers are checked + +The client sends `api_key`, `app_name`, `IdentityOptions.instance_id` and `connection_id` as request headers, so the constructor raises a `ValueError` if one contains a line break, and a `TypeError` if one isn't a str. The message names the header but never contains its value. + +The most common cause is an API token read from a file or an environment variable with a trailing newline. Strip it before passing it: + +``` python +client = UnleashClient(url, app_name, api_key=os.environ["UNLEASH_API_KEY"].strip()) +``` + ## `environment` has been removed The `environment` argument has been removed, with no replacement. Your API token decides which environment the client gets flags for.