Reapply "Add validation for remaining setting types (#37519)" (#38031)

This reverts commit d78f324c, which reverted 7b0a906a.

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)))

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)))))))))