Skip to content

fix: prevent SSRF and open redirect in alert targets and OIDC logout #1615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikhilsinhaparseable wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: prevent SSRF and open redirect in alert targets and OIDC logout #1615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 185 additions & 1 deletion src/alerts/target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

use std::{
collections::HashMap,
net::{IpAddr, SocketAddr, ToSocketAddrs},
sync::{Arc, Mutex},
time::Duration,
};
Expand Down Expand Up @@ -473,12 +474,164 @@ impl TargetType {
TargetType::AlertManager(target) => target.call(payload).await,
}
}

pub fn endpoint_url(&self) -> &Url {
match self {
TargetType::Slack(h) => &h.endpoint,
TargetType::Other(h) => &h.endpoint,
TargetType::AlertManager(h) => &h.endpoint,
}
}
}

fn default_client_builder() -> ClientBuilder {
ClientBuilder::new()
}

fn mask_endpoint(url: &Url) -> String {
let s = url.to_string();
format!("{}********", &s[..4])
}

fn is_private_ip(ip: IpAddr) -> bool {
match ip {
IpAddr::V4(v4) => {
v4.is_loopback()
|| v4.is_private()
|| v4.is_link_local()
|| v4.is_broadcast()
|| v4.is_multicast()
|| v4.is_unspecified()
// "This network" 0.0.0.0/8 — on some systems 0.x.x.x routes to localhost
|| v4.octets()[0] == 0
}
IpAddr::V6(v6) => {
v6.is_loopback()
|| v6.is_multicast()
|| v6.is_unspecified()
// Unique local addresses (ULA): fc00::/7
|| (v6.segments()[0] & 0xfe00) == 0xfc00
// Link-local: fe80::/10
|| (v6.segments()[0] & 0xffc0) == 0xfe80
// IPv4-mapped IPv6 (e.g. ::ffff:127.0.0.1)
|| v6.to_ipv4_mapped().is_some_and(|v4| is_private_ip(IpAddr::V4(v4)))
}
}
}

/// Validate a target endpoint URL to prevent SSRF.
/// Blocks private/loopback/link-local IP addresses (both literal and DNS-resolved).
pub async fn validate_target_endpoint(url: &Url) -> Result<(), AlertError> {
match url.scheme() {
"http" | "https" => {}
_ => {
return Err(AlertError::CustomError(
"Target endpoint must use http or https scheme".into(),
));
}
}

let host = url
.host()
.ok_or_else(|| AlertError::CustomError("Target endpoint must have a valid host".into()))?;

match host {
url::Host::Ipv4(v4) => {
if is_private_ip(IpAddr::V4(v4)) {
return Err(AlertError::CustomError(
"Target endpoint must not point to a private or internal address".into(),
));
}
}
url::Host::Ipv6(v6) => {
if is_private_ip(IpAddr::V6(v6)) {
return Err(AlertError::CustomError(
"Target endpoint must not point to a private or internal address".into(),
));
}
}
url::Host::Domain(hostname) => {
let port = url.port_or_known_default().unwrap_or(80);
let addr = format!("{hostname}:{port}");
let resolved = tokio::task::spawn_blocking(move || {
addr.to_socket_addrs().map(|i| i.collect::<Vec<_>>())
})
.await
.map_err(|e| AlertError::CustomError(format!("DNS resolution task failed: {e}")))?;

let addrs = resolved.map_err(|e| {
AlertError::CustomError(format!("Could not resolve target endpoint host: {e}"))
})?;
for sock_addr in addrs {
if is_private_ip(sock_addr.ip()) {
return Err(AlertError::CustomError(
"Target endpoint must not point to a private or internal address".into(),
));
}
}
}
}

Ok(())
}

/// Resolve DNS and validate IPs at call time to prevent DNS rebinding.
/// Returns `Some((hostname, addr))` for domain-based URLs (to pin via `ClientBuilder::resolve`),
/// or `None` for IP-literal URLs (no pinning needed).
async fn resolve_and_pin(url: &Url) -> Result<Option<(String, SocketAddr)>, String> {
let host = url.host().ok_or("No host in target URL")?;

match host {
url::Host::Ipv4(v4) => {
if is_private_ip(IpAddr::V4(v4)) {
return Err("Target resolves to a private/internal address".into());
}
Ok(None)
}
url::Host::Ipv6(v6) => {
if is_private_ip(IpAddr::V6(v6)) {
return Err("Target resolves to a private/internal address".into());
}
Ok(None)
}
url::Host::Domain(hostname) => {
let port = url.port_or_known_default().unwrap_or(80);
let addr_str = format!("{hostname}:{port}");
let hostname = hostname.to_string();
let addrs = tokio::task::spawn_blocking(move || {
addr_str.to_socket_addrs().map(|i| i.collect::<Vec<_>>())
})
.await
.map_err(|e| format!("DNS resolution task failed: {e}"))?
.map_err(|e| format!("Could not resolve host: {e}"))?;

for sock_addr in &addrs {
if is_private_ip(sock_addr.ip()) {
return Err("Target resolves to a private/internal address".into());
}
}

let first = addrs
.into_iter()
.next()
.ok_or("DNS resolution returned no addresses")?;
Ok(Some((hostname, first)))
}
}
}

/// Apply DNS pinning to a `ClientBuilder` to prevent DNS rebinding attacks.
/// Resolves the endpoint, validates all IPs, and pins the connection to the validated address.
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)
}
Comment on lines +625 to +633
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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 (and Client) uses redirect::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 2

Repository: parseablehq/parseable

Length of output: 1306

🏁 Script executed:

#!/bin/bash
# Look for other redirect policy usages in the codebase
rg "redirect" -t rs -i

Repository: 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.rs

Repository: 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 -100

Repository: 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 rust

Repository: 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.


#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
pub struct SlackWebHook {
endpoint: Url,
Expand All @@ -487,7 +640,17 @@ pub struct SlackWebHook {
#[async_trait]
impl CallableTarget for SlackWebHook {
async fn call(&self, payload: &Context) {
let client = default_client_builder()
let builder = match apply_dns_pinning(default_client_builder(), &self.endpoint).await {
Ok(b) => b,
Err(e) => {
error!(
"SSRF protection blocked request to {}: {e}",
mask_endpoint(&self.endpoint)
);
return;
}
};
let client = builder
.build()
.expect("Client can be constructed on this system");

Expand Down Expand Up @@ -526,6 +689,16 @@ impl CallableTarget for OtherWebHook {
if self.skip_tls_check {
builder = builder.danger_accept_invalid_certs(true)
}
builder = match apply_dns_pinning(builder, &self.endpoint).await {
Ok(b) => b,
Err(e) => {
error!(
"SSRF protection blocked request to {}: {e}",
mask_endpoint(&self.endpoint)
);
return;
}
};

let client = builder
.build()
Expand Down Expand Up @@ -576,6 +749,17 @@ impl CallableTarget for AlertManager {
builder = builder.default_headers(headers)
}

builder = match apply_dns_pinning(builder, &self.endpoint).await {
Ok(b) => b,
Err(e) => {
error!(
"SSRF protection blocked request to {}: {e}",
mask_endpoint(&self.endpoint)
);
return;
}
};

let client = builder
.build()
.expect("Client can be constructed on this system");
Expand Down
20 changes: 16 additions & 4 deletions src/handlers/http/oidc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,23 @@ pub async fn login(
}
}

pub async fn logout(req: HttpRequest, query: web::Query<RedirectAfterLogin>) -> HttpResponse {
pub async fn logout(
req: HttpRequest,
query: web::Query<RedirectAfterLogin>,
) -> Result<HttpResponse, OIDCError> {
// 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(),
));
}
Comment on lines +174 to +181
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


let oidc_client = OIDC_CLIENT.get();

let Some(session) = extract_session_key_from_req(&req).ok() else {
return redirect_to_client(query.redirect.as_str(), None);
return Ok(redirect_to_client(query.redirect.as_str(), None));
};
let tenant_id = get_tenant_id_from_key(&session);
let user = Users.remove_session(&session);
Expand All @@ -181,14 +193,14 @@ pub async fn logout(req: HttpRequest, query: web::Query<RedirectAfterLogin>) ->
None
};

match (user, logout_endpoint) {
Ok(match (user, logout_endpoint) {
(Some(username), Some(logout_endpoint))
if Users.is_oauth(&username, &tenant_id).unwrap_or_default() =>
{
redirect_to_oidc_logout(logout_endpoint, &query.redirect)
}
_ => redirect_to_client(query.redirect.as_str(), None),
}
})
}

/// Handler for code callback
Expand Down
10 changes: 4 additions & 6 deletions src/handlers/http/targets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use ulid::Ulid;
use crate::{
alerts::{
AlertError,
target::{TARGETS, Target},
target::{TARGETS, Target, validate_target_endpoint},
},
utils::get_tenant_id_from_request,
};
Expand All @@ -38,8 +38,7 @@ pub async fn post(
) -> Result<impl Responder, AlertError> {
let tenant_id = get_tenant_id_from_request(&req);
target.tenant = tenant_id;
// should check for duplicacy and liveness (??)
// add to the map
validate_target_endpoint(target.target.endpoint_url()).await?;
TARGETS.update(target.clone()).await?;

// Ok(web::Json(target.mask()))
Expand Down Expand Up @@ -88,11 +87,10 @@ pub async fn update(
));
}

// esnure that the supplied target id is assigned to the target config
// ensure that the supplied target id is assigned to the target config
target.id = target_id;
target.tenant = tenant_id;
// should check for duplicacy and liveness (??)
// add to the map
validate_target_endpoint(target.target.endpoint_url()).await?;
TARGETS.update(target.clone()).await?;

// Ok(web::Json(target.mask()))
Expand Down
Loading