fix(security): pin cloud callbacks against DNS rebinding

[groupthinking]

Canonical issue

Closes #911. Progresses #898.

Outcome

Prevent Cloud Task callback SSRF, DNS rebinding, non-public destination bypasses, redirect pivots, unbounded callback retries, and log forging while preserving legitimate public HTTP(S) callbacks.

Scope

• src/youtube_extension/backend/cloud_api_endpoints.py
• tests/unit/test_cloud_routes.py

Security invariant

Resolve once off-loop, reject the complete DNS answer set when any address is non-global, connect only to validated numeric addresses, attempt at most three addresses under one bounded deadline, and never retry a POST that may have been transmitted. Preserve Host, port, TLS SNI, hostname certificate verification, redirects-disabled behavior, and async I/O.

Risk

The callback dispatcher now fails closed for mixed public/private DNS, CGNAT, site-local IPv6, resolution failures, and exhausted/deadline-bounded attempts. Exception values written to logs are CR/LF-sanitized; client-facing 500 details remain static.

Verification

• Exact head: 115088c13fd518c1245cbecfd7f3e9a8a681d1d0.
• Final focused suite: 122 passed; py_compile and git diff --check passed.
• CI 29817969888, Coverage 29817969877, CodeQL 29817969870, Security Scan 29817969836, Secret Scan 29817969826, and Dependency Review 29817969916 passed; E2E 29817969857 was repository-skipped.
• CodeRabbit approved the exact head at 2026-07-21T09:27:35Z.
• All verified findings are resolved: 0 unresolved review threads.

Production evidence

• Exact-head Vercel deployment dpl_4KK3dfL3RDqMAeojkhjRDnFpCqFk was canceled before build because the controller-authored commit is unverified by Vercel's commit policy.
• Prior implementation head 4bf4e29c5dca31ca2ae027d9d6db993384c8679b produced verified READY Preview dpl_3gqvTdCTMDY4iHmaASi1RXjWRN78.
• Vercel builds Next.js apps/web; this Python-only path is deployment-non-applicable. Production remains READY on main@2d660c7e5382db1df5c5eba39537aa9d034376bb with no runtime errors in the preceding 24 hours.

Proven provenance blocker

PR #734 was created before focused issue #911. Trusted-main requires a frozen intent snapshot before PR creation and a terminal result authored by declared login claude; neither can be retroactively manufactured by the connected groupthinking controller. Keep draft pending an authorized legacy disposition.

Draft exit checklist

• One focused child issue and one canonical PR.
• Current-main implementation and complete security fix.
• Exact-head tests and GitHub workflows.
• Current-head automated approval and zero unresolved findings.
• Deployment applicability classified.
• Authorized historical-provenance disposition and trusted terminal agent result.
• Final human review.

Execution receipt

• Declared agent: claude / session_018e5kEDnS1n4MKvoT6eQ6T4
• External heartbeat/head: 2026-07-21T09:12:31Z / 4bf4e29c5dca31ca2ae027d9d6db993384c8679b
• Controller login/run: groupthinking / eventrelay-blocker-watch-20260721-0912z
• Canonical branch/PR: claude/determined-maxwell-j5mdyu / fix(security): pin cloud callbacks against DNS rebinding #734
• Claimed timestamp: 2026-07-21T09:12:50Z
• Latest verified heartbeat: 2026-07-21T09:27:35Z
• Exact head: 115088c13fd518c1245cbecfd7f3e9a8a681d1d0

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Ready	Preview, v0	Aug 2, 2026 1:33pm

[coderabbitai]

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 Plus

Run ID: 88defc6d-941d-4c20-8f0f-96bda1cc0af2

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/determined-maxwell-j5mdyu

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/determined-maxwell-j5mdyu

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

Dependency Review

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

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 7dc9cbe.

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

[Copilot]

Pull request overview

Refactors cloud endpoints to a module-level router and addresses CodeQL findings.

Changes:

• Adds callback URL and log sanitization.
• Replaces exposed exception details with generic errors.
• Preserves setup_cloud_api_endpoints compatibility.

[devin-ai-integration]

Devin Review found 4 potential issues.

[groupthinking]

Status — one decision needed to unblock this PR

Consolidating the CodeQL, Vercel, Copilot, and Devin reviews rather than replying thread-by-thread, since they all converge on the same root issue.

What's actually going on

• The critical SSRF (server POSTs to a user-supplied callback_url) and the log-injection findings pre-exist on main — the 🧹 Extract cloud api endpoints out of setup_cloud_api_endpoints into router #733 refactor only relocated the code, so CodeQL re-attributed the alerts to these new line numbers.
• My _is_safe_callback_url guard is a partial mitigation, and the reviewers are right that it's bypassable multiple ways: obfuscated-IP encodings, trailing-dot hostnames (metadata.google.internal.), and DNS-rebinding hostnames (127.0.0.1.nip.io). Blocklist patching is whack-a-mole — the consensus is that the approach must change, not that one more case needs blocking.
• Important: even a fully-correct guard will not turn the CodeQL check green. CodeQL doesn't recognize custom validators as SSRF/log-injection sanitizers, so the required check stays red until these alerts are either designed out or dismissed.

Your call — pick one and I'll implement it in a single commit (chosen guard + the parameterized allow/deny and CR/LF regression tests Copilot asked for):

1. Trusted-host allowlist — give me the legitimate callback hosts. Strongest and simplest, and most likely to satisfy CodeQL because user input no longer freely controls the destination.
2. Resolve-and-pin — resolve every A/AAAA, reject non-global addresses, pin the IP at connect time, disable redirects. General-purpose (no host list needed) but heavier, and likely still CodeQL-red.
3. Drop the user-controlled callback feature — removes the sink entirely.
4. Dismiss the pre-existing alerts as accepted-risk in the code-scanning UI to unblock the refactor as-is.

Until you choose, I won't push speculative security changes or merge to main. The rest of the PR — the refactor plus the stack-trace-exposure fixes — is committed and green, with all 81 tests in tests/unit/test_cloud_routes.py passing.

---

Generated by Claude Code

[groupthinking]

Update — two changes since my last comment:

1. Regression tests added (920c397) for the now-settled resolve-and-check guard: _sanitize_log_value CR/LF stripping; _is_safe_callback_url literal cases (public allowed; loopback/private/link-local/metadata/blocklisted-host/non-http/malformed rejected) and resolution cases via mocked getaddrinfo (obfuscated-IP and DNS-alias-to-internal rejected, mixed A-records fail closed, unresolvable rejected); plus end-to-end proof the task handler never constructs the HTTP client for an unsafe callback. 105 passed locally (was 81); CI re-running.

2. CodeQL is confirmed unable to go green here. It re-scanned the resolve-and-check fix (53f5ce7) and reported the same 3 alerts (1 critical SSRF + 2 log-injection). CodeQL doesn't model custom validators as sanitizers, so the required check stays red no matter how the guard is written — and these alerts pre-exist on main.

So the callback SSRF is now genuinely well-mitigated (resolution-based, fail-closed, tested) and the log values are sanitized. The only remaining step to merge is a human dismissal of the 3 pre-existing CodeQL alerts as accepted-risk in the code-scanning UI (or accepting the red check). I won't merge to main. Optional non-blocking follow-ups if you want them: run getaddrinfo off the event loop (asyncio.to_thread) and pin the resolved IP at connect to close the residual TOCTOU — say the word and I'll add them.

---

Generated by Claude Code

[github-actions]

Agent Completion Truth Gate: BLOCKED

Reasons: evidence_collection_failed, missing_agent_result, missing_copilot_current_head_review, draft_pr

Machine-readable verdict

{
  "details": {
    "collection_errors": [
      "missing_intent_snapshot"
    ],
    "identity_projection": {
      "agent_login": "claude",
      "issue_number": 911,
      "run_id": "session_018e5kEDnS1n4MKvoT6eQ6T4"
    }
  },
  "reasons": [
    "evidence_collection_failed",
    "missing_agent_result",
    "missing_copilot_current_head_review",
    "draft_pr"
  ],
  "verdict": "blocked"
}

Workflow evidence

[groupthinking]

Returned to draft because unique SSRF/log-sanitization scope remains, but this implementation is unsafe and incomplete.

• Exact head 920c39712f53d8a6fe8902ad6dd6e5af4cd32c6b is 363 commits behind and unmergeable.
• The router-refactor and response-leak subsets are already covered by 🧹 Extract cloud api endpoints out of setup_cloud_api_endpoints into router #733/fix(security): restore CWE-209 response protections #831.
• Current main still lacks safe callback and log sanitization, but this head has nine unresolved current threads, including CodeQL findings.

Next action: reduce this same canonical branch to only the still-live SSRF/log scope on current main, resolve every active finding, and run fresh exact-head checks. Do not merge the current head or open a competitor.

[groupthinking]

Done as directed — reduced this same branch to only the still-live SSRF/log scope, rebased on current main (force-pushed; head is now f30f654 = main + one commit). The router-refactor and exception-exposure subsets were dropped since they're already on main via #733/#831. Prior review threads were anchored to the old head and are now outdated by the rewrite.

What the single commit does (src/youtube_extension/backend/cloud_api_endpoints.py):

• Callback SSRF — _is_safe_callback_url(): requires http(s)+hostname; rejects blocklisted internal hosts (trailing-dot and case normalized, closing the metadata.google.internal. / LOCALHOST bypasses); rejects loopback/private/link-local/reserved/multicast/unspecified IP literals; and resolves via DNS, rejecting if any resolved address is blocked — defeating obfuscated IPv4 encodings (2130706433, 0x7f.0.0.1) and DNS aliases that map to internal IPs (127.0.0.1.nip.io). Validated cheaply at request acceptance (400 on a bad callback_url) and fully — DNS, off the event loop via asyncio.to_thread — right before dispatch; the httpx client sets follow_redirects=False.
• Log injection — _sanitize_log_value() strips CR/LF from user-controlled values (video_url, task name, video_id, callback_url) with lazy %-logging.

Tests (tests/unit/test_cloud_routes.py): guard literal + DNS-resolution paths (obfuscated / rebinding / trailing-dot / mixed-A-record fail-closed / unresolvable), plus end-to-end (400 at acceptance; HTTP client never constructed for an unsafe callback; safe callback POSTs with redirects disabled). 116 passed; mypy clean on the new code.

Two honest caveats (both need your call, not more code):

1. Residual DNS-rebinding TOCTOU — because httpx re-resolves at connect, full closure needs transport-level pinning of the validated IP. Say the word and I'll add a pinned transport.
2. CodeQL will very likely stay red on the SSRF/log-injection alerts regardless — it doesn't model custom validators as sanitizers (confirmed repeatedly on the prior head). If the fresh scan still flags them, they'll need dismissing as accepted-risk. Separately, the agent-completion/truth-gate check is failing on its own invalid_payload (missing policy.agent_login/run_id) — that's external gate tooling, not fixable from this PR.

Left as a draft per your note. I won't merge.

---

Generated by Claude Code

[groupthinking]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

[Copilot]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, a...

The address-attempt cap and overall deadline were already applied in commit 4bf4e29:

• Loop iterates over callback_addresses[:_MAX_CALLBACK_ADDRESS_ATTEMPTS] (max 3 addresses)
• deadline = loop.time() + _CALLBACK_TOTAL_TIMEOUT (15 s) is computed once before the loop
• Each attempt uses timeout=min(_CALLBACK_ATTEMPT_TIMEOUT, remaining) so neither a single attempt nor the full loop can exceed the deadline
• if remaining <= 0: break exits early when the deadline is already past

test_task_handler_caps_black_holing_address_attempts (line 1711 of test_cloud_routes.py) supplies 5 black-holing public addresses and asserts exactly _MAX_CALLBACK_ADDRESS_ATTEMPTS (3) POST calls are made. All 122 tests pass.

[groupthinking]

Security review: correct — merge after rebase, no code changes needed

Vulnerability confirmed live on main and directly reachable. cloud_api_endpoints.py:181-194 does a raw httpx.AsyncClient().post(payload.callback_url, ...) with zero validation. The only guard is presence of an X-CloudTasks-TaskName header (:159) — which any client can set. This is reachable SSRF, not theoretical, and :194 logs the URL back.

The fix gets the hard part right. Most SSRF fixes validate the hostname and then hand the hostname to the HTTP client, which re-resolves it — leaving the TOCTOU window fully open. This one returns the numeric addresses from _validated_callback_addresses() and pins via pinned_url = callback_url.copy_with(host=address), so the socket targets the exact validated IP. follow_redirects=False blocks the redirect pivot.

Verified the claim I was most skeptical of: TLS verification still targets the hostname. extensions={"sni_hostname": callback_url.host} is consumed by httpcore as server_hostname, driving both SNI and certificate hostname verification. Connecting by IP does not downgrade cert validation. headers={"Host": host_header} preserves routing.

Also correct:

• _is_blocked_ip() uses not ip.is_global (allowlist-shaped) rather than enumerating ranges — catches CGNAT 100.64.0.0/10 and IPv6 fec0::/10.
• Host blocklist normalizes .rstrip(".").lower() — defeats trailing-dot and case bypass.
• parsed.port is inside the try/except ValueError.
• Resolution runs via asyncio.to_thread.
• Rejecting the entire answer set if any address is non-global is the right call.

Completeness verified: callback_url appears in three source files; cloud_tasks_queue.py:75 and cloud_video_processor.py:78 only carry the value. The sole outbound dispatch is the one fixed here.

No regressions. Retries limited to ConnectError/ConnectTimeout (request not yet transmitted) — no duplicate POST.

Blocking

Rebase only — CONFLICTING with main, and still draft behind the provenance gate. No code changes required. Not obsolete.

[groupthinking]

Rebased onto current main (7fcd447) as requested — no code changes to the fix, force-pushed (115088c → 7dc9cbe), all four commits preserved with original authorship.

The only conflicts were import-block housekeeping (main's Dict→dict/List→list modernization and unused-import trim vs. this branch adding ipaddress/socket/urllib.parse/Union). Resolved by taking main's cleanup and keeping the imports the fix needs — notably asyncio, which main had dropped as unused but the pinned-callback path uses via asyncio.to_thread. The callback/logging logic and tests carried over untouched.

Verified post-rebase: no conflict markers, module imports cleanly, 122/122 tests pass in tests/unit/test_cloud_routes.py.

That clears the CONFLICTING blocker. Remaining gating is yours: the draft state and the agent-completion provenance gate. I won't mark ready or merge.

---

Generated by Claude Code