fix: redact Redis password from cache connection logs (#869)

* fix: redact Redis password from cache connection logs

The cache client logged the full Redis URL — including the password —
at INFO and WARNING levels on every connection attempt and failure.
This exposed the live Redis credential in stdout/container logs and any
downstream log aggregation.

Add _redact_cache_url() to mask the password component before logging.
URLs without a password are returned unchanged.

Closes #866

* fix: handle malformed URLs and IPv6 in _redact_cache_url

Address review feedback from VVoruganti and CodeRabbit:

- Wrap urlparse/urlunparse in try/except so malformed URLs (e.g. invalid
  port) don't raise ValueError inside except blocks, which would crash
  startup instead of degrading gracefully
- Preserve IPv6 brackets (e.g. [::1]) in reconstructed URLs
- Add Google-style Args/Returns docstring sections
- Add unit tests for password masking, no-password URLs, IPv6,
  malformed inputs, and the invalid-port regression

* test: use real secrets in redaction test fixtures

Three fixtures were weakened by copy-paste mangling: literal '***'
placeholders instead of real passwords (assertions trivially true),
an unescaped '#' that truncated netloc parsing via the URL fragment,
and a no-password case that actually contained userinfo. Restore
inputs that genuinely exercise the masking paths.

* fix: never leak password through malformed-URL fallback

The catch-all fallback returned the original URL when parsing failed,
so a Redis URL with a password and an invalid port (typo, out-of-range)
was logged in clear text - the exact leak #866 exists to fix. Narrow
the handling: .port access gets its own try/except (invalid port is
omitted from the output; userinfo/hostname masking never raises), and
the outer fallback now returns a generic placeholder instead of the
raw input. Tightened the invalid-port test to assert the password is
absent and added out-of-range-port and unparseable-URL cases.

* fix: redact secrets in query params and scheme-less URLs

_redact_cache_url only masked userinfo, but a credential can reach the
URL through two other real configuration paths: redis-py accepts
?password= (all querystring options become client kwargs) and cashews
accepts ?secret= (its HMAC signing key) - and honcho's own default
CACHE.URL already uses a query param (?suppress=true), so this is the
expected configuration style. Separately, a URL missing its scheme
(':pass@host:6379/0') parses with an empty netloc, making the password
invisible to .password and echoing it back verbatim.

Mask sensitive query values in place on the raw query string (no
decode/re-encode, so non-secret params are preserved byte-for-byte)
and return the generic placeholder for @-carrying strings with no
parseable authority. Verified with a 20k-case randomized fuzz run in
addition to the unit tests: no functional credential reaches the
output.

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 ``"<redacted-unparseable-url>"`` 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 "<redacted-unparseable-url>"
+            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 "<redacted-unparseable-url>"
+
+
 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:

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)