From 60164919858566abdebf5bb52d34b597e0c533ed Mon Sep 17 00:00:00 2001 From: Sid Jain Date: Tue, 10 Mar 2026 14:42:29 +0530 Subject: [PATCH] fix(slack): harden redirect chain and auth health checks --- src/channels/slack.rs | 138 ++++++++++++++++++++++++++---------------- 1 file changed, 87 insertions(+), 51 deletions(-) diff --git a/src/channels/slack.rs b/src/channels/slack.rs index c9619308c..aae540652 100644 --- a/src/channels/slack.rs +++ b/src/channels/slack.rs @@ -47,6 +47,7 @@ const SLACK_ATTACHMENT_FILENAME_MAX_CHARS: usize = 128; const SLACK_ATTACHMENT_SAVE_SUBDIR: &str = "slack_files"; const SLACK_ATTACHMENT_MAX_FILES_PER_MESSAGE: usize = 8; const SLACK_ATTACHMENT_RENDER_CONCURRENCY: usize = 3; +const SLACK_MEDIA_REDIRECT_MAX_HOPS: usize = 5; const SLACK_ALLOWED_MEDIA_HOST_SUFFIXES: &[&str] = &["slack.com", "slack-edge.com", "slack-files.com"]; const SLACK_SUPPORTED_IMAGE_MIME_TYPES: &[&str] = &[ @@ -711,72 +712,73 @@ impl SlackChannel { Some(target) } - fn slack_media_http_client_no_redirect(&self) -> reqwest::Client { + fn slack_media_http_client_no_redirect(&self) -> anyhow::Result { let builder = crate::config::apply_runtime_proxy_to_builder( reqwest::Client::builder().redirect(reqwest::redirect::Policy::none()), "channel.slack", ); - builder.build().unwrap_or_else(|err| { - tracing::warn!( - "Failed to build Slack media no-redirect HTTP client, using default client: {err}" - ); - reqwest::Client::new() - }) + builder + .build() + .context("failed to build Slack media no-redirect HTTP client") } async fn fetch_slack_private_file(&self, raw_url: &str) -> Option { let parsed = Self::validate_slack_private_file_url(raw_url)?; let redacted_parsed = Self::redact_slack_url(&parsed); - let client = self.slack_media_http_client_no_redirect(); - let initial = match client - .get(parsed.clone()) - .bearer_auth(&self.bot_token) - .send() - .await - { - Ok(response) => response, + let client = match self.slack_media_http_client_no_redirect() { + Ok(client) => client, Err(err) => { tracing::warn!("Slack file fetch failed for {}: {}", redacted_parsed, err); return None; } }; + let mut current_url = parsed; - if !initial.status().is_redirection() { - return Some(initial); - } + for redirect_hop in 0..=SLACK_MEDIA_REDIRECT_MAX_HOPS { + let redacted_current = Self::redact_slack_url(¤t_url); + let response = match client + .get(current_url.clone()) + .bearer_auth(&self.bot_token) + .send() + .await + { + Ok(response) => response, + Err(err) => { + tracing::warn!("Slack file fetch failed for {}: {}", redacted_current, err); + return None; + } + }; - let Some(location) = initial.headers().get(reqwest::header::LOCATION) else { - return Some(initial); - }; - let Ok(location) = location.to_str() else { - tracing::warn!( - "Slack file redirect location header is not valid UTF-8 for {}", - redacted_parsed - ); - return Some(initial); - }; - let Some(redirect_url) = Self::resolve_https_redirect_target(&parsed, location) else { - return Some(initial); - }; - let redacted_redirect = Self::redact_slack_url(&redirect_url); - - match self - .http_client() - .get(redirect_url.clone()) - .bearer_auth(&self.bot_token) - .send() - .await - { - Ok(response) => Some(response), - Err(err) => { - tracing::warn!( - "Slack redirected file fetch failed for {}: {}", - redacted_redirect, - err - ); - None + if !response.status().is_redirection() { + return Some(response); } + + if redirect_hop == SLACK_MEDIA_REDIRECT_MAX_HOPS { + tracing::warn!( + "Slack file redirect limit exceeded for {} after {} hops", + redacted_current, + SLACK_MEDIA_REDIRECT_MAX_HOPS + ); + return Some(response); + } + + let Some(location) = response.headers().get(reqwest::header::LOCATION) else { + return Some(response); + }; + let Ok(location) = location.to_str() else { + tracing::warn!( + "Slack file redirect location header is not valid UTF-8 for {}", + redacted_current + ); + return Some(response); + }; + let Some(next_url) = Self::resolve_https_redirect_target(¤t_url, location) else { + return Some(response); + }; + current_url = next_url; } + + None } async fn fetch_image_marker(&self, file: &serde_json::Value) -> Option { @@ -1852,6 +1854,18 @@ impl SlackChannel { true } + fn slack_api_call_succeeded(status: reqwest::StatusCode, body: &str) -> bool { + if !status.is_success() { + return false; + } + + let parsed: serde_json::Value = serde_json::from_str(body).unwrap_or_default(); + parsed + .get("ok") + .and_then(|value| value.as_bool()) + .unwrap_or(false) + } + async fn fetch_history_with_retry( &self, channel_id: &str, @@ -2148,14 +2162,20 @@ impl Channel for SlackChannel { } async fn health_check(&self) -> bool { - let bot_ok = self + let bot_ok = match self .http_client() .get("https://slack.com/api/auth.test") .bearer_auth(&self.bot_token) .send() .await - .map(|r| r.status().is_success()) - .unwrap_or(false); + { + Ok(response) => { + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + Self::slack_api_call_succeeded(status, &body) + } + Err(_) => false, + }; let socket_mode_enabled = self.configured_app_token().is_some(); let socket_mode_ok = if socket_mode_enabled { self.open_socket_mode_url().await.is_ok() @@ -2639,6 +2659,22 @@ mod tests { assert!(SlackChannel::evaluate_health(true, true, true)); } + #[test] + fn slack_api_call_succeeded_requires_ok_true_in_body() { + assert!(!SlackChannel::slack_api_call_succeeded( + reqwest::StatusCode::OK, + r#"{"ok":false,"error":"invalid_auth"}"# + )); + } + + #[test] + fn slack_api_call_succeeded_accepts_ok_true() { + assert!(SlackChannel::slack_api_call_succeeded( + reqwest::StatusCode::OK, + r#"{"ok":true}"# + )); + } + #[test] fn specific_allowlist_filters() { let ch = SlackChannel::new(