diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index ce1c45ce22e9b0754755729e6d987ae4cde331c1..90aecc99ba92f12fd683ff77e0ded82601d6dbca 100644
--- a/src/metabase/models/setting.clj
+++ b/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)))
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index ec880a91ed09069b222dc6a51c4f22ccc7c94f0b..42288f4b888c8847cb11910dd2f425e134dae28b 100644
--- a/src/metabase/public_settings.clj
+++ b/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)
diff --git a/src/metabase/util/i18n/impl.clj b/src/metabase/util/i18n/impl.clj
index 62c4b34d3ad90a5216e94efba6e968dc85826ca9..743eb9bcf86269f3bfa86ecb21c4300c7342584f 100644
--- a/src/metabase/util/i18n/impl.clj
+++ b/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]
diff --git a/test/metabase/cmd/dump_to_h2_test.clj b/test/metabase/cmd/dump_to_h2_test.clj
index c0ef21b3de574195d56c3b6fd0266355aa617d3b..7300b08a8d8c0df3603acb7cfdd106a98cb4cf40 100644
--- a/test/metabase/cmd/dump_to_h2_test.clj
+++ b/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*
diff --git a/test/metabase/cmd/load_and_dump_test.clj b/test/metabase/cmd/load_and_dump_test.clj
index 6570a2c9df72de7059014e5bcb311a68f661f651..f344c68e0f8e4305c65f60aa6fe19e0f0e40afcc 100644
--- a/test/metabase/cmd/load_and_dump_test.clj
+++ b/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)
diff --git a/test/metabase/public_settings_test.clj b/test/metabase/public_settings_test.clj
index de14f8a50d0fbe8de0a0b54d7c3cbb31919dc585..4202873e9bf77d314f3603d956993b65221a2874 100644
--- a/test/metabase/public_settings_test.clj
+++ b/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"
diff --git a/test/metabase/util/i18n/impl_test.clj b/test/metabase/util/i18n/impl_test.clj
index eddf455476688dd6ef44d28f67d79fa45c2bb2d9..ec60aa51e6a83d236f36c6db4e4cbd61e59ebb38 100644
--- a/test/metabase/util/i18n/impl_test.clj
+++ b/test/metabase/util/i18n/impl_test.clj
@@ -1,7 +1,12 @@
(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")))))))))))