From 9e8f5fd6420df1f5267e9c544157878b31a1628e Mon Sep 17 00:00:00 2001
From: Hayden <154503486+groupthinking@users.noreply.github.com>
Date: Thu, 30 Jul 2026 23:35:56 -0500
Subject: [PATCH 1/5] fix(security): stop proxy credentials leaking from
 subprocess errors (#1113)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`WEBSHARE_PROXY_URL` carries `user:password` in its userinfo. Two paths
leaked it verbatim:

1. `enhanced_video_processor._get_openai_whisper_transcript` runs yt-dlp via
   `subprocess.run(..., check=True)`. The resulting `CalledProcessError`
   stringifies the whole argv, including `--proxy http://user:pass@host`.
   That string was written to `logger.warning` (CWE-532) *and* returned to
   the caller in the `error` field of the response (CWE-209).
2. `robust._get_metadata_ytdlp` raised `yt-dlp failed: {result.stderr}`;
   yt-dlp echoes the `--proxy` value back on stderr for connection failures.
   `TimeoutExpired` from the same call site stringifies the argv too.

Separately, `get_proxy_url()` documented "malformed => None" but did not
honour it: `urllib.parse` raises `ValueError` on an unterminated IPv6
literal at parse time, and on a non-numeric or out-of-range port when
`.port` is read. The exception escaped to callers that log it, which put
the offending URL — credentials and all — into the log a third way.

Changes:

- `utils/proxy.get_proxy_url` contains `ValueError` from both `urlparse`
  and the `.port` access, adds `socks5h` to the allowed schemes, and keeps
  the URL out of the "malformed" warning.
- `utils/proxy.redact_proxy_credentials` now accepts any object, never
  raises (it runs inside `except` blocks, where a failure would mask the
  original error), and sweeps in two passes: an exact replacement of the
  configured env value that preserves host:port for triage, then a generic
  `scheme://user:pass@` regex for normalised stderr echoes, argv dumps and
  other proxy variables. The user/password classes exclude `/`, so a path
  containing `@` is not over-redacted.
- Both leak sites redact before logging or returning.
- `shared/libs/youtube_proxy.py` (a drifted duplicate, loaded both as a
  package and standalone via importlib) now delegates to the canonical
  helper with an equivalent local fallback, matching the pattern already
  used in `gemini_video_master_agent.py`. This also fixes a latent
  `UnboundLocalError` on a portless proxy URL.

16 of the 29 new tests in `tests/unit/test_proxy_utils.py` fail against the
pre-fix code. Full `tests/unit` run shows no new failures.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 shared/libs/youtube_proxy.py                  |  90 ++++++--
 .../backend/enhanced_video_processor.py       |  14 +-
 .../services/youtube/adapters/robust.py       |  16 +-
 src/youtube_extension/utils/proxy.py          |  82 +++++--
 tests/unit/test_enhanced_video_processor.py   |  37 +++
 tests/unit/test_proxy_utils.py                | 211 ++++++++++++++++++
 tests/unit/test_robust_youtube_service.py     |  28 +++
 7 files changed, 438 insertions(+), 40 deletions(-)
 create mode 100644 tests/unit/test_proxy_utils.py

diff --git a/shared/libs/youtube_proxy.py b/shared/libs/youtube_proxy.py
index 77c354095..d29421ad4 100644
--- a/shared/libs/youtube_proxy.py
+++ b/shared/libs/youtube_proxy.py
@@ -12,6 +12,7 @@
 import logging
 import os
 import random
+import re
 import time
 import urllib.parse
 from dataclasses import dataclass
@@ -39,15 +40,42 @@
 logger = logging.getLogger("youtube_api_proxy")
 
 
+_ALLOWED_SCHEMES = ("http", "https", "socks5", "socks5h")
+
+# Matches the ``user[:password]@`` userinfo segment of any URL. The user and
+# password classes exclude "/" so a path containing "@" is never mistaken for
+# credentials. Kept in sync with youtube_extension.utils.proxy.
+_USERINFO_RE = re.compile(
+    r"(?P<scheme>[A-Za-z][A-Za-z0-9+.\-]*://)"
+    r"(?P<user>[^\s/:@]+)"
+    r"(?::(?P<password>[^\s/@]*))?"
+    r"@"
+)
+
+
 def _get_webshare_proxy_url() -> str | None:
-    """Return the validated WEBSHARE_PROXY_URL, or None for direct connection."""
+    """Return the validated WEBSHARE_PROXY_URL, or None for direct connection.
+
+    Mirrors ``youtube_extension.utils.proxy.get_proxy_url``. ``urllib.parse``
+    raises ``ValueError`` on several malformed inputs (an unterminated IPv6
+    literal at parse time; a non-numeric or out-of-range port when ``.port`` is
+    read). Contain it here so the exception -- which callers would log next to
+    the credential-bearing URL -- never escapes.
+    """
     url = os.getenv("WEBSHARE_PROXY_URL", "").strip()
     if not url:
         return None
-    parsed = urllib.parse.urlparse(url)
-    if parsed.scheme not in ("http", "https", "socks5") or not parsed.hostname:
+    try:
+        parsed = urllib.parse.urlparse(url)
+        valid = parsed.scheme in _ALLOWED_SCHEMES and bool(parsed.hostname)
+        if valid:
+            parsed.port  # noqa: B018 - validates port, raises ValueError if bad
+    except ValueError:
+        valid = False
+    if not valid:
         logger.warning(
-            "WEBSHARE_PROXY_URL is set but malformed — falling back to direct connection"
+            "WEBSHARE_PROXY_URL is set but malformed - falling back to direct "
+            "connection"
         )
         return None
     return url
@@ -65,27 +93,53 @@ def _get_transcript_proxy_config() -> GenericProxyConfig | None:
     if GenericProxyConfig is None:
         logger.warning(
             "WEBSHARE_PROXY_URL is set but youtube-transcript-api proxy support "
-            "is unavailable (requires youtube-transcript-api>=1.0) — falling back "
+            "is unavailable (requires youtube-transcript-api>=1.0) - falling back "
             "to direct connection"
         )
         return None
     return GenericProxyConfig(http_url=url, https_url=url)
 
 
-def _redact_proxy_credentials(text: str) -> str:
-    """Strip user:pass credentials of the configured proxy URL from text."""
+def _redact_proxy_credentials(text: Any) -> str:
+    """Strip URL userinfo (``user:pass@``) from ``text``.
+
+    Delegates to the canonical helper when the ``youtube_extension`` package is
+    importable; this module is also loaded standalone by path (see
+    ``src/agents/mcp_enhanced_video_processor.py``), so an equivalent local
+    implementation is retained as a fallback. Never raises -- it is called from
+    exception handlers, where a failure would mask the original error.
+    """
+    try:  # pragma: no cover - exercised only when the package is importable
+        from youtube_extension.utils.proxy import (
+            redact_proxy_credentials as _canonical,
+        )
+
+        return _canonical(text)
+    except ImportError:
+        pass
+
+    if not isinstance(text, str):
+        text = str(text)
+
     url = os.getenv("WEBSHARE_PROXY_URL", "").strip()
-    if not url or url not in text:
-        return text
-    try:
-        parsed = urllib.parse.urlparse(url)
-        netloc = parsed.hostname or ""
-        if parsed.port:
-            netloc = f"{netloc}:{parsed.port}"
-        redacted = parsed._replace(netloc=netloc).geturl()
-    except (ValueError, AttributeError):
-        redacted = "<proxy-url>"
-    return text.replace(url, redacted)
+    if url and url in text:
+        try:
+            parsed = urllib.parse.urlparse(url)
+            netloc = parsed.hostname or ""
+            if parsed.port:
+                netloc = f"{netloc}:{parsed.port}"
+            redacted = parsed._replace(netloc=netloc).geturl()
+        except (ValueError, AttributeError):
+            redacted = "<proxy-url>"
+        text = text.replace(url, redacted)
+
+    def _mask(match: re.Match[str]) -> str:
+        if match.group("password") is None:
+            return f"{match.group('scheme')}***@"
+        return f"{match.group('scheme')}***:***@"
+
+    return _USERINFO_RE.sub(_mask, text)
+
 
 class YouTubeErrorType(Enum):
     """YouTube API specific error types"""
diff --git a/src/youtube_extension/backend/enhanced_video_processor.py b/src/youtube_extension/backend/enhanced_video_processor.py
index 2b36769cf..818923d63 100644
--- a/src/youtube_extension/backend/enhanced_video_processor.py
+++ b/src/youtube_extension/backend/enhanced_video_processor.py
@@ -25,7 +25,11 @@
 # Load environment variables
 load_dotenv()
 
-from youtube_extension.utils.proxy import get_proxy_url, get_transcript_proxy_config
+from youtube_extension.utils.proxy import (
+    get_proxy_url,
+    get_transcript_proxy_config,
+    redact_proxy_credentials,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -316,8 +320,12 @@ async def _get_openai_whisper_transcript(self, video_id: str, video_url: str) ->
                 'processing_time': datetime.now().isoformat()
             }
         except Exception as e:
-            logger.warning(f"OpenAI Whisper failed: {e}")
-            return {'text': '', 'source': 'failed', 'error': str(e)}
+            # subprocess.run(check=True) raises CalledProcessError whose str()
+            # embeds the whole argv — including "--proxy <url-with-credentials>".
+            # Redact before it reaches the log or the returned error field.
+            detail = redact_proxy_credentials(e)
+            logger.warning(f"OpenAI Whisper failed: {detail}")
+            return {'text': '', 'source': 'failed', 'error': detail}
     
     async def _get_youtube_transcript_fallback(self, video_id: str) -> Dict[str, Any]:
         """Fallback to YouTube transcript API"""
diff --git a/src/youtube_extension/backend/services/youtube/adapters/robust.py b/src/youtube_extension/backend/services/youtube/adapters/robust.py
index 30d9e33ae..3f71a649c 100644
--- a/src/youtube_extension/backend/services/youtube/adapters/robust.py
+++ b/src/youtube_extension/backend/services/youtube/adapters/robust.py
@@ -20,7 +20,11 @@
 
 import httpx
 
-from youtube_extension.utils.proxy import get_proxy_url, get_transcript_proxy_config
+from youtube_extension.utils.proxy import (
+    get_proxy_url,
+    get_transcript_proxy_config,
+    redact_proxy_credentials,
+)
 
 # Import our cost monitor
 try:
@@ -140,7 +144,9 @@ async def get_video_metadata(self, video_url: str) -> RobustYouTubeMetadata:
         try:
             return await self._get_metadata_ytdlp(video_url, video_id)
         except Exception as e:
-            logger.warning(f"yt-dlp fallback failed: {e}")
+            # subprocess.TimeoutExpired stringifies the whole argv, which carries
+            # "--proxy <url-with-credentials>"; redact before logging.
+            logger.warning(f"yt-dlp fallback failed: {redact_proxy_credentials(e)}")
 
         raise Exception("All YouTube metadata APIs failed")
 
@@ -167,7 +173,11 @@ def _get_ytdlp_metadata():
                 timeout=30,
             )
             if result.returncode != 0:
-                raise Exception(f"yt-dlp failed: {result.stderr}")
+                # yt-dlp echoes the --proxy value back on stderr for connection
+                # failures, so redact before the message is raised and logged.
+                raise Exception(
+                    f"yt-dlp failed: {redact_proxy_credentials(result.stderr)}"
+                )
             return json.loads(result.stdout)
 
         data = await asyncio.get_event_loop().run_in_executor(None, _get_ytdlp_metadata)
diff --git a/src/youtube_extension/utils/proxy.py b/src/youtube_extension/utils/proxy.py
index def0da69e..cb274ab63 100644
--- a/src/youtube_extension/utils/proxy.py
+++ b/src/youtube_extension/utils/proxy.py
@@ -13,6 +13,7 @@
 
 import logging
 import os
+import re
 import urllib.parse
 from typing import Any
 
@@ -28,14 +29,41 @@
 
 _PROXY_ENV_VAR = "WEBSHARE_PROXY_URL"
 
+_ALLOWED_SCHEMES = ("http", "https", "socks5", "socks5h")
+
+# Matches the ``user[:password]@`` userinfo segment of any URL. The user and
+# password classes exclude "/" so a path containing "@" (e.g.
+# "https://example.com/a@b") is never mistaken for credentials.
+_USERINFO_RE = re.compile(
+    r"(?P<scheme>[A-Za-z][A-Za-z0-9+.\-]*://)"
+    r"(?P<user>[^\s/:@]+)"
+    r"(?::(?P<password>[^\s/@]*))?"
+    r"@"
+)
+
+_REDACTED = "***"
+
 
 def get_proxy_url() -> str | None:
-    """Return the validated Webshare proxy URL, or None for direct connection."""
+    """Return the validated Webshare proxy URL, or None for direct connection.
+
+    Never raises and never emits the URL (which carries credentials) into logs.
+    ``urllib.parse`` raises ``ValueError`` for several malformed inputs — an
+    unterminated IPv6 literal at parse time, a non-numeric or out-of-range port
+    when ``.port`` is read — so both are contained here rather than escaping to
+    callers that would log the exception alongside the offending URL.
+    """
     url = os.getenv(_PROXY_ENV_VAR, "").strip()
     if not url:
         return None
-    parsed = urllib.parse.urlparse(url)
-    if parsed.scheme not in ("http", "https", "socks5") or not parsed.hostname:
+    try:
+        parsed = urllib.parse.urlparse(url)
+        valid = parsed.scheme in _ALLOWED_SCHEMES and bool(parsed.hostname)
+        if valid:
+            parsed.port  # noqa: B018 - validates the port, raises ValueError if bad
+    except ValueError:
+        valid = False
+    if not valid:
         logger.warning(
             "%s is set but malformed — falling back to direct connection",
             _PROXY_ENV_VAR,
@@ -67,17 +95,39 @@ def get_transcript_proxy_config() -> Any | None:
     return GenericProxyConfig(http_url=url, https_url=url)
 
 
-def redact_proxy_credentials(text: str) -> str:
-    """Strip user:pass credentials of the configured proxy URL from text."""
+def redact_proxy_credentials(text: Any) -> str:
+    """Strip URL userinfo (``user:pass@``) from ``text``.
+
+    Two passes, because either alone is insufficient:
+
+    1. Exact replacement of the configured ``WEBSHARE_PROXY_URL`` — preserves
+       the host so operators can still tell *which* proxy was in play.
+    2. A generic ``scheme://user:pass@`` sweep — catches credentials that never
+       match the env value verbatim: yt-dlp echoing a normalised/percent-encoded
+       form back on stderr, a ``CalledProcessError`` repr of the argv, or a
+       different proxy variable (``HTTPS_PROXY`` and friends) entirely.
+
+    Always returns a string and never raises; it is called from exception
+    handlers, where a failure would mask the original error.
+    """
+    if not isinstance(text, str):
+        text = str(text)
+
     url = os.getenv(_PROXY_ENV_VAR, "").strip()
-    if not url or url not in text:
-        return text
-    try:
-        parsed = urllib.parse.urlparse(url)
-        netloc = parsed.hostname or ""
-        if parsed.port:
-            netloc = f"{netloc}:{parsed.port}"
-        redacted = parsed._replace(netloc=netloc).geturl()
-    except (ValueError, AttributeError):
-        redacted = "<proxy-url>"
-    return text.replace(url, redacted)
+    if url and url in text:
+        try:
+            parsed = urllib.parse.urlparse(url)
+            netloc = parsed.hostname or ""
+            if parsed.port:
+                netloc = f"{netloc}:{parsed.port}"
+            redacted = parsed._replace(netloc=netloc).geturl()
+        except (ValueError, AttributeError):
+            redacted = "<proxy-url>"
+        text = text.replace(url, redacted)
+
+    def _mask(match: re.Match[str]) -> str:
+        if match.group("password") is None:
+            return f"{match.group('scheme')}{_REDACTED}@"
+        return f"{match.group('scheme')}{_REDACTED}:{_REDACTED}@"
+
+    return _USERINFO_RE.sub(_mask, text)
diff --git a/tests/unit/test_enhanced_video_processor.py b/tests/unit/test_enhanced_video_processor.py
index a25f7fdfc..c1ad6ffe3 100644
--- a/tests/unit/test_enhanced_video_processor.py
+++ b/tests/unit/test_enhanced_video_processor.py
@@ -8,6 +8,7 @@
 from __future__ import annotations
 
 import json
+import logging
 import sys
 from contextlib import asynccontextmanager
 from pathlib import Path
@@ -639,6 +640,42 @@ async def test_yt_dlp_uses_canonical_url_after_option_terminator(self, tmp_path)
         assert hostile_url not in command
         assert result["text"] == "safe transcript"
 
+    async def test_subprocess_failure_never_leaks_proxy_credentials(
+        self, tmp_path, monkeypatch, caplog
+    ):
+        """Regression for #1113.
+
+        ``subprocess.run(..., check=True)`` raises ``CalledProcessError`` whose
+        ``str()`` renders the entire argv — which carries
+        ``--proxy http://user:pass@host``. That string reached both the warning
+        log (CWE-532) and the returned ``error`` field (CWE-209).
+        """
+        import subprocess
+
+        password = "sup3r-s3cret-pw"
+        proxy_url = f"http://wsuser:{password}@p.webshare.io:80"
+        monkeypatch.setenv("WEBSHARE_PROXY_URL", proxy_url)
+
+        proc = _make_processor()
+
+        def _raise(cmd, *args, **kwargs):
+            raise subprocess.CalledProcessError(returncode=1, cmd=cmd)
+
+        with patch.dict(sys.modules, {"openai": MagicMock()}):
+            with patch("tempfile.TemporaryDirectory") as temp_dir:
+                temp_dir.return_value.__enter__.return_value = str(tmp_path)
+                with patch("subprocess.run", side_effect=_raise):
+                    with caplog.at_level(logging.WARNING):
+                        result = await proc._get_openai_whisper_transcript(
+                            _VIDEO_ID, _VIDEO_URL
+                        )
+
+        assert result["source"] == "failed"
+        assert password not in result["error"]
+        assert "wsuser" not in result["error"]
+        assert password not in caplog.text
+        assert "wsuser" not in caplog.text
+
 
 # ===========================================================================
 # _get_gemini_transcript
diff --git a/tests/unit/test_proxy_utils.py b/tests/unit/test_proxy_utils.py
new file mode 100644
index 000000000..0e8efb6c6
--- /dev/null
+++ b/tests/unit/test_proxy_utils.py
@@ -0,0 +1,211 @@
+"""Unit tests for ``youtube_extension.utils.proxy`` credential hygiene.
+
+Regression coverage for #1113 — "Prevent proxy credential leakage from urlparse
+errors". Three distinct defects are pinned here:
+
+1. ``get_proxy_url()`` raised ``ValueError`` out of ``urllib.parse`` for several
+   malformed inputs instead of honouring its documented "malformed ⇒ None"
+   contract, so the exception escaped to callers that log it.
+2. The warning emitted for a malformed value must never carry the URL, because
+   the URL is exactly where the credentials live.
+3. ``redact_proxy_credentials()`` only stripped an *exact* match of the
+   configured env value, so a ``CalledProcessError`` argv dump, a yt-dlp stderr
+   echo, or a different proxy variable leaked ``user:pass`` verbatim.
+"""
+
+from __future__ import annotations
+
+import logging
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+_SRC = Path(__file__).resolve().parents[2] / "src"
+if str(_SRC) not in sys.path:
+    sys.path.insert(0, str(_SRC))
+
+from youtube_extension.utils.proxy import (  # noqa: E402
+    _PROXY_ENV_VAR,
+    get_proxy_dict,
+    get_proxy_url,
+    redact_proxy_credentials,
+)
+
+USER = "wsuser"
+PASSWORD = "sup3r-s3cret-pw"
+PROXY_URL = f"http://{USER}:{PASSWORD}@p.webshare.io:80"
+
+SECRETS = (USER, PASSWORD)
+
+
+def _assert_no_secrets(text: str) -> None:
+    for secret in SECRETS:
+        assert secret not in text, f"credential {secret!r} leaked into: {text!r}"
+
+
+# ---------------------------------------------------------------------------
+# get_proxy_url — validation must never raise and never log the URL
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    "raw",
+    [
+        pytest.param(f"http://{USER}:{PASSWORD}@[::1", id="unterminated-ipv6"),
+        pytest.param(f"http://{USER}:{PASSWORD}@host:notaport", id="non-numeric-port"),
+        pytest.param(f"http://{USER}:{PASSWORD}@host:99999", id="port-out-of-range"),
+        pytest.param(f"ftp://{USER}:{PASSWORD}@host:21", id="unsupported-scheme"),
+        pytest.param(f"{USER}:{PASSWORD}@host:80", id="no-scheme"),
+        pytest.param("http://", id="no-host"),
+    ],
+)
+def test_malformed_proxy_url_returns_none_without_raising(monkeypatch, raw):
+    """Malformed values fall back to a direct connection instead of exploding.
+
+    Before the fix, the IPv6 and port cases raised ``ValueError`` out of
+    ``urllib.parse``, breaking the documented contract.
+    """
+    monkeypatch.setenv(_PROXY_ENV_VAR, raw)
+    assert get_proxy_url() is None
+    assert get_proxy_dict() is None
+
+
+@pytest.mark.parametrize(
+    "raw",
+    [
+        f"http://{USER}:{PASSWORD}@[::1",
+        f"http://{USER}:{PASSWORD}@host:notaport",
+        f"ftp://{USER}:{PASSWORD}@host:21",
+    ],
+)
+def test_malformed_proxy_warning_never_contains_credentials(monkeypatch, caplog, raw):
+    """The 'malformed' warning names the variable, never its value."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, raw)
+    with caplog.at_level(logging.WARNING, logger="youtube_extension.utils.proxy"):
+        assert get_proxy_url() is None
+
+    assert caplog.records, "expected a warning for a malformed proxy URL"
+    for record in caplog.records:
+        _assert_no_secrets(record.getMessage())
+        assert raw not in record.getMessage()
+    assert _PROXY_ENV_VAR in caplog.text
+
+
+@pytest.mark.parametrize(
+    "raw",
+    [
+        PROXY_URL,
+        "https://p.webshare.io",
+        f"socks5://{USER}:{PASSWORD}@p.webshare.io:1080",
+        f"socks5h://{USER}:{PASSWORD}@p.webshare.io:1080",
+    ],
+)
+def test_valid_proxy_urls_are_returned_verbatim(monkeypatch, raw):
+    """A well-formed proxy is passed through untouched — the value is the secret
+    the outbound client needs, so redaction belongs at the log boundary only."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, raw)
+    assert get_proxy_url() == raw
+    assert get_proxy_dict() == {"http": raw, "https": raw}
+
+
+@pytest.mark.parametrize("raw", ["", "   "])
+def test_unset_or_blank_proxy_is_direct_connection(monkeypatch, raw):
+    monkeypatch.setenv(_PROXY_ENV_VAR, raw)
+    assert get_proxy_url() is None
+    assert get_proxy_dict() is None
+
+
+def test_missing_env_var_is_direct_connection(monkeypatch):
+    monkeypatch.delenv(_PROXY_ENV_VAR, raising=False)
+    assert get_proxy_url() is None
+
+
+# ---------------------------------------------------------------------------
+# redact_proxy_credentials
+# ---------------------------------------------------------------------------
+
+
+def test_exact_configured_url_is_redacted_but_host_preserved(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    out = redact_proxy_credentials(f"connect failed via {PROXY_URL} after 3 tries")
+    _assert_no_secrets(out)
+    assert "p.webshare.io:80" in out, "host should survive so operators can triage"
+
+
+def test_credentials_are_redacted_even_when_env_var_is_unset(monkeypatch):
+    """The generic sweep is what makes this safe for stderr echoes and for
+    ``HTTPS_PROXY``-style variables the helper does not own."""
+    monkeypatch.delenv(_PROXY_ENV_VAR, raising=False)
+    out = redact_proxy_credentials(
+        f"ERROR: unable to connect to socks5://{USER}:{PASSWORD}@10.0.0.1:1080"
+    )
+    _assert_no_secrets(out)
+    assert "10.0.0.1:1080" in out
+
+
+def test_credentials_are_redacted_when_they_differ_from_the_env_var(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    out = redact_proxy_credentials("via http://other-user:other-pass@10.0.0.1:1080")
+    assert "other-pass" not in out
+    assert "other-user" not in out
+    assert "10.0.0.1:1080" in out
+
+
+def test_userinfo_without_password_is_redacted(monkeypatch):
+    monkeypatch.delenv(_PROXY_ENV_VAR, raising=False)
+    out = redact_proxy_credentials(f"http://{USER}@10.0.0.1:1080")
+    assert USER not in out
+    assert "10.0.0.1:1080" in out
+
+
+def test_path_containing_at_sign_is_not_over_redacted(monkeypatch):
+    """``@`` after a path separator is not userinfo — do not mangle real URLs."""
+    monkeypatch.delenv(_PROXY_ENV_VAR, raising=False)
+    text = "fetched https://example.com/users/a@b.txt ok"
+    assert redact_proxy_credentials(text) == text
+
+
+@pytest.mark.parametrize(
+    "value",
+    [None, 42, ValueError("boom"), ["a", "b"], {"k": "v"}],
+)
+def test_non_string_input_is_coerced_and_never_raises(monkeypatch, value):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    assert isinstance(redact_proxy_credentials(value), str)
+
+
+def test_redaction_survives_a_malformed_configured_url(monkeypatch):
+    """Redaction runs inside ``except`` blocks; a bad env value must not make it
+    raise and mask the original error."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, f"http://{USER}:{PASSWORD}@[::1")
+    out = redact_proxy_credentials(f"boom http://{USER}:{PASSWORD}@[::1 boom")
+    assert isinstance(out, str)
+    _assert_no_secrets(out)
+
+
+# ---------------------------------------------------------------------------
+# The concrete leak: subprocess errors stringify the whole argv
+# ---------------------------------------------------------------------------
+
+
+def test_called_process_error_argv_is_redacted(monkeypatch):
+    """``subprocess.run(..., check=True)`` raises ``CalledProcessError`` whose
+    ``str()`` embeds ``--proxy <url-with-credentials>``. That string was logged
+    *and* returned in an API error field (CWE-532 / CWE-209)."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    argv = ["yt-dlp", "-x", "--proxy", PROXY_URL, "--", "https://youtu.be/auJzb1D-fag"]
+    error = subprocess.CalledProcessError(returncode=1, cmd=argv)
+
+    assert PASSWORD in str(error), "precondition: the raw error does leak"
+    _assert_no_secrets(redact_proxy_credentials(error))
+
+
+def test_timeout_expired_argv_is_redacted(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    argv = ["yt-dlp", "--dump-json", "--proxy", PROXY_URL, "--", "https://youtu.be/x"]
+    error = subprocess.TimeoutExpired(cmd=argv, timeout=30)
+
+    assert PASSWORD in str(error), "precondition: the raw error does leak"
+    _assert_no_secrets(redact_proxy_credentials(error))
diff --git a/tests/unit/test_robust_youtube_service.py b/tests/unit/test_robust_youtube_service.py
index 964e32cf1..90243c5f5 100644
--- a/tests/unit/test_robust_youtube_service.py
+++ b/tests/unit/test_robust_youtube_service.py
@@ -697,6 +697,34 @@ async def test_missing_optional_fields(self):
 
 
 class TestGetMetadataYtdlp:
+    async def test_ytdlp_stderr_never_leaks_proxy_credentials(self, monkeypatch):
+        """Regression for #1113.
+
+        yt-dlp echoes the ``--proxy`` value back on stderr for connection
+        failures, and the raised message was previously logged verbatim.
+        """
+        password = "sup3r-s3cret-pw"
+        proxy_url = f"http://wsuser:{password}@p.webshare.io:80"
+        monkeypatch.setenv("WEBSHARE_PROXY_URL", proxy_url)
+
+        svc = RobustYouTubeService(api_key="KEY")
+        completed = MagicMock(
+            returncode=1,
+            stdout="",
+            stderr=f"Unable to connect to proxy {proxy_url}",
+        )
+
+        # robust.py imports subprocess inside the method, so patch the module itself.
+        with patch("subprocess.run", return_value=completed):
+            with pytest.raises(Exception) as excinfo:
+                await svc._get_metadata_ytdlp(VIDEO_URL, VIDEO_ID)
+
+        message = str(excinfo.value)
+        assert "yt-dlp failed" in message
+        assert password not in message
+        assert "wsuser" not in message
+        assert "p.webshare.io:80" in message, "host retained for triage"
+
     async def test_ytdlp_successful(self):
         svc = RobustYouTubeService(api_key="KEY")
 

From 3076bad5f8e7dd25755a5e781213ebbf08cc7af5 Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Fri, 31 Jul 2026 04:48:27 +0000
Subject: [PATCH 2/5] fix(security): guard str() coercion in
 redact_proxy_credentials

Both redaction helpers documented a "never raises" contract but called
str(text) unguarded. Since the helper runs inside except blocks, an object
whose __str__ itself raises would propagate out of the sanitizer, masking the
original failure and suppressing the sanitized log/response.

Wrap the coercion in try/except and fall back to a fixed, non-sensitive
"<unprintable error>" placeholder on failure, in both the canonical helper
(src/youtube_extension/utils/proxy.py) and the standalone fallback
(shared/libs/youtube_proxy.py). Add a regression test exercising an object
with a raising __str__.

Addresses CodeRabbit (critical) and Copilot review findings on PR #1118.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PTuvfPb1mbuCq7CRK5v2zS
---
 shared/libs/youtube_proxy.py         |  8 +++++++-
 src/youtube_extension/utils/proxy.py |  9 ++++++++-
 tests/unit/test_proxy_utils.py       | 15 +++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/shared/libs/youtube_proxy.py b/shared/libs/youtube_proxy.py
index d29421ad4..81bedd785 100644
--- a/shared/libs/youtube_proxy.py
+++ b/shared/libs/youtube_proxy.py
@@ -119,7 +119,13 @@ def _redact_proxy_credentials(text: Any) -> str:
         pass
 
     if not isinstance(text, str):
-        text = str(text)
+        try:
+            text = str(text)
+        except Exception:
+            # "Never raises" contract: this fallback runs inside exception
+            # handlers, so a failing __str__ must not propagate and mask the
+            # original error. Return a fixed, non-sensitive placeholder.
+            return "<unprintable error>"
 
     url = os.getenv("WEBSHARE_PROXY_URL", "").strip()
     if url and url in text:
diff --git a/src/youtube_extension/utils/proxy.py b/src/youtube_extension/utils/proxy.py
index cb274ab63..597821b34 100644
--- a/src/youtube_extension/utils/proxy.py
+++ b/src/youtube_extension/utils/proxy.py
@@ -111,7 +111,14 @@ def redact_proxy_credentials(text: Any) -> str:
     handlers, where a failure would mask the original error.
     """
     if not isinstance(text, str):
-        text = str(text)
+        try:
+            text = str(text)
+        except Exception:
+            # The contract is "never raises": an object whose __str__ fails
+            # here is called from an exception handler, so propagating would
+            # mask the original error. Fall back to a fixed, non-sensitive
+            # placeholder instead.
+            return "<unprintable error>"
 
     url = os.getenv(_PROXY_ENV_VAR, "").strip()
     if url and url in text:
diff --git a/tests/unit/test_proxy_utils.py b/tests/unit/test_proxy_utils.py
index 0e8efb6c6..f21563125 100644
--- a/tests/unit/test_proxy_utils.py
+++ b/tests/unit/test_proxy_utils.py
@@ -176,6 +176,21 @@ def test_non_string_input_is_coerced_and_never_raises(monkeypatch, value):
     assert isinstance(redact_proxy_credentials(value), str)
 
 
+def test_object_with_raising_str_returns_placeholder(monkeypatch):
+    """The ``never raises`` contract must hold even when ``str(text)`` itself
+    fails: an object whose ``__str__`` raises must not defeat the sanitizer and
+    mask the original error inside an ``except`` block."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+
+    class Unprintable:
+        def __str__(self) -> str:
+            raise RuntimeError("cannot stringify")
+
+    out = redact_proxy_credentials(Unprintable())
+    assert out == "<unprintable error>"
+    _assert_no_secrets(out)
+
+
 def test_redaction_survives_a_malformed_configured_url(monkeypatch):
     """Redaction runs inside ``except`` blocks; a bad env value must not make it
     raise and mask the original error."""

From 3c80dd4beff83ba9a56d6a78324baec85b21cac8 Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Fri, 31 Jul 2026 04:56:13 +0000
Subject: [PATCH 3/5] fix(security): harden userinfo regex against empty-user
 leak and query over-redaction

Address Copilot review findings on the _USERINFO_RE credential sweep (both the
canonical helper and the standalone fallback):

- Empty username: `user` required >=1 char, so `http://:secret@host` (valid
  userinfo with no username) escaped the generic pass and, unless it was the
  configured WEBSHARE_PROXY_URL, leaked the password. Make `user` optional.
- Query/fragment over-redaction: the user/password classes did not exclude `?`
  or `#`, so `https://example.com?email=a@b` was mis-parsed as userinfo and
  mangled. Add `?`/`#` as authority delimiters.

Path handling (`/`) and the two-pass host:port preservation are unchanged.
Add regression tests for both cases (verified failing against the prior regex).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PTuvfPb1mbuCq7CRK5v2zS
---
 shared/libs/youtube_proxy.py         | 10 ++++++----
 src/youtube_extension/utils/proxy.py | 11 +++++++----
 tests/unit/test_proxy_utils.py       | 21 +++++++++++++++++++++
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/shared/libs/youtube_proxy.py b/shared/libs/youtube_proxy.py
index 81bedd785..96054d30b 100644
--- a/shared/libs/youtube_proxy.py
+++ b/shared/libs/youtube_proxy.py
@@ -43,12 +43,14 @@
 _ALLOWED_SCHEMES = ("http", "https", "socks5", "socks5h")
 
 # Matches the ``user[:password]@`` userinfo segment of any URL. The user and
-# password classes exclude "/" so a path containing "@" is never mistaken for
-# credentials. Kept in sync with youtube_extension.utils.proxy.
+# password classes exclude "/", "?" and "#" so a path, query, or fragment
+# containing "@" is never mistaken for credentials, and the user may be empty so
+# credentials with no username ("http://:pass@host") are still redacted. Kept in
+# sync with youtube_extension.utils.proxy.
 _USERINFO_RE = re.compile(
     r"(?P<scheme>[A-Za-z][A-Za-z0-9+.\-]*://)"
-    r"(?P<user>[^\s/:@]+)"
-    r"(?::(?P<password>[^\s/@]*))?"
+    r"(?P<user>[^\s/:@?#]*)"
+    r"(?::(?P<password>[^\s/@?#]*))?"
     r"@"
 )
 
diff --git a/src/youtube_extension/utils/proxy.py b/src/youtube_extension/utils/proxy.py
index 597821b34..b2af39666 100644
--- a/src/youtube_extension/utils/proxy.py
+++ b/src/youtube_extension/utils/proxy.py
@@ -32,12 +32,15 @@
 _ALLOWED_SCHEMES = ("http", "https", "socks5", "socks5h")
 
 # Matches the ``user[:password]@`` userinfo segment of any URL. The user and
-# password classes exclude "/" so a path containing "@" (e.g.
-# "https://example.com/a@b") is never mistaken for credentials.
+# password classes exclude "/", "?" and "#" so a path, query, or fragment
+# containing "@" (e.g. "https://example.com/a@b" or
+# "https://example.com?e=a@b") is never mistaken for credentials. The user is
+# allowed to be empty so credentials with no username (e.g. "http://:pass@host")
+# are still redacted.
 _USERINFO_RE = re.compile(
     r"(?P<scheme>[A-Za-z][A-Za-z0-9+.\-]*://)"
-    r"(?P<user>[^\s/:@]+)"
-    r"(?::(?P<password>[^\s/@]*))?"
+    r"(?P<user>[^\s/:@?#]*)"
+    r"(?::(?P<password>[^\s/@?#]*))?"
     r"@"
 )
 
diff --git a/tests/unit/test_proxy_utils.py b/tests/unit/test_proxy_utils.py
index f21563125..40ef505b0 100644
--- a/tests/unit/test_proxy_utils.py
+++ b/tests/unit/test_proxy_utils.py
@@ -167,6 +167,27 @@ def test_path_containing_at_sign_is_not_over_redacted(monkeypatch):
     assert redact_proxy_credentials(text) == text
 
 
+def test_credentials_with_empty_username_are_redacted(monkeypatch):
+    """Userinfo with no username (``http://:secret@host``) still carries a
+    password. The generic pass must redact it even when the URL is not the
+    configured ``WEBSHARE_PROXY_URL`` (e.g. a different proxy variable)."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    secret = "empty-user-s3cret"
+    out = redact_proxy_credentials(
+        f"HTTP_PROXY=http://:{secret}@other.example.com:8080"
+    )
+    assert secret not in out
+    assert "***" in out
+
+
+def test_query_string_at_sign_is_not_over_redacted(monkeypatch):
+    """A query value containing ``@`` must not be mistaken for userinfo: ``?``
+    (and ``#``) are authority delimiters, so redaction must stop there."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    text = "fetched https://example.com?email=a@b.com ok"
+    assert redact_proxy_credentials(text) == text
+
+
 @pytest.mark.parametrize(
     "value",
     [None, 42, ValueError("boom"), ["a", "b"], {"k": "v"}],

From 14e2f29d49c374aadbd492d2955fc1e41f6ce3e8 Mon Sep 17 00:00:00 2001
From: Hayden <154503486+groupthinking@users.noreply.github.com>
Date: Thu, 30 Jul 2026 23:54:19 -0500
Subject: [PATCH 4/5] fix(security): make proxy redaction non-raising for
 hostile __str__

Review of #1113 surfaced that redact_proxy_credentials() called str(text)
outside any guard, contradicting its own "never raises" contract. The helper
is only ever invoked from except blocks, so an object whose __str__ raises
replaced the original error with a secondary traceback -- no log line, no
response body, and the operator loses the failure entirely.

- Guard stringification and the redaction passes independently in both the
  canonical helper and the shared/ duplicate.
- Fail closed: when redaction machinery itself fails, return a placeholder
  rather than the unvouched original, which could still carry the credential
  the function exists to strip.
- Fix three shared/libs/youtube_proxy.py call sites that passed str(error),
  stringifying outside the guard and defeating the protection.

Verification: 5 new regression tests fail against the pre-fix helper and pass
after; full tests/unit delta shows 0 regressions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 shared/libs/youtube_proxy.py         | 46 +++++++++++++------
 src/youtube_extension/utils/proxy.py | 33 ++++++++++----
 tests/unit/test_proxy_utils.py       | 68 ++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 23 deletions(-)

diff --git a/shared/libs/youtube_proxy.py b/shared/libs/youtube_proxy.py
index 96054d30b..2850ca849 100644
--- a/shared/libs/youtube_proxy.py
+++ b/shared/libs/youtube_proxy.py
@@ -54,6 +54,13 @@
     r"@"
 )
 
+# Returned when the input cannot be stringified, or when redaction itself
+# fails. Both are fail-closed: emitting a fixed placeholder is preferable to
+# raising (which would mask the original error) or to returning text we cannot
+# guarantee is clean.
+_UNPRINTABLE = "<unprintable error>"
+_REDACTION_FAILED = "<redaction failed>"
+
 
 def _get_webshare_proxy_url() -> str | None:
     """Return the validated WEBSHARE_PROXY_URL, or None for direct connection.
@@ -108,27 +115,38 @@ def _redact_proxy_credentials(text: Any) -> str:
     Delegates to the canonical helper when the ``youtube_extension`` package is
     importable; this module is also loaded standalone by path (see
     ``src/agents/mcp_enhanced_video_processor.py``), so an equivalent local
-    implementation is retained as a fallback. Never raises -- it is called from
-    exception handlers, where a failure would mask the original error.
+    implementation is retained as a fallback.
+
+    Never raises -- it is called from exception handlers, where a failure would
+    mask the original error. If ``text`` cannot be stringified or redaction
+    fails, a fixed non-sensitive placeholder is returned instead.
     """
     try:  # pragma: no cover - exercised only when the package is importable
         from youtube_extension.utils.proxy import (
             redact_proxy_credentials as _canonical,
         )
+    except ImportError:
+        _canonical = None  # type: ignore[assignment]
 
+    if _canonical is not None:  # pragma: no cover - see above
         return _canonical(text)
-    except ImportError:
-        pass
 
-    if not isinstance(text, str):
+    if isinstance(text, str):
+        candidate = text
+    else:
         try:
-            text = str(text)
-        except Exception:
-            # "Never raises" contract: this fallback runs inside exception
-            # handlers, so a failing __str__ must not propagate and mask the
-            # original error. Return a fixed, non-sensitive placeholder.
-            return "<unprintable error>"
+            candidate = str(text)
+        except Exception:  # noqa: BLE001 - a hostile __str__ must not propagate
+            return _UNPRINTABLE
+
+    try:
+        return _redact_local(candidate)
+    except Exception:  # noqa: BLE001 - never return text we cannot vouch for
+        return _REDACTION_FAILED
+
 
+def _redact_local(text: str) -> str:
+    """Run the two redaction passes over an already-stringified ``text``."""
     url = os.getenv("WEBSHARE_PROXY_URL", "").strip()
     if url and url in text:
         try:
@@ -399,20 +417,20 @@ async def _execute_with_retry(self, operation_func, operation_name: str, *args,
 
                 # Check if we should retry
                 if attempt > self.retry_config.max_retries:
-                    logger.error(f"❌ {operation_name} failed after {attempt-1} retries: {_redact_proxy_credentials(str(error))}")
+                    logger.error(f"❌ {operation_name} failed after {attempt-1} retries: {_redact_proxy_credentials(error)}")
                     self.circuit_breaker.record_failure()
                     self.consecutive_errors += 1
                     break
 
                 # Non-retryable errors
                 if error_type in [YouTubeErrorType.VIDEO_NOT_FOUND, YouTubeErrorType.PRIVATE_VIDEO]:
-                    logger.warning(f"⚠️ {operation_name} non-retryable error: {_redact_proxy_credentials(str(error))}")
+                    logger.warning(f"⚠️ {operation_name} non-retryable error: {_redact_proxy_credentials(error)}")
                     break
 
                 # Calculate retry delay
                 retry_delay = self._calculate_retry_delay(attempt, error_type)
 
-                logger.warning(f"⚠️ {operation_name} attempt {attempt} failed ({error_type.value}), retrying in {retry_delay:.2f}s: {_redact_proxy_credentials(str(error))}")
+                logger.warning(f"⚠️ {operation_name} attempt {attempt} failed ({error_type.value}), retrying in {retry_delay:.2f}s: {_redact_proxy_credentials(error)}")
 
                 self.stats["retries_executed"] += 1
                 await asyncio.sleep(retry_delay)
diff --git a/src/youtube_extension/utils/proxy.py b/src/youtube_extension/utils/proxy.py
index b2af39666..c1ae52ca4 100644
--- a/src/youtube_extension/utils/proxy.py
+++ b/src/youtube_extension/utils/proxy.py
@@ -46,6 +46,13 @@
 
 _REDACTED = "***"
 
+# Returned when the input cannot be stringified, or when redaction itself
+# fails. Both are fail-closed: emitting a fixed placeholder is preferable to
+# raising (which would mask the original error) or to returning text we cannot
+# guarantee is clean (which could leak the credential we are trying to strip).
+_UNPRINTABLE = "<unprintable error>"
+_REDACTION_FAILED = "<redaction failed>"
+
 
 def get_proxy_url() -> str | None:
     """Return the validated Webshare proxy URL, or None for direct connection.
@@ -111,18 +118,26 @@ def redact_proxy_credentials(text: Any) -> str:
        different proxy variable (``HTTPS_PROXY`` and friends) entirely.
 
     Always returns a string and never raises; it is called from exception
-    handlers, where a failure would mask the original error.
+    handlers, where a failure would mask the original error. If ``text`` cannot
+    be stringified (a ``__str__`` that itself raises) or redaction fails, a
+    fixed non-sensitive placeholder is returned instead.
     """
-    if not isinstance(text, str):
+    if isinstance(text, str):
+        candidate = text
+    else:
         try:
-            text = str(text)
-        except Exception:
-            # The contract is "never raises": an object whose __str__ fails
-            # here is called from an exception handler, so propagating would
-            # mask the original error. Fall back to a fixed, non-sensitive
-            # placeholder instead.
-            return "<unprintable error>"
+            candidate = str(text)
+        except Exception:  # noqa: BLE001 - a hostile __str__ must not propagate
+            return _UNPRINTABLE
+
+    try:
+        return _redact(candidate)
+    except Exception:  # noqa: BLE001 - never return text we cannot vouch for
+        return _REDACTION_FAILED
+
 
+def _redact(text: str) -> str:
+    """Run the two redaction passes over an already-stringified ``text``."""
     url = os.getenv(_PROXY_ENV_VAR, "").strip()
     if url and url in text:
         try:
diff --git a/tests/unit/test_proxy_utils.py b/tests/unit/test_proxy_utils.py
index 40ef505b0..47ad6122a 100644
--- a/tests/unit/test_proxy_utils.py
+++ b/tests/unit/test_proxy_utils.py
@@ -26,6 +26,7 @@
 if str(_SRC) not in sys.path:
     sys.path.insert(0, str(_SRC))
 
+from youtube_extension.utils import proxy as proxy_module  # noqa: E402
 from youtube_extension.utils.proxy import (  # noqa: E402
     _PROXY_ENV_VAR,
     get_proxy_dict,
@@ -245,3 +246,70 @@ def test_timeout_expired_argv_is_redacted(monkeypatch):
 
     assert PASSWORD in str(error), "precondition: the raw error does leak"
     _assert_no_secrets(redact_proxy_credentials(error))
+
+
+# ---------------------------------------------------------------------------
+# "Never raises" is a hard contract: the helper only ever runs inside an
+# ``except`` block, so if it raises it masks the original failure entirely --
+# no log line, no API response, just a different traceback.
+# ---------------------------------------------------------------------------
+
+
+class _HostileStr(Exception):
+    """An exception whose ``__str__`` raises.
+
+    Not hypothetical: third-party errors that format their message lazily can
+    fail this way (a missing interpolation key, a repr that touches a closed
+    resource), and they surface exactly where this helper is used.
+    """
+
+    def __str__(self):
+        raise RuntimeError("boom in __str__")
+
+
+class _HostileRepr:
+    def __str__(self):
+        raise ValueError("boom in __str__")
+
+    def __repr__(self):
+        raise ValueError("boom in __repr__")
+
+
+def test_unstringifiable_exception_does_not_propagate(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    result = redact_proxy_credentials(_HostileStr())
+    assert isinstance(result, str)
+    _assert_no_secrets(result)
+
+
+def test_unstringifiable_plain_object_does_not_propagate(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    assert isinstance(redact_proxy_credentials(_HostileRepr()), str)
+
+
+def test_unstringifiable_input_returns_placeholder_not_empty():
+    """A silent empty string would make the log line useless; the caller needs
+    to be able to tell *why* there is no detail."""
+    result = redact_proxy_credentials(_HostileStr())
+    assert result.strip(), "placeholder must be non-empty"
+    assert "boom" not in result, "must not smuggle the secondary failure through"
+
+
+def test_redaction_failure_returns_placeholder_not_raw_text(monkeypatch):
+    """If the redaction machinery itself fails we must fail *closed*: returning
+    the unredacted text would leak the credential this function exists to strip."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+
+    class _ExplodingPattern:
+        def sub(self, *_args, **_kwargs):
+            raise RuntimeError("regex exploded")
+
+    monkeypatch.setattr(proxy_module, "_USERINFO_RE", _ExplodingPattern())
+    result = redact_proxy_credentials(f"failed via {PROXY_URL}")
+    _assert_no_secrets(result)
+    assert "regex exploded" not in result
+
+
+def test_hostile_input_still_safe_when_no_proxy_configured(monkeypatch):
+    monkeypatch.delenv(_PROXY_ENV_VAR, raising=False)
+    assert isinstance(redact_proxy_credentials(_HostileStr()), str)

From 370f7cd3c84f9aaccb221125d9d033d545b3bfed Mon Sep 17 00:00:00 2001
From: Hayden <154503486+groupthinking@users.noreply.github.com>
Date: Fri, 31 Jul 2026 00:06:57 -0500
Subject: [PATCH 5/5] fix(proxy): redact passwords containing a literal @
 (#1113)

RFC 3986 requires "@" inside userinfo to be percent-encoded, but real
*_PROXY values are routinely set with a raw "@" in the password. Excluding
"@" from the user/password character classes made the match terminate at the
FIRST "@", so the password tail survived into the log line:

  in:  HTTPS_PROXY=http://user:pa@ss@proxy.internal:8080
  out: http://***:***@ss@proxy.internal:8080   <-- "ss" leaked

Permit "@" inside both classes. The classes remain bounded by the authority
delimiters (\s, /, ?, #), so greedy backtracking now settles on the LAST "@"
within a single authority rather than the first, while:

  - \s exclusion stops a match spanning two space-separated URLs;
  - /  exclusion stops a match spanning comma-separated URLs (the following
       "http://" contains "/") and preserves paths such as example.com/a@b;
  - ?/# exclusion preserves "@" in query strings and fragments.

The mandatory literal "@" before the host still prevents a bare "host:port"
from being mistaken for "user:password".

Applied to the canonical helper and to the drifted standalone copy in
shared/libs/ so the importlib fallback path is covered too.

Adds 5 regression tests; the 2 leak assertions fail against the previous
regex, the 3 over-redaction guards pin the preservation behaviour.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 shared/libs/youtube_proxy.py         | 19 +++++----
 src/youtube_extension/utils/proxy.py | 22 ++++++----
 tests/unit/test_proxy_utils.py       | 61 ++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 14 deletions(-)

diff --git a/shared/libs/youtube_proxy.py b/shared/libs/youtube_proxy.py
index 2850ca849..28dc6434c 100644
--- a/shared/libs/youtube_proxy.py
+++ b/shared/libs/youtube_proxy.py
@@ -42,15 +42,20 @@
 
 _ALLOWED_SCHEMES = ("http", "https", "socks5", "socks5h")
 
-# Matches the ``user[:password]@`` userinfo segment of any URL. The user and
-# password classes exclude "/", "?" and "#" so a path, query, or fragment
-# containing "@" is never mistaken for credentials, and the user may be empty so
-# credentials with no username ("http://:pass@host") are still redacted. Kept in
-# sync with youtube_extension.utils.proxy.
+# Matches the ``user[:password]@`` userinfo segment of any URL. The classes
+# exclude whitespace, "/", "?" and "#" -- the delimiters that end a URL
+# authority -- so a path, query, or fragment containing "@" is never mistaken
+# for credentials, and a match cannot span from one URL into the next. They
+# deliberately permit a literal "@": the classes are greedy, so the engine
+# backtracks to the LAST "@" in the authority and an unencoded "@" in the
+# password ("http://user:pa@ss@host") is consumed whole rather than leaving the
+# password tail behind. The user may be empty so credentials with no username
+# ("http://:pass@host") are still redacted. Kept in sync with
+# youtube_extension.utils.proxy.
 _USERINFO_RE = re.compile(
     r"(?P<scheme>[A-Za-z][A-Za-z0-9+.\-]*://)"
-    r"(?P<user>[^\s/:@?#]*)"
-    r"(?::(?P<password>[^\s/@?#]*))?"
+    r"(?P<user>[^\s/:?#]*)"
+    r"(?::(?P<password>[^\s/?#]*))?"
     r"@"
 )
 
diff --git a/src/youtube_extension/utils/proxy.py b/src/youtube_extension/utils/proxy.py
index c1ae52ca4..44382d147 100644
--- a/src/youtube_extension/utils/proxy.py
+++ b/src/youtube_extension/utils/proxy.py
@@ -31,16 +31,24 @@
 
 _ALLOWED_SCHEMES = ("http", "https", "socks5", "socks5h")
 
-# Matches the ``user[:password]@`` userinfo segment of any URL. The user and
-# password classes exclude "/", "?" and "#" so a path, query, or fragment
-# containing "@" (e.g. "https://example.com/a@b" or
-# "https://example.com?e=a@b") is never mistaken for credentials. The user is
-# allowed to be empty so credentials with no username (e.g. "http://:pass@host")
+# Matches the ``user[:password]@`` userinfo segment of any URL.
+#
+# The classes exclude whitespace, "/", "?" and "#" -- the delimiters that end a
+# URL authority -- so a path, query, or fragment containing "@" (e.g.
+# "https://example.com/a@b" or "https://example.com?e=a@b") is never mistaken
+# for credentials, and a match can never span from one URL into the next.
+#
+# They deliberately *permit* a literal "@". Because the classes are greedy, the
+# engine backtracks to the LAST "@" inside the authority, so an unencoded "@"
+# in the password ("http://user:pa@ss@host") is consumed whole instead of the
+# match stopping at the first separator and leaving the password tail behind.
+#
+# The user may be empty so credentials with no username ("http://:pass@host")
 # are still redacted.
 _USERINFO_RE = re.compile(
     r"(?P<scheme>[A-Za-z][A-Za-z0-9+.\-]*://)"
-    r"(?P<user>[^\s/:@?#]*)"
-    r"(?::(?P<password>[^\s/@?#]*))?"
+    r"(?P<user>[^\s/:?#]*)"
+    r"(?::(?P<password>[^\s/?#]*))?"
     r"@"
 )
 
diff --git a/tests/unit/test_proxy_utils.py b/tests/unit/test_proxy_utils.py
index 47ad6122a..9a5344e71 100644
--- a/tests/unit/test_proxy_utils.py
+++ b/tests/unit/test_proxy_utils.py
@@ -181,6 +181,67 @@ def test_credentials_with_empty_username_are_redacted(monkeypatch):
     assert "***" in out
 
 
+def test_unencoded_at_sign_in_password_is_fully_redacted(monkeypatch):
+    """RFC 3986 requires ``@`` in userinfo to be percent-encoded, but real
+    ``*_PROXY`` values are frequently set with a raw ``@`` in the password. The
+    userinfo classes must therefore permit ``@`` and rely on greedy backtracking
+    to the *last* separator, otherwise the match ends at the first ``@`` and the
+    password tail survives into the log line."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    secret = "pa@ss"
+    out = redact_proxy_credentials(
+        f"HTTPS_PROXY=http://user:{secret}@proxy.internal:8080 connection refused"
+    )
+    # The tail after the first "@" is the part that used to leak.
+    assert "ss@proxy.internal" not in out
+    assert secret not in out
+    assert "proxy.internal:8080" in out, "host:port must survive for triage"
+
+
+def test_multiple_at_signs_in_password_are_fully_redacted(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    secret = "se@cr@et"
+    out = redact_proxy_credentials(f"http://u:{secret}@h.example.com:1 failed")
+    assert secret not in out
+    for fragment in ("cr@et", "et@h.example.com"):
+        assert fragment not in out
+    assert "h.example.com:1" in out
+
+
+def test_at_sign_greed_does_not_span_two_urls(monkeypatch):
+    """Greedy userinfo matching must stay inside one authority. Whitespace and
+    ``/`` are excluded from the classes, so a match cannot run from the first
+    URL's userinfo to the second URL's ``@``."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    for text in (
+        "http://u1:p1@h1.example.com:80 and http://u2:p2@h2.example.com:80",
+        "http://u1:p1@h1.example.com:80,http://u2:p2@h2.example.com:80",
+    ):
+        out = redact_proxy_credentials(text)
+        for secret in ("p1", "p2", "u1", "u2"):
+            assert secret not in out, f"{secret!r} leaked from {text!r}: {out!r}"
+        assert "h1.example.com:80" in out
+        assert "h2.example.com:80" in out
+
+
+def test_fragment_at_sign_is_not_over_redacted(monkeypatch):
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    text = "opened https://example.com#tag@x done"
+    assert redact_proxy_credentials(text) == text
+
+
+def test_credential_free_url_with_port_is_untouched(monkeypatch):
+    """``host:port`` looks like ``user:password`` until the required ``@`` fails
+    to appear. Permitting ``@`` in the classes must not make these match."""
+    monkeypatch.setenv(_PROXY_ENV_VAR, PROXY_URL)
+    for text in (
+        "http://proxy.internal:8080/path",
+        "http://proxy.internal:8080",
+        "http://api.example.com contacted by user@corp.com",
+    ):
+        assert redact_proxy_credentials(text) == text
+
+
 def test_query_string_at_sign_is_not_over_redacted(monkeypatch):
     """A query value containing ``@`` must not be mistaken for userinfo: ``?``
     (and ``#``) are authority delimiters, so redaction must stop there."""