From b5d4cd1f0bf35a0537844a32237ca09b2902d2d3 Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Mon, 28 Sep 2026 16:59:06 +0200 Subject: [PATCH 1/8] :bug: Fix problems with react loops (#11941) --- .../app/main/data/workspace/libraries.cljs | 45 ++++++--- .../ui/workspace/colorpicker/libraries.cljs | 98 +++++++++---------- .../main/ui/workspace/colorpicker/ramp.cljs | 7 +- .../main/ui/workspace/sidebar/options.cljs | 42 ++++---- 4 files changed, 105 insertions(+), 87 deletions(-) diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index fd2250c379..df0a514ca5 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -1448,12 +1448,17 @@ (rx/filter (complement :translation?)) ;; Keep waits pending while component changes are checked. (rx/map start-sync-barrier) - (rx/observe-on :async)) + (rx/share)) - get-component-events - (fn [[event old-data]] - (let [{:keys [file-id changes save-undo? undo-group]} event - changed-components + ;; Buffers commits until a timer turn passes with no new ones. The + ;; debounce timer (not a microtask) queues behind any timers already + ;; due, so a burst of commits is inspected as one batch in one task. + commit-batches-s + (rx/buffer-until (rx/debounce 0 commits-s) commits-s) + + get-component-changes + (fn [old-data {:keys [file-id changes save-undo? undo-group]}] + (let [changed-components (when (and old-data (or (nil? file-id) (= file-id (:id old-data)))) (into #{} @@ -1461,29 +1466,39 @@ changes))] (cond (empty? changed-components) - (rx/empty) + nil save-undo? (do (log/info :hint "detected component changes" :ids (map str changed-components) :undo-group undo-group) - (->> (rx/from changed-components) - (rx/map #(component-changed - % (:id old-data) undo-group)))) + (map #(vector ::component-changed % undo-group) + changed-components)) :else ;; Undos only bump :modified-at. - (->> (rx/from changed-components) - (rx/map touch-component))))) + (map #(vector ::touch-component %) changed-components)))) + + ;; One event per distinct component change in the batch + get-component-events + (fn [batch old-data] + (->> batch + (mapcat #(get-component-changes old-data (first %))) + (distinct) + (map (fn [[type component-id undo-group]] + (if (= type ::component-changed) + (component-changed component-id (:id old-data) undo-group) + (touch-component component-id)))) + (rx/from))) component-events-s - (->> commits-s + (->> commit-batches-s (rx/with-latest-from workspace-buffer-s) (rx/mapcat - (fn [[[event task] old-data]] - (->> (get-component-events [event old-data]) - (rx/finalize #(finish-sync-barrier! task))))) + (fn [[batch old-data]] + (->> (get-component-events batch old-data) + (rx/finalize #(run! (comp finish-sync-barrier! second) batch))))) ;; Close barriers left behind when the page shuts down. (rx/finalize #(wrf/finish-tasks! @pending-sync-barriers*)) (rx/share)) diff --git a/frontend/src/app/main/ui/workspace/colorpicker/libraries.cljs b/frontend/src/app/main/ui/workspace/colorpicker/libraries.cljs index 5e01c95785..d4ae459cbf 100644 --- a/frontend/src/app/main/ui/workspace/colorpicker/libraries.cljs +++ b/frontend/src/app/main/ui/workspace/colorpicker/libraries.cljs @@ -202,12 +202,6 @@ file-id (mf/use-ctx ctx/current-file-id) - current-colors* (mf/use-state []) - current-colors (deref current-colors*) - - grouped-colors* (mf/use-state {}) - grouped-colors (deref grouped-colors*) - open-groups* (mf/use-state #{}) open-groups (deref open-groups*) @@ -221,13 +215,15 @@ [{:value "recent" :label (tr "workspace.libraries.colors.recent-colors") :id "recent"} {:value "file" :label (tr "workspace.libraries.colors.file-library") :id "file"}]) + ;; Equal-stable, since `select*` resets its selection when options change options - (mf/with-memo [library-options libraries file-id] - (into library-options - (comp - (map val) - (map (fn [lib] {:value (d/name (:id lib)) :label (:name lib) :id (d/name (:id lib))}))) - (dissoc libraries file-id))) + (h/use-equal-memo + (mf/with-memo [library-options libraries file-id] + (into library-options + (comp + (map val) + (map (fn [lib] {:value (d/name (:id lib)) :label (:name lib) :id (d/name (:id lib))}))) + (dissoc libraries file-id)))) on-library-change (mf/use-fn @@ -281,49 +277,49 @@ (fn [s] (if (contains? s path) (disj s path) - (conj s path))))))] + (conj s path)))))) - ;; Load library colors when the selected library (or filter options) change. - ;; - ;; flat current-colors* -- used for the grid view and the recent list view. - ;; grouped grouped-colors* -- used for the library grouped list view. - ;; - ;; Library colors are fully converted with `library-color->color` here so - ;; the render path never needs to do it. `flat-colors` is materialised as - ;; an eager vector so realisation does not leak into render time. - ;; open-groups* is reset to #{} (all groups expanded) on every library switch. - (mf/with-effect [selected recent-colors libraries file-id valid-color?] - (let [resolved-file-id (if (= selected :file) file-id selected)] - (reset! open-groups* #{}) - (if (= selected :recent) - (let [colors (into [] - (comp - (filter valid-color?) - (map-indexed (fn [index color] - (let [color (if (map? color) color {:color color})] - (vary-meta color assoc ::id (dm/str index))))) - (take-while some?)) - (sort ctc/sort-colors (reverse recent-colors)))] - (reset! current-colors* colors) - (reset! grouped-colors* {})) + resolved-file-id + (if (= selected :file) file-id selected) - (let [raw-colors (->> (dm/get-in libraries [resolved-file-id :data :colors]) - (vals) - (filter valid-color?) - (sort-by :name)) + ;; Colors of the selected library; stable across unrelated commits + library-colors + (when (not= selected :recent) + (dm/get-in libraries [resolved-file-id :data :colors])) - ;; Eager vector for the grid view -- index-based ::id for keying. - flat-colors (into [] - (map-indexed (fn [index color] - (-> (ctc/library-color->color color resolved-file-id) - (vary-meta assoc ::id (dm/str index))))) - raw-colors) + ;; Flat (grid and recent list) and grouped (library list) colors, converted for rendering + [current-colors grouped-colors] + (mf/with-memo [selected recent-colors library-colors resolved-file-id valid-color?] + (if (= selected :recent) + [(into [] + (comp + (filter valid-color?) + (map-indexed (fn [index color] + (let [color (if (map? color) color {:color color})] + (vary-meta color assoc ::id (dm/str index))))) + (take-while some?)) + (sort ctc/sort-colors (reverse recent-colors))) + {}] - ;; Group tree with colors already converted -- no conversions at render time. - grouped (some-> (grp/group-assets raw-colors false) - (convert-grouped-colors resolved-file-id))] - (reset! current-colors* flat-colors) - (reset! grouped-colors* (or grouped {})))))) + (let [raw-colors (->> (vals library-colors) + (filter valid-color?) + (sort-by :name)) + + ;; Eager vector for the grid view -- index-based ::id for keying. + flat-colors (into [] + (map-indexed (fn [index color] + (-> (ctc/library-color->color color resolved-file-id) + (vary-meta assoc ::id (dm/str index))))) + raw-colors) + + ;; Group tree with colors already converted -- no conversions at render time. + grouped (some-> (grp/group-assets raw-colors false) + (convert-grouped-colors resolved-file-id))] + [flat-colors (or grouped {})])))] + + ;; Expands all groups when the selected library changes + (mf/with-effect [selected] + (reset! open-groups* #{})) [:div {:class (stl/css :libraries)} [:div {:class (stl/css :select-wrapper)} diff --git a/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs b/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs index 01804c409c..85617c307e 100644 --- a/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs +++ b/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs @@ -121,10 +121,15 @@ (reset! internal-color* color) (on-change color))))] + ;; Syncs with color changes made outside the ramp; the colors the ramp + ;; emits come back with the same components and are skipped (mf/use-effect (mf/deps color) (fn [] - (reset! internal-color* (enrich-color-map color)))) + (let [color (enrich-color-map color)] + (when (not= (select-keys color [:h :s :v :alpha]) + (select-keys internal-color [:h :s :v :alpha])) + (reset! internal-color* color))))) [:* [:> value-saturation-selector* diff --git a/frontend/src/app/main/ui/workspace/sidebar/options.cljs b/frontend/src/app/main/ui/workspace/sidebar/options.cljs index f07dce3891..c1bcdb19a1 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options.cljs @@ -98,6 +98,21 @@ (when (= (:type panel) :component-swap) [:> component-menu* {:shapes (:shapes panel) :is-swap-opened true}])) +(defn- get-shapes-with-children + "Returns the shapes of `selected` together with all their descendants." + [objects selected] + (loop [queue (into #queue [] selected) + visited selected] + (if-let [id (peek queue)] + (let [shape (get objects id) + children (:shapes shape)] + (if (seq children) + (let [new-children (remove visited children)] + (recur (into (pop queue) new-children) + (into visited new-children))) + (recur (pop queue) visited))) + (sequence (keep (d/getf objects)) visited)))) + (mf/defc design-menu* {::mf/private true} [{:keys [selected objects page-id file-id shapes]}] @@ -123,29 +138,16 @@ (->> (dm/get-in grid-edition [edition :selected]) (map #(dm/get-in objects [edition :layout-grid-cells %]))) - shapes-with-children* - (mf/use-state nil) + ;; Deferred, so the subtree walk runs in a background render + deferred-selected + (mf/use-deferred selected) - _ (mf/use-effect - (mf/deps selected objects shapes) - (fn [] - (reset! shapes-with-children* nil) - (let [result - (loop [queue (into #queue [] selected) - visited selected] - (if-let [id (peek queue)] - (let [shape (get objects id) - children (:shapes shape)] - (if (seq children) - (let [new-children (remove visited children)] - (recur (into (pop queue) new-children) - (into visited new-children))) - (recur (pop queue) visited))) - (sequence (keep (d/getf objects)) visited)))] - (reset! shapes-with-children* result)))) + deferred-objects + (mf/use-deferred objects) shapes-with-children - (deref shapes-with-children*) + (mf/with-memo [deferred-selected deferred-objects] + (get-shapes-with-children deferred-objects deferred-selected)) total-selected (count selected)] From 238a4d8613d4799b92eab0e38e5194fb72dc116d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 5 Oct 2026 11:56:59 +0200 Subject: [PATCH 2/8] :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")] From 4a4f50e7ea4cb453cda48d7a96e907209ab1f63c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 5 Oct 2026 10:15:16 +0000 Subject: [PATCH 3/8] :fire: Stop printing separator lines in reports Drop the equals-sign banners from the CLJS throwable formatter, so new frontend error reports are born without them. The JVM branch never printed them. Adds a test asserting full reports carry sections but no separator runs. AI-assisted-by: Muse Spark 1.3 Free --- common/src/app/common/exceptions.cljc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/common/src/app/common/exceptions.cljc b/common/src/app/common/exceptions.cljc index 86785c58ff..588391601a 100644 --- a/common/src/app/common/exceptions.cljc +++ b/common/src/app/common/exceptions.cljc @@ -245,7 +245,6 @@ (defn format-throwable [cause & {:as opts}] (with-out-str - (println "====================") (when-let [exdata (ex-data cause)] (when-let [hint (or (get exdata :hint) (ex-message cause))] @@ -275,9 +274,7 @@ (when-let [trace (.-stack cause)] (println "Trace:") (println "--------------------") - (println (.-stack cause))) - - (println "====================")))) + (println (.-stack cause)))))) (defn first-line [s] From cf46b53bcb61c2a9be8f202bec03647a5470d43d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 5 Oct 2026 12:18:37 +0200 Subject: [PATCH 4/8] :books: Update changelog --- CHANGES.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 720cbc0fb9..56d1046320 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,12 @@ # CHANGELOG +## 2.18.2 + +### :bug: Bugs fixed + +- Fix workspace showing an internal error when syncing components or dragging in the color picker [#11933](https://github.com/penpot/penpot/issues/11933) (PR: [#11941](https://github.com/penpot/penpot/pull/11941)) +- Fix SVG sanitizer keeping namespace-prefixed script elements when uploading SVG files [#12071](https://github.com/penpot/penpot/issues/12071) (PR: [#12073](https://github.com/penpot/penpot/pull/12073)) + ## 2.18.1 ### :bug: Bugs fixed From ab58922f2c38270bb6c2120940be684b8f35f50e Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 5 Oct 2026 16:47:07 +0200 Subject: [PATCH 5/8] :ambulance: Revoke file access when a member leaves the team (#12097) Delete the team-scoped file, project and pin relations on leave-team and delete-team-member, require a live team membership in the file, project, search and listing permission queries, and clean pre-existing orphan rows with a migration. Former members now get not-found on file actions instead of keeping read and rename access (GHSA-v9r9-h77c-55m2). AI-assisted-by: muse-spark-1.3-contributor --- .../auth-permissions-product-domains.md | 1 + backend/src/app/binfile/common.clj | 12 +- backend/src/app/migrations.clj | 5 +- ...l-orphan-profile-rels-after-team-leave.sql | 27 ++ backend/src/app/rpc/commands/projects.clj | 12 +- backend/src/app/rpc/commands/search.clj | 4 + backend/src/app/rpc/commands/teams.clj | 38 ++- backend/test/backend_tests/helpers.clj | 4 +- backend/test/backend_tests/rpc_team_test.clj | 312 ++++++++++++++++++ 9 files changed, 404 insertions(+), 11 deletions(-) create mode 100644 backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index c3057118d5..130cd62609 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -15,6 +15,7 @@ - `app.rpc.permissions` provides predicate/check factories. Failed permission checks intentionally raise `:not-found` / `:object-not-found`, not an authorization-specific error, to avoid leaking object existence. - Team role flags are normalized as owner > admin > editor > viewer. Owner/admin imply edit; any membership row implies read. +- File/project role rows only count with a live team membership: the file/project permission queries require a matching `team_profile_rel` row, and leaving or removing a member deletes their team-scoped `file`/`project`/`team-project-profile-rel` rows (GHSA-v9r9-h77c-55m2). - File/project/comment checks are implemented in the owning command namespaces, often via helpers imported from `files`, `teams`, or `projects`; do not bypass those helpers with direct DB lookups unless preserving their not-found semantics. - Comment permission includes both logged-in state and the file/team comment policy. Shared viewer paths may pass `share-id`; preserve that path when changing comment queries. diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 3e4402be92..e2aed9a837 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -344,8 +344,13 @@ fpr.can_edit from file_profile_rel as fpr inner join file as f on (f.id = fpr.file_id) + inner join project as p on (p.id = f.project_id) where fpr.file_id = ? and fpr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = fpr.profile_id) union all select tpr.is_owner, tpr.is_admin, @@ -361,8 +366,13 @@ ppr.can_edit from project_profile_rel as ppr inner join file as f on (f.project_id = ppr.project_id) + inner join project as p on (p.id = ppr.project_id) where f.id = ? - and ppr.profile_id = ?") + and ppr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id)") (defn- get-file-permissions* [conn profile-id file-id] diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index 2edb8614d5..13f99170fe 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -499,7 +499,10 @@ :fn (mg/resource "app/migrations/sql/0152-improve-uuid-defaults-and-drop-extension.sql")} {:name "0152-rename-version-and-add-indexes-to-server-error-report" - :fn (mg/resource "app/migrations/sql/0152-rename-version-and-add-indexes-to-server-error-report.sql")}]) + :fn (mg/resource "app/migrations/sql/0152-rename-version-and-add-indexes-to-server-error-report.sql")} + + {:name "0153-del-orphan-profile-rels-after-team-leave" + :fn (mg/resource "app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql b/backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql new file mode 100644 index 0000000000..41f30d7796 --- /dev/null +++ b/backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql @@ -0,0 +1,27 @@ +-- Remove team-scoped role rows whose profile is no longer a member of +-- the team that owns the project/file. Leftover from before +-- leave-team/delete-team-member cleaned them up (GHSA-v9r9-h77c-55m2). +-- Rows with a live team membership are kept. + +DELETE FROM file_profile_rel AS fpr +USING file AS f +INNER JOIN project AS p ON (p.id = f.project_id) +WHERE fpr.file_id = f.id + AND NOT EXISTS (SELECT 1 + FROM team_profile_rel AS tpr + WHERE tpr.team_id = p.team_id + AND tpr.profile_id = fpr.profile_id); + +DELETE FROM project_profile_rel AS ppr +USING project AS p +WHERE ppr.project_id = p.id + AND NOT EXISTS (SELECT 1 + FROM team_profile_rel AS tpr + WHERE tpr.team_id = p.team_id + AND tpr.profile_id = ppr.profile_id); + +DELETE FROM team_project_profile_rel AS tppr +WHERE NOT EXISTS (SELECT 1 + FROM team_profile_rel AS tpr + WHERE tpr.team_id = tppr.team_id + AND tpr.profile_id = tppr.profile_id); diff --git a/backend/src/app/rpc/commands/projects.clj b/backend/src/app/rpc/commands/projects.clj index 05d5ae79aa..ab678db840 100644 --- a/backend/src/app/rpc/commands/projects.clj +++ b/backend/src/app/rpc/commands/projects.clj @@ -41,8 +41,13 @@ ppr.is_admin, ppr.can_edit from project_profile_rel as ppr + inner join project as p on (p.id = ppr.project_id) where ppr.project_id = ? - and ppr.profile_id = ?") + and ppr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id)") (defn- get-permissions [conn profile-id project-id] @@ -143,7 +148,12 @@ inner join team as t on (t.id = p2.team_id) where p2.id in (select project_id from project_profile_rel as ppr + inner join project as p on (p.id = ppr.project_id) where ppr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id) and (ppr.can_edit = true or ppr.is_owner = true or ppr.is_admin = true)) diff --git a/backend/src/app/rpc/commands/search.clj b/backend/src/app/rpc/commands/search.clj index 7b60e6db30..463bfb4abd 100644 --- a/backend/src/app/rpc/commands/search.clj +++ b/backend/src/app/rpc/commands/search.clj @@ -32,6 +32,10 @@ where ppr.profile_id = ? and p.team_id = ? and (p.deleted_at is null) + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id) and (ppr.is_admin = true or ppr.is_owner = true or ppr.can_edit = true) diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index 9c13d6b37d..1a329773f2 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -732,6 +732,37 @@ ;; --- Mutation: Leave Team +;; Remove every team-scoped relation of a profile that leaves (or is +;; removed from) a team. File and project roles hang from the team +;; membership: keeping them would let the former member keep reading +;; and editing files through `sql:file-permissions`. Pins are personal +;; state of the same team, so they go too. Other teams are untouched, +;; and rejoining grants fresh roles. Runs inside the caller's +;; transaction. +(def ^:private sql:delete-team-file-rels + "DELETE FROM file_profile_rel AS fpr + USING file AS f + INNER JOIN project AS p ON (p.id = f.project_id) + WHERE fpr.file_id = f.id + AND p.team_id = ? + AND fpr.profile_id = ?") + +(def ^:private sql:delete-team-project-rels + "DELETE FROM project_profile_rel AS ppr + USING project AS p + WHERE ppr.project_id = p.id + AND p.team_id = ? + AND ppr.profile_id = ?") + +(defn- delete-team-rels + [conn profile-id team-id] + (db/exec! conn [sql:delete-team-file-rels team-id profile-id]) + (db/exec! conn [sql:delete-team-project-rels team-id profile-id]) + (db/delete! conn :team-project-profile-rel {:profile-id profile-id + :team-id team-id}) + (db/delete! conn :team-profile-rel {:profile-id profile-id + :team-id team-id})) + (defn leave-team [{:keys [::db/conn ::mbus/msgbus]} {:keys [profile-id id reassign-to]}] (let [perms (get-permissions conn profile-id id) @@ -782,9 +813,7 @@ :code :owner-cant-leave-team :hint "releasing owner before leave")) - (db/delete! conn :team-profile-rel - {:profile-id profile-id - :team-id id}) + (delete-team-rels conn profile-id id) nil)) @@ -966,8 +995,7 @@ (ex/raise :type :validation :code :cant-remove-owner)) - (db/delete! conn :team-profile-rel {:profile-id member-id - :team-id team-id}) + (delete-team-rels conn member-id team-id) ;; A removed member that owns the organization of this team keeps ;; read-only access to it, so instead of kicking them out we degrade diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 25aa8b20bc..a9dd895176 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -299,9 +299,7 @@ ([params] (create-project-role* *system* params)) ([system {:keys [project-id profile-id role] :or {role :owner}}] (dm/with-open [conn (db/open system)] - (#'teams/create-project-role conn {:project-id project-id - :profile-id profile-id - :role role})))) + (#'teams/create-project-role conn profile-id project-id role)))) (defn create-file-role* ([params] (create-file-role* *system* params)) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 1c30415db2..0665c92570 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -1471,3 +1471,315 @@ (t/is (not (th/success? out))) (t/is (th/ex-of-type? (:error out) :not-found)) (t/is (th/ex-of-code? (:error out) :member-does-not-exist))))) + +(t/deftest leaver-loses-file-access + ;; GHSA-v9r9-h77c-55m2: leaving the team must revoke the file and + ;; project roles granted in that team. + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project)})] + + (t/testing "editor can rename and read summary before leaving" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file) + :name "renamed"})] + (t/is (th/success? out))) + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id editor) + :id (:id file)})] + (t/is (th/success? out)))) + + (t/testing "leave team" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out)))) + + (t/testing "renaming after leaving fails with not-found" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file) + :name "renamed-again"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "file summary after leaving fails with not-found" + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id editor) + :id (:id file)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "get-file after leaving fails with not-found" + (let [out (th/command! {::th/type :get-file + ::rpc/profile-id (:id editor) + :id (:id file) + :components-v2 true}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest removed-member-loses-file-access + ;; Same as above through the expulsion path. + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project)})] + + (t/testing "remove member" + (let [out (th/command! {::th/type :delete-team-member + ::rpc/profile-id (:id owner) + :team-id (:id team) + :member-id (:id editor)})] + (t/is (th/success? out)))) + + (t/testing "renaming after removal fails with not-found" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file) + :name "renamed-again"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "file summary after removal fails with not-found" + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id editor) + :id (:id file)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest orphan-file-role-without-team-membership-grants-nothing + ;; Defense in depth: a file role row without a live team membership + ;; (leftover from before the leave-team cleanup) grants no access. + (let [owner (th/create-profile* 1 {:is-active true}) + stranger (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)})] + + ;; Plant an orphan file role for a profile that was never a member. + (th/create-file-role* {:file-id (:id file) + :profile-id (:id stranger) + :role :editor}) + + (t/testing "orphan file role cannot rename" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id stranger) + :id (:id file) + :name "renamed"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "orphan file role cannot read summary" + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id stranger) + :id (:id file)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))))) + +(t/deftest leaver-loses-project-access + ;; Same revocation at project level (covers sql:project-permissions). + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)})] + + (t/testing "leave team" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out)))) + + (t/testing "get-project after leaving fails with not-found" + (let [out (th/command! {::th/type :get-project + ::rpc/profile-id (:id editor) + :id (:id project)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "rename-project after leaving fails with not-found" + (let [out (th/command! {::th/type :rename-project + ::rpc/profile-id (:id editor) + :id (:id project) + :name "renamed"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest leaver-vanishes-from-search + ;; The old team's files are no longer searchable by the leaver. + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project) + :name "searchable-file"})] + + (t/testing "member finds the file before leaving" + (let [out (th/command! {::th/type :search-files + ::rpc/profile-id (:id editor) + :team-id (:id team) + :search-term "searchable"})] + (t/is (th/success? out)) + (t/is (= (:id file) (-> out :result first :id))))) + + (t/testing "leave team" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out)))) + + (t/testing "search on the old team fails with not-found" + (let [out (th/command! {::th/type :search-files + ::rpc/profile-id (:id editor) + :team-id (:id team) + :search-term "searchable"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest pins-are-cleaned-on-leave + ;; The project pin (team-project-profile-rel) goes with the leaver. + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)})] + + (t/testing "pin the project" + (let [out (th/command! {::th/type :update-project-pin + ::rpc/profile-id (:id editor) + :id (:id project) + :team-id (:id team) + :is-pinned true})] + (t/is (th/success? out))) + (t/is (= 1 (count (th/db-query :team-project-profile-rel + {:team-id (:id team) + :profile-id (:id editor)}))))) + + (t/testing "leave team removes the pin row" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out))) + (t/is (= 0 (count (th/db-query :team-project-profile-rel + {:team-id (:id team) + :profile-id (:id editor)})))))))) + +(t/deftest leaving-one-team-keeps-other-team-access + ;; Deletes are scoped by team: leaving A keeps B's files usable. + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team-a (th/create-team* 1 {:profile-id (:id owner)}) + team-b (th/create-team* 2 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team-a) + :profile-id (:id editor) + :role :editor}) + (th/create-team-role* {:team-id (:id team-b) + :profile-id (:id editor) + :role :editor}) + + (let [project-a (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team-a)}) + file-a (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project-a)}) + project-b (th/create-project* 2 {:profile-id (:id editor) + :team-id (:id team-b)}) + file-b (th/create-file* 2 {:profile-id (:id editor) + :project-id (:id project-b)})] + + (t/testing "leave team A" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team-a)})] + (t/is (th/success? out)))) + + (t/testing "team A file is gone" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file-a) + :name "renamed"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "team B file still works" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file-b) + :name "renamed"})] + (t/is (th/success? out))))))) + +(t/deftest orphan-project-role-grants-nothing-and-lists-nothing + ;; Mirror of the file orphan test at project level, including the + ;; get-all-projects listing (covers the F1 guard). + (let [owner (th/create-profile* 1 {:is-active true}) + stranger (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)})] + + ;; Plant an orphan project role for a profile that was never a member. + (th/create-project-role* {:project-id (:id project) + :profile-id (:id stranger) + :role :editor}) + + (t/testing "orphan project role cannot read the project" + (let [out (th/command! {::th/type :get-project + ::rpc/profile-id (:id stranger) + :id (:id project)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "orphan project is not listed" + (let [out (th/command! {::th/type :get-all-projects + ::rpc/profile-id (:id stranger)})] + (t/is (th/success? out)) + (t/is (nil? (some #(= (:id project) (:id %)) (:result out)))))))) From 4b70c9aa210cbd70bb39283273ee551a226f924c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 5 Oct 2026 15:15:41 +0000 Subject: [PATCH 6/8] :bug: Reserve S3 download path without creating the file The S3 file download opens the temp file with CREATE_NEW, so it fails when the path already exists. tempfile creates the empty file on reserve, which made every large download depend on the SDK deleting the placeholder between its own retries. Add tmp/temp-path, which reserves a unique path and registers cleanup without creating the file, and use it for S3 downloads. The old retry on FileAlreadyExists stays as a safety net for a name clash. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/storage/s3.clj | 2 +- backend/src/app/storage/tmp.clj | 19 +++++++++ .../test/backend_tests/storage_tmp_test.clj | 40 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 backend/test/backend_tests/storage_tmp_test.clj diff --git a/backend/src/app/storage/s3.clj b/backend/src/app/storage/s3.clj index 025749bee1..676660c95a 100644 --- a/backend/src/app/storage/s3.clj +++ b/backend/src/app/storage/s3.clj @@ -316,7 +316,7 @@ ;; to the filesystem and then read with buffered inputstream; if ;; not, read the contento into memory using bytearrays. (if (> ^long size (* 1024 1024 2)) - (let [path (tmp/tempfile :prefix "penpot.storage.s3." :min-age "6h") + (let [path (tmp/temp-path :prefix "penpot.storage.s3." :min-age "6h") rxf (AsyncResponseTransformer/toFile ^Path path)] (->> (.getObject ^S3AsyncClient client ^GetObjectRequest gor diff --git a/backend/src/app/storage/tmp.clj b/backend/src/app/storage/tmp.clj index b7a3076159..987ea4a1de 100644 --- a/backend/src/app/storage/tmp.clj +++ b/backend/src/app/storage/tmp.clj @@ -93,6 +93,25 @@ (sp/offer! queue [path (some-> min-age ct/duration)]) path)) +(defn temp-path + "Reserve a unique temp path WITHOUT creating the file. + + Use it when an external writer needs to create the file itself and + fails if it already exists (like the S3 SDK file download, which + opens with CREATE_NEW). The path is still registered for cleanup, + so a crash between reserve and write does not leak." + [& {:keys [suffix prefix dir min-age] + :or {prefix "penpot." + suffix ".tmp" + dir default-tmp-dir}}] + (fs/create-dir dir) + (loop [path (fs/join dir (str prefix (uuid/next) suffix))] + (if (fs/exists? path) + (recur (fs/join dir (str prefix (uuid/next) suffix))) + (do + (sp/offer! queue [path (some-> min-age ct/duration)]) + path)))) + (defn tempfile-from "Create a new tempfile from from consuming the stream" [input & {:as options}] diff --git a/backend/test/backend_tests/storage_tmp_test.clj b/backend/test/backend_tests/storage_tmp_test.clj new file mode 100644 index 0000000000..b9141cf20b --- /dev/null +++ b/backend/test/backend_tests/storage_tmp_test.clj @@ -0,0 +1,40 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns backend-tests.storage-tmp-test + (:require + [app.storage.tmp :as tmp] + [clojure.test :as t] + [datoteka.fs :as fs]) + (:import + java.nio.file.Files + java.nio.file.Path)) + +(t/deftest temp-path-does-not-create-the-file + (let [path (tmp/temp-path :prefix "penpot.test." :min-age "6h")] + (t/testing "the reserved path does not exist yet" + (t/is (not (fs/exists? path)))) + (t/testing "an external writer can create it with CREATE_NEW" + (Files/createFile ^Path path (into-array java.nio.file.attribute.FileAttribute [])) + (t/is (fs/exists? path))) + (fs/delete path))) + +(t/deftest temp-path-is-unique + (let [a (tmp/temp-path :prefix "penpot.test.") + b (tmp/temp-path :prefix "penpot.test.")] + (t/is (not= a b)) + (t/is (not (fs/exists? a))) + (t/is (not (fs/exists? b))))) + +(t/deftest tempfile-creates-the-file-so-create-new-fails + (t/testing "documents why s3 downloads cannot use tempfile: the SDK opens with CREATE_NEW" + (let [path (tmp/tempfile :prefix "penpot.test.")] + (try + (t/is (fs/exists? path)) + (t/is (thrown? java.nio.file.FileAlreadyExistsException + (Files/createFile ^Path path (into-array java.nio.file.attribute.FileAttribute [])))) + (finally + (fs/delete path)))))) From 10504ff2bfc71f9f858c50bb268b97399ac60a2b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 6 Oct 2026 14:21:12 +0200 Subject: [PATCH 7/8] :bug: Sanitize SVG on binfile import (#12105) * :bug: Sanitize smuggled SVG on binfile import Route imported SVG storage objects through the SVG sanitizer on all three binfile paths (v3, v1, v2), closing the second bypass of GHSA-ffhp-m958-qxvr. Integrity checks still run on the raw bundle bytes first; only the persisted copy is the sanitized one. Adds a shared sanitize-imported-svg helper with unit tests and a v3 export-tamper-import test proving smuggled scripts no longer survive the import. AI-assisted-by: muse-spark-1.3-contributor * :bug: Normalize SVG content-type on binfile import Close the residual GHSA-ffhp-m958-qxvr bypass where a crafted content-type spelling (uppercase, parameters) skipped the import sanitizer. Detection now uses a shared case-insensitive predicate, stored values are canonicalized, and the v3 re-read honors the import size limit. Extends the tamper tests to obfuscated spellings and adds exhaustive v1 coverage, including the tempfile branch. AI-assisted-by: muse-spark-1.3-contributor * :bug: Gate binfile content-type on known storage types Define the complete set of storage content-types in app.common.media and enforce it on the binfile import schema, so unknown types fail closed instead of passing through. Detection keeps a single normalization at the boundary with an exact predicate, and the string helpers use cuerdas. AI-assisted-by: muse-spark-1.3-contributor * :bug: Keep only produced types in storage-object-types Drop apng, avif, penpot and plain text from the set: no flow stores them, they only exist in the extension mapping table. Every member must have a producing code path; unproduced types stay out until some flow actually stores them. AI-assisted-by: muse-spark-1.3-contributor * :bug: Reject unknown content-type on v1 binfile import Enforce the storage content-type allowlist on the v1 import path, like v3 already does via schema. Bundles declaring types outside the set, or none at all, fail closed with the same media-type-not-allowed error as uploads. The sanitize branch is explicit: SVG bytes are sanitized, anything else passes through. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/binfile/common.clj | 73 +++ backend/src/app/binfile/v1.clj | 12 +- backend/src/app/binfile/v2.clj | 12 +- backend/src/app/binfile/v3.clj | 15 +- .../test/backend_tests/binfile_svg_test.clj | 425 ++++++++++++++++++ common/src/app/common/media.cljc | 17 + common/test/common_tests/media_test.cljc | 7 + 7 files changed, 556 insertions(+), 5 deletions(-) create mode 100644 backend/test/backend_tests/binfile_svg_test.clj diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index e2aed9a837..7fe8e6a532 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -15,6 +15,7 @@ [app.common.files.migrations :as fmg] [app.common.files.validate :as fval] [app.common.logging :as l] + [app.common.media :as cm] [app.common.schema :as sm] [app.common.time :as ct] [app.common.types.file :as ctf] @@ -27,6 +28,7 @@ [app.features.file-migrations :as fmigr] [app.loggers.audit :as-alias audit] [app.loggers.webhooks :as-alias webhooks] + [app.media.svg :as svg] [app.storage :as sto] [app.util.blob :as blob] [app.util.pointer-map :as pmap] @@ -898,3 +900,74 @@ (cons (:id file))) load-fn #(get-file cfg % :migrate? false)] (weak/loadable-weak-value-map library-ids load-fn {id file}))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; SVG IMPORT SANITIZATION +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(def svg-content-type + "Canonical content type of SVG media objects." + "image/svg+xml") + +(defn normalize-content-type + "Canonicalize a stored content-type: lowercase, trimmed, + parameters after `;` dropped. Returns nil for missing or unusable + values so ancient bundle entries without content-type keep passing + through untouched." + [ctype] + (when (string? ctype) + (let [clean (-> ctype + (str/split #";" 2) + (first) + (str/trim) + (str/lower))] + (when-not (str/empty? clean) + clean)))) + +(defn svg-object? + "True when the storage `object` claims the canonical SVG content + type. Expects an already normalized object (see + `normalize-content-type`): every import path normalizes the + metadata right after reading it, so the comparison stays an exact + match in a single place." + [object] + (= svg-content-type (:content-type object))) + +(def schema:content-type + "Typed content-type for binfile storage objects: a member of + `cm/storage-object-types`. Values are canonicalized with + `normalize-content-type` before validation, so legacy spellings + keep importing while unknown types are rejected." + [::sm/one-of {:format :string} cm/storage-object-types]) + +(defn check-storage-content-type + "Check an imported storage `object` (already normalized, see + `normalize-content-type`) against `cm/storage-object-types`. + Returns nil when the type is allowed; raises `:type :validation` + with `:code :media-type-not-allowed` (same as the upload path) + when it is missing or unknown, so crafted bundles fail closed." + [object] + (when-not (contains? cm/storage-object-types (:content-type object)) + (ex/raise :type :validation + :code :media-type-not-allowed + :hint "storage object declares an unknown content-type" + :content-type (:content-type object)))) + +(defn sanitize-imported-svg + "Sanitize the raw `bytes` of an imported storage `object` when it + holds an SVG document. Expects an already normalized object (see + `normalize-content-type`); returns nil when the object is not an + SVG. + + Otherwise returns a map with the sanitized `:bytes`, their `:size` + and their blake2b `:hash`, ready to persist with `sto/put-object!`. + + Raises a `:validation` exception when the SVG cannot be parsed, the + same error the upload path reports." + [object ^bytes raw] + (when (svg-object? object) + (let [sanitized (svg/sanitize-svg (String. ^bytes raw "UTF-8")) + bytes (.getBytes ^String sanitized "UTF-8")] + {:bytes bytes + :size (alength ^bytes bytes) + :hash (sto/calculate-hash (java.io.ByteArrayInputStream. bytes))}))) diff --git a/backend/src/app/binfile/v1.clj b/backend/src/app/binfile/v1.clj index be65b423c9..fd340a487d 100644 --- a/backend/src/app/binfile/v1.clj +++ b/backend/src/app/binfile/v1.clj @@ -607,16 +607,26 @@ (doseq [expected-storage-id ids] (let [id (read-uuid! input) - mdata (read-obj! input)] + mdata (d/update-when (read-obj! input) :content-type bfc/normalize-content-type)] (when (not= id expected-storage-id) (ex/raise :type :validation :code :inconsistent-penpot-file :hint "the penpot file seems corrupt, found unexpected uuid (storage-object-id)")) + (bfc/check-storage-content-type mdata) + (l/dbg :hint "readed storage object" :id (str id) ::l/sync? true) (let [[size resource] (read-stream! input) + [resource size] (if (bfc/svg-object? mdata) + (let [raw (if (bytes? resource) + resource + (with-open [istream (jio/input-stream resource)] + (io/read istream))) + {:keys [bytes size]} (bfc/sanitize-imported-svg mdata raw)] + [bytes size]) + [resource size]) hash (sto/calculate-hash resource) content (-> (sto/content resource size) (sto/wrap-with-hash hash)) diff --git a/backend/src/app/binfile/v2.clj b/backend/src/app/binfile/v2.clj index 347074586b..2e0d04107a 100644 --- a/backend/src/app/binfile/v2.clj +++ b/backend/src/app/binfile/v2.clj @@ -197,9 +197,17 @@ (defn- read-storage-object! [{:keys [::sto/storage ::bfc/timestamp] :as cfg} id] - (let [mdata (read-obj cfg :storage-object id) + (let [mdata (d/update-when (read-obj cfg :storage-object id) + :content-type bfc/normalize-content-type) data (read-blob cfg :storage-object id) - hash (sto/calculate-hash data) + ;; NOTE: v2 bundles are legacy and no longer produced; the + ;; sanitizer call stays as defense in depth and is covered by + ;; the shared helper unit tests, with no dedicated v2 + ;; import test. + clean (when (bfc/svg-object? mdata) + (bfc/sanitize-imported-svg mdata data)) + data (or (:bytes clean) data) + hash (or (:hash clean) (sto/calculate-hash data)) content (-> (sto/content data) (sto/wrap-with-hash hash)) diff --git a/backend/src/app/binfile/v3.clj b/backend/src/app/binfile/v3.clj index cc488f6ab8..cdc3dd66bb 100644 --- a/backend/src/app/binfile/v3.clj +++ b/backend/src/app/binfile/v3.clj @@ -74,7 +74,7 @@ [:map {:title "StorageObject"} [:id ::sm/uuid] [:size ::sm/int] - [:content-type :string] + [:content-type bfc/schema:content-type] [:bucket [::sm/one-of {:format :string} sto/valid-buckets]] [:hash {:optional true} :string]]) @@ -871,6 +871,7 @@ (doseq [{:keys [id entry]} entries] (let [object (-> (read-entry input entry) (decode-storage-object) + (d/update-when :content-type bfc/normalize-content-type) (update :bucket d/nilv sto/default-bucket) (validate-storage-object)) @@ -906,7 +907,17 @@ :expected-hash (:hash object) :found-hash (sto/get-hash content)))) - (let [params (-> object + (let [clean (when (bfc/svg-object? object) + (let [limit (::bfc/import-max-object-size cfg) + raw (with-open [istream (cond-> (zip-entry-stream input (get-zip-entry input path)) + limit (size-limiting-stream limit))] + (io/read istream))] + (bfc/sanitize-imported-svg object raw))) + content (if-let [{:keys [bytes hash]} clean] + (-> (sto/content bytes) + (sto/wrap-with-hash hash)) + content) + params (-> object (dissoc :id :size) (assoc ::sto/content content) (assoc ::sto/deduplicate? true) diff --git a/backend/test/backend_tests/binfile_svg_test.clj b/backend/test/backend_tests/binfile_svg_test.clj new file mode 100644 index 0000000000..8b63de4ab8 --- /dev/null +++ b/backend/test/backend_tests/binfile_svg_test.clj @@ -0,0 +1,425 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns backend-tests.binfile-svg-test + "SVG sanitization on the binfile import path. + + Second case of GHSA-ffhp-m958-qxvr: a crafted `.penpot` bundle can + smuggle an unsanitized SVG storage object past the upload-path + sanitizer, and the raw bytes are served same-origin as + `image/svg+xml`." + (:require + [app.binfile.common :as bfc] + [app.binfile.v1 :as v1] + [app.binfile.v3 :as v3] + [app.common.features :as cfeat] + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [app.db :as db] + [app.rpc :as-alias rpc] + [app.storage :as sto] + [app.storage.tmp :as tmp] + [backend-tests.helpers :as th] + [clojure.data.json :as json] + [clojure.java.io :as jio] + [clojure.string :as str] + [clojure.test :as t] + [datoteka.fs :as fs] + [datoteka.io :as io]) + (:import + java.io.ByteArrayInputStream + java.util.zip.ZipEntry + java.util.zip.ZipFile + java.util.zip.ZipOutputStream)) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each th/database-reset) + +(def ^:private evil-svg + "") + +(def ^:private evil-xhref-svg + "click") + +(def ^:private clean-svg + "") + +(defn- utf8bytes + [^String s] + (.getBytes s "UTF-8")) + +(defn- bytes-str + [^bytes data] + (String. data "UTF-8")) + +(t/deftest sanitize-imported-svg-removes-script + (let [result (bfc/sanitize-imported-svg {:content-type "image/svg+xml" :bucket "file-media-object"} + (utf8bytes evil-svg))] + (t/is (some? result)) + (t/is (not (str/includes? (bytes-str (:bytes result)) "")))))) + +(t/deftest check-storage-content-type-allows-known-types + (doseq [ctype ["image/svg+xml" "image/jpeg" "font/woff2" "application/octet-stream"]] + (t/is (nil? (bfc/check-storage-content-type {:content-type ctype})) + (str "expected pass for " (pr-str ctype))))) + +(t/deftest check-storage-content-type-rejects-unknown-types + (doseq [ctype ["text/html" "application/x-font-woff" nil "" " "]] + (let [object (if (nil? ctype) {} {:content-type ctype}) + out (try (bfc/check-storage-content-type object) + nil + (catch clojure.lang.ExceptionInfo e + (ex-data e)))] + (t/is (= :validation (:type out)) (str "expected validation for " (pr-str ctype))) + (t/is (= :media-type-not-allowed (:code out)) (str "expected media-type-not-allowed for " (pr-str ctype)))))) + +(t/deftest storage-object-schema-constrains-content-type + (t/testing "known stored types validate, unknown types do not" + (let [base {:id (uuid/random) :size 10 :bucket "file-media-object"}] + (doseq [ctype ["image/svg+xml" "image/jpeg" "image/png" + "font/woff2" "font/ttf" "application/octet-stream" "application/pdf"]] + (t/is (some? (v3/validate-storage-object (assoc base :content-type ctype))) + (str "expected valid for " (pr-str ctype)))) + (doseq [ctype ["" " " "image /svg" "image/apng" "image/avif" + "application/x-font-woff" (apply str (repeat 200 "x"))]] + (t/is (thrown? clojure.lang.ExceptionInfo + (v3/validate-storage-object (assoc base :content-type ctype))) + (str "expected rejection for " (pr-str ctype))))))) + +(defn- write-temp-svg + "Write `text` to a tempfile and return `[path size]`." + [text] + (let [path (fs/create-tempfile :prefix "penpot-svg-import-test-" :suffix ".svg") + data (utf8bytes text)] + (spit (str path) text :encoding "UTF-8") + [path (alength data)])) + +(defn- tamper-svg-entry! + "Copy `src` zip to `dst` zip replacing the `objects/.svg` + payload with `evil-bytes` and fixing its `.json` sidecar (size and + blake2b hash) so the bundle passes the integrity checks. When + `content-type` is given, the sidecar declares it instead of the + original spelling." + [src dst storage-id ^bytes evil-bytes & {:keys [content-type]}] + (let [svg-name (str "objects/" storage-id ".svg") + json-name (str "objects/" storage-id ".json")] + (with-open [zin (ZipFile. (.toFile ^java.nio.file.Path src)) + zout (ZipOutputStream. (io/output-stream dst))] + (doseq [entry (enumeration-seq (.entries zin))] + (let [name (.getName ^ZipEntry entry)] + (cond + (= name svg-name) + (do (.putNextEntry zout (ZipEntry. ^String name)) + (.write zout evil-bytes 0 (alength evil-bytes)) + (.closeEntry zout)) + + (= name json-name) + (let [raw (slurp (.getInputStream zin entry) :encoding "UTF-8") + parsed (json/read-str raw) + updated (cond-> (assoc parsed + "size" (alength evil-bytes) + "hash" (sto/calculate-hash (ByteArrayInputStream. evil-bytes))) + content-type (assoc "contentType" content-type))] + (.putNextEntry zout (ZipEntry. ^String name)) + (.write zout (utf8bytes (json/write-str updated))) + (.closeEntry zout)) + + :else + (do (.putNextEntry zout (ZipEntry. ^String name)) + (with-open [in (.getInputStream zin entry)] + (io/copy in zout)) + (.closeEntry zout)))))))) + +(defn- setup-file-with-svg-media + "Upload `clean-svg`, attach it to a shape fill and return + `[file uploaded]` ready to export." + [profile n] + (let [file (th/create-file* n {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + [svg-path svg-size] (write-temp-svg clean-svg) + out (th/command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id profile) + :file-id (:id file) + :is-local true + :name "benign.svg" + :content {:filename "benign.svg" + :path svg-path + :mtype "image/svg+xml" + :size svg-size}}) + _ (t/is (nil? (:error out)) (str "upload failed: " (pr-str (:error out)))) + uploaded (:result out) + page-id (first (get-in file [:data :pages])) + shape-id (uuid/random) + upd (th/command! {::th/type :update-file + ::rpc/profile-id (:id profile) + :id (:id file) + :session-id (uuid/random) + :revn 0 + :vern 0 + :features cfeat/supported-features + :changes + [{:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :components-v2 true + :obj (cts/setup-shape + {:id shape-id + :name "image" + :frame-id uuid/zero + :parent-id uuid/zero + :type :rect + :fills [{:fill-opacity 1 + :fill-image {:id (:id uploaded) + :width (:width uploaded) + :height (:height uploaded) + :mtype "image/svg+xml"}}]})}]})] + (t/is (nil? (:error upd)) (str "update-file failed: " (pr-str (:error upd)))) + (t/is (uuid? (:media-id uploaded))) + [file uploaded])) + +(defn- export-tamper-import-v3 + "Export `file` to v3, tamper its SVG payload with `evil-text` + (declaring `content-type` when given) and import the bundle. + Returns `[served-bytes stored-content-type]` of the imported + media." + [profile file media-id evil-text content-type] + (let [bundle (tmp/tempfile :prefix "penpot-export-" :suffix ".zip")] + (v3/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/embed-assets false) + (assoc ::bfc/include-libraries false)) + (io/output-stream bundle)) + + (let [evil (tmp/tempfile :prefix "penpot-evil-" :suffix ".zip") + _ (tamper-svg-entry! bundle evil media-id (utf8bytes evil-text) :content-type content-type) + result (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input evil) + (v3/import-files!))] + (t/is (= 1 (count result))) + (let [rows (th/db-query :file-media-object {:file-id (first result)}) + storage (:app.storage/storage th/*system*)] + (t/is (= 1 (count rows))) + (let [object (sto/get-object storage (:media-id (first rows)))] + [(bytes-str (sto/get-object-bytes storage object)) + (:content-type (meta object))]))))) + +(t/deftest import-binfile-v3-sanitizes-smuggled-svg + (let [profile (th/create-profile* 1) + [file uploaded] (setup-file-with-svg-media profile 1) + [served _stored] (export-tamper-import-v3 profile file (:media-id uploaded) evil-svg nil)] + (t/is (not (str/includes? served " (sto/content raw) + (sto/wrap-with-hash (sto/calculate-hash (ByteArrayInputStream. raw)))) + sobj (sto/put-object! storage {::sto/content content + :content-type content-type + :bucket "file-media-object"}) + fmo-id (uuid/random) + _ (th/db-insert! :file-media-object + {:id fmo-id + :file-id (:id file) + :is-local true + :name "seed.svg" + :media-id (:id sobj) + :width 64 + :height 64 + :mtype content-type}) + page-id (first (get-in file [:data :pages])) + shape-id (uuid/random) + upd (th/command! {::th/type :update-file + ::rpc/profile-id (:id profile) + :id (:id file) + :session-id (uuid/random) + :revn 0 + :vern 0 + :features cfeat/supported-features + :changes + [{:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :components-v2 true + :obj (cts/setup-shape + {:id shape-id + :name "image" + :frame-id uuid/zero + :parent-id uuid/zero + :type :rect + :fills [{:fill-opacity 1 + :fill-image {:id fmo-id + :width 64 + :height 64 + :mtype "image/svg+xml"}}]})}]})] + (t/is (nil? (:error upd)) (str "update-file failed: " (pr-str (:error upd)))) + [file (:id sobj)])) + +(defn- export-v1 + "Export `file` to v1 and return the bundle path." + [profile file] + (let [bundle (tmp/tempfile :prefix "penpot-export-v1-" :suffix ".bin")] + (v1/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/embed-assets false) + (assoc ::bfc/include-libraries false)) + (io/output-stream bundle)) + bundle)) + +(defn- export-import-v1 + "Export `file` to v1, import it back and return the served bytes + of its single media object." + [profile file] + (let [bundle (export-v1 profile file)] + (let [result (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input (io/input-stream bundle)) + (v1/import-files!))] + (t/is (= 1 (count result))) + (let [rows (th/db-query :file-media-object {:file-id (first result)}) + storage (:app.storage/storage th/*system*)] + (t/is (= 1 (count rows))) + (let [object (sto/get-object storage (:media-id (first rows)))] + [(bytes-str (sto/get-object-bytes storage object)) + (:content-type (meta object))]))))) + +(t/deftest import-binfile-v1-sanitizes-seeded-svg + (let [profile (th/create-profile* 4) + [file _seed] (seed-file-with-svg-media profile 4 evil-svg "IMAGE/SVG+XML") + [served stored] (export-import-v1 profile file)] + (t/is (not (str/includes? served " th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input (io/input-stream bundle)) + (v1/import-files!)) + nil + (catch clojure.lang.ExceptionInfo e + (ex-data e)))] + (t/is (= :validation (:type out))) + (t/is (= :media-type-not-allowed (:code out))) + (t/is (= [(:id file)] + (mapv :id (th/db-query :file {:project-id (:default-project-id profile)})))))) + +(def ^:private large-clean-svg + (let [pad (apply str (repeat 110000 ""))] + (str "" pad ""))) + +(t/deftest import-binfile-v1-roundtrips-large-svg + (t/testing "objects over the tempfile threshold (tempfile branch) import fine" + (t/is (> (alength (utf8bytes large-clean-svg)) bfc/temp-file-threshold)) + (let [profile (th/create-profile* 5) + [file _seed] (seed-file-with-svg-media profile 5 large-clean-svg "image/svg+xml") + [served _stored] (export-import-v1 profile file)] + (t/is (str/includes? served "circle"))))) diff --git a/common/src/app/common/media.cljc b/common/src/app/common/media.cljc index a5a74e6c75..b7ec9b5a71 100644 --- a/common/src/app/common/media.cljc +++ b/common/src/app/common/media.cljc @@ -25,6 +25,23 @@ (def tempfile-types (conj image-types "application/pdf" "application/zip")) +(def storage-object-types + "Every content-type the system stores in storage objects: uploaded + images and fonts, generated thumbnails, temporary files and the + `application/octet-stream` fallback used by file data and font + variants. Sorted for determinism. Every member must have a + producing code path; anticipated but unproduced types (such as + `image/apng`) stay out until some flow actually stores them. + Import boundaries normalize the stored string (lowercase, no + parameters) before checking membership, so legacy spelling + variants keep importing while unknown types are rejected." + (into (sorted-set) + (concat image-types + font-types + ["application/octet-stream" + "application/pdf" + "application/zip"]))) + (defn format->extension [format] (case format diff --git a/common/test/common_tests/media_test.cljc b/common/test/common_tests/media_test.cljc index c6916e3216..c0440331bc 100644 --- a/common/test/common_tests/media_test.cljc +++ b/common/test/common_tests/media_test.cljc @@ -9,6 +9,13 @@ [app.common.media :as media] [clojure.test :as t])) +(t/deftest test-storage-object-types + (t/testing "covers every image and font type the system stores" + (t/is (every? #(contains? media/storage-object-types %) media/image-types)) + (t/is (every? #(contains? media/storage-object-types %) media/font-types)) + (t/is (contains? media/storage-object-types "image/svg+xml")) + (t/is (contains? media/storage-object-types "application/octet-stream")))) + (t/deftest test-parse-font-weight (t/testing "matches weight tokens with proper boundaries" (t/is (= 700 (media/parse-font-weight "Roboto-Bold"))) From ff7a66f51938b2293260ea27a64e2c4e0833b5e3 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 6 Oct 2026 14:25:50 +0200 Subject: [PATCH 8/8] :books: Update changelog --- CHANGES.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 56d1046320..c48637725a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,12 @@ # CHANGELOG +## 2.18.3 + +### :bug: Bugs fixed + +- Fix binfile import storing unsanitized SVG and missing obfuscated SVG content-types [#12104](https://github.com/penpot/penpot/issues/12104) (PR: [#12105](https://github.com/penpot/penpot/pull/12105)) +- Fix removed team members keeping file access after leaving the team [#12106](https://github.com/penpot/penpot/issues/12106) (PR: [#12097](https://github.com/penpot/penpot/pull/12097)) + ## 2.18.2 ### :bug: Bugs fixed