From 238a4d8613d4799b92eab0e38e5194fb72dc116d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 5 Oct 2026 11:56:59 +0200 Subject: [PATCH] :bug: Sanitize SVG namespace-prefixed elements and attributes (#12073) * :bug: Sanitize SVG namespace-prefixed elements and attributes The SVG sanitizer compared qualified tag and attribute names against its blocklists. The XML parser keeps the namespace prefix in the keyword name (:x:script), so a script element written with a prefix bound to the SVG namespace passed sanitization untouched and was later served as image/svg+xml, executing in the Penpot origin. Match tags and attributes on the local name (the part after the last colon) instead, so dangerous elements and attributes are removed no matter which prefix they use. Legitimately prefixed attributes such as xlink:href keep working. Closes #12071 AI-assisted-by: glm-5.3-flash * :recycle: Cover prefixed SVG vectors and hoist sanitizer blocklist Apply the code review findings on top of the sanitizer fix: - Add a test for the javascript: href vector using a prefix bound to the xlink namespace, the other payload variant the old exact-key check missed. - Add a test asserting that legitimately prefixed attributes such as xml:space are preserved by the local-name matching. - Hoist the dangerous-tags blocklist next to the other sanitizer policy defs and bind the keyword name once in local-name. Relates to #12071 AI-assisted-by: glm-5.3-flash --- backend/src/app/media/svg.clj | 57 +++++++++++++------- backend/test/backend_tests/media_test.clj | 66 +++++++++++++++++++++++ 2 files changed, 103 insertions(+), 20 deletions(-) diff --git a/backend/src/app/media/svg.clj b/backend/src/app/media/svg.clj index 1de52d4030..4cad562d91 100644 --- a/backend/src/app/media/svg.clj +++ b/backend/src/app/media/svg.clj @@ -52,30 +52,47 @@ (def ^:private dangerous-attrs-pattern #"(?i)^on\w+$") (def ^:private javascript-href-pattern #"(?i)^javascript:") +(def ^:private dangerous-tags + #{"script" "foreignObject" "set" + "animate" "animateTransform" "animateColor" "animateMotion"}) + +(defn- local-name + "Return the local name of a parsed tag or attribute keyword. + + The XML parser interns qualified names with the prefix included in + the keyword name and no keyword namespace: `` parses as + the keyword `:x:script`. Dangerous elements and attributes behave + the same in the browser regardless of the prefix used, so matching + is done on the local name: the part after the last `:`." + [k] + (let [n (name k)] + (if (str/includes? n ":") + (last (str/split n ":")) + n))) + (defn- sanitize-svg-element "Recursively sanitize an SVG element by removing dangerous tags and attributes." [{:keys [tag attrs content] :as element}] (when (and (map? element) tag) - (let [dangerous-tags #{:script :foreignObject :set :animate :animateTransform :animateColor :animateMotion}] - (when-not (contains? dangerous-tags tag) - (let [clean-attrs (->> attrs - (remove (fn [[k v]] - (or (re-matches dangerous-attrs-pattern (name k)) - (and (#{:href :xlink:href} k) - (string? v) - (re-find javascript-href-pattern (str/trim v)))))) - (into {})) - clean-content (when content - (->> content - (filter #(or (string? %) (map? %))) - (map (fn [child] - (if (map? child) - (sanitize-svg-element child) - child))) - (filter some?) - vec))] - (cond-> {:tag tag :attrs clean-attrs} - (seq clean-content) (assoc :content clean-content))))))) + (when-not (contains? dangerous-tags (local-name tag)) + (let [clean-attrs (->> attrs + (remove (fn [[k v]] + (or (re-matches dangerous-attrs-pattern (local-name k)) + (and (= "href" (local-name k)) + (string? v) + (re-find javascript-href-pattern (str/trim v)))))) + (into {})) + clean-content (when content + (->> content + (filter #(or (string? %) (map? %))) + (map (fn [child] + (if (map? child) + (sanitize-svg-element child) + child))) + (filter some?) + vec))] + (cond-> {:tag tag :attrs clean-attrs} + (seq clean-content) (assoc :content clean-content)))))) (defn sanitize-svg "Sanitize SVG content by removing dangerous elements and attributes. diff --git a/backend/test/backend_tests/media_test.clj b/backend/test/backend_tests/media_test.clj index eb2f2517c7..3c3a887032 100644 --- a/backend/test/backend_tests/media_test.clj +++ b/backend/test/backend_tests/media_test.clj @@ -137,6 +137,72 @@ (t/is (not (clojure.string/includes? result "onmouseover"))) (t/is (clojure.string/includes? result "alert('xss')" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "alert('xss')" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "alert('xss')" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "foreignObject"))) + (t/is (not (clojure.string/includes? result "" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "javascript:"))) + (t/is (not (clojure.string/includes? result "alert"))) + (t/is (clojure.string/includes? result "" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "onload"))) + (t/is (not (clojure.string/includes? result "onclick"))) + (t/is (not (clojure.string/includes? result "alert"))) + (t/is (clojure.string/includes? result "" + result (svg/sanitize-svg svg)] + (t/is (not (clojure.string/includes? result "javascript:"))) + (t/is (not (clojure.string/includes? result "alert"))) + (t/is (clojure.string/includes? result "hola" + result (svg/sanitize-svg svg)] + (t/is (clojure.string/includes? result "xml:space")) + (t/is (clojure.string/includes? result "hola"))))) + (t/deftest info-invalid-image (t/testing "info on invalid image raises error" (let [path (fs/create-tempfile :prefix "penpot-test-" :suffix ".jpg")]