fix: prevent SSRF and open redirect in alert targets and OIDC logout#1615
fix: prevent SSRF and open redirect in alert targets and OIDC logout#1615nikhilsinhaparseable wants to merge 1 commit intoparseablehq:mainfrom
Conversation
Add endpoint validation for alert targets to block SSRF attacks. - Validates scheme (http/https only) - rejects private/loopback/link-local - IPs for both literal and DNS-resolved addresses, including IPv4-mapped IPv6 and 0.0.0.0/8 - DNS resolution is re-validated and pinned at request time via reqwest's resolve() to prevent DNS rebinding. Validate redirect URL in OIDC logout to prevent open redirect attacks.
WalkthroughThe changes introduce SSRF and DNS-rebinding protection mechanisms for alert target endpoints by adding validation and DNS pinning helpers, integrate endpoint validation into target creation and update handlers, and enhance the OIDC logout handler with redirect URL validation and error result type wrapping. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Handler as Target Handler
participant Validator as Validator
participant Resolver as DNS Resolver
participant Client as HTTP Client
participant Target as Target Endpoint
App->>Handler: Create/Update Target
Handler->>Validator: validate_target_endpoint(url)
Validator->>Validator: Check scheme is HTTP(S)
Validator->>Resolver: resolve_and_pin(url)
alt Domain Host
Resolver->>Resolver: DNS lookup
Resolver->>Validator: Check resolved IP<br/>(not private/internal)
Validator-->>Handler: Return socket address
else IP Literal Host
Resolver-->>Handler: Return None (no pinning)
end
Handler->>Client: apply_dns_pinning(ClientBuilder, url)
Client->>Target: Send request with pinned resolver
Target-->>Client: Response
Client-->>Handler: Success/Error
Handler-->>App: Target created/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/handlers/http/targets.rs (1)
41-42: Keep the endpoint invariant below the HTTP layer.These handlers now validate the URL, but
src/alerts/target.rs::TargetConfigs::update()andTargetConfigs::load()still accept arbitrary endpoints. That means non-HTTP write paths and preexisting metastore entries bypass this guard until delivery time. Moving the check into the target/storage layer would keep the invariant consistent.Also applies to: 93-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/targets.rs` around lines 41 - 42, The HTTP handler currently validates endpoints but TargetConfigs::update() and TargetConfigs::load() still accept arbitrary endpoint URLs, letting non-HTTP write paths and existing metastore records bypass the invariant; move the endpoint validation into the target/storage layer by calling the same validation routine (or embedding its logic) inside TargetConfigs::update() and TargetConfigs::load() so any write or load of a Target runs validate_target_endpoint (or equivalent) and returns an error on invalid endpoints, ensuring TARGETS.update(...) and any load from storage cannot produce Targets with invalid endpoint URLs.
🤖 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/alerts/target.rs`:
- Around line 625-633: apply_dns_pinning currently only sets DNS resolution but
leaves reqwest's default redirect behavior enabled, which allows SSRF via
redirect responses; update apply_dns_pinning (which is used by SlackWebHook,
OtherWebHook, and AlertManager) to disable redirects on the ClientBuilder before
returning (set the builder's redirect policy to none via the reqwest redirect
Policy API), so the returned ClientBuilder has redirects disabled in addition to
DNS pinning; keep resolve_and_pin and existing host-resolution logic unchanged.
In `@src/handlers/http/oidc.rs`:
- Around line 174-181: Replace the request-derived host check with validation
against the configured public base URL: in the logout handler replace using
req.connection_info().host() / base_url_without_scheme and call
is_valid_redirect_url() with the application-configured external origin/public
base URL (the same helper used for login) when validating query.redirect,
returning the same OIDCError::BadRequest on failure; apply the identical change
to login() so both paths validate redirect URLs against the configured public
base URL rather than Host/X-Forwarded-Host.
---
Nitpick comments:
In `@src/handlers/http/targets.rs`:
- Around line 41-42: The HTTP handler currently validates endpoints but
TargetConfigs::update() and TargetConfigs::load() still accept arbitrary
endpoint URLs, letting non-HTTP write paths and existing metastore records
bypass the invariant; move the endpoint validation into the target/storage layer
by calling the same validation routine (or embedding its logic) inside
TargetConfigs::update() and TargetConfigs::load() so any write or load of a
Target runs validate_target_endpoint (or equivalent) and returns an error on
invalid endpoints, ensuring TARGETS.update(...) and any load from storage cannot
produce Targets with invalid endpoint URLs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37c4d920-f3ce-4956-8c8a-062ffe721fac
📒 Files selected for processing (3)
src/alerts/target.rssrc/handlers/http/oidc.rssrc/handlers/http/targets.rs
| async fn apply_dns_pinning( | ||
| mut builder: ClientBuilder, | ||
| endpoint: &Url, | ||
| ) -> Result<ClientBuilder, String> { | ||
| if let Some((host, addr)) = resolve_and_pin(endpoint).await? { | ||
| builder = builder.resolve(&host, addr); | ||
| } | ||
| Ok(builder) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does reqwest::ClientBuilderfollow HTTP redirects by default for POST/307/308 responses, and isredirect(reqwest::redirect::Policy::none()) the supported way to disable them?
💡 Result:
Yes.
- Default behavior:
reqwest::ClientBuilder(andClient) usesredirect::Policy::default(), which automatically follows redirects up to 10 hops. This applies to redirects in general, including 307/308 responses. [1], [2] - POST + 307/308: For 307/308, the redirect must be able to re-send the original request body (i.e., the body must be clonable). Reqwest’s redirect implementation explicitly notes it must be able to clone the body “to make 307 and 308 redirects work”, meaning it will follow them when it can reissue the POST with the same body. [3]
- Disabling redirects (supported): Yes—
redirect(reqwest::redirect::Policy::none())is the supported way to disable all redirect behavior. [2], [4]
Sources:
[1] https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html
[2] https://docs.rs/reqwest/latest/reqwest/redirect/
[3] https://docs.rs/reqwest/latest/src/reqwest/redirect.rs.html
[4] https://docs.rs/reqwest/latest/reqwest/redirect/struct.Policy.html
🏁 Script executed:
#!/bin/bash
# Find all references to apply_dns_pinning
rg "apply_dns_pinning" -A 2 -B 2Repository: parseablehq/parseable
Length of output: 1306
🏁 Script executed:
#!/bin/bash
# Look for other redirect policy usages in the codebase
rg "redirect" -t rs -iRepository: parseablehq/parseable
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Get broader context around the apply_dns_pinning function
sed -n '600,650p' src/alerts/target.rsRepository: parseablehq/parseable
Length of output: 1820
🏁 Script executed:
#!/bin/bash
# Find all webhook/callable target implementations that use apply_dns_pinning
rg "impl CallableTarget" -A 10 src/alerts/target.rs | head -100Repository: parseablehq/parseable
Length of output: 1413
🏁 Script executed:
#!/bin/bash
# Search for redirect-related configuration in the codebase
rg "redirect\(" -i --type-add 'rust:*.rs' --type rustRepository: parseablehq/parseable
Length of output: 290
Disable HTTP redirects to prevent SSRF via redirect-based attacks.
HTTP redirects bypass DNS pinning. An attacker-controlled webhook can respond with a 30x Location header pointing to an internal address like 169.254.169.254. Reqwest follows redirects by default (including 307/308 responses), allowing this follow-up request to reach internal services unvalidated.
Disable redirects for all webhook clients:
Fix
async fn apply_dns_pinning(
mut builder: ClientBuilder,
endpoint: &Url,
) -> Result<ClientBuilder, String> {
+ builder = builder.redirect(reqwest::redirect::Policy::none());
if let Some((host, addr)) = resolve_and_pin(endpoint).await? {
builder = builder.resolve(&host, addr);
}
Ok(builder)
}This affects all webhook implementations that call apply_dns_pinning: SlackWebHook, OtherWebHook, and AlertManager.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/alerts/target.rs` around lines 625 - 633, apply_dns_pinning currently
only sets DNS resolution but leaves reqwest's default redirect behavior enabled,
which allows SSRF via redirect responses; update apply_dns_pinning (which is
used by SlackWebHook, OtherWebHook, and AlertManager) to disable redirects on
the ClientBuilder before returning (set the builder's redirect policy to none
via the reqwest redirect Policy API), so the returned ClientBuilder has
redirects disabled in addition to DNS pinning; keep resolve_and_pin and existing
host-resolution logic unchanged.
| // Validate redirect URL against server host to prevent open redirect attacks | ||
| let conn = req.connection_info().clone(); | ||
| let base_url_without_scheme = format!("{}/", conn.host()); | ||
| if !is_valid_redirect_url(&base_url_without_scheme, query.redirect.as_str()) { | ||
| return Err(OIDCError::BadRequest( | ||
| "Bad Request, Invalid Redirect URL!".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Use the configured external origin for logout redirects.
req.connection_info().host() is request-derived, and is_valid_redirect_url() ignores the scheme, so this still accepts http://<same-host>/ on HTTPS deployments and depends on Host/X-Forwarded-Host being trustworthy. Please validate query.redirect against the configured public base URL instead of the incoming host.
🔒 Suggested direction
- let conn = req.connection_info().clone();
- let base_url_without_scheme = format!("{}/", conn.host());
- if !is_valid_redirect_url(&base_url_without_scheme, query.redirect.as_str()) {
+ let configured = Url::parse(&PARSEABLE.options.address)
+ .map_err(|_| OIDCError::BadRequest("Invalid server base URL".to_string()))?;
+ if query.redirect.scheme() != configured.scheme()
+ || query.redirect.host_str() != configured.host_str()
+ || query.redirect.port_or_known_default() != configured.port_or_known_default()
+ {
return Err(OIDCError::BadRequest(
"Bad Request, Invalid Redirect URL!".to_string(),
));
}Applying the same helper change to login() would close the same gap there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/http/oidc.rs` around lines 174 - 181, Replace the
request-derived host check with validation against the configured public base
URL: in the logout handler replace using req.connection_info().host() /
base_url_without_scheme and call is_valid_redirect_url() with the
application-configured external origin/public base URL (the same helper used for
login) when validating query.redirect, returning the same OIDCError::BadRequest
on failure; apply the identical change to login() so both paths validate
redirect URLs against the configured public base URL rather than
Host/X-Forwarded-Host.
1 participant
Add endpoint validation for alert targets to block SSRF attacks.
Validate redirect URL in OIDC logout to prevent open redirect attacks.
Summary by CodeRabbit
New Features
Bug Fixes