make site url validation more permissive. (#11710)

close #10469

src/metabase/util.clj

@@ -25,7 +25,7 @@
            java.util.concurrent.TimeoutException
            java.util.Locale
            javax.xml.bind.DatatypeConverter
-           org.apache.commons.validator.routines.UrlValidator))
+           [org.apache.commons.validator.routines RegexValidator UrlValidator]))
 
 ;; This is the very first log message that will get printed.
 ;;
@@ -100,7 +100,9 @@
 (defn url?
   "Is `s` a valid HTTP/HTTPS URL string?"
   ^Boolean [s]
-  (let [validator (UrlValidator. (varargs String ["http" "https"]) UrlValidator/ALLOW_LOCAL_URLS)]
+  (let [validator (UrlValidator. (varargs String ["http" "https"])
+                                 (RegexValidator. "^\\p{Alnum}+([\\.|\\-]\\p{Alnum}+)*(:\\d*)?")
+                                 UrlValidator/ALLOW_LOCAL_URLS)]
     (.isValid validator (str s))))
 
 (defn maybe?

test/metabase/public_settings_test.clj

 (ns metabase.public-settings-test
   (:require [clojure.test :refer :all]
             [environ.core :as env]
-            [expectations :refer [expect]]
             [metabase.models.setting :as setting]
             [metabase.public-settings :as public-settings]
             [metabase.test
@@ -13,95 +12,88 @@
 
 (use-fixtures :once (fixtures/initialize :db))
 
- ;; double-check that setting the `site-url` setting will automatically strip off trailing slashes
-(expect
-  "http://localhost:3000"
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "http://localhost:3000/")
-    (public-settings/site-url)))
+;; double-check that setting the `site-url` setting will automatically strip off trailing slashes
+(deftest site-url-settings
+  (is (= "http://localhost:3000"
+         (tu/with-temporary-setting-values [site-url nil]
+           (public-settings/site-url "http://localhost:3000/")
+           (public-settings/site-url)))))
 
- ;; double-check that setting the `site-url` setting will prepend `http://` if no protocol was specified
-(expect
-  "http://localhost:3000"
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "localhost:3000")
-    (public-settings/site-url)))
+;; double-check that setting the `site-url` setting will prepend `http://` if no protocol was specified
+(deftest site-url-settings-prepend-http
+  (is (= "http://localhost:3000"
+         (tu/with-temporary-setting-values [site-url nil]
+           (public-settings/site-url "localhost:3000")
+           (public-settings/site-url)))))
 
-(expect
-  "http://localhost:3000"
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "localhost:3000")
-    (public-settings/site-url)))
-
-(expect
-  "http://localhost:3000"
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "http://localhost:3000")
-    (public-settings/site-url)))
+(deftest site-url-settings-with-no-trailing-slash
+  (is (= "http://localhost:3000"
+         (tu/with-temporary-setting-values [site-url nil]
+           (public-settings/site-url "http://localhost:3000")
+           (public-settings/site-url)))))
 
 ;; if https:// was specified it should keep it
-(expect
-  "https://localhost:3000"
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "https://localhost:3000")
-    (public-settings/site-url)))
+(deftest site-url-settings-https
+  (is (= "https://localhost:3000"
+         (tu/with-temporary-setting-values [site-url nil]
+           (public-settings/site-url "https://localhost:3000")
+           (public-settings/site-url)))))
 
 ;; we should not be allowed to set an invalid `site-url` (#9850)
-(expect
-  AssertionError
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "http://https://www.camsaul.com")))
+(deftest site-url-settings-validate-site-url
+  (is (thrown? AssertionError
+               (tu/with-temporary-setting-values [site-url nil]
+                 (public-settings/site-url "http://https://www.camsaul.com")))))
 
-(expect
-  AssertionError
-  (tu/with-temporary-setting-values [site-url nil]
-    (public-settings/site-url "https://www.camsaul.x")))
+(deftest site-url-settings-set-valid-domain-name
+  (is (tu/with-temporary-setting-values [site-url nil]
+        (public-settings/site-url "https://www.camsaul.x"))))
 
 ;; if `site-url` in the database is invalid, the getter for `site-url` should return `nil` (#9849)
-(expect
-  {:get-string "https://www.camsaul.x", :site-url nil}
-  (tu.log/suppress-output
-    (tu/with-temporary-setting-values [site-url "https://metabase.com"]
-      (setting/set-string! :site-url "https://www.camsaul.x")
-      {:get-string (setting/get-string :site-url)
-       :site-url   (public-settings/site-url)})))
+(deftest site-url-settings-nil-getter-when-invalid
+  (is (= {:get-string "https://&", :site-url nil}
+         (tu.log/suppress-output
+          (tu/with-temporary-setting-values [site-url "https://metabase.com"]
+            (setting/set-string! :site-url "https://&")
+            {:get-string (setting/get-string :site-url)
+             :site-url   (public-settings/site-url)})))))
 
 ;; We should normalize `site-url` when set via env var we should still normalize it (#9764)
-(expect
-  {:get-string "localhost:3000/", :site-url "http://localhost:3000"}
-  (with-redefs [env/env (assoc env/env :mb-site-url "localhost:3000/")]
-    (tu/with-temporary-setting-values [site-url nil]
-      {:get-string (setting/get-string :site-url)
-       :site-url   (public-settings/site-url)})))
+(deftest site-url-settings-normalize
+  (is (= {:get-string "localhost:3000/", :site-url "http://localhost:3000"}
+         (with-redefs [env/env (assoc env/env :mb-site-url "localhost:3000/")]
+           (tu/with-temporary-setting-values [site-url nil]
+             {:get-string (setting/get-string :site-url)
+              :site-url   (public-settings/site-url)})))))
 
 (deftest invalid-site-url-env-var-test
   {:get-string "asd_12w31%$;", :site-url nil}
   (testing (str "If `site-url` is set via an env var, and it's invalid, we should return `nil` rather than having the"
                 " whole instance break")
     (tu.log/suppress-output
-      (with-redefs [env/env (assoc env/env :mb-site-url "asd_12w31%$;")]
-        (tu/with-temporary-setting-values [site-url nil]
-          (is (= "asd_12w31%$;"
-                 (setting/get-string :site-url)))
-          (is (= nil
-                 (public-settings/site-url))))))))
+     (with-redefs [env/env (assoc env/env :mb-site-url "asd_12w31%$;")]
+       (tu/with-temporary-setting-values [site-url nil]
+         (is (= "asd_12w31%$;"
+                (setting/get-string :site-url)))
+         (is (= nil
+                (public-settings/site-url))))))))
 
-(expect
-  "HOST"
-  (let [zz (i18n/string-as-locale "zz")]
-    (i18n/with-user-locale zz
-      (str (:display-name (first (get-in (public-settings/public-settings) [:engines :postgres :details-fields])))))))
+(deftest translate-public-setting
+  (is (= "HOST"
+         (let [zz (i18n/string-as-locale "zz")]
+           (i18n/with-user-locale zz
+             (str (:display-name (first (get-in (public-settings/public-settings) [:engines :postgres :details-fields])))))))))
 
-(expect
-  [true "HOST"]
-  (let [zz (i18n/string-as-locale "zz")]
-    (i18n/with-user-locale zz
-      [(= zz (i18n/user-locale))
-       (tru "Host")])))
+(deftest tru-translates
+  (is (= [true "HOST"]
+         (let [zz (i18n/string-as-locale "zz")]
+           (i18n/with-user-locale zz
+             [(= zz (i18n/user-locale))
+              (tru "Host")])))))
 
 ;; Make sure Max Cache Entry Size can be set via with a string value, which is what comes back from the API (#9143)
-(expect
-  "1000"
-  ;; use with temp value macro so original value gets reset after test run
-  (tu/with-temporary-setting-values [query-caching-max-kb nil]
-    (public-settings/query-caching-max-kb "1000")))
+(deftest max-cache-entry
+  (is (= "1000"
+         ;; use with temp value macro so original value gets reset after test run
+         (tu/with-temporary-setting-values [query-caching-max-kb nil]
+           (public-settings/query-caching-max-kb "1000")))))

test/metabase/util_test.clj

@@ -32,10 +32,12 @@
 (expect true  (u/url? "http://www.cool.com:3000"))
 (expect true  (u/url? "http://localhost:3000/auth/reset_password/144_f98987de-53ca-4335-81da-31bb0de8ea2b#new"))
 (expect true  (u/url? "http://192.168.1.10/"))
+(expect true  (u/url? "http://metabase.intranet/"))
 (expect false (u/url? "google.com"))                      ; missing protocol
 (expect false (u/url? "ftp://metabase.com"))              ; protocol isn't HTTP/HTTPS
 (expect false (u/url? "http://.com"))                     ; no domain
 (expect false (u/url? "http://google."))                  ; no TLD
+(expect false (u/url? "http://"))                         ; no domain or tld
 (expect false (u/url? "http:/"))                          ; nil .getAuthority needs to be handled or NullPointerException