security: harden screenshot tool against SSRF via redirect chains#274
Closed
security: harden screenshot tool against SSRF via redirect chains#274
Conversation
Add preflight HTTP HEAD request with per-hop redirect validation before launching headless browser. Reuses the same SSRF protections (blocked host check, DNS resolution pinning, scheme validation, redirect limit) already applied by web_fetch and http_request tools. Closes #271 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds hop-by-hop and post-navigation SSRF redirect validation to the web screenshot flow, introduces request interception and paused-request handling to block unsafe navigations, exposes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ScreenshotTool
participant PreflightHTTP
participant Validator
participant BrowserPage
participant Screenshot
Client->>ScreenshotTool: request screenshot(url)
ScreenshotTool->>PreflightHTTP: preflight (follow redirects, per-hop)
PreflightHTTP->>Validator: validate redirect target (each hop)
Validator-->>PreflightHTTP: ok / reject
PreflightHTTP-->>ScreenshotTool: preflight result
alt preflight ok
ScreenshotTool->>BrowserPage: navigate(original url) (interception enabled)
BrowserPage->>ScreenshotTool: navigation paused (on redirect / request)
ScreenshotTool->>Validator: validate paused request target
Validator-->>ScreenshotTool: ok / block
alt allowed
BrowserPage-->>ScreenshotTool: final_url after redirects
ScreenshotTool->>Validator: validate_redirect_target(final_url)
Validator-->>ScreenshotTool: ok / reject
alt final ok
ScreenshotTool->>Screenshot: capture page
Screenshot-->>Client: image
else rejected
ScreenshotTool-->>Client: error (SSRF final-url)
end
else blocked
ScreenshotTool-->>Client: error (SSRF blocked during navigation)
end
else rejected
ScreenshotTool-->>Client: error (preflight failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/screenshot.rs`:
- Around line 203-223: The preflight redirect-validation block that builds
preflight_client (using validate_redirect_target_for_policy and
MAX_SCREENSHOT_REDIRECTS) must be executed before starting the browser and
before spawning the handler task; move the entire preflight_client creation and
its request logic to occur prior to the browser launch and handler spawn so
failures return fast and avoid paying Chromium startup cost or leaving the
handler detached; ensure error mapping (ZeptoError::Tool) remains the same and
keep the redirect policy logic intact when relocating the block.
- Around line 224-237: The preflight currently uses HEAD which can differ from
browser GET behavior; change the preflight request in the screenshot flow to use
GET instead of HEAD so SSRF validation observes the same redirect/response path
as the browser. Locate the call that builds and sends the preflight request (the
preflight_client.head(url_str).send().await map_err block) and replace head(...)
with get(...), preserving the existing error mapping (checking e.is_redirect()
to return ZeptoError::SecurityViolation or ZeptoError::Tool) and keeping the
subsequent validate_redirect_target(preflight_resp.url()).await? call intact.
Comment on lines
+203
to
+223
| // ---- Preflight: follow redirects with SSRF validation ---- | ||
| // Before launching the browser, do an HTTP HEAD request to discover | ||
| // the final destination and validate each redirect hop. This catches | ||
| // SSRF via redirect chains before the browser fetches any content. | ||
| let preflight_client = reqwest::Client::builder() | ||
| .redirect(reqwest::redirect::Policy::custom(|attempt| { | ||
| if attempt.previous().len() >= MAX_SCREENSHOT_REDIRECTS { | ||
| return attempt.error(format!( | ||
| "Too many redirects (max {})", | ||
| MAX_SCREENSHOT_REDIRECTS | ||
| )); | ||
| } | ||
| match super::web::validate_redirect_target_for_policy(attempt.url()) { | ||
| Ok(()) => attempt.follow(), | ||
| Err(e) => attempt.error(e), | ||
| } | ||
| })) | ||
| .timeout(Duration::from_secs(10)) | ||
| .build() | ||
| .map_err(|e| ZeptoError::Tool(format!("Failed to build preflight client: {}", e)))?; | ||
|
|
There was a problem hiding this comment.
Run preflight before launching Chromium to fail fast.
Line 192 launches the browser and Line 197 spawns the handler before preflight starts at Line 203. On preflight failure, you still pay browser startup cost and can leave the handler task detached until shutdown. Move the entire preflight block before browser launch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/screenshot.rs` around lines 203 - 223, The preflight
redirect-validation block that builds preflight_client (using
validate_redirect_target_for_policy and MAX_SCREENSHOT_REDIRECTS) must be
executed before starting the browser and before spawning the handler task; move
the entire preflight_client creation and its request logic to occur prior to the
browser launch and handler spawn so failures return fast and avoid paying
Chromium startup cost or leaving the handler detached; ensure error mapping
(ZeptoError::Tool) remains the same and keep the redirect policy logic intact
when relocating the block.
Comment on lines
+224
to
+237
| let preflight_resp = preflight_client.head(url_str).send().await.map_err(|e| { | ||
| if e.is_redirect() { | ||
| ZeptoError::SecurityViolation(format!( | ||
| "Screenshot blocked: redirect chain failed SSRF validation: {}", | ||
| e | ||
| )) | ||
| } else { | ||
| ZeptoError::Tool(format!("Screenshot preflight request failed: {}", e)) | ||
| } | ||
| })?; | ||
|
|
||
| // Validate the final destination after all redirects. | ||
| validate_redirect_target(preflight_resp.url()).await?; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the context around lines 224-241 in screenshot.rs
sed -n '210,250p' src/tools/screenshot.rs
echo "---"
# Search for redirect policy configuration
rg -n 'redirect' src/tools/screenshot.rs | head -20Repository: qhkm/zeptoclaw
Length of output: 3225
🏁 Script executed:
#!/bin/bash
# Check the browser configuration and if it has redirect policy
rg -n 'browser' src/tools/screenshot.rs -B2 -A5 | head -60
echo "---"
# Look for browser creation/setup
rg -n 'launch\|new_browser' src/tools/screenshot.rs -B3 -A3
echo "---"
# Check if new_page has any redirect configuration
rg -n 'new_page' src/tools/screenshot.rs -B5 -A5Repository: qhkm/zeptoclaw
Length of output: 3171
🏁 Script executed:
#!/bin/bash
# Check if browser or page has any redirect policy or validation
rg -n 'redirect' src/tools/screenshot.rs -A3 -B3
echo "---"
# Look at the browser config setup to see if there's any redirect handling
sed -n '176,202p' src/tools/screenshot.rs
echo "---"
# Check what validate_redirect_target and validate_redirect_target_for_policy do
rg -n 'validate_redirect_target' src/tools/web.rs -B2 -A8 | head -80Repository: qhkm/zeptoclaw
Length of output: 10975
🏁 Script executed:
#!/bin/bash
# Check if there's any redirect handling or interception on the browser page
rg -n 'on_request\|interceptRequest\|setRequestInterception' src/tools/screenshot.rs
echo "---"
# Look for any event handlers on the page
rg -n 'page\.' src/tools/screenshot.rs | grep -i 'on\|intercept\|request'
echo "---"
# Check documentation comments about the browser navigation
sed -n '203,250p' src/tools/screenshot.rsRepository: qhkm/zeptoclaw
Length of output: 2329
🏁 Script executed:
#!/bin/bash
# Check chromiumoxide library to understand its redirect behavior
# Look for any documentation or comments about how new_page handles redirects
rg -n 'new_page' src/tools/screenshot.rs -B10 -A10
echo "---"
# Verify if there's any way the browser's redirect chain is validated
rg -n 'navigate\|goto' src/tools/screenshot.rs
echo "---"
# Check if there are any tests that demonstrate the redirect behavior
rg -n 'test.*redirect' src/tools/screenshot.rs -A10 | head -50Repository: qhkm/zeptoclaw
Length of output: 3039
🏁 Script executed:
#!/bin/bash
# Check if chromiumoxide or the page object has any redirect interception
# Search for any event listeners or navigation handlers
rg -n 'eventListener\|addEventListener\|on_request\|intercept' src/tools/
echo "---"
# Look at the entire execute function to see if there's any protection after navigation
sed -n '120,270p' src/tools/screenshot.rs | cat -n
echo "---"
# Check the exact flow - does the browser navigate and then we validate, or vice versa?
sed -n '235,265p' src/tools/screenshot.rsRepository: qhkm/zeptoclaw
Length of output: 8076
🏁 Script executed:
#!/bin/bash
# Check chromiumoxide's behavior - does new_page follow redirects?
# Search for any documentation in the repo about chromiumoxide usage
rg -n 'chromiumoxide\|new_page' src/tools/screenshot.rs -B3 -A3
echo "---"
# Check if there's validation AFTER navigation that we might have missed
rg -n 'page\.goto\|page\.navigate\|currentUrl\|getUrl' src/tools/screenshot.rs
echo "---"
# Verify: after new_page, is there any validation of the FINAL URL the browser landed on?
sed -n '240,260p' src/tools/screenshot.rs | cat -nRepository: qhkm/zeptoclaw
Length of output: 942
🏁 Script executed:
#!/bin/bash
# Final check: is there ANY validation after new_page() completes?
# Check if page.url() or final URL validation happens anywhere
rg -n 'page\.' src/tools/screenshot.rs | grep -i 'url\|location\|validate'
echo "---"
# Check the chromiumoxide source to understand new_page default behavior
# See if we can find how chromiumoxide handles redirects
find . -name 'Cargo.lock' -o -name 'Cargo.toml' | head -5 | xargs grep -l chromiumoxide
echo "---"
# Verify: confirm HEAD vs GET can have different responses from a server
# This is a fundamental HTTP fact, but let's see if there are any comments about it
rg -n 'HEAD\|GET\|method' src/tools/screenshot.rs | head -20Repository: qhkm/zeptoclaw
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check what version of chromiumoxide and its redirect behavior
sed -n '/chromiumoxide/,/^$/p' Cargo.toml
echo "---"
# Look at chromiumoxide's README or docs in the repo if they exist
find . -type f -name 'README*' -o -name 'ARCHITECTURE*' | xargs grep -l chromiumoxide 2>/dev/null
echo "---"
# Verify the key insight: can HEAD and GET have different responses?
# This is HTTP semantics - let's confirm from comments in the code
rg -n 'HEAD.*GET\|method.*different\|semantics' src/tools/Repository: qhkm/zeptoclaw
Length of output: 1416
🏁 Script executed:
#!/bin/bash
# Let me verify the core HTTP semantics fact and check if this is truly exploitable
# The key question: can a server respond differently to HEAD vs GET?
# First, let's confirm the vulnerability logic:
# 1. Preflight HEAD request is validated
# 2. Browser GET request might differ
# 3. Is there ANY validation after browser navigation?
rg -n 'page\.url\|currentUrl\|navigation\|frameNavigated' src/tools/screenshot.rs
echo "---"
# Check if chromiumoxide provides any way to intercept or validate redirects
rg -n 'intercept\|requestFailed\|navigationFailed' src/tools/screenshot.rs
echo "---"
# Check the learning about redirect policy - does it apply to browser too?
rg -n 'validate_redirect_target_for_policy\|validate_redirect_target' src/tools/screenshot.rsRepository: qhkm/zeptoclaw
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Final verification: confirm HTTP method semantics and the actual chromiumoxide behavior
# Check what methods chromiumoxide's new_page uses
cat << 'EOF' > /tmp/check_http.py
import subprocess
# Verify that HTTP HEAD and GET can differ - this is fundamental HTTP spec
print("HTTP Method Semantics:")
print("- HEAD: Same as GET but server should NOT return message body")
print("- GET: Returns full response including body and headers")
print("")
print("Key point: Servers CAN legitimately respond differently to HEAD vs GET")
print("- HEAD → benign response")
print("- GET → malicious redirect")
print("")
print("chromiumoxide behavior:")
print("- new_page(url) initiates browser navigation (implicitly GET/page load)")
print("- Chromium follows redirects automatically")
print("- No hooks for intercepting/validating redirect chain")
EOF
python /tmp/check_http.py
echo "---"
# Verify: after new_page, there's no validation of final URL
rg -n 'screenshot_result\|Ok::<Vec<u8>' src/tools/screenshot.rs -A3 -B3Repository: qhkm/zeptoclaw
Length of output: 520
Change HEAD preflight to GET to match browser navigation method.
The preflight request uses HEAD with SSRF validation, but the actual browser navigation uses GET without validation. Since HTTP servers can respond differently to HEAD vs GET requests, an attacker can serve a benign HEAD redirect chain (passing preflight) while sending a malicious GET redirect chain to the browser, bypassing SSRF protection.
🔧 Minimal fix
- let preflight_resp = preflight_client.head(url_str).send().await.map_err(|e| {
+ let preflight_resp = preflight_client.get(url_str).send().await.map_err(|e| {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let preflight_resp = preflight_client.head(url_str).send().await.map_err(|e| { | |
| if e.is_redirect() { | |
| ZeptoError::SecurityViolation(format!( | |
| "Screenshot blocked: redirect chain failed SSRF validation: {}", | |
| e | |
| )) | |
| } else { | |
| ZeptoError::Tool(format!("Screenshot preflight request failed: {}", e)) | |
| } | |
| })?; | |
| // Validate the final destination after all redirects. | |
| validate_redirect_target(preflight_resp.url()).await?; | |
| let preflight_resp = preflight_client.get(url_str).send().await.map_err(|e| { | |
| if e.is_redirect() { | |
| ZeptoError::SecurityViolation(format!( | |
| "Screenshot blocked: redirect chain failed SSRF validation: {}", | |
| e | |
| )) | |
| } else { | |
| ZeptoError::Tool(format!("Screenshot preflight request failed: {}", e)) | |
| } | |
| })?; | |
| // Validate the final destination after all redirects. | |
| validate_redirect_target(preflight_resp.url()).await?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/screenshot.rs` around lines 224 - 237, The preflight currently uses
HEAD which can differ from browser GET behavior; change the preflight request in
the screenshot flow to use GET instead of HEAD so SSRF validation observes the
same redirect/response path as the browser. Locate the call that builds and
sends the preflight request (the preflight_client.head(url_str).send().await
map_err block) and replace head(...) with get(...), preserving the existing
error mapping (checking e.is_redirect() to return ZeptoError::SecurityViolation
or ZeptoError::Tool) and keeping the subsequent
validate_redirect_target(preflight_resp.url()).await? call intact.
The HEAD preflight had two problems: 1. TOCTOU: server can return 200 to HEAD but redirect GET to internal IP 2. Compatibility: sites that reject HEAD (405/403) broke valid screenshots Now we let the browser navigate normally, then validate the final URL via page.url() before capturing the screenshot. This checks what the browser actually landed on — no TOCTOU gap, no method compatibility issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/screenshot.rs`:
- Around line 214-218: The code currently skips SSRF validation when
page.url().await or Url::parse fails; change the block to fail closed by
returning an error instead of silently skipping: call page.url().await and
propagate errors (use ?), require the Option be Some (return an Err if None),
parse with Url::parse(&final_url_str) and propagate parse errors (map or ?),
then call validate_redirect_target(&final_url).await? so any inability to read
or parse the final navigated URL triggers an error path; update the code around
page.url(), Url::parse, and validate_redirect_target accordingly.
- Around line 199-218: The current code only calls
validate_redirect_target(&final_url) after navigation, which misses malicious
intermediate redirects; update the flow to validate the initial URL and every
URL in the navigation/response redirect chain (not just the final landed URL) by
extracting the redirect history from the navigation response or page response
objects returned by browser.new_page/page.goto (or any Response with
redirected_from/redirected_to), and call validate_redirect_target for each
intermediate URL; if any validation fails, abort navigation/return an error and
close the page. Ensure you still keep the existing final-url check using
validate_redirect_target(&final_url) and reference the symbols browser.new_page,
page.goto/page.url, and validate_redirect_target when making the change.
Comment on lines
+214
to
+218
| if let Ok(Some(final_url_str)) = page.url().await { | ||
| if let Ok(final_url) = Url::parse(&final_url_str) { | ||
| validate_redirect_target(&final_url).await?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail closed when final navigated URL cannot be read or parsed.
Line 214 and Line 215 currently skip SSRF validation on page.url()/parse failure, which turns the security check into best-effort. This should return an error instead.
Minimal fail-closed fix
- if let Ok(Some(final_url_str)) = page.url().await {
- if let Ok(final_url) = Url::parse(&final_url_str) {
- validate_redirect_target(&final_url).await?;
- }
- }
+ let final_url_str = page.url().await.map_err(|e| {
+ ZeptoError::SecurityViolation(format!(
+ "Could not determine final navigated URL: {}",
+ e
+ ))
+ })?.ok_or_else(|| {
+ ZeptoError::SecurityViolation(
+ "Could not determine final navigated URL".to_string()
+ )
+ })?;
+
+ let final_url = Url::parse(&final_url_str).map_err(|e| {
+ ZeptoError::SecurityViolation(format!(
+ "Final navigated URL is invalid: {}",
+ e
+ ))
+ })?;
+
+ validate_redirect_target(&final_url).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Ok(Some(final_url_str)) = page.url().await { | |
| if let Ok(final_url) = Url::parse(&final_url_str) { | |
| validate_redirect_target(&final_url).await?; | |
| } | |
| } | |
| let final_url_str = page.url().await.map_err(|e| { | |
| ZeptoError::SecurityViolation(format!( | |
| "Could not determine final navigated URL: {}", | |
| e | |
| )) | |
| })?.ok_or_else(|| { | |
| ZeptoError::SecurityViolation( | |
| "Could not determine final navigated URL".to_string() | |
| ) | |
| })?; | |
| let final_url = Url::parse(&final_url_str).map_err(|e| { | |
| ZeptoError::SecurityViolation(format!( | |
| "Final navigated URL is invalid: {}", | |
| e | |
| )) | |
| })?; | |
| validate_redirect_target(&final_url).await?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/screenshot.rs` around lines 214 - 218, The code currently skips
SSRF validation when page.url().await or Url::parse fails; change the block to
fail closed by returning an error instead of silently skipping: call
page.url().await and propagate errors (use ?), require the Option be Some
(return an Err if None), parse with Url::parse(&final_url_str) and propagate
parse errors (map or ?), then call validate_redirect_target(&final_url).await?
so any inability to read or parse the final navigated URL triggers an error
path; update the code around page.url(), Url::parse, and
validate_redirect_target accordingly.
Replace post-navigation URL check with CDP Fetch.requestPaused interception. Each Document navigation request is validated via validate_redirect_target() BEFORE the browser follows it — no network request reaches blocked destinations. - enable_request_intercept() on BrowserConfig - EventRequestPaused listener validates Document requests at request stage - Blocked requests get FailRequestParams with BlockedByClient - Defense-in-depth: still validates final page.url() after navigation - 4 new tests for interception logic (40 total screenshot tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/tools/screenshot.rs (1)
322-327:⚠️ Potential issue | 🟠 MajorFail closed if the final navigated URL cannot be inspected.
If
page.url()orUrl::parse()fails here, the final SSRF check is silently skipped and the screenshot can still succeed. Even as defense-in-depth, this should return an error instead of making the last validation step optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/screenshot.rs` around lines 322 - 327, The final URL validation currently swallows failures from page.url().await and Url::parse, so change the optional/if-let flow around page.url(), Url::parse, and validate_redirect_target to fail fast: if page.url().await returns an Err or None, return an error (propagate or convert to a descriptive error) instead of skipping validation, and if Url::parse(&final_url_str) fails likewise return an error; then call validate_redirect_target(&final_url).await? to propagate its error. Ensure you update the code paths that reference page.url(), Url::parse, and validate_redirect_target to reflect this fail-closed behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/screenshot.rs`:
- Around line 68-73: The SSRF check currently only runs for
ResourceType::Document in should_validate_navigation_request, allowing other
resource types (img, script, fetch, link, etc.) to bypass validation; update
should_validate_navigation_request to trigger validation for outbound HTTP(S)
requests regardless of resource_type (i.e., remove the strict
ResourceType::Document requirement) and ensure the function still checks
response_status_code.is_none() and that the request URL scheme is http/https
before returning true; modify the logic in should_validate_navigation_request
and any callers that rely on it so all network fetches (identified via
ResourceType and URL scheme) are validated rather than only Document
navigations.
- Around line 88-98: The current validate_navigation_target(url_str) only
validates the hostname via validate_redirect_target()/resolve_and_check_host()
but doesn't pin the resolved safe IP for the subsequent Chromium navigation,
leaving a DNS-rebinding TOCTOU hole; change the flow so resolution and safety
checks return the concrete safe socket address (from resolve_and_check_host) and
use that pinned address for the actual browser request instead of the original
host—e.g., have validate_redirect_target/resolve_and_check_host return the
resolved IP:port and update validate_navigation_target to produce a navigation
target that uses the IP literal for the connection while preserving the original
hostname for Host/SNI (or equivalent browser navigation headers/options) so TLS
and virtual-hosting still work; update any callers that perform the Chromium
navigation to accept and use this pinned address (and keep
wrap_blocked_navigation_error behavior unchanged).
---
Duplicate comments:
In `@src/tools/screenshot.rs`:
- Around line 322-327: The final URL validation currently swallows failures from
page.url().await and Url::parse, so change the optional/if-let flow around
page.url(), Url::parse, and validate_redirect_target to fail fast: if
page.url().await returns an Err or None, return an error (propagate or convert
to a descriptive error) instead of skipping validation, and if
Url::parse(&final_url_str) fails likewise return an error; then call
validate_redirect_target(&final_url).await? to propagate its error. Ensure you
update the code paths that reference page.url(), Url::parse, and
validate_redirect_target to reflect this fail-closed behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d0a629a-965d-42bb-b297-2490f9e755b5
📒 Files selected for processing (3)
AGENTS.mdCLAUDE.mdsrc/tools/screenshot.rs
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
Comment on lines
+68
to
+73
| fn should_validate_navigation_request( | ||
| resource_type: ResourceType, | ||
| response_status_code: Option<i64>, | ||
| ) -> bool { | ||
| response_status_code.is_none() && resource_type == ResourceType::Document | ||
| } |
There was a problem hiding this comment.
Validate all outbound HTTP(S) fetches, not only Document navigations.
Restricting SSRF checks to ResourceType::Document leaves img, script, link, fetch, and similar browser requests unvalidated. Those requests are still continued at Line 284, so a public page can make Chromium hit private/internal endpoints without using a redirect chain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/screenshot.rs` around lines 68 - 73, The SSRF check currently only
runs for ResourceType::Document in should_validate_navigation_request, allowing
other resource types (img, script, fetch, link, etc.) to bypass validation;
update should_validate_navigation_request to trigger validation for outbound
HTTP(S) requests regardless of resource_type (i.e., remove the strict
ResourceType::Document requirement) and ensure the function still checks
response_status_code.is_none() and that the request URL scheme is http/https
before returning true; modify the logic in should_validate_navigation_request
and any callers that rely on it so all network fetches (identified via
ResourceType and URL scheme) are validated rather than only Document
navigations.
Comment on lines
+88
to
+98
| async fn validate_navigation_target(url_str: &str) -> Result<()> { | ||
| let parsed = Url::parse(url_str).map_err(|e| { | ||
| ZeptoError::Tool(format!( | ||
| "Failed to parse browser navigation target '{}': {}", | ||
| url_str, e | ||
| )) | ||
| })?; | ||
| validate_redirect_target(&parsed) | ||
| .await | ||
| .map_err(|e| wrap_blocked_navigation_error(url_str, e)) | ||
| } |
There was a problem hiding this comment.
This still leaves a DNS-rebinding TOCTOU gap.
validate_redirect_target() proves that the hostname resolved safely at validation time, but Chromium does a fresh DNS lookup after the request is continued. Because the safe address returned by resolve_and_check_host() is never pinned into the browser request, a rebinding attacker can answer with a public IP during validation and a private IP for the actual fetch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/screenshot.rs` around lines 88 - 98, The current
validate_navigation_target(url_str) only validates the hostname via
validate_redirect_target()/resolve_and_check_host() but doesn't pin the resolved
safe IP for the subsequent Chromium navigation, leaving a DNS-rebinding TOCTOU
hole; change the flow so resolution and safety checks return the concrete safe
socket address (from resolve_and_check_host) and use that pinned address for the
actual browser request instead of the original host—e.g., have
validate_redirect_target/resolve_and_check_host return the resolved IP:port and
update validate_navigation_target to produce a navigation target that uses the
IP literal for the connection while preserving the original hostname for
Host/SNI (or equivalent browser navigation headers/options) so TLS and
virtual-hosting still work; update any callers that perform the Chromium
navigation to accept and use this pinned address (and keep
wrap_blocked_navigation_error behavior unchanged).
Owner
Author
|
Closing in favor of #272 which has a better implementation (tokio::select! loop, redirect hop counting, loop detection). |
This was referenced Mar 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate_redirect_target_for_policy()(per-hop) andvalidate_redirect_target()(final destination) fromweb.rsProblem
The screenshot tool validated the initial URL but not redirect destinations. An attacker could redirect
https://attacker.com→http://169.254.169.254/to SSRF internal endpoints via the headless browser.Approach
Since chromiumoxide doesn't expose per-hop redirect control (it's a browser, not an HTTP client), we do a preflight HEAD request with reqwest's custom redirect policy before launching the browser. This catches SSRF via redirect chains before any content is fetched.
Test plan
cargo test --lib --features screenshot -- screenshot)cargo clippy -- -D warningscleancargo fmt -- --checkcleanCloses #271
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests