diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index ae7e480b9c5f6aafa878069758e5063281ad3ae0..6977a4cdd39cf9e63930af585c53db72c14af883 100644
--- a/src/metabase/models/setting.clj
+++ b/src/metabase/models/setting.clj
@@ -1398,18 +1398,36 @@
(ex-info (trs "Error of type {0} thrown while parsing a setting" (type ex))
{:ex-type (type ex)}))
-(defn- may-contain-raw-token? [ex setting]
- (case (:type setting)
- :json
- (cond
- (instance? JsonEOFException ex) false
- (instance? JsonParseException ex) true
- :else (do (log/warn ex "Unexpected exception while parsing JSON")
- ;; err on the side of caution
- true))
+(defmulti may-contain-raw-token?
+ "Indicate whether we must redact an exception to avoid leaking sensitive env vars"
+ (fn [_ex setting] (:type setting)))
+
+;; fallback to redaction if we have not defined behaviour for a given format
+(defmethod may-contain-raw-token? :default [_ _] false)
+
+(defmethod may-contain-raw-token? :boolean [_ _] false)
+
+;; Non EOF exceptions will mention the next character
+(defmethod may-contain-raw-token? :csv [ex _] (not (instance? java.io.EOFException ex)))
+
+;; Number parsing exceptions will quote the entire input
+(defmethod may-contain-raw-token? :double [_ _] true)
+(defmethod may-contain-raw-token? :integer [_ _] true)
+(defmethod may-contain-raw-token? :positive-integer [_ _] true)
- ;; TODO: handle the remaining formats explicitly
- true))
+;; Date parsing may quote the entire input, or a particular sub-portion, e.g. a misspelled month name
+(defmethod may-contain-raw-token? :timestamp [_ _] true)
+
+;; Keyword parsing can never fail, but let's be paranoid
+(defmethod may-contain-raw-token? :keyword [_ _] true)
+
+(defmethod may-contain-raw-token? :json [ex _]
+ (cond
+ (instance? JsonEOFException ex) false
+ (instance? JsonParseException ex) true
+ :else (do (log/warn ex "Unexpected exception while parsing JSON")
+ ;; err on the side of caution
+ true)))
(defn- redact-sensitive-tokens [ex raw-value]
(if (may-contain-raw-token? ex raw-value)
@@ -1420,18 +1438,17 @@
"Test whether the value configured for a given setting can be parsed as the expected type.
Returns an map containing the exception if an issue is encountered, or nil if the value passes validation."
[setting]
- (when (= :json (:type setting))
- (try
- (binding [*disable-init* true]
- (get-value-of-type (:type setting) setting))
- nil
- (catch clojure.lang.ExceptionInfo e
- (let [parse-error (or (ex-cause e) e)
- parse-error (redact-sensitive-tokens parse-error setting)
- env-var? (set-via-env-var? setting)]
- (assoc (select-keys setting [:name :type])
- :parse-error parse-error
- :env-var? env-var?))))))
+ (try
+ (binding [*disable-init* true]
+ (get-value-of-type (:type setting) setting))
+ nil
+ (catch clojure.lang.ExceptionInfo e
+ (let [parse-error (or (ex-cause e) e)
+ parse-error (redact-sensitive-tokens parse-error setting)
+ env-var? (set-via-env-var? setting)]
+ (assoc (select-keys setting [:name :type])
+ :parse-error parse-error
+ :env-var? env-var?)))))
(defn validate-settings-formatting!
"Check whether there are any issues with the format of application settings, e.g. an invalid JSON string.
@@ -1441,7 +1458,7 @@
(doseq [invalid-setting (keep validate-setting (vals @registered-settings))]
(if (:env-var? invalid-setting)
(throw (ex-info (trs "Invalid {0} configuration for setting: {1}"
- #_:clj-kondo/ignore (str/upper-case (name (:type invalid-setting)))
+ (u/upper-case-en (name (:type invalid-setting)))
(name (:name invalid-setting)))
(dissoc invalid-setting :parse-error)
(:parse-error invalid-setting)))
diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj
index 04bcd6cfb2f18c5fef97b447d5fa6cb1256e476c..800be56b35c2dd6d92219e5111bcb195211b7d88 100644
--- a/test/metabase/models/setting_test.clj
+++ b/test/metabase/models/setting_test.clj
@@ -1271,45 +1271,186 @@
#"^Surprise!$"
(#'setting/realize x)))))))
-(deftest valid-json-setting-test
- (mt/with-temp-env-var-value! ["MB_TEST_JSON_SETTING" "[1, 2]"]
- (is (nil? (setting/validate-settings-formatting!)))))
-
-(defn- get-parse-exception [raw-value]
- (mt/with-temp-env-var-value! ["MB_TEST_JSON_SETTING" raw-value]
- (try
- (setting/validate-settings-formatting!)
- (throw (java.lang.RuntimeException. "This code should never be reached."))
- (catch java.lang.Exception e e))))
-
-(defn- assert-parser-exception! [ex cause-message]
- (is (= "Invalid JSON configuration for setting: test-json-setting" (ex-message ex)))
+(defn- validation-setting-symbol [format]
+ (symbol (str "test-" (name format) "-validation-setting")))
+
+(defmacro define-setting-for-type [format]
+ `(defsetting ~(validation-setting-symbol format)
+ "Setting to test validation of this format - this only shows up in dev"
+ :type ~(keyword (name format))))
+
+(defmacro get-parse-exception [format raw-value]
+ `(mt/with-temp-env-var-value! [~(symbol (str "mb-" (validation-setting-symbol format))) ~raw-value]
+ (try
+ (setting/validate-settings-formatting!)
+ nil
+ (catch java.lang.Exception e# e#))))
+
+(defn- assert-parser-exception! [format-type ex cause-message]
+ (is (= (format "Invalid %s configuration for setting: %s"
+ (u/upper-case-en (name format-type))
+ (validation-setting-symbol format-type))
+ (ex-message ex)))
(is (= cause-message (ex-message (ex-cause ex)))))
+(define-setting-for-type :json)
+
+(deftest valid-json-setting-test
+ (testing "Validation is a no-op if the JSON is valid"
+ (is (nil? (get-parse-exception :json "[1, 2]")))))
+
(deftest invalid-json-setting-test
(testing "Validation will throw an exception if a setting has invalid JSON via an environment variable"
- (let [ex (get-parse-exception "[1, 2,")]
+ (let [ex (get-parse-exception :json "[1, 2,")]
(assert-parser-exception!
+ :json ex
;; TODO it would be safe to expose the raw Jackson exception here, we could improve redaction logic
#_(str "Unexpected end-of-input within/between Array entries\n"
" at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 7]")
- ex "Error of type class com.fasterxml.jackson.core.JsonParseException thrown while parsing a setting"))))
+ "Error of type class com.fasterxml.jackson.core.JsonParseException thrown while parsing a setting"))))
(deftest sensitive-data-redacted-test
(testing "The exception thrown by validation will not contain sensitive info from the config"
(let [password "$ekr3t"
- ex (get-parse-exception (str "[" password))]
+ ex (get-parse-exception :json (str "[" password))]
(is (not (str/includes? (pr-str ex) password)))
(assert-parser-exception!
- ex "Error of type class com.fasterxml.jackson.core.JsonParseException thrown while parsing a setting"))))
+ :json ex "Error of type class com.fasterxml.jackson.core.JsonParseException thrown while parsing a setting"))))
(deftest safe-exceptions-not-redacted-test
(testing "An exception known not to contain sensitive info will not be redacted"
(let [password "123abc"
- ex (get-parse-exception "{\"a\": \"123abc\", \"b\": 2")]
+ ex (get-parse-exception :json "{\"a\": \"123abc\", \"b\": 2")]
(is (not (str/includes? (pr-str ex) password)))
(assert-parser-exception!
- ex
+ :json ex
(str "Unexpected end-of-input: expected close marker for Object (start marker at [Source: REDACTED"
" (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 1])\n"
" at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 23]")))))
+
+(define-setting-for-type :csv)
+
+(deftest valid-csv-setting-test
+ (testing "Validation is a no-op if the CSV is valid"
+ (is (nil? (get-parse-exception :csv "1, 2")))))
+
+(deftest invalid-csv-setting-eof-test
+ (testing "Validation will throw an exception if a setting has invalid CSV via an environment variable"
+ (let [ex (get-parse-exception :csv "1,2,2,\",,")]
+ (assert-parser-exception!
+ :csv ex "CSV error (unexpected end of file)"))))
+
+(deftest invalid-csv-setting-char-test
+ (testing "Validation will throw an exception if a setting has invalid CSV via an environment variable"
+ (let [ex (get-parse-exception :csv "\"1\"$ekr3t")]
+ (assert-parser-exception!
+ :csv ex
+ ;; we don't expose the raw exception here, as it would give away the first character of the secret
+ #_"CSV error (unexpected character: $)"
+ "Error of type class java.lang.Exception thrown while parsing a setting"))))
+
+(define-setting-for-type :boolean)
+
+(deftest valid-boolean-setting-test
+ (testing "Validation is a no-op if the string represents a boolean"
+ (is (nil? (get-parse-exception :boolean "")))
+ (is (nil? (get-parse-exception :boolean "true")))
+ (is (nil? (get-parse-exception :boolean "false")))))
+
+(deftest invalid-boolean-setting-test
+ (doseq [raw-value ["0" "1" "2" "a" ":b" "[true]"]]
+ (testing (format "Validation will throw an exception when trying to parse %s as a boolean" raw-value)
+ (let [ex (get-parse-exception :boolean raw-value)]
+ (assert-parser-exception!
+ :boolean ex "Invalid value for string: must be either \"true\" or \"false\" (case-insensitive).")))))
+
+(define-setting-for-type :double)
+
+(deftest valid-double-setting-test
+ (testing "Validation is a no-op if the string represents a double"
+ (is (nil? (get-parse-exception :double "1")))
+ (is (nil? (get-parse-exception :double "-1")))
+ (is (nil? (get-parse-exception :double "2.4")))
+ (is (nil? (get-parse-exception :double "1e9")))))
+
+(deftest invalid-double-setting-test
+ (doseq [raw-value ["a" "1.2.3" "0x3" "[2]"]]
+ (testing (format "Validation will throw an exception when trying to parse %s as a double" raw-value)
+ (let [ex (get-parse-exception :double raw-value)]
+ (assert-parser-exception!
+ #_"For input string: \"{raw-value}\""
+ :double ex "Error of type class java.lang.NumberFormatException thrown while parsing a setting")))))
+
+(define-setting-for-type :keyword)
+
+(deftest valid-keyword-setting-test
+ (testing "Validation is a no-op if the string represents a keyword"
+ (is (nil? (get-parse-exception :keyword "1")))
+ (is (nil? (get-parse-exception :keyword "a")))
+ (is (nil? (get-parse-exception :keyword "a/b")))
+ ;; [[keyword]] actually accepts any string without complaint, there is no way to have a parse failure
+ (is (nil? (get-parse-exception :keyword ":a/b")))
+ (is (nil? (get-parse-exception :keyword "a/b/c")))
+ (is (nil? (get-parse-exception :keyword "\"")))))
+
+(define-setting-for-type :integer)
+
+(deftest valid-integer-setting-test
+ (testing "Validation is a no-op if the string represents a integer"
+ (is (nil? (get-parse-exception :integer "1")))
+ (is (nil? (get-parse-exception :integer "-1")))))
+
+(deftest invalid-integer-setting-test
+ (doseq [raw-value ["a" "2.4" "1e9" "1.2.3" "0x3" "[2]"]]
+ (testing (format "Validation will throw an exception when trying to parse %s as a integer" raw-value)
+ (let [ex (get-parse-exception :integer raw-value)]
+ (assert-parser-exception!
+ #_"For input string: \"{raw-value}\""
+ :integer ex "Error of type class java.lang.NumberFormatException thrown while parsing a setting")))))
+
+(define-setting-for-type :positive-integer)
+
+(deftest valid-positive-integer-setting-test
+ (testing "Validation is a no-op if the string represents a positive-integer"
+ (is (nil? (get-parse-exception :positive-integer "1")))
+ ;; somewhat un-intuitively this is legal input, and parses to nil
+ (is (nil? (get-parse-exception :positive-integer "-1")))))
+
+(deftest invalid-positive-integer-setting-test
+ (doseq [raw-value ["a" "2.4" "1e9" "1.2.3" "0x3" "[2]"]]
+ (testing (format "Validation will throw an exception when trying to parse %s as a positive-integer" raw-value)
+ (let [ex (get-parse-exception :positive-integer raw-value)]
+ (assert-parser-exception!
+ #_"For input string: \"{raw-value}\""
+ :positive-integer ex "Error of type class java.lang.NumberFormatException thrown while parsing a setting")))))
+
+(define-setting-for-type :timestamp)
+
+(deftest valid-timestamp-setting-test
+ (testing "Validation is a no-op if the string represents a timestamp"
+ (is (nil? (get-parse-exception :timestamp "2024-01-01")))))
+
+(deftest invalid-timestamp-setting-test
+ (testing "Validation will throw an exception when trying to parse an invalid timestamp"
+ (let [ex (get-parse-exception :timestamp "2024-01-48")]
+ (assert-parser-exception!
+ #_"Text '{raw-value}' could not be parsed, unparsed text found at index 0"
+ :timestamp ex "Error of type class java.time.format.DateTimeParseException thrown while parsing a setting"))))
+
+(defn ns-validation-setting-symbol [format]
+ (symbol "metabase.models.setting-test" (name (validation-setting-symbol format))))
+
+(deftest validation-completeness-test
+ (let [string-formats #{:string :metabase.public-settings/uuid-nonce}
+ formats-to-check (remove string-formats (keys (methods setting/get-value-of-type)))]
+
+ (testing "Every settings format has its redaction predicate defined"
+ (doseq [format formats-to-check]
+ (testing (format "We have defined a redaction multimethod for the %s format" format)
+ (is (some? (format (methods setting/may-contain-raw-token?)))))))
+
+ (testing "Every settings format has tests for its validation"
+ (doseq [format formats-to-check]
+ ;; We operate on trust that tests are added along with this var
+ (testing (format "We have defined a setting for the %s validation tests" format)
+ (is (var? (resolve (ns-validation-setting-symbol format)))))))))