Fix infinite loop when logging error when fetching `site-locale` (#32378)

* Fix infinite loop when logging error when fetching `:site-locale` Setting

* Fix bad performance change

* Remove more dumb optimizations

* Fix references to removed var

src/metabase/models/setting.clj

@@ -481,18 +481,19 @@
         db-is-set-up? (or (requiring-resolve 'metabase.db/db-is-set-up?)
                           ;; this should never be hit. it is just overly cautious against a NPE here. But no way this
                           ;; cannot resolve
-                          (constantly false))]
+                          (constantly false))
+        db-value      #(t2/select-one-fn :value Setting :key (setting-name setting-definition-or-name))]
     ;; cannot use db (and cache populated from db) if db is not set up
     (when (and (db-is-set-up?) (allows-site-wide-values? setting))
       (let [v (if *disable-cache*
-                (t2/select-one-fn :value Setting :key (setting-name setting-definition-or-name))
+                (db-value)
                 (do
                   (setting.cache/restore-cache-if-needed!)
                   (let [cache (setting.cache/cache)]
                     (if (nil? cache)
                       ;; If another thread is populating the cache for the first time, we will have a nil value for
                       ;; the cache and must hit the db while the cache populates
-                      (t2/select-one-fn :value Setting :key (setting-name setting-definition-or-name))
+                      (db-value)
                       (clojure.core/get cache (setting-name setting-definition-or-name))))))]
         (not-empty v)))))
 
@@ -615,12 +616,10 @@
   if any."
   [setting-definition-or-name]
   (let [{:keys [cache? getter enabled? default]} (resolve-setting setting-definition-or-name)
-        disable-cache?                           (not cache?)]
+        disable-cache?                           (or *disable-cache* (not cache?))]
     (if (or (nil? enabled?) (enabled?))
-      (if (= *disable-cache* disable-cache?)
-        (getter)
-        (binding [*disable-cache* disable-cache?]
-          (getter)))
+      (binding [*disable-cache* disable-cache?]
+        (getter))
       default)))
 
 

src/metabase/public_settings.clj

@@ -201,6 +201,10 @@
     (application-name-for-setting-descriptions))
   :default    "en"
   :visibility :public
+  :getter     (fn []
+                (let [value (setting/get-value-of-type :string :site-locale)]
+                  (when (i18n/available-locale? value)
+                    value)))
   :setter     (fn [new-value]
                 (when new-value
                   (when-not (i18n/available-locale? new-value)

src/metabase/util/i18n/impl.clj

@@ -203,31 +203,25 @@
              (log/errorf e "Invalid format string %s" (pr-str format-string))
              format-string)))))))
 
-;; We can't fetch the system locale until the application DB has been initiailized. Once that's done, we don't need to
-;; do the check anymore -- swapping out the getter fn with the simpler one speeds things up substantially
-(def ^:private site-locale-from-setting-fn
-  (atom
-   (fn []
-     (when-let [db-is-set-up? (resolve 'metabase.db/db-is-set-up?)]
-       (when (and (bound? db-is-set-up?)
-                  (db-is-set-up?))
-         (when-let [get-value-of-type (resolve 'metabase.models.setting/get-value-of-type)]
-           (when (bound? get-value-of-type)
-             (let [f (fn [] (get-value-of-type :string :site-locale))]
-               (reset! site-locale-from-setting-fn f)
-               (f)))))))))
+(def ^:private ^:dynamic *in-site-locale-from-setting*
+  "Whether we're currently inside a call to [[site-locale-from-setting]], so we can prevent infinite recursion."
+  false)
 
 (defn site-locale-from-setting
-  "Fetch the value of the `site-locale` Setting.
-  When metabase is shutting down, we need to log some messages after the db connection is closed, so we keep around a
-  cached-site-locale for that purpose."
+  "Fetch the value of the `site-locale` Setting, or `nil` if it is unset."
   []
-  (let [cached-site-locale (atom "en")]
-    (try
-      (let [site-locale (@site-locale-from-setting-fn)]
-        (reset! cached-site-locale site-locale)
-        site-locale)
-      (catch Exception _ @cached-site-locale))))
+  (when-let [get-value-of-type (resolve 'metabase.models.setting/get-value-of-type)]
+    (when (bound? get-value-of-type)
+      ;; make sure we don't try to recursively fetch the site locale when we're actively in the process of fetching it,
+      ;; otherwise that will cause infinite loops if we try to log anything... see #32376
+      (when-not *in-site-locale-from-setting*
+        (binding [*in-site-locale-from-setting* true]
+          ;; if there is an error fetching the Setting, e.g. if the app DB is in the process of shutting down, then just
+          ;; return nil.
+          (try
+            (get-value-of-type :string :site-locale)
+            (catch Exception _
+              nil)))))))
 
 (defmethod print-method Locale
   [locale ^java.io.Writer writer]

test/metabase/cmd/dump_to_h2_test.clj

@@ -71,7 +71,7 @@
                           h2-file-enc         (format "out-%s.db" (mt/random-name))
                           h2-file-default-enc (format "out-%s.db" (mt/random-name))]
         (mt/test-drivers #{:h2 :postgres :mysql}
-          (with-redefs [i18n.impl/site-locale-from-setting-fn (atom (constantly false))]
+          (with-redefs [i18n.impl/site-locale-from-setting (constantly nil)]
             (binding [setting/*disable-cache*         true
                       mdb.connection/*application-db* (mdb.connection/application-db
                                                        driver/*driver*

test/metabase/cmd/load_and_dump_test.clj

@@ -38,7 +38,7 @@
                                (mdb.spec/spec driver/*driver* details))))]
           (binding [setting/*disable-cache*         true
                     mdb.connection/*application-db* (mdb.connection/application-db driver/*driver* data-source)]
-            (with-redefs [i18n.impl/site-locale-from-setting-fn (atom (constantly false))]
+            (with-redefs [i18n.impl/site-locale-from-setting (constantly nil)]
               (when-not (= driver/*driver* :h2)
                 (tx/create-db! driver/*driver* {:database-name db-name}))
               (load-from-h2/load-from-h2! h2-fixture-db-file)

test/metabase/public_settings_test.clj

@@ -127,41 +127,44 @@
            #"Values greater than 204,800 \(200\.0 MB\) are not allowed"
            (public-settings/query-caching-max-kb! (* 1024 1024)))))))
 
-(deftest site-locale-test
-  (testing "site-locale Setting"
-    (testing "should validate input"
-      (testing "invalid format"
-        (testing "blank string"
-          (mt/with-temporary-setting-values [site-locale "en_US"]
-            (is (thrown-with-msg?
-                 clojure.lang.ExceptionInfo
-                 #"Invalid locale \"\""
-                 (public-settings/site-locale! "")))
-            (is (= "en_US"
-                   (public-settings/site-locale)))))
-
-        (testing "non-existant locale"
-          (mt/with-temporary-setting-values [site-locale "en_US"]
-            (is (thrown-with-msg?
-                 clojure.lang.ExceptionInfo
-                 #"Invalid locale \"en_EN\""
-                 (public-settings/site-locale! "en_EN")))
-            (is (= "en_US"
-                   (public-settings/site-locale)))))))
-
-    (testing "should normalize input"
-      (mt/discard-setting-changes [site-locale]
-        (public-settings/site-locale! "en-us")
+(deftest site-locale-validate-input-test
+  (testing "site-locale should validate input"
+    (testing "blank string"
+      (mt/with-temporary-setting-values [site-locale "en_US"]
+        (is (thrown-with-msg?
+             clojure.lang.ExceptionInfo
+             #"Invalid locale \"\""
+             (public-settings/site-locale! "")))
         (is (= "en_US"
                (public-settings/site-locale)))))
-
-    (testing "should be able to unset site locale"
-      (mt/discard-setting-changes [site-locale]
-        (public-settings/site-locale! "es")
-        (public-settings/site-locale! nil)
-        (is (= "en"
-               (public-settings/site-locale))
-            "should default to English")))))
+    (testing "non-existant locale"
+      (mt/with-temporary-setting-values [site-locale "en_US"]
+        (is (thrown-with-msg?
+             clojure.lang.ExceptionInfo
+             #"Invalid locale \"en_EN\""
+             (public-settings/site-locale! "en_EN")))
+        (is (= "en_US"
+               (public-settings/site-locale)))))))
+
+(deftest site-locale-normalize-input-test
+  (testing "site-locale should normalize input"
+    (mt/discard-setting-changes [site-locale]
+      (public-settings/site-locale! "en-us")
+      (is (= "en_US"
+             (public-settings/site-locale))))))
+
+(deftest unset-site-locale-test
+  (testing "should be able to unset site-locale"
+    (mt/discard-setting-changes [site-locale]
+      (public-settings/site-locale! "es")
+      (public-settings/site-locale! nil)
+      (is (= "en"
+             (public-settings/site-locale))
+          "should default to English"))))
+
+(deftest site-locale-only-return-valid-locales-test
+  (mt/with-temporary-raw-setting-values [site-locale "wow_this_in_not_a_locale"]
+    (is (nil? (public-settings/site-locale)))))
 
 (deftest redirect-all-requests-to-https-test
   (testing "Shouldn't be allowed to set `redirect-all-requests-to-https` to `true` unless `site-url` is HTTPS"

test/metabase/util/i18n/impl_test.clj

 (ns ^:mb/once metabase.util.i18n.impl-test
   (:require
    [clojure.test :refer :all]
+   [metabase.models.setting :as setting]
+   [metabase.public-settings :as public-settings]
    [metabase.test :as mt]
+   [metabase.util.encryption :as encryption]
+   [metabase.util.encryption-test :as encryption-test]
+   [metabase.util.i18n :as i18n]
    [metabase.util.i18n.impl :as i18n.impl])
   (:import
    (java.util Locale)))
@@ -117,3 +122,32 @@
     (testing "if the original format string is busted, should just return format-string as-is (better than nothing)"
       (is (= "Bad original {a}"
              (i18n.impl/translate "ba-DD" "Bad original {a}" [100]))))))
+
+(deftest avoid-infinite-i18n-loops-test
+  (testing "recursive calls to site-locale should not result in infinite loops (#32376)"
+    (mt/discard-setting-changes [site-locale]
+      (encryption-test/with-secret-key "secret_key__1"
+        ;; set `site-locale` to something encrypted with the first encryption key.
+        (mt/with-temporary-setting-values [site-locale "en"]
+          (binding [setting/*disable-cache* true]
+            (is (= "en"
+                   (i18n.impl/site-locale-from-setting)))
+            ;; rotate the encryption key, which will trigger an error being logged
+            ;; in [[metabase.util.encryption/maybe-decrypt]]... this will cause a Stack Overflow if `log/error` tries to
+            ;; access `:site-locale` recursively to log the message.
+            (encryption-test/with-secret-key "secret_key__2"
+              (testing "low-level functions should return the encrypted String since we can't successfully decrypt it"
+                ;; not 100% sure this general behavior makes sense for values that we cannot decrypt, but invalid
+                ;; locales are handled by the high-level functions below.
+                (is (encryption/possibly-encrypted-string? (#'setting/db-or-cache-value :site-locale))
+                    `setting/db-or-cache-value)
+                (is (encryption/possibly-encrypted-string? (i18n.impl/site-locale-from-setting))
+                    `i18n.impl/site-locale-from-setting))
+              (testing "since the encrypted string is an invalid value for a Locale, high-level functions should return nil"
+                (is (nil? (i18n/site-locale))
+                    `i18n/site-locale)
+                (is (nil? (public-settings/site-locale))
+                    `public-settings/site-locale))
+              (testing "we should still be able to (no-op) i18n stuff"
+                (is (= "Testing"
+                       (i18n/trs "Testing")))))))))))