fix: prevent open redirect with /login?redirect=<url> (#4)

Co-authored-by: Marvin Hagemeister <[email protected]> --------- Co-authored-by: Marvin Hagemeister <[email protected]>
jsr-io · Feb 28, 2024 · 35caf88 · 35caf88
1 parent 2415e28
commit 35caf88
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 2 deletions.
diff --git 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<Body>) -> ApiResult<Response<Body>> {
     .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::<Database>().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<Body>) -> ApiResult<Response<Body>> {
-  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
@@ -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<Body>) -> (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<Uuid, ApiError>;
   fn param_scope(&self) -> Result<ScopeName, ApiError>;
@@ -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");
+  }
 }