diff --git a/src/cache/client.py b/src/cache/client.py index 29d8c187..1ad8e3f5 100644 --- a/src/cache/client.py +++ b/src/cache/client.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio import logging from typing import Any, cast +from urllib.parse import urlparse, urlunparse import sentry_sdk from cashews import cache @@ -20,10 +21,99 @@ from src.config import settings logger = logging.getLogger(__name__) - _cache_lock = asyncio.Lock() +# Query parameters that carry secrets when configured via URL: +# redis-py accepts ``?password=`` (all querystring options become client +# kwargs) and cashews accepts ``?secret=`` (HMAC key for value signing). +_SENSITIVE_QUERY_PARAMS = frozenset({"password", "secret"}) + + +def _mask_sensitive_query(query: str) -> str: + """Mask values of secret-bearing query parameters. + + Operates on the raw query string (no decode/re-encode round trip) + so non-secret parameters are preserved byte-for-byte. + + Args: + query: The raw query string from a parsed URL. + + Returns: + The query string with sensitive values replaced by ``***``, or + the original string if no sensitive parameter is present. + """ + if not query: + return query + parts: list[str] = [] + changed = False + for part in query.split("&"): + name, sep, _value = part.partition("=") + if sep and name.lower() in _SENSITIVE_QUERY_PARAMS: + parts.append(f"{name}=***") + changed = True + else: + parts.append(part) + return "&".join(parts) if changed else query + + +def _redact_cache_url(url: str) -> str: + """Mask credentials in a Redis connection URL before logging. + + Given ``redis://:password@host:port/db`` returns + ``redis://:***@host:port/db``; secret-bearing query parameters + (``?password=``, ``?secret=``) are masked as well. A URL carrying + no credentials is returned unchanged. This function never raises + and never returns a credential: an invalid port is omitted from + the output, and a URL that cannot be parsed at all is replaced by + a generic placeholder rather than echoed back, so that logging + inside ``except`` blocks can neither crash startup nor leak the + secrets this helper exists to hide. + + Args: + url: The Redis connection URL to redact. + + Returns: + The URL with its credentials masked, the original URL if it + carries none, or ``""`` if parsing + fails entirely. + """ + try: + parsed = urlparse(url) + query = _mask_sensitive_query(parsed.query) + # .password only splits netloc and never raises, unlike .port + if parsed.password is None and query == parsed.query: + # A string with an "@" but no parsed authority (e.g. a URL + # missing its scheme, ":pass@host:6379/0") may still carry + # userinfo that urlparse could not see — never echo it. + if "@" in url and not parsed.netloc: + return "" + return url + netloc = parsed.netloc + if parsed.password is not None: + userinfo = parsed.username or "" + hostname = parsed.hostname or "" + # Preserve IPv6 brackets (urlparse strips them from .hostname) + if hostname and ":" in hostname and not hostname.startswith("["): + hostname = f"[{hostname}]" + netloc = f"{userinfo}:***@{hostname}" + try: + port = parsed.port + except ValueError: + # Invalid or out-of-range port: omit it rather than let + # the outer fallback echo the raw URL (and its password) + # back. + port = None + if port is not None: + netloc += f":{port}" + parsed = parsed._replace(netloc=netloc, query=query) + return urlunparse(parsed) + except (ValueError, TypeError): + # Unparseable URL: never return the raw input — it may contain + # the very password this helper exists to hide. + return "" + + def is_cache_enabled() -> bool: return settings.CACHE.ENABLED @@ -59,7 +149,7 @@ async def init_cache() -> None: except Exception as setup_err: logger.warning( "Cache setup failed for %s: %s. Falling back to in-memory cache", - settings.CACHE.URL, + _redact_cache_url(settings.CACHE.URL), setup_err, ) if settings.SENTRY.ENABLED: @@ -87,7 +177,10 @@ async def init_cache() -> None: with attempt: async with asyncio.timeout(2): await cache.ping() - logger.info("Connected to cache at %s", settings.CACHE.URL) + logger.info( + "Connected to cache at %s", + _redact_cache_url(settings.CACHE.URL), + ) except ( redis_exc.TimeoutError, redis_exc.ConnectionError, @@ -96,7 +189,7 @@ async def init_cache() -> None: ) as e: logger.warning( "Failed to connect to cache at %s: %s. Falling back to in-memory cache", - settings.CACHE.URL, + _redact_cache_url(settings.CACHE.URL), e, ) if settings.SENTRY.ENABLED: @@ -107,7 +200,7 @@ async def init_cache() -> None: except Exception as e: logger.warning( "Unexpected cache error at %s: %s. Falling back to in-memory cache", - settings.CACHE.URL, + _redact_cache_url(settings.CACHE.URL), e, ) if settings.SENTRY.ENABLED: diff --git a/tests/test_cache_redaction.py b/tests/test_cache_redaction.py new file mode 100644 index 00000000..0ff2bbc7 --- /dev/null +++ b/tests/test_cache_redaction.py @@ -0,0 +1,126 @@ +"""Unit tests for the cache client's _redact_cache_url helper.""" + +import pytest + +from src.cache.client import _redact_cache_url + + +class TestRedactCacheUrl: + """Tests for _redact_cache_url — a security-relevant logging helper + that must never raise and must never leak a password.""" + + # --- Password masking --- + + def test_password_only_userinfo(self): + assert ( + _redact_cache_url("redis://:secret@localhost:6379/0") + == "redis://:***@localhost:6379/0" + ) + + def test_user_and_password(self): + result = _redact_cache_url("redis://user:s3cret@10.0.0.1:6380/2") + assert "***" in result + assert "s3cret" not in result + assert "user" in result + + def test_rediss_protocol(self): + result = _redact_cache_url("rediss://:secret@redis.example.com:6380") + assert result.startswith("rediss://") + assert "***" in result + assert "secret" not in result + + def test_complex_password(self): + result = _redact_cache_url("redis://:p%40ssw0rd!%24@host:6379/0") + assert "***" in result + assert "p%40ssw0rd" not in result + + def test_password_never_leaked(self): + """The original password must never appear in the redacted output.""" + for url in [ + "redis://:hunter2@localhost:6379/0", + "redis://admin:hunter2@localhost:6379/0", + "rediss://:hunter2@[::1]:6380/1", + ]: + assert "hunter2" not in _redact_cache_url(url) + + # --- Secrets in query parameters --- + # redis-py accepts ?password= (querystring options become client + # kwargs) and cashews accepts ?secret= (HMAC signing key), so both + # are real configuration paths that must not reach the logs. + + @pytest.mark.parametrize("param", ["password", "secret", "PASSWORD"]) + def test_query_param_secret_masked(self, param: str): + result = _redact_cache_url(f"redis://host:6379/0?{param}=s3cret") + assert "s3cret" not in result + assert f"{param}=***" in result + + def test_query_param_masking_preserves_other_params(self): + result = _redact_cache_url("redis://host:6379/0?db=1&password=s3cret&ssl=true") + assert "s3cret" not in result + assert "db=1" in result + assert "ssl=true" in result + + def test_userinfo_and_query_secret_both_masked(self): + result = _redact_cache_url("redis://:hunter2@host:6379/0?secret=s3cret") + assert "hunter2" not in result + assert "s3cret" not in result + + def test_non_secret_query_params_unchanged(self): + url = "redis://localhost:6379/0?suppress=true" + assert _redact_cache_url(url) == url + + # --- No-password URLs (returned unchanged) --- + + def test_user_without_password_unchanged(self): + assert ( + _redact_cache_url("redis://user@localhost:6379/0") + == "redis://user@localhost:6379/0" + ) + + def test_in_memory_url_unchanged(self): + assert _redact_cache_url("mem://") == "mem://" + + # --- IPv6 --- + + def test_ipv6_brackets_preserved(self): + result = _redact_cache_url("rediss://:secret@[::1]:6380/1") + assert "[::1]" in result + assert "***" in result + assert "secret" not in result + + # --- Malformed URLs (must NOT raise) --- + + def test_invalid_port_redacts_password(self): + """Regression test for two review findings: accessing + ``parsed.port`` on a URL with a non-numeric port raises + ``ValueError`` (must not crash startup inside an except block), + and the fallback must never echo the raw URL back — the + password has to be masked even when the port is unparseable. + """ + result = _redact_cache_url("redis://:pass@host:notaport/0") + assert "pass" not in result + assert "***" in result + + def test_out_of_range_port_redacts_password(self): + result = _redact_cache_url("redis://:supersecret@host:99999/0") + assert "supersecret" not in result + assert "***" in result + + def test_unparseable_url_never_echoed(self): + # Unbalanced IPv6 bracket makes urlparse itself raise; the + # fallback must return a placeholder, not the raw input. + result = _redact_cache_url("redis://:secret@[::1:6379/0") + assert "secret" not in result + + def test_missing_scheme_never_echoed(self): + # Without "redis://" urlparse sees no netloc, so the userinfo + # (and its password) is invisible to .password — the string + # must not be echoed back. + result = _redact_cache_url(":hunter2@host:6379/0") + assert "hunter2" not in result + + def test_garbage_input_does_not_raise(self): + assert isinstance(_redact_cache_url("not a url at all"), str) + + def test_empty_string_does_not_raise(self): + assert isinstance(_redact_cache_url(""), str)