site-url setting fixes

project.clj

@@ -50,7 +50,8 @@
    [amalloy/ring-gzip-middleware "0.1.3"]                             ; Ring middleware to GZIP responses if client can handle it
    [aleph "0.4.6" :exclusions [org.clojure/tools.logging]]            ; Async HTTP library; WebSockets
    [bigml/histogram "4.1.3"]                                          ; Histogram data structure
-   [buddy/buddy-core "1.5.0"]                                         ; various cryptograhpic functions
+   [buddy/buddy-core "1.5.0"                                          ; various cryptograhpic functions
+    :exclusions [commons-codec]]
    [buddy/buddy-sign "3.0.0"]                                         ; JSON Web Tokens; High-Level message signing library
    [cheshire "5.8.1"]                                                 ; fast JSON encoding (used by Ring JSON middleware)
    [clj-http "3.9.1"                                                  ; HTTP client
@@ -75,6 +76,12 @@
    [com.mattbertolini/liquibase-slf4j "2.0.0"]                        ; Java Migrations lib logging. We don't actually use this AFAIK (?)
    [com.mchange/c3p0 "0.9.5.3"]                                       ; connection pooling library
    [com.taoensso/nippy "2.14.0"]                                      ; Fast serialization (i.e., GZIP) library for Clojure
+   [commons-codec/commons-codec "1.12"]                               ; Apache Commons -- useful codec util fns
+   [commons-io/commons-io "2.6"]                                      ; Apache Commons -- useful IO util fns
+   [commons-validator/commons-validator "1.6"                         ; Apache Commons -- useful validation util fns
+    :exclusions [commons-beanutils
+                 commons-digester
+                 commons-logging]]
    [compojure "1.6.1" :exclusions [ring/ring-codec]]                  ; HTTP Routing library built on Ring
    [crypto-random "1.2.0"]                                            ; library for generating cryptographically secure random bytes and strings
    [dk.ative/docjure "1.13.0"]                                        ; Excel export

src/metabase/api/email.clj

@@ -74,7 +74,7 @@
   (api/check-superuser)
   (let [email-settings (select-keys settings (keys mb-to-smtp-settings))
         smtp-settings  (-> (set/rename-keys email-settings mb-to-smtp-settings)
-                           (assoc :port (Integer/parseInt (:email-smtp-port settings))))
+                           (assoc :port (some-> (:email-smtp-port settings) Integer/parseInt)))
         response       (if-not config/is-test?
                          ;; in normal conditions, validate connection
                          (email/test-smtp-connection smtp-settings)

src/metabase/email.clj

@@ -35,7 +35,7 @@
   (tru "SMTP secure connection protocol. (tls, ssl, starttls, or none)")
   :default (tru "none")
   :setter  (fn [new-value]
-             (when-not (nil? new-value)
+             (when (some? new-value)
                (assert (contains? #{"tls" "ssl" "none" "starttls"} new-value)))
              (setting/set-string! :email-smtp-security new-value)))
 
@@ -52,12 +52,14 @@
   (boolean (email-smtp-host)))
 
 (defn- add-ssl-settings [m ssl-setting]
-  (merge m (case (keyword ssl-setting)
-             :tls {:tls true}
-             :ssl {:ssl true}
-             :starttls {:starttls.enable true
-                        :starttls.required true}
-             {})))
+  (merge
+   m
+   (case (keyword ssl-setting)
+     :tls      {:tls true}
+     :ssl      {:ssl true}
+     :starttls {:starttls.enable   true
+                :starttls.required true}
+     {})))
 
 (defn- smtp-settings []
   (-> {:host (email-smtp-host)

src/metabase/middleware/misc.clj

@@ -45,7 +45,10 @@
     (when-not (public-settings/site-url)
       (when-let [site-url (or origin host)]
         (log/info (trs "Setting Metabase site URL to {0}" site-url))
-        (public-settings/site-url site-url)))))
+        (try
+          (public-settings/site-url site-url)
+          (catch Throwable e
+            (log/warn e (trs "Failed to set site-url"))))))))
 
 (defn maybe-set-site-url
   "Middleware to set the `site-url` Setting if it's unset the first time a request is made."

src/metabase/public_settings.clj

 (ns metabase.public-settings
   (:require [clojure.string :as str]
+            [clojure.tools.logging :as log]
             [metabase
              [config :as config]
-             [types :as types]]
+             [types :as types]
+             [util :as u]]
             [metabase.driver.util :as driver.u]
             [metabase.models
              [common :as common]
              [setting :as setting :refer [defsetting]]]
             [metabase.public-settings.metastore :as metastore]
             [metabase.util
-             [i18n :refer [available-locales-with-names set-locale tru]]
+             [i18n :refer [available-locales-with-names set-locale trs tru]]
              [password :as password]]
             [toucan.db :as db])
   (:import [java.util TimeZone UUID]))
@@ -42,14 +44,29 @@
                      (setting/set-string! :site-uuid value)
                      value))))
 
+(defn- normalize-site-url [^String s]
+  (let [ ;; remove trailing slashes
+        s (str/replace s #"/$" "")
+        ;; add protocol if missing
+        s (if (str/starts-with? s "http")
+            s
+            (str "http://" s))]
+    ;; check that the URL is valid
+    (assert (u/url? s)
+      (str (tru "Invalid site URL: {0}" s)))
+    s))
+
 ;; This value is *guaranteed* to never have a trailing slash :D
 ;; It will also prepend `http://` to the URL if there's not protocol when it comes in
 (defsetting site-url
   (tru "The base URL of this Metabase instance, e.g. \"http://metabase.my-company.com\".")
+  :getter (fn []
+            (try
+              (some-> (setting/get-string :site-url) normalize-site-url)
+              (catch AssertionError e
+                (log/error e (trs "site-url is invalid; returning nil for now. Will be reset on next request.")))))
   :setter (fn [new-value]
-            (setting/set-string! :site-url (when new-value
-                                             (cond->> (str/replace new-value #"/$" "")
-                                               (not (str/starts-with? new-value "http")) (str "http://"))))))
+            (setting/set-string! :site-url (some-> new-value normalize-site-url))))
 
 (defsetting site-locale
   (str  (tru "The default language for this Metabase instance.")

test/metabase/public_settings_test.clj

 (ns metabase.public-settings-test
-  (:require [expectations :refer :all]
+  (:require [environ.core :as env]
+            [expectations :refer [expect]]
+            [metabase.models.setting :as setting]
             [metabase.public-settings :as public-settings]
             [metabase.test.util :as tu]
             [puppetlabs.i18n.core :as i18n :refer [tru]]))
@@ -37,6 +39,41 @@
     (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")))
+
+(expect
+  AssertionError
+  (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/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)}))
+
+;; 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)})))
+
+;; if `site-url` is set via an env var, and it's invalid, we should return `nil` rather than having the whole instance break
+(expect
+  {:get-string "asd_12w31%$;", :site-url nil}
+  (with-redefs [env/env (assoc env/env :mb-site-url "asd_12w31%$;")]
+    (tu/with-temporary-setting-values [site-url nil]
+      {:get-string (setting/get-string :site-url)
+       :site-url   (public-settings/site-url)})))
+
 (expect
   "HOST"
   (let [zz (i18n/string-as-locale "zz")]