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/2] 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[A-Za-z][A-Za-z0-9+.\-]*://)" + r"(?P[^\s/:@]+)" + r"(?::(?P[^\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 = "" - 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 = "" + 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 ". + # 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 "; 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[A-Za-z][A-Za-z0-9+.\-]*://)" + r"(?P[^\s/:@]+)" + r"(?::(?P[^\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 = "" - 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 = "" + 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 ``. 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 Date: Fri, 31 Jul 2026 04:48:27 +0000 Subject: [PATCH 2/2] 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 "" 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) 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 "" 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 "" 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 == "" + _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."""