fix: oidc redirect

- add redirect checks for logout
- add `P_ALLOW_ORIGINS` env var to allow redirection to other origins

Author: parmesant

src/cli.rs

@@ -150,6 +150,17 @@ pub struct Options {
     )]
     pub address: String,
 
+    // Origins to allow for cors
+    #[arg(
+        long,
+        env = "P_ALLOW_ORIGINS",
+        required = false,
+        value_delimiter = ',',
+        value_parser = validation::url,
+        help = "Comma separated URLs to allow as origins"
+    )]
+    pub allow_origins: Vec<Url>,
+
     // Actix request timeout in seconds
     #[arg(
         long,
@@ -807,3 +818,59 @@ impl Options {
             })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{Cli, StorageOptions};
+    use clap::Parser;
+
+    #[test]
+    fn parses_comma_separated_allowed_origins() {
+        let cli = Cli::parse_from([
+            "parseable",
+            "local-store",
+            "--allowed-origins",
+            "https://www.google.com/,https://app-staging.parseable.com/",
+        ]);
+
+        let StorageOptions::Local(args) = cli.storage else {
+            panic!("expected local-store args");
+        };
+
+        assert_eq!(args.options.allow_origins.len(), 2);
+        assert_eq!(
+            args.options.allow_origins[0].as_str(),
+            "https://www.google.com/"
+        );
+        assert_eq!(
+            args.options.allow_origins[1].as_str(),
+            "https://app-staging.parseable.com/"
+        );
+    }
+
+    #[test]
+    fn parses_repeated_allowed_origins() {
+        let cli = Cli::parse_from([
+            "parseable",
+            "local-store",
+            "--allowed-origins",
+            "https://www.google.com/",
+            "--allowed-origins",
+            "https://app-staging.parseable.com/",
+        ]);
+
+        let StorageOptions::Local(args) = cli.storage else {
+            panic!("expected local-store args");
+        };
+
+        assert_eq!(args.options.allow_origins.len(), 2);
+        assert_eq!(
+            args.options.allow_origins[0].as_str(),
+            "https://www.google.com/"
+        );
+        assert_eq!(
+            args.options.allow_origins[1].as_str(),
+            "https://app-staging.parseable.com/"
+        );
+    }
+}

src/handlers/http/oidc.rs

@@ -36,7 +36,6 @@ pub fn set_cookie_cross_site(enabled: bool) {
 }
 use chrono::{Duration, TimeDelta};
 use openid::Bearer;
-use regex::Regex;
 use serde::Deserialize;
 use ulid::Ulid;
 use url::Url;
@@ -79,8 +78,8 @@ pub async fn login(
     query: web::Query<RedirectAfterLogin>,
 ) -> Result<HttpResponse, OIDCError> {
     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 base_url = format!("{}://{}/", conn.scheme(), conn.host());
+    if !is_valid_redirect_url(&base_url, query.redirect.as_str()) {
         return Err(OIDCError::BadRequest(
             "Bad Request, Invalid Redirect URL!".to_string(),
         ));
@@ -167,11 +166,22 @@ 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> {
     let oidc_client = OIDC_CLIENT.get();
+    let conn = req.connection_info().clone();
+    let base_url = format!("{}://{}/", conn.scheme(), conn.host());
+
+    if !is_valid_redirect_url(&base_url, query.redirect.as_str()) {
+        return Err(OIDCError::BadRequest(
+            "Bad Request, Invalid Redirect URL!".to_string(),
+        ));
+    }
 
     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);
@@ -185,9 +195,9 @@ pub async fn logout(req: HttpRequest, query: web::Query<RedirectAfterLogin>) ->
         (Some(username), Some(logout_endpoint))
             if Users.is_oauth(&username, &tenant_id).unwrap_or_default() =>
         {
-            redirect_to_oidc_logout(logout_endpoint, &query.redirect)
+            Ok(redirect_to_oidc_logout(logout_endpoint, &query.redirect))
         }
-        _ => redirect_to_client(query.redirect.as_str(), None),
+        _ => Ok(redirect_to_client(query.redirect.as_str(), None)),
     }
 }
 
@@ -643,9 +653,17 @@ impl actix_web::ResponseError for OIDCError {
     }
 }
 
-fn is_valid_redirect_url(base_url_without_scheme: &str, redirect_url: &str) -> bool {
-    let http_scheme_match_regex = Regex::new(r"^(https?://)").unwrap();
-    let redirect_url_without_scheme = http_scheme_match_regex.replace(redirect_url, "");
-
-    base_url_without_scheme == redirect_url_without_scheme
+fn is_valid_redirect_url(base_url: &str, redirect_url: &str) -> bool {
+    // check if redirect_url is part of allowed origins
+    if let allowed_origins = &PARSEABLE.options.allow_origins
+        && (allowed_origins
+            .iter()
+            .any(|url| url.as_str().eq(redirect_url)))
+    {
+        true
+    }
+    // if redirect_url is not part of allowed origins, then it must be equal to base url
+    else {
+        base_url == redirect_url
+    }
 }

src/option.rs

@@ -93,11 +93,10 @@ pub mod validation {
         path::{Path, PathBuf},
     };
 
+    use super::{Compression, Mode};
     use crate::cli::DATASET_FIELD_COUNT_LIMIT;
     use path_clean::PathClean;
 
-    use super::{Compression, Mode};
-
     pub fn file_path(s: &str) -> Result<PathBuf, String> {
         if s.is_empty() {
             return Err("empty path".to_owned());