fix(security): stop proxy credentials leaking from subprocess/urlparse errors by groupthinking · Pull Request #1120 · groupthinking/EventRelay

groupthinking · 2026-07-31T04:54:13Z

Canonical issue

Outcome

Proxy credentials (WEBSHARE_PROXY_URL userinfo, and any scheme://user:pass@host URL) can no longer leak into logs or returned error fields when a subprocess or URL parse fails. This closes CWE-532 (log injection of secrets) and CWE-209 (secret disclosure via error responses) on the YouTube proxy path.

Scope

Included:
- src/youtube_extension/utils/proxy.py — get_proxy_url() now contains ValueError from urllib.parse (unterminated IPv6 literal, non-numeric / out-of-range port) instead of letting it escape to callers that log it; honours the documented "malformed ⇒ direct connection" contract. redact_proxy_credentials() rewritten as two passes: (1) exact replacement of the configured env URL preserving the host for triage, (2) a generic scheme://user:pass@ sweep that catches credentials that never match the env value verbatim (yt-dlp stderr echoes, CalledProcessError argv dumps, other proxy vars). Accepts non-str input and never raises.
- shared/libs/youtube_proxy.py — mirrors the same validation + redaction (this module is also loaded standalone by path, so it keeps an equivalent local fallback).
- enhanced_video_processor.py / adapters/robust.py — redact before logging/raising on Whisper CalledProcessError and yt-dlp TimeoutExpired / stderr.
- Regression tests: tests/unit/test_proxy_utils.py (new), plus additions to test_robust_youtube_service.py and test_enhanced_video_processor.py.
Explicitly excluded: no changes to proxy selection/rotation behaviour; no repo-wide lint cleanup (pre-existing ruff debt left untouched).

Risk

Risk level: low
Failure mode: over-redaction could obscure a non-credential @ in a log line; mitigated by anchoring the regex to a scheme:// prefix and excluding / from the user/password classes so paths containing @ are not matched.
Rollback: revert this branch; the prior behaviour returns (with the credential-leak bug).

Verification

Tied to head SHA 3076bad5f8e7dd25755a5e781213ebbf08cc7af5:

Focused tests — PYTHONPATH=src pytest tests/unit/test_proxy_utils.py tests/unit/test_robust_youtube_service.py tests/unit/test_enhanced_video_processor.py → 199 passed
Functional check — exact-URL redaction, generic userinfo sweep, non-str/argv (CalledProcessError) redaction, and malformed-URL→None contract all confirmed
Lint — ruff check on changed files shows no new errors vs the main baseline (new test file is clean; pre-existing counts unchanged)
Required CI — pending on GitHub (note: agent-completion/truth-gate requires the PR to leave draft state)
Review threads resolved — none yet (CodeRabbit is excluded by label config on this repo)

Production evidence

Not applicable — this is a backend credential-hygiene fix on the YouTube proxy error path with no user-facing surface or deployable artifact. Behaviour is exercised by the unit regression tests above.

Agent handoff

One canonical issue is linked (Prevent proxy credential leakage from urlparse errors #1113)
No competing PR implements the same issue (searched: no open/merged PR for this work)
Acceptance criteria are satisfied (parse errors no longer leak the URL; yt-dlp/Whisper failures redact credentials)
Required checks pass on the current head (blocked while draft; see note above)
Human decision is requested only for the final merge to the protected main branch

Agent provenance

Prepared by Claude Code under the PR remediation runbook. Kept as a draft pending human review and merge to the protected main branch.

Generated by Claude Code

…1113) `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>

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

vercel · 2026-07-31T04:54:20Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Ready	Preview, v0	Jul 31, 2026 4:55am

coderabbitai · 2026-07-31T04:54:22Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

[‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4a85bce5-77b8-4b57-a586-95c81af40848

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-31T04:54:25Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 3076bad.

Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

github-actions · 2026-07-31T04:54:27Z

🔍 PR Validation

⚠️ Large PR detected (506 lines changed)

github-actions · 2026-07-31T04:54:40Z

Agent Completion Truth Gate: BLOCKED

Reasons: invalid_payload

Machine-readable verdict

{
  "details": {
    "invalid_fields": [
      "policy.agent_login",
      "policy.run_id"
    ]
  },
  "reasons": [
    "invalid_payload"
  ],
  "verdict": "blocked"
}

Workflow evidence

groupthinking · 2026-07-31T04:57:17Z

Status & blocker (automated remediation run)

Tractable CI is green; the one red check is a by-design human governance gate — not a code issue.

Verification on head 3076bad:

✅ Focused tests — PYTHONPATH=src pytest tests/unit/test_proxy_utils.py tests/unit/test_robust_youtube_service.py tests/unit/test_enhanced_video_processor.py → 199 passed
✅ Functional checks — exact-URL redaction, generic scheme://user:pass@ sweep, non-str/argv (CalledProcessError) redaction, and malformed-URL→None contract all confirmed
✅ Lint — ruff on changed files shows no new errors vs the main baseline (new test file clean)
✅ Vercel — deployment Ready
✅ Dependency Review — no vulnerabilities / license / OpenSSF issues

Blocking check — agent-completion/truth-gate → invalid_payload (won't fix here):

This is not a defect in this PR. agent-completion-enforcement.yml only passes when a trusted GitHub App publishes an "Agent Lock trusted publication" check-run whose payload validates against .github/agent-lock/trusted-publishers.json. That file currently ships empty allowlists with custom_role_policy: "fail_closed", and its own note states the empty allowlist "intentionally blocks rather than downgrading agent work to not_applicable." So the gate fail-closes on every PR (the same invalid_payload / evidence failures appear on #831, #1049, etc.).

Clearing it is a deliberate human, protected-default-branch action — provisioning trusted_check_app_slugs / trusted_label_actors after verifying the independent App identity — and cannot be satisfied by pushing commits to this branch.

Requesting a human decision (product/security/irreversible-infra/production-approval, per the PR template): this PR is intentionally left as a draft. To land it, a maintainer needs to (1) provision the agent-lock trusted publisher (or grant the documented human exemption), (2) mark the PR ready-for-review, and (3) approve the merge to the protected main branch. Everything within automation's reach is done and green.

Generated by Claude Code

vercel

Additional Suggestion:

Debug logs in _transcript_operation interpolate raw exceptions that can embed the user:pass@ proxy URL, writing credentials to logs unredacted (CWE-532).

groupthinking · 2026-07-31T04:59:31Z

Closing as a duplicate of #1118, which is the older PR for #1113 and a strict superset of this one.

Containment proof

$ git merge-base --is-ancestor 3076bad5f 14e2f29d4   # exit 0
$ git log --oneline 14e2f29d4..3076bad5f             # commits unique to #1120
(none)
$ git log --oneline 3076bad5f..14e2f29d4             # commits unique to #1118
14e2f29d4 fix(security): make proxy redaction non-raising for hostile __str__
3c80dd4be fix(security): harden userinfo regex against empty-user leak and query over-redaction

This PR's head 3076bad5f is an ancestor of #1118's head 14e2f29d4, so no work is lost — both of the commits authored on this branch (3076bad5f guard-str(), 3c80dd4be regex hardening) are already in #1118 with authorship preserved.

Why this had to be resolved

.github/workflows/pr-governance.yml enforces one open implementation PR per canonical issue. With both open, the Canonical issue and evidence check failed on #1118 with:

Issue #1113 already has another open implementation PR: #1120

Keeping the superset and closing this one clears that gate.

What #1118 adds on top

The 3076bad5f guard covered str(text) inside the helper, but three call sites in shared/libs/youtube_proxy.py (lines 418/425/431) passed _redact_proxy_credentials(str(error)) — stringifying outside the guard, so a hostile __str__ still raised before the helper was entered. #1118 fixes those call sites, additionally guards the redaction transformation itself (fail-closed, so a redaction failure returns a placeholder rather than unvouched text), and adds 5 further regression tests (verified failing pre-fix).

Please continue any follow-up review on #1118.

groupthinking · 2026-07-31T04:59:32Z

Superseded by #1118 (strict superset, contains this branch's commits). See containment proof above.

groupthinking · 2026-07-31T05:00:47Z

Closing as superseded by #1118.

Both PRs close #1113 with the same proxy-credential-redaction fix. #1118's head (3c80dd4) is a direct child of this PR's head (3076bad) — i.e. #1118 contains every commit here plus the follow-up hardening commit (empty-user regex leak + query over-redaction). #1118 is out of draft, has completed a full CodeRabbit review (both findings verified addressed), and is the canonical survivor.

Keeping both open trips the "Canonical issue and evidence" gate on #1118 ("Issue #1113 already has another open implementation PR: #1120"). Closing this one clears that gate. No work is lost — it all lives in #1118.

Generated by Claude Code

groupthinking and others added 2 commits July 30, 2026 23:35

github-actions Bot added the python label Jul 31, 2026

groupthinking added bug Something isn't working copilot-rabbit security tests labels Jul 31, 2026 — with Claude

vercel Bot reviewed Jul 31, 2026

View reviewed changes

groupthinking closed this Jul 31, 2026

groupthinking mentioned this pull request Jul 31, 2026

fix(security): stop proxy credentials leaking from subprocess errors #1118

Closed

10 tasks

groupthinking mentioned this pull request Jul 31, 2026

[PROGRAM][OWNER:Codex] Autonomous EventRelay delivery control plane #898

Open

Navigation Menu

fix(security): stop proxy credentials leaking from subprocess/urlparse errors - #1120

fix(security): stop proxy credentials leaking from subprocess/urlparse errors#1120
groupthinking wants to merge 2 commits into
mainfrom
claude/determined-maxwell-sdzggo

groupthinking commented Jul 31, 2026

Uh oh!

vercel Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

groupthinking commented Jul 31, 2026

Uh oh!

vercel Bot left a comment

Uh oh!

groupthinking commented Jul 31, 2026

Uh oh!

groupthinking commented Jul 31, 2026

Uh oh!

groupthinking commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

groupthinking commented Jul 31, 2026

Canonical issue

Outcome

Scope

Risk

Verification

Production evidence

Agent handoff

Agent provenance

Uh oh!

vercel Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Jul 31, 2026

Dependency Review

Snapshot Warnings

Scanned Files

Uh oh!

github-actions Bot commented Jul 31, 2026

🔍 PR Validation

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Agent Completion Truth Gate: BLOCKED

Uh oh!

groupthinking commented Jul 31, 2026

Status & blocker (automated remediation run)

Uh oh!

vercel Bot left a comment

Choose a reason for hiding this comment

Uh oh!

groupthinking commented Jul 31, 2026

Containment proof

Why this had to be resolved

What #1118 adds on top

Uh oh!

groupthinking commented Jul 31, 2026

Uh oh!

groupthinking commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented Jul 31, 2026 •

edited

Loading

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

github-actions Bot commented Jul 31, 2026 •

edited

Loading