From 35caf88a2aa68fc767f458fc24e5923a524f3a1b Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Wed, 28 Feb 2024 15:38:40 +0100 Subject: [PATCH] fix: prevent open redirect with `/login?redirect=` (#4) Co-authored-by: Marvin Hagemeister --------- Co-authored-by: Marvin Hagemeister --- api/src/auth.rs | 10 ++++++++-- api/src/util.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/api/src/auth.rs b/api/src/auth.rs index 8efb1518..2b18d72f 100644 --- a/api/src/auth.rs +++ b/api/src/auth.rs @@ -3,6 +3,7 @@ use crate::api::ApiError; use crate::db::*; +use crate::util::sanitize_redirect_url; use crate::util::ApiResult; use chrono::DateTime; use chrono::Duration; @@ -203,10 +204,13 @@ pub async fn login_handler(req: Request) -> ApiResult> { .set_pkce_challenge(pkce_code_challenge) .url(); - let redirect_url = req + let mut redirect_url = req .query("redirect") .and_then(|url| urlencoding::decode(url).map(|url| url.into_owned()).ok()) .unwrap_or("/".to_string()); + + redirect_url = sanitize_redirect_url(&redirect_url); + Span::current().record("redirect", &redirect_url); let db = req.data::().unwrap(); @@ -286,10 +290,12 @@ pub async fn login_callback_handler( #[instrument(name = "GET /logout", skip(req), err, fields(redirect))] pub async fn logout_handler(req: Request) -> ApiResult> { - let redirect_url = req + let mut redirect_url = req .query("redirect") .and_then(|url| urlencoding::decode(url).map(|url| url.into_owned()).ok()) .unwrap_or("/".to_string()); + + redirect_url = sanitize_redirect_url(&redirect_url); Span::current().record("redirect", &redirect_url); Ok( diff --git a/api/src/util.rs b/api/src/util.rs index ef403819..641e1264 100644 --- a/api/src/util.rs +++ b/api/src/util.rs @@ -18,6 +18,7 @@ use tracing::error; use tracing::field; use tracing::instrument; use tracing::Span; +use url::Url; use uuid::Uuid; use crate::api::ApiError; @@ -237,6 +238,38 @@ pub fn pagination(req: &Request) -> (i64, i64) { (start, limit) } +// Sanitize redirect urls +// - Remove origin from Url: https://evil.com -> / +// - Replace multiple slashes with one slash to remove prevent +// relative url bypass: //evil.com/foo -> /foo +// - Remove /../ and /./ from path segments +pub fn sanitize_redirect_url(raw: &str) -> String { + let base = Url::parse("http://localhost/").unwrap(); + let url = base.join(raw).unwrap_or(base.clone()); + + let mut sanitized = "".to_string(); + + if let Some(segments) = url.path_segments() { + for seg in segments { + if seg.is_empty() { + continue; + } + + sanitized.push_str(&format!("/{}", seg)); + } + } + + if let Some(query) = url.query() { + sanitized.push_str(&format!("?{}", query)); + } + + if sanitized.is_empty() { + "/".to_string() + } else { + sanitized + } +} + pub trait RequestIdExt { fn param_uuid(&self, name: &str) -> Result; fn param_scope(&self) -> Result; @@ -333,6 +366,7 @@ pub mod test { use crate::db::{Database, NewUser, User}; use crate::errors_internal::ApiErrorStruct; use crate::gcp::FakeGcsTester; + use crate::util::sanitize_redirect_url; use crate::ApiError; use crate::MainRouterOptions; use hyper::http::HeaderName; @@ -768,4 +802,18 @@ pub mod test { assert!(!t.user3.user.is_staff); assert!(t.staff_user.user.is_staff); } + + #[test] + fn sanitize_url_test() { + assert_eq!(sanitize_redirect_url("/foo"), "/foo"); + assert_eq!(sanitize_redirect_url("//evil.com/bar"), "/bar"); + assert_eq!( + sanitize_redirect_url("//evil.com//bar?foo=bar"), + "/bar?foo=bar" + ); + assert_eq!(sanitize_redirect_url("https://evil.com"), "/"); + assert_eq!(sanitize_redirect_url("/../foo"), "/foo"); + assert_eq!(sanitize_redirect_url("/../foo/../bar"), "/bar"); + assert_eq!(sanitize_redirect_url("/foo/./bar"), "/foo/bar"); + } }