Clear error message when unable to decrypt encrypted json field (#36701)

src/metabase/models/interface.clj

@@ -17,7 +17,7 @@
    [metabase.util :as u]
    [metabase.util.cron :as u.cron]
    [metabase.util.encryption :as encryption]
-   [metabase.util.i18n :refer [trs tru]]
+   [metabase.util.i18n :refer [tru]]
    [metabase.util.log :as log]
    [metabase.util.malli.registry :as mr]
    [methodical.core :as methodical]
@@ -140,7 +140,7 @@
     (try
       (json/parse-string s keywordize-keys?)
       (catch Throwable e
-        (log/error e (str (trs "Error parsing JSON")))
+        (log/error e "Error parsing JSON")
         s))
     s))
 
@@ -225,9 +225,18 @@
   "Serialize encrypted json."
   (comp encryption/maybe-encrypt json-in))
 
-(def encrypted-json-out
+(defn encrypted-json-out
   "Deserialize encrypted json."
-  (comp json-out-with-keywordization encryption/maybe-decrypt))
+  [v]
+  (let [decrypted (encryption/maybe-decrypt v)]
+    (try
+      (json/parse-string decrypted true)
+      (catch Throwable e
+        (if (or (encryption/possibly-encrypted-string? decrypted)
+                (encryption/possibly-encrypted-bytes? decrypted))
+          (log/error e "Could not decrypt encrypted field! Have you forgot to set MB_ENCRYPTION_SECRET_KEY?")
+          (log/error e "Error parsing JSON"))  ; same message as in `json-out`
+        v))))
 
 ;; cache the decryption/JSON parsing because it's somewhat slow (~500µs vs ~100µs on a *fast* computer)
 ;; cache the decrypted JSON for one hour

src/metabase/util/encryption.clj

@@ -147,22 +147,18 @@
   "If `MB_ENCRYPTION_SECRET_KEY` is set and `v` is encrypted, decrypt `v`; otherwise return `s` as-is. Attempts to check
   whether `v` is an encrypted String, in which case the decrypted String is returned, or whether `v` is encrypted bytes,
   in which case the decrypted bytes are returned."
-  {:arglists '([secret-key? s & {:keys [log-errors?], :or {log-errors? true}}])}
+  {:arglists '([secret-key? s])}
   [& args]
-  (let [[secret-key & more]        (if (and (bytes? (first args)) (string? (second args))) ;TODO: fix hackiness
-                                     args
-                                     (cons default-secret-key args))
-        [v & options]              more
-        {:keys [log-errors?]
-         :or   {log-errors? true}} (apply hash-map options)
+  ;; secret-key as an argument so that tests can pass it directly without using `with-redefs` to run in parallel
+  (let [[secret-key v]     (if (and (bytes? (first args)) (string? (second args)))
+                             args
+                             (cons default-secret-key args))
         log-error-fn (fn [kind ^Throwable e]
-                       (when log-errors?
-                         (log/warn (trs "Cannot decrypt encrypted {0}. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?"
-                                        kind)
-                                   (.getMessage e)
-                                   (u/pprint-to-str (u/filtered-stacktrace e)))))]
+                       (log/warnf e
+                                  "Cannot decrypt encrypted %s. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?"
+                                  kind))]
 
-    (cond (not (some? secret-key))
+    (cond (nil? secret-key)
           v
 
           (possibly-encrypted-string? v)

test/metabase/models/interface_test.clj

@@ -8,9 +8,13 @@
    [metabase.models.field :refer [Field]]
    [metabase.models.interface :as mi]
    [metabase.models.table :refer [Table]]
+   [metabase.test.util.log :as tu.log]
    [metabase.util :as u]
+   [metabase.util.encryption :as encryption]
+   [metabase.util.encryption-test :as encryption-test]
    [toucan2.core :as t2]
-   [toucan2.tools.with-temp :as t2.with-temp]))
+   [toucan2.tools.with-temp :as t2.with-temp])
+  (:import (com.fasterxml.jackson.core JsonParseException)))
 
 ;; let's make sure the `transform-metabase-query`/`transform-metric-segment-definition`/`transform-parameters-list`
 ;; normalization functions respond gracefully to invalid stuff when pulling them out of the Database. See #8914
@@ -130,3 +134,28 @@
                (migrate {:pie.show_legend          legend
                          :pie.show_legend_perecent percent
                          :pie.show_data_labels     labels})))))))
+
+(deftest encrypted-data-with-no-secret-test
+  (encryption-test/with-secret-key nil
+    (testing "Just parses string normally when there is no key and the string is JSON"
+      (is (= {:a 1}
+             (mi/encrypted-json-out "{\"a\": 1}"))))
+    (testing "Also parses string if it's encrypted and JSON"
+      (is (= {:a 1}
+             (encryption-test/with-secret-key "qwe"
+               (mi/encrypted-json-out
+                (encryption/encrypt (encryption/secret-key->hash "qwe") "{\"a\": 1}"))))))
+    (testing "Logs an error message when incoming data looks encrypted"
+      (is (=? [[:error JsonParseException "Could not decrypt encrypted field! Have you forgot to set MB_ENCRYPTION_SECRET_KEY?"]]
+              (tu.log/with-log-messages-for-level :error
+                (mi/encrypted-json-out
+                 (encryption/encrypt (encryption/secret-key->hash "qwe") "{\"a\": 1}"))))))
+    (testing "Invalid JSON throws correct error"
+      (is (=? [[:error JsonParseException "Error parsing JSON"]]
+              (tu.log/with-log-messages-for-level :error
+                (mi/encrypted-json-out "{\"a\": 1"))))
+      (is (=? [[:error JsonParseException "Error parsing JSON"]]
+              (tu.log/with-log-messages-for-level :error
+                (encryption-test/with-secret-key "qwe"
+                  (mi/encrypted-json-out
+                   (encryption/encrypt (encryption/secret-key->hash "qwe") "{\"a\": 1")))))))))

test/metabase/util/encryption_test.clj

@@ -88,7 +88,7 @@
   (some (fn [[level _ message]]
           (and (= level :warn)
                (str/includes? message (str "Cannot decrypt encrypted String. Have you changed or forgot to set "
-                                           "MB_ENCRYPTION_SECRET_KEY? Message seems corrupt or manipulated."))))
+                                           "MB_ENCRYPTION_SECRET_KEY?"))))
         log-messages))
 
 (deftest no-errors-for-unencrypted-test