fix(security): SSRF-guard save_url_video redirects

[zapabob]

Summary

• Apply is_safe_url and manual redirect re-validation to save_url_video (sibling of the image path).
• Prevent cloud-metadata / private-target pivots via redirected CDN URLs when downloading generated video assets.
• Add focused regression tests.

Salvage / credit

Sibling coverage for the incomplete image-side work tracked around #44743 / #44728 (video download path was still unprotected).

Test plan

• pytest tests/agent/test_save_url_video_ssrf.py -q

[teknium1]

Thanks for covering the currently unguarded video delivery-URL path: current main still calls raw requests.get() in agent/video_gen_provider.py:274.

Problems

• agent/video_gen_provider.py:291 still uses requests.get() after a DNS preflight. tools/url_safety.py:15-21 documents that this leaves a DNS-rebinding TOCTOU window; direct host-side fetches need create_ssrf_safe_client() for connect-time validation.
• agent/video_gen_provider.py:294 only handles a redirect when Location is present. A 3xx response without that header falls through to response.raise_for_status() and then the body-write path at lines 312-345 instead of failing closed.

Suggested changes

• Use create_ssrf_safe_client(follow_redirects=False) with the explicit per-hop validation loop.
• Explicitly reject every 3xx response lacking Location, and cover it with a regression test plus a real local redirect-chain test.

Automated hermes-sweeper review.

[teknium1]

is_safe_url() is only a preflight DNS check; this raw requests.get() resolves and connects independently. tools/url_safety.py:15-21 documents this DNS-rebinding TOCTOU and provides create_ssrf_safe_client() for connect-time validation. Please use that client here.

[teknium1]

Please detect every 3xx response explicitly and reject it when Location is absent. This condition treats a malformed 3xx-without-Location response as final, so it reaches raise_for_status() and then the download body path instead of failing closed.

[egilewski]

suggesting changes

The redirect checks prevent literal private and metadata URLs, but DNS rebinding can bypass them. A provider-controlled delivery hostname can resolve to a public address during validation and then resolve to an internal address when the download connects, allowing SSRF. Route every hop through a connect-time SSRF-safe transport (or equivalent DNS pinning), preserve the request hostname for Host/SNI, keep automatic redirects disabled with per-hop checks, and add a regression test that changes DNS between validation and connection and confirms that no private connection occurs.

Security evidence:

• trust boundary: Provider-supplied video URLs and redirect targets reach host-side video downloads and are untrusted.
• source/sink/invariant: save_url_video checks the initial URL and each redirect target, but the download connection performs a separate unpinned DNS resolution, so the no-private-connect invariant is incomplete.
• current-main reproduction: The pre-change implementation allowed a loopback URL to reach the download call.
• PR-head or patch-replay validation: The change adds initial and per-redirect checks while leaving the connection unpinned.
• positive/negative cases: Literal private URLs and redirect pivots are rejected, and public downloads succeed; DNS rebinding is not covered.
• residual bypass search: The provider download flow and shared URL-safety logic were traced; connect-time pinning is unused here.
• reviewer validation: Source inspection and focused security tests support this actionable SSRF finding.

Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub main because the submitted branch is stale or conflicted; this does not mean the submitted branch itself merges cleanly.

Not checked:

• Ruff validation
• Connect-time DNS rebinding execution
• Broader URL-safety suite
• CodeRabbit review

Signed: GPT-5.6-luna-max in Codex

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(security): SSRF-guard save_url_video redirects

1. A 3xx response without a usable Location header falls through as a "final" response: is_redirect is True but the Location truthiness check skips the hop, break exits the loop, and raise_for_status() does not raise on 3xx. The response is then treated as a successful video (content-type sniffing on the redirect page → confusing "unsupported content type" or a bogus file with a .None extension). Consider explicitly rejecting redirects that cannot be followed (missing/empty Location) instead of treating them as terminal.
2. Residual TOCTOU/DNS-rebinding risk (same posture as save_url_image / [Security] xAI image generation provider performs an unguarded host-side fetch of provider-controlled image URLs #44728, so consistent): is_safe_url validates the URL string, but the TCP connection happens afterwards, so a backend-controlled hostname could resolve to a loopback/cloud-metadata address at connect time. Fine to accept as residual risk, but worth a code comment stating the assumption.
3. Minor positive: wrapping the streaming download in try/finally: response.close() also fixes a pre-existing connection leak on the max_bytes cap path — good catch.

[zapabob]

Refreshed against the fixed official main ancestor 8e475ed27b1199b8d0bbf094cf2e15fcd555f8cf and pushed d55c0f327b31a5ef9b6023bc82cf83aa54d65ab3 with an exact lease. The SSRF guard validates the provider URL and every manual redirect, bounds redirect hops, streams within the size cap, and closes responses. Focused validation: tests/agent/test_save_url_video_ssrf.py — 4 passed; Python compile and git diff --check also passed.

[zapabob]

Current-main refresh: rebased this PR onto official upstream/main 7a54ab2. Published head: f7c8614. In the isolated worktree, git merge-base --is-ancestor and git diff --check both pass. This comment records the rebase/whitespace gates only; required GitHub checks remain subject to the new head.