github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-07-22 22:17:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

✨ Make plugin API validation errors precise and crash-safe (#10667)

Browse Source 
Schema validation reported a generic "Invalid data" message. Report the expected schema and the received value, with a bounded cycle-safe renderer so the error path cannot itself crash.

Fixes #10072

Signed-off-by: Akshit Nassa <akshitnassa412@gmail.com>
Co-authored-by: Akshit Nassa <akshitnassa412@gmail.com>
This commit is contained in:
AK 2026-07-22 03:29:19 -04:00 committed by GitHub
parent 1c917951b6
commit b8e3089ae7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 420 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

209
frontend/src/app/plugins/utils.cljs
View File

@ -328,10 +328,217 @@
[[(str/join "." path) (:message v)]])))
m)))
(def ^:private max-repr-length 100)
(def ^:private max-repr-depth 2)
(def ^:private max-repr-items 5)
(defn- abbreviate
"Shorten `s` to `max-repr-length` code points. Cutting on a UTF-16 code
unit would split surrogate pairs and render astral characters as mojibake."
[s]
(if (> (count s) max-repr-length)
(let [points (js/Array.from s)]
(if (> (alength points) max-repr-length)
(dm/str (.join (.slice points 0 max-repr-length) "") "…")
s))
s))
(defn- value->type
[value]
(cond
(string? value) "string"
(boolean? value) "boolean"
(number? value) "number"
(keyword? value) "keyword"
(map? value) "object"
(coll? value) "array"
(array? value) "array"
(fn? value) "function"
(instance? js/Object value) "object"
:else "unknown"))
(defn- data-property
"Own property `k` of the JS object `o`, or `::skip` when `k` is an accessor.
Plugin proxies expose their contents through getters that read the
application state and may throw, so the error path must not run them."
[o k]
(let [descriptor (js/Object.getOwnPropertyDescriptor o k)]
(if (and (some? descriptor)
(undefined? (unchecked-get descriptor "get")))
(unchecked-get descriptor "value")
::skip)))
(defn- value->repr
"Bounded representation of a value received from a plugin. Such values are
arbitrary JS data: `pr-str` never returns on a self referencing object
(`a.parent.child === a`), so the traversal is capped in depth and in width
and never descends into an ancestor."
([value]
(value->repr value 0 []))
([value depth seen]
(cond
(string? value)
(pr-str (abbreviate value))
(or (nil? value) (number? value) (boolean? value) (keyword? value))
(pr-str value)
(fn? value)
"#function"
(some #(identical? % value) seen)
"#recursive"
(>= depth max-repr-depth)
"…"
(map? value)
(let [seen (conj seen value)]
(dm/str "{" (->> (take max-repr-items value)
(map (fn [[k v]]
(dm/str (value->repr k (inc depth) seen) " "
(value->repr v (inc depth) seen))))
(str/join ", "))
(when (> (count value) max-repr-items) ", …") "}"))
(or (array? value) (coll? value))
(let [seen (conj seen value)
items (if (array? value) (array-seq value) (seq value))]
(dm/str "[" (->> (take max-repr-items items)
(map #(value->repr % (inc depth) seen))
(str/join " "))
(when (seq (drop max-repr-items items)) " …") "]"))
(instance? js/Object value)
(let [seen (conj seen value)
ks (js/Object.keys value)]
(dm/str "{" (->> (take max-repr-items ks)
(keep (fn [k]
(let [v (data-property value k)]
(when-not (= ::skip v)
(dm/str k " " (value->repr v (inc depth) seen))))))
(str/join ", "))
(when (> (alength ks) max-repr-items) ", …") "}"))
:else
(value->type value))))
(defn- printable?
"True when a schema form contains only data, so it can be shown to a plugin
author. `[:fn pred]` forms embed the predicate itself, which prints as an
unreadable `#object[…]` with the internal munged name."
[form]
(cond
(map? form) (every? printable? (vals form))
(coll? form) (every? printable? form)
(keyword? form) true
(string? form) true
(number? form) true
(boolean? form) true
(symbol? form) true
(regexp? form) true
(nil? form) true
:else false))
(defn- simplify-form
"Drop from a schema form the properties that are not data, so a schema is
described by its shape alone: `[::sm/text {:error/fn f}]` becomes
`::sm/text`. Yields `::unprintable` when what is left cannot describe the
schema, as in `[:fn pred]`, where dropping the predicate would advertise a
schema (`fn`) that means nothing to a plugin author."
[form]
(cond
(map? form)
(not-empty (into {} (filter (comp printable? val)) form))
(vector? form)
(let [items (into [] (keep simplify-form) form)]
(cond
(some #(= ::unprintable %) items) ::unprintable
(= 1 (count items)) (first items)
:else items))
(printable? form)
form
:else
::unprintable))
(defn- expected-form
"Readable representation of the schema a problem failed against, or nil when
the schema cannot be shown."
[schema]
(let [title (or (:title (sm/properties schema))
(:title (sm/type-properties schema)))]
(if (some? title)
title
(let [form (simplify-form (sm/form schema))]
(cond
(= ::unprintable form) nil
(keyword? form) (name form)
:else (pr-str form))))))
(defn- schema-message
"Message rendered by `csm/interpret-schema-problem` for a problem, or nil when
it degrades to the generic \"invalid data\": the token value schemas declare an
`:error/fn` that only speaks about empty values, so it renders nothing at all
for a wrong typed one, and saying nothing must not win over reporting what was
expected and what was received (#10072)."
[problem field]
(let [message (-> (csm/interpret-schema-problem {} problem)
(get-in field)
(get :message))]
(when (and (some? message)
(not= message (tr "errors.invalid-data")))
message)))
(defn- interpret-problem
"Like `csm/interpret-schema-problem`, but when the schema renders no message of
its own it reports the expected schemas together with the received value and
its type instead of a generic \"Invalid data\" (#10072).
Malli reports one problem per `:or` branch, all on the same field, so the
branches are accumulated: reporting only one of them would tell the plugin
author that an alternative that is in fact valid is not accepted."
[acc {:keys [schema in value] :as problem}]
(let [field (or (:error/field (sm/properties schema)) in)
field (if (vector? field) field [field])
current (when (seq field) (get-in acc field))]
(cond
(empty? field)
acc
;; A message the schema renders itself always wins over the generic
;; expected/received one, and is never overwritten by it.
(and (some? current) (not (contains? current :expected)))
acc
:else
(if-let [message (schema-message problem field)]
(assoc-in acc field {:message message})
(let [form (expected-form schema)
complete? (and (get current :complete? true) (some? form))
expected (if complete?
(-> (get current :expected [])
(conj form)
(distinct)
(vec))
[])
received (value->type value)
repr (abbreviate (value->repr value))
message (if (seq expected)
(tr "plugins.validation.invalid-value"
(abbreviate (str/join " or " expected))
received repr)
(tr "plugins.validation.received-value" received repr))]
(assoc-in acc field {:message message
:expected expected
:complete? complete?}))))))
(defn error-messages
[explain]
(let [msg (->> (:errors explain)
(reduce csm/interpret-schema-problem {})
(reduce interpret-problem {})
(flatten-error-map)
(map (fn [[field message]]
(tr "plugins.validation.message" field message)))

204
frontend/test/frontend_tests/plugins/utils_test.cljs
View File

@ -6,8 +6,23 @@
(ns frontend-tests.plugins.utils-test
(:require
[app.common.files.tokens :as cfo]
[app.common.schema :as sm]
[app.plugins.utils :as plugins.utils]
[cljs.test :as t :include-macros true]))
[app.util.i18n :as i18n]
[cljs.test :as t :include-macros true]
[cuerdas.core :as str]))
;; The plugin error renderer goes through `tr`, which returns the bare
;; translation code when no locale data is loaded. Load the strings this
;; namespace asserts on so the assertions exercise the real rendering.
(i18n/set-default-translations
#js {"plugins.validation.message" "Field %s is invalid: %s"
"plugins.validation.invalid-value" "expected %s, got %s (%s)"
"plugins.validation.received-value" "got %s (%s)"
"errors.invalid-data" "Invalid data"
"errors.field-missing" "Missing field"
"errors.tokens.empty-input" "Empty input"})
;; Access the private flattener for direct testing.
(def ^:private flatten-error-map @#'plugins.utils/flatten-error-map)
@ -87,3 +102,190 @@
;; `error-messages` returns nil (not "") on an explain with no mappable
;; errors, so `handle-error` can distinguish "no message" from a real one.
(t/is (nil? (plugins.utils/error-messages {:errors []}))))
;; ---------------------------------------------------------------------
;; Issue #10072 — schema validation errors must say what was expected and
;; what was received, instead of collapsing into a generic "Invalid data".
(t/deftest test-error-messages-reports-expected-and-received
(let [explain (sm/explain [:map [:value :string]] {:value 16})
message (plugins.utils/error-messages explain)]
(t/is (= "Field value is invalid: expected string, got number (16)" message))))
(t/deftest test-error-messages-reports-nested-field-path
(let [explain (sm/explain [:map [:sets [:vector [:map [:name :string]]]]]
{:sets [{:name true}]})
message (plugins.utils/error-messages explain)]
(t/is (= "Field sets.0.name is invalid: expected string, got boolean (true)" message))))
(t/deftest test-error-messages-honors-custom-schema-message
;; A schema that declares its own `:error/message` must keep it; the
;; expected/received rendering is only a fallback.
(let [explain (sm/explain [:map [:value [:string {:error/message "must not be empty"}]]]
{:value 16})
message (plugins.utils/error-messages explain)]
(t/is (= "Field value is invalid: must not be empty" message))))
(t/deftest test-error-messages-missing-key
;; Missing keys keep the "field missing" message: there is no received
;; value to report.
(let [explain (sm/explain [:map [:value :string]] {})
message (plugins.utils/error-messages explain)]
(t/is (= "Field value is invalid: Missing field" message))))
(t/deftest test-error-messages-never-generic
(let [explain (sm/explain [:map [:value :string]] {:value 16})
message (plugins.utils/error-messages explain)]
(t/is (not (str/includes? message "Invalid data")))))
(t/deftest test-error-messages-truncates-long-values
(let [value (apply str (repeat 500 "a"))
explain (sm/explain [:map [:value :int]] {:value value})
message (plugins.utils/error-messages explain)]
(t/is (str/includes? message "got string"))
(t/is (< (count message) 200))))
(t/deftest test-error-messages-unicode-value
(let [explain (sm/explain [:map [:value :int]] {:value "ñ😀"})
message (plugins.utils/error-messages explain)]
(t/is (str/includes? message "ñ😀"))))
(t/deftest test-error-messages-collection-value
(let [explain (sm/explain [:map [:value :string]] {:value [1 2]})
message (plugins.utils/error-messages explain)]
(t/is (str/includes? message "got array"))))
(defn- proxy? [value] (and (map? value) (contains? value :proxy)))
(defn- well-formed?
"False when the string contains a lone UTF-16 surrogate."
[s]
(try
(js/encodeURIComponent s)
true
(catch :default _ false)))
(t/deftest test-error-messages-union-reports-every-branch
;; Malli emits one problem per `:or` branch, all sharing the same path.
;; Reporting a single branch as if it were the only requirement is worse
;; than saying nothing: `plugins.tokens` declares the token value setter
;; as `[:or :string base]`, so a plugin author would be told a vector is
;; required when a plain string is equally valid.
(let [explain (sm/explain [:map [:value [:or [:vector :string] :string]]] {:value 16})
message (plugins.utils/error-messages explain)]
(t/is (= "Field value is invalid: expected [:vector :string] or string, got number (16)"
message))))
(t/deftest test-error-messages-union-reports-every-branch-nested
(let [explain (sm/explain [:map [:sets [:vector [:or :string :int]]]] {:sets [true]})
message (plugins.utils/error-messages explain)]
(t/is (= "Field sets.0 is invalid: expected string or int, got boolean (true)"
message))))
(t/deftest test-error-messages-never-leaks-function-objects
;; `[:fn pred]` schemas (used by the token-set arguments) render their form
;; as `#object[app$plugins$tokens$token_set_proxy_QMARK_]`. That must never
;; reach a plugin author.
(let [explain (sm/explain [:map [:value [:or [:fn proxy?] :string]]] {:value 16})
message (plugins.utils/error-messages explain)]
(t/is (not (str/includes? message "#object")))
(t/is (= "Field value is invalid: got number (16)" message))))
(t/deftest test-error-messages-function-schema-falls-back-to-custom-message
;; When one branch of the union carries its own message, that message wins
;; over the un-renderable `[:fn …]` branch.
(let [explain (sm/explain [:map [:value [:or [:fn proxy?] ::sm/uuid]]] {:value "nope"})
message (plugins.utils/error-messages explain)]
(t/is (not (str/includes? message "#object")))
(t/is (= "Field value is invalid: should be an uuid" message))))
(t/deftest test-error-messages-truncation-keeps-surrogate-pairs
;; Truncating at a UTF-16 code-unit boundary splits astral characters and
;; produces mojibake in a message whose whole purpose is legibility.
(let [value (apply str (repeat 200 "😀"))
explain (sm/explain [:map [:value :int]] {:value value})
message (plugins.utils/error-messages explain)]
(t/is (well-formed? message))
(t/is (str/includes? message "…"))))
(t/deftest test-error-messages-truncation-counts-code-points
;; 60 emoji are 120 UTF-16 code units but only 60 code points: below the
;; limit, so the value must be reported whole.
(let [value (apply str (repeat 60 "😀"))
explain (sm/explain [:map [:value :int]] {:value value})
message (plugins.utils/error-messages explain)]
(t/is (well-formed? message))
(t/is (str/includes? message value))))
;; ---------------------------------------------------------------------
;; The value reported back to the plugin author is arbitrary JS data, so
;; rendering it must terminate and must not run plugin code.
(t/deftest test-error-messages-self-referencing-object
;; A plugin argument is a native JS value, and an object graph with a back
;; reference (`a.parent.child === a`) is ordinary. `pr-str` walks it forever,
;; so the error renderer would throw a RangeError from inside the error
;; handler itself: the author gets a stack overflow instead of a validation
;; error.
(let [a #js {}
b #js {}
_ (unchecked-set a "parent" b)
_ (unchecked-set b "child" a)
explain (sm/explain [:map [:value :string]] {:value a})
message (plugins.utils/error-messages explain)]
(t/is (str/includes? message "got object"))))
(t/deftest test-error-messages-object-with-getters
;; Penpot's own proxies expose their contents through getters that read the
;; application state and may throw for a stale id. Rendering a value must
;; never invoke them.
(let [called (atom 0)
value (js/Object.defineProperty
#js {:id "x"} "children"
#js {:enumerable true
:get (fn [] (swap! called inc) (throw (js/Error. "boom")))})
explain (sm/explain [:map [:value :string]] {:value value})
message (plugins.utils/error-messages explain)]
(t/is (str/includes? message "got object"))
(t/is (zero? @called))))
(t/deftest test-error-messages-deeply-nested-object
;; A deep graph must not produce an unbounded message.
(let [value (reduce (fn [acc _] #js {:child acc}) #js {:leaf 1} (range 200))
explain (sm/explain [:map [:value :string]] {:value value})
message (plugins.utils/error-messages explain)]
(t/is (str/includes? message "got object"))
(t/is (< (count message) 200))))
;; ---------------------------------------------------------------------
;; Issue #10072 — the reported example: `addToken` with a wrong token value.
;;
;; The token value schemas declare an `:error/fn` that only speaks about empty
;; values, so it returns nil for a wrong-typed one and `interpret-schema-problem`
;; degrades to the generic "Invalid data". A message that says nothing must not
;; win over one that says what was expected and what was received.
(t/deftest test-error-messages-token-value-numeric
(let [schema [:map [:value (cfo/make-token-value-schema :font-size)]]
message (plugins.utils/error-messages (sm/explain schema {:value 16}))]
(t/is (= "Field value is invalid: expected string, got number (16)" message))))
(t/deftest test-error-messages-token-value-opacity
(let [schema [:map [:value (cfo/make-token-value-schema :opacity)]]
message (plugins.utils/error-messages (sm/explain schema {:value true}))]
(t/is (= "Field value is invalid: expected string, got boolean (true)" message))))
(t/deftest test-error-messages-token-value-font-family
;; Both branches of the union are printable, so both are reported.
(let [schema [:map [:value (cfo/make-token-value-schema :font-family)]]
message (plugins.utils/error-messages (sm/explain schema {:value 16}))]
(t/is (= (str "Field value is invalid: "
"expected [:vector :app.common.schema/text] or TokenRef, got number (16)")
message))))
(t/deftest test-error-messages-token-value-empty-keeps-custom-message
;; When the schema's own `:error/fn` does render a message, it still wins.
(let [schema [:map [:value (cfo/make-token-value-schema :font-size)]]
message (plugins.utils/error-messages (sm/explain schema {:value ""}))]
(t/is (= "Field value is invalid: Empty input" message))))

10
frontend/translations/en.po
View File

@ -5042,10 +5042,18 @@ msgstr "This token does not exist or has been deleted."
msgid "options.deleted-token-with-name"
msgstr "{%s} token does not exist or has been deleted."
#: src/app/plugins/utils.cljs:318
#: src/app/plugins/utils.cljs:533
msgid "plugins.validation.invalid-value"
msgstr "expected %s, got %s (%s)"
#: src/app/plugins/utils.cljs:547
msgid "plugins.validation.message"
msgstr "Field %s is invalid: %s"
#: src/app/plugins/utils.cljs:536
msgid "plugins.validation.received-value"
msgstr "got %s (%s)"
#: src/app/main/ui/auth/recovery.cljs:88
msgid "profile.recovery.go-to-login"
msgstr "Go to login"