Sanitize next url (#323)

Signed-off-by: Sergio Castaño Arteaga <[email protected]>

Author: tegioz

Cargo.lock

@@ -40,6 +40,7 @@ openidconnect = { version = "4.0.1", features = ["accept-rfc3339-timestamps"] }
 openssl = { version = "0.10.73", features = ["vendored"] }
 palette = "0.7.6"
 password-auth = "1.0.0"
+percent-encoding = "2.3.2"
 postgres-openssl = "0.5.1"
 rand = "0.9.2"
 regex = "1.11.2"

Cargo.toml

@@ -33,6 +33,7 @@ openidconnect = { workspace = true }
 openssl = { workspace = true }
 palette = { workspace = true }
 password-auth = { workspace = true }
+percent-encoding = { workspace = true }
 postgres-openssl = { workspace = true }
 rand = { workspace = true }
 regex = { workspace = true }

gitjobs-server/Cargo.toml

@@ -13,6 +13,7 @@ use axum_extra::extract::Form;
 use axum_messages::Messages;
 use openidconnect as oidc;
 use password_auth::verify_password;
+use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode};
 use serde::Deserialize;
 use tower_sessions::Session;
 use tracing::instrument;
@@ -69,13 +70,17 @@ pub(crate) async fn log_in_page(
         return Ok(Redirect::to("/").into_response());
     }
 
+    // Sanitize and encode the next url (if provided)
+    let next_url =
+        sanitize_next_url(query.get("next_url").map(String::as_str)).map(|value| encode_next_url(&value));
+
     // Prepare template
     let template = templates::auth::LogInPage {
         auth_provider: None,
         login: cfg.login.clone(),
         cfg: cfg.into(),
         messages: messages.into_iter().collect(),
-        next_url: query.get("next_url").cloned(),
+        next_url,
         page_id: PageId::LogIn,
         user: User::default(),
     };
@@ -96,13 +101,17 @@ pub(crate) async fn sign_up_page(
         return Ok(Redirect::to("/").into_response());
     }
 
+    // Sanitize and encode the next url (if provided)
+    let next_url =
+        sanitize_next_url(query.get("next_url").map(String::as_str)).map(|value| encode_next_url(&value));
+
     // Prepare template
     let template = templates::auth::SignUpPage {
         auth_provider: None,
         login: cfg.login.clone(),
         cfg: cfg.into(),
         messages: messages.into_iter().collect(),
-        next_url: query.get("next_url").cloned(),
+        next_url,
         page_id: PageId::SignUp,
         user: User::default(),
     };
@@ -122,14 +131,17 @@ pub(crate) async fn log_in(
     State(db): State<DynDB>,
     Form(creds): Form<PasswordCredentials>,
 ) -> Result<impl IntoResponse, HandlerError> {
+    // Sanitize next url
+    let next_url = sanitize_next_url(query.get("next_url").map(String::as_str));
+
     // Authenticate user
     let Some(user) = auth_session
         .authenticate(Credentials::Password(creds.clone()))
         .await
         .map_err(|e| HandlerError::Auth(e.to_string()))?
     else {
         messages.error("Invalid credentials. Please make sure you have verified your email address.");
-        let log_in_url = get_log_in_url(query.get("next_url"));
+        let log_in_url = get_log_in_url(next_url.as_deref());
         return Ok(Redirect::to(&log_in_url));
     };
 
@@ -147,13 +159,7 @@ pub(crate) async fn log_in(
             .await?;
     }
 
-    // Prepare next url
-    let next_url = if let Some(next_url) = query.get("next_url") {
-        next_url
-    } else {
-        "/"
-    };
-
+    let next_url = next_url.as_deref().unwrap_or("/");
     Ok(Redirect::to(next_url))
 }
 
@@ -191,8 +197,12 @@ pub(crate) async fn oauth2_callback(
     }
 
     // Get next url from session (if any)
-    let next_url = session.remove::<Option<String>>(NEXT_URL_KEY).await?.flatten();
-    let log_in_url = get_log_in_url(next_url.as_ref());
+    let next_url = session
+        .remove::<Option<String>>(NEXT_URL_KEY)
+        .await?
+        .flatten()
+        .and_then(|value| sanitize_next_url(Some(value.as_str())));
+    let log_in_url = get_log_in_url(next_url.as_deref());
 
     // Authenticate user
     let creds = OAuth2Credentials { code, provider };
@@ -222,10 +232,8 @@ pub(crate) async fn oauth2_callback(
             .await?;
     }
 
-    // Prepare next url
-    let next_url = next_url.unwrap_or("/".to_string());
-
-    Ok(Redirect::to(&next_url))
+    let next_url = next_url.as_deref().unwrap_or("/");
+    Ok(Redirect::to(next_url))
 }
 
 /// Handler that redirects the user to the oauth2 provider.
@@ -242,6 +250,9 @@ pub(crate) async fn oauth2_redirect(
     }
     let (authorize_url, csrf_state) = builder.url();
 
+    // Sanitize the next url (if any)
+    let next_url = sanitize_next_url(next_url.as_deref());
+
     // Save the csrf state and next url in the session
     session.insert(OAUTH2_CSRF_STATE_KEY, csrf_state.secret()).await?;
     session.insert(NEXT_URL_KEY, next_url).await?;
@@ -279,8 +290,12 @@ pub(crate) async fn oidc_callback(
     };
 
     // Get next url from session (if any)
-    let next_url = session.remove::<Option<String>>(NEXT_URL_KEY).await?.flatten();
-    let log_in_url = get_log_in_url(next_url.as_ref());
+    let next_url = session
+        .remove::<Option<String>>(NEXT_URL_KEY)
+        .await?
+        .flatten()
+        .and_then(|value| sanitize_next_url(Some(value.as_str())));
+    let log_in_url = get_log_in_url(next_url.as_deref());
 
     // Authenticate user
     let creds = OidcCredentials {
@@ -317,10 +332,8 @@ pub(crate) async fn oidc_callback(
             .await?;
     }
 
-    // Prepare next url
-    let next_url = next_url.unwrap_or("/".to_string());
-
-    Ok(Redirect::to(&next_url))
+    let next_url = next_url.as_deref().unwrap_or("/");
+    Ok(Redirect::to(next_url))
 }
 
 /// Handler that redirects the user to the oidc provider.
@@ -341,6 +354,9 @@ pub(crate) async fn oidc_redirect(
     }
     let (authorize_url, csrf_state, nonce) = builder.url();
 
+    // Sanitize the next url (if any)
+    let next_url = sanitize_next_url(next_url.as_deref());
+
     // Save the csrf state, nonce and next url in the session
     session.insert(OAUTH2_CSRF_STATE_KEY, csrf_state.secret()).await?;
     session.insert(OIDC_NONCE_KEY, nonce.secret()).await?;
@@ -393,7 +409,8 @@ pub(crate) async fn sign_up(
     }
 
     // Redirect to the log in page on success
-    let log_in_url = get_log_in_url(query.get("next_url"));
+    let next_url = sanitize_next_url(query.get("next_url").map(String::as_str));
+    let log_in_url = get_log_in_url(next_url.as_deref());
     Ok(Redirect::to(&log_in_url).into_response())
 }
 
@@ -466,15 +483,6 @@ pub(crate) async fn verify_email(
     Ok(Redirect::to(LOG_IN_URL).into_response())
 }
 
-/// Get the log in url including the next url if provided.
-fn get_log_in_url(next_url: Option<&String>) -> String {
-    let mut log_in_url = LOG_IN_URL.to_string();
-    if let Some(next_url) = next_url {
-        log_in_url = format!("{log_in_url}?next_url={next_url}");
-    }
-    log_in_url
-}
-
 // Deserialization helpers.
 
 /// `OAuth2` authorization response containing code and CSRF state.
@@ -632,3 +640,31 @@ pub(crate) async fn user_owns_job(
 
     next.run(request).await.into_response()
 }
+
+// Helpers.
+
+/// Percent-encode a `next_url` so it can be safely embedded in a query string.
+fn encode_next_url(next_url: &str) -> String {
+    utf8_percent_encode(next_url, NON_ALPHANUMERIC).to_string()
+}
+
+/// Get the log in url including the next url if provided.
+fn get_log_in_url(next_url: Option<&str>) -> String {
+    let mut log_in_url = LOG_IN_URL.to_string();
+    if let Some(next_url) = sanitize_next_url(next_url) {
+        log_in_url = format!("{log_in_url}?next_url={}", encode_next_url(&next_url));
+    }
+    log_in_url
+}
+
+/// Sanitize a `next_url` value ensuring it points to an in-site path.
+fn sanitize_next_url(next_url: Option<&str>) -> Option<String> {
+    let value = next_url?.trim();
+    if value.is_empty() {
+        return None;
+    }
+    if !value.starts_with('/') || value.starts_with("//") {
+        return None;
+    }
+    Some(value.to_string())
+}