coderabbit suggestion

Author: parmesant

src/cli.rs

@@ -818,59 +818,3 @@ 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

@@ -654,16 +654,22 @@ impl actix_web::ResponseError for OIDCError {
 }
 
 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)))
+    let Ok(redirect_url) = Url::parse(redirect_url) else {
+        return false;
+    };
+
+    let redirect_origin = redirect_url.origin();
+
+    if PARSEABLE
+        .options
+        .allow_origins
+        .iter()
+        .any(|url| url.origin() == redirect_origin)
     {
-        true
-    }
-    // if redirect_url is not part of allowed origins, then it must be equal to base url
-    else {
-        base_url == redirect_url
+        return true;
     }
+
+    Url::parse(base_url)
+        .map(|url| url.origin() == redirect_origin)
+        .unwrap_or(false)
 }