restrict open redirects imitating no host in them (#45278)

enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj

@@ -54,7 +54,6 @@
    [saml20-clj.core :as saml]
    [toucan2.core :as t2])
   (:import
-   (java.net URI URISyntaxException)
    (java.util Base64)))
 
 (set! *warn-on-reflection* true)
@@ -129,11 +128,6 @@
   (api/check (sso-settings/saml-enabled)
     [400 (tru "SAML has not been enabled and/or configured")]))
 
-(defn- has-host? [uri]
-  (try
-    (-> uri URI. .getHost some?)
-    (catch URISyntaxException _ false)))
-
 (defmethod sso.i/sso-get :saml
   ;; Initial call that will result in a redirect to the IDP along with information about how the IDP can authenticate
   ;; and redirect them back to us
@@ -145,9 +139,9 @@
                        (do
                          (log/warn "Warning: expected `redirect` param, but none is present")
                          (public-settings/site-url))
-                       (if (has-host? redirect)
-                         redirect
-                         (str (public-settings/site-url) redirect)))]
+                       (if (sso-utils/relative-uri? redirect)
+                         (str (public-settings/site-url) redirect)
+                         redirect))]
     (sso-utils/check-sso-redirect redirect-url)
     (try
       (let [idp-url      (sso-settings/saml-identity-provider-uri)

enterprise/backend/src/metabase_enterprise/sso/integrations/sso_utils.clj

@@ -16,7 +16,7 @@
    [toucan2.core :as t2])
   (:import
    (clojure.lang ExceptionInfo)
-   (java.net URI)))
+   (java.net URI URISyntaxException)))
 
 (set! *warn-on-reflection* true)
 
@@ -85,15 +85,30 @@
           (t2/update! User id user-data)
           (t2/select-one User :id id))))))
 
+(defn relative-uri?
+  "Checks that given `uri` is not an absolute (so no scheme and no host)."
+  [uri]
+  (let [^URI uri (if (string? uri)
+                   (try
+                     (URI. uri)
+                     (catch URISyntaxException _
+                       nil))
+                   uri)]
+    (or (nil? uri)
+        (and (nil? (.getHost uri))
+             (nil? (.getScheme uri))))))
+
 (defn check-sso-redirect
   "Check if open redirect is being exploited in SSO. If so, or if the redirect-url is invalid, throw a 400."
   [redirect-url]
   (try
-    (let [host        (some-> redirect-url (URI.) (.getHost))
-          our-host    (some-> (public-settings/site-url) (URI.) (.getHost))]
-      (api/check-400 (or (nil? redirect-url) (nil? host) (= host our-host))))
+    (let [redirect (some-> redirect-url (URI.))
+          our-host (some-> (public-settings/site-url) (URI.) (.getHost))]
+      (api/check-400 (or (nil? redirect-url)
+                         (relative-uri? redirect)
+                         (= (.getHost redirect) our-host))))
     (catch Exception e
       (log/error e "Invalid redirect URL")
       (throw (ex-info (tru "Invalid redirect URL")
-                      {:status-code 400
+                      {:status-code  400
                        :redirect-url redirect-url})))))

enterprise/backend/test/metabase_enterprise/sso/integrations/jwt_test.clj

@@ -177,7 +177,8 @@
   (testing "Check that we prevent open redirects to untrusted sites"
     (with-jwt-default-setup
       (doseq [redirect-uri ["https://badsite.com"
-                            "//badsite.com"]]
+                            "//badsite.com"
+                            "https:///badsite.com"]]
         (is (= "Invalid redirect URL"
                (-> (client/client
                     :get 400 "/auth/sso" {:request-options {:redirect-strategy :none}}

enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj

@@ -421,7 +421,8 @@
                          "   "
                          "/"
                          "https://badsite.com"
-                         "//badsite.com"]]
+                         "//badsite.com"
+                         "https:///badsite.com"]]
       (testing (format "\nRelayState = %s" (pr-str relay-state))
         (with-saml-default-setup
           (do-with-some-validators-disabled
@@ -436,7 +437,8 @@
 
   (testing "if the RelayState leads us to the wrong host, avoid the open redirect (boat#160)"
     (doseq [redirect-url ["https://badsite.com"
-                          "//badsite.com"]]
+                          "//badsite.com"
+                          "https:///badsite.com"]]
       (with-saml-default-setup
         (mt/with-temporary-setting-values [site-url "http://localhost:3000"]
           (do-with-some-validators-disabled
@@ -444,7 +446,8 @@
              (let [get-response (client/client :get 400 "/auth/sso"
                                   {:request-options {:redirect-strategy :none}}
                                   :redirect redirect-url)]
-               (is (= "Invalid redirect URL" (:message get-response)))))))))))
+               (testing (format "\n%s should not redirect" redirect-url)
+                 (is (= "Invalid redirect URL" (:message get-response))))))))))))
 
 (deftest login-create-account-test
   (testing "A new account will be created for a SAML user we haven't seen before"

enterprise/backend/test/metabase_enterprise/sso/integrations/sso_utils_test.clj

@@ -10,7 +10,6 @@
        "/"
        "/test"
        "localhost"
-       "localhost:3000"
        "http://localhost:3000"
        "http://localhost:3000/dashboard/1-test-dashboard?currency=British%20Pound"))
 
@@ -19,6 +18,7 @@
        "http://example.com"
        "//example.com"
        "not a url"
+       "localhost:3000" ; URI thinks `localhost` here is scheme
        "http://localhost:3000?a=not a param"))))
 
 (deftest create-new-sso-user-test