github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-07 02:20:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Validate binfile library relations against the package (#12862)

Browse Source 
Ignore imported library relations that point outside the
package or self-link a file on the v3 import path. Delete
leftover cross-team and self relations from file_library_rel
and their matching file_library_sync rows with migration
0158, sync rows first.

Move the get-files-rels SQL to a var, without behaviour change.

Cover the write-side invariants with state-based tests:
tampered manifests, package containment, self links and the
migration cleanup.

AI-assisted-by: deepseek-v4.1-flash
AI-assisted-by: muse-spark-1.3-contributor
This commit is contained in:
Andrey Antukh 2026-11-06 08:37:02 +01:00 committed by GitHub
parent 84e0b33b74
commit 0b8a428d6b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 313 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
backend/src/app/binfile/common.clj
View File

@ -472,6 +472,18 @@
{:team-id team-id
:deleted-at nil}))
(def ^:private sql:get-files-rels
"SELECT flr.*,
fls.synced_at
FROM file_library_rel AS flr
JOIN file AS l
ON flr.library_file_id = l.id
LEFT JOIN file_library_sync AS fls
ON fls.file_id = flr.file_id
AND fls.library_file_id = flr.library_file_id
WHERE flr.file_id = ANY(?)
AND l.deleted_at IS NULL;")
(defn get-files-rels
"Given a set of file-id's, return all matching relations with the libraries"
[cfg ids]
@ -480,18 +492,8 @@
(assert (every? uuid? ids) "expected a set of uuids")
(db/run! cfg (fn [{:keys [::db/conn]}]
(let [ids (db/create-array conn "uuid" ids)
sql (str "SELECT flr.*,"
" fls.synced_at"
" FROM file_library_rel AS flr"
" JOIN file AS l"
" ON flr.library_file_id = l.id"
" LEFT JOIN file_library_sync AS fls"
" ON fls.file_id = flr.file_id"
" AND fls.library_file_id = flr.library_file_id"
" WHERE flr.file_id = ANY(?)"
" AND l.deleted_at IS NULL;")]
(db/exec! conn [sql ids])))))
(let [ids (db/create-array conn "uuid" ids)]
(db/exec! conn [sql:get-files-rels ids])))))
(def ^:private sql:upsert-file-library-sync
"INSERT INTO file_library_sync (file_id, library_file_id, synced_at)

32
backend/src/app/binfile/v3.clj
View File

@ -989,20 +989,24 @@
(defn- import-file-relations
[{:keys [::db/conn ::manifest ::bfc/timestamp] :as cfg}]
(events/tap :progress {:section :relations})
(doseq [[file-id libr-id] (:relations manifest)]
(let [file-id (bfc/lookup-index file-id)
libr-id (bfc/lookup-index libr-id)]
(when (and file-id libr-id)
(l/dbg :hint "create file library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
(let [rel-params {:file-id file-id
:library-file-id libr-id}]
(db/insert! conn :file-library-rel rel-params)
(bfc/upsert-file-library-sync! conn (assoc rel-params :synced-at timestamp)))))))
(let [file-ids (into #{} (map :id) (:files manifest))]
(doseq [[file-id libr-id] (:relations manifest)]
(if (and (not= file-id libr-id)
(contains? file-ids file-id)
(contains? file-ids libr-id))
(let [file-id (bfc/lookup-index file-id)
libr-id (bfc/lookup-index libr-id)]
(l/dbg :hint "create file library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
(let [rel-params {:file-id file-id
:library-file-id libr-id}]
(db/insert! conn :file-library-rel rel-params)
(bfc/upsert-file-library-sync! conn (assoc rel-params :synced-at timestamp))))
(l/warn :hint "ignoring invalid file library relation"
:file-id (str file-id)
:lib-id (str libr-id))))))
(defn- import-storage-objects
[{:keys [::bfc/input ::entries-index ::bfc/timestamp] :as cfg}]

5
backend/src/app/migrations.clj
View File

@ -529,7 +529,10 @@
:fn (mg/resource "app/migrations/sql/0156-reset-pg-storage-defaults.sql")}
{:name "0157-migrate-legacy-file-data"
:fn (mg/resource "app/migrations/sql/0157-migrate-legacy-file-data.sql")}])
:fn (mg/resource "app/migrations/sql/0157-migrate-legacy-file-data.sql")}
{:name "0158-del-forbidden-file-library-rels"
:fn (mg/resource "app/migrations/sql/0158-del-forbidden-file-library-rels.sql")}])
(defn apply-migrations!
[pool name migrations]

38
backend/src/app/migrations/sql/0158-del-forbidden-file-library-rels.sql Normal file
View File

@ -0,0 +1,38 @@
-- Remove file-library relations that violate the same-team invariant:
-- relations across teams and self-relations. Leftover from before the
-- binfile export, the view-only bundle, link-file-to-library and the
-- manifest writers enforced same-team libraries. Relations with both
-- endpoints in the same team are kept.
--
-- The sync rows go first: the second statement reads file_library_rel
-- while it still contains the forbidden rows.
WITH forbidden AS (
SELECT flr.file_id, flr.library_file_id
FROM file_library_rel flr
JOIN file f ON f.id = flr.file_id
JOIN project fp ON fp.id = f.project_id
JOIN file l ON l.id = flr.library_file_id
JOIN project lp ON lp.id = l.project_id
WHERE fp.team_id <> lp.team_id
OR flr.file_id = flr.library_file_id
)
DELETE FROM file_library_sync fls
USING forbidden
WHERE fls.file_id = forbidden.file_id
AND fls.library_file_id = forbidden.library_file_id;
WITH forbidden AS (
SELECT flr.file_id, flr.library_file_id
FROM file_library_rel flr
JOIN file f ON f.id = flr.file_id
JOIN project fp ON fp.id = f.project_id
JOIN file l ON l.id = flr.library_file_id
JOIN project lp ON lp.id = l.project_id
WHERE fp.team_id <> lp.team_id
OR flr.file_id = flr.library_file_id
)
DELETE FROM file_library_rel flr
USING forbidden
WHERE flr.file_id = forbidden.file_id
AND flr.library_file_id = forbidden.library_file_id;

239
backend/test/backend_tests/binfile_test.clj
View File

@ -30,6 +30,7 @@
[app.storage.tmp :as tmp]
[backend-tests.helpers :as th]
[backend-tests.storage-test :as stt]
[clojure.data.json :as json]
[clojure.java.io :as jio]
[clojure.test :as t]
[cuerdas.core :as str]
@ -37,6 +38,9 @@
[datoteka.io :as io])
(:import
java.io.ByteArrayInputStream
java.io.ByteArrayOutputStream
java.io.DataInputStream
java.io.DataOutputStream
java.io.OutputStreamWriter
java.io.Writer
java.util.zip.Deflater
@ -216,6 +220,63 @@
:token-radius token-radius
:token-opacity token-opacity}))
(defn- export-file
[file & {:keys [include-libraries]}]
(let [output (tmp/tempfile :suffix ".zip")]
(v3/export-files!
(cond-> (-> th/*system*
(assoc ::bfc/ids #{(:id file)}))
include-libraries
(assoc ::bfc/export-type :include-libraries))
(io/output-stream output))
output))
(defn- import-bundle
[profile path]
(-> th/*system*
(assoc ::bfc/project-id (:default-project-id profile))
(assoc ::bfc/team-id (:default-team-id profile))
(assoc ::bfc/profile-id (:id profile))
(assoc ::bfc/input path)
(v3/import-files!)
(:file-ids)))
(defn- read-manifest
[path]
(with-open [zip (ZipFile. (.toFile ^java.nio.file.Path path))]
(let [entry (.getEntry zip "manifest.json")]
(with-open [input (.getInputStream zip entry)]
(json/read-str (slurp input :encoding "UTF-8"))))))
(defn- tamper-manifest-relations
[src dst relations]
(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)]
(if (= name "manifest.json")
(let [manifest (-> (slurp (.getInputStream zin entry) :encoding "UTF-8")
(json/read-str)
(assoc "relations" relations))]
(.putNextEntry zout (ZipEntry. name))
(.write zout (.getBytes (json/write-str manifest) "UTF-8"))
(.closeEntry zout))
(do
(.putNextEntry zout (ZipEntry. name))
(with-open [input (.getInputStream zin entry)]
(io/copy input zout))
(.closeEntry zout)))))))
(defn- prepare-victim-file
[n]
(let [profile (th/create-profile* n)
team (th/create-team* n {:profile-id (:id profile)})
project (th/create-project* n {:team-id (:id team)
:profile-id (:id profile)})
file (th/create-file* n {:profile-id (:id profile)
:project-id (:id project)})]
[profile file]))
(def ^:private svg-raw-page-id (uuid/custom 1 1))
(def ^:private svg-raw-root-id (uuid/custom 3 1))
(def ^:private svg-raw-child-id (uuid/custom 3 2))
@ -2898,3 +2959,181 @@
(t/is (= (str (first result) "/" thumb-page-id "/" thumb-frame-id "/" thumb-tag)
(:object-id thumb)))
(t/is (some? (:media-id thumb)))))))
;; --- Library relations security tests
(t/deftest import-v3-ignores-relations-outside-package
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
[_ victim] (prepare-victim-file 3)
bundle (export-file file)
evil (tmp/tempfile :suffix ".zip")
_ (tamper-manifest-relations bundle evil
[[(str (:id file)) (str (:id victim))]])
[file-id :as result] (import-bundle profile evil)]
(t/is (= 1 (count result)))
(t/is (empty? (th/db-query :file-library-rel {:file-id file-id})))
(t/is (empty? (th/db-query :file-library-sync {:file-id file-id})))))
(defn- prepare-other-team
[n]
(let [profile (th/create-profile* n)
team (th/create-team* n {:profile-id (:id profile)})
project (th/create-project* n {:team-id (:id team)
:profile-id (:id profile)})
file (th/create-file* n {:profile-id (:id profile)
:project-id (:id project)})
library (th/create-file* (inc n) {:profile-id (:id profile)
:project-id (:id project)})]
[profile file library]))
(t/deftest import-v3-ignores-foreign-file-id
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
[_ victim] (prepare-victim-file 3)
bundle (export-file file)
evil (tmp/tempfile :suffix ".zip")
_ (tamper-manifest-relations bundle evil
[[(str (:id victim)) (str (:id file))]])
result (import-bundle profile evil)]
(t/is (= 1 (count result)))
(t/is (empty? (th/db-query :file-library-rel {:file-id (:id victim)})))
(t/is (empty? (th/db-query :file-library-sync {:file-id (:id victim)})))))
(t/deftest import-v3-ignores-unknown-library
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
bundle (export-file file)
evil (tmp/tempfile :suffix ".zip")
_ (tamper-manifest-relations bundle evil
[[(str (:id file)) (str (uuid/random))]])
[file-id :as result] (import-bundle profile evil)]
(t/is (= 1 (count result)))
(t/is (empty? (th/db-query :file-library-rel {:file-id file-id})))))
(t/deftest import-v3-ignores-self-library-relation
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
bundle (export-file file)
evil (tmp/tempfile :suffix ".zip")
_ (tamper-manifest-relations bundle evil
[[(str (:id file)) (str (:id file))]])
[file-id :as result] (import-bundle profile evil)]
(t/is (= 1 (count result)))
(t/is (empty? (th/db-query :file-library-rel {:file-id file-id})))))
(t/deftest import-v3-keeps-valid-library-relation
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
library (th/create-file* 2 {:profile-id (:id profile)
:project-id (:default-project-id profile)})]
(th/link-file-to-library* {:file-id (:id file)
:library-id (:id library)})
(let [bundle (export-file file :include-libraries true)
result (import-bundle profile bundle)
new-file (->> result
(map #(bfc/get-file th/*system* %))
(some #(when (= (:name %) (:name file)) %)))]
(t/is (= 2 (count result)))
(t/is (some? new-file))
(t/is (= 1 (count (th/db-query :file-library-rel
{:file-id (:id new-file)})))))))
(t/deftest export-multi-file-includes-package-relations
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
library (th/create-file* 2 {:profile-id (:id profile)
:project-id (:default-project-id profile)})
[_ other-file other-library] (prepare-other-team 3)
_ (th/link-file-to-library* {:file-id (:id file)
:library-id (:id library)})
_ (th/link-file-to-library* {:file-id (:id other-file)
:library-id (:id other-library)})
_ (th/db-insert! :file-library-rel
{:file-id (:id file)
:library-file-id (:id other-library)})
output (tmp/tempfile :suffix ".zip")]
(v3/export-files!
(-> th/*system*
(assoc ::bfc/ids #{(:id file) (:id other-file)})
(assoc ::bfc/export-type :include-libraries))
(io/output-stream output))
(let [rels (set (map vec (get (read-manifest output) "relations")))]
(t/is (contains? rels [(str (:id file)) (str (:id library))]))
(t/is (contains? rels [(str (:id other-file)) (str (:id other-library))]))
(t/is (contains? rels [(str (:id file)) (str (:id other-library))])))))
;; --- Forbidden library relations cleanup (migration candidate)
;;
;; The two statements below are the exact cleanup proposed for a future
;; migration (see tmp/CHECK-LIBRARY-RELATIONS.sql, section 6). The sync
;; rows must go first: the second statement reads file_library_rel
;; while it still contains the forbidden rows.
(def ^:private sql:delete-forbidden-library-sync
"WITH forbidden AS (
SELECT flr.file_id, flr.library_file_id
FROM file_library_rel flr
JOIN file f ON f.id = flr.file_id
JOIN project fp ON fp.id = f.project_id
JOIN file l ON l.id = flr.library_file_id
JOIN project lp ON lp.id = l.project_id
WHERE fp.team_id <> lp.team_id
OR flr.file_id = flr.library_file_id
)
DELETE FROM file_library_sync fls
USING forbidden
WHERE fls.file_id = forbidden.file_id
AND fls.library_file_id = forbidden.library_file_id")
(def ^:private sql:delete-forbidden-library-rel
"WITH forbidden AS (
SELECT flr.file_id, flr.library_file_id
FROM file_library_rel flr
JOIN file f ON f.id = flr.file_id
JOIN project fp ON fp.id = f.project_id
JOIN file l ON l.id = flr.library_file_id
JOIN project lp ON lp.id = l.project_id
WHERE fp.team_id <> lp.team_id
OR flr.file_id = flr.library_file_id
)
DELETE FROM file_library_rel flr
USING forbidden
WHERE flr.file_id = forbidden.file_id
AND flr.library_file_id = forbidden.library_file_id")
(t/deftest cleanup-deletes-forbidden-library-relations
(let [profile (th/create-profile* 1)
file (prepare-simple-file profile)
library (th/create-file* 2 {:profile-id (:id profile)
:project-id (:default-project-id profile)})
[_ victim] (prepare-victim-file 3)
_ (th/link-file-to-library* {:file-id (:id file)
:library-id (:id library)})
_ (th/db-insert! :file-library-rel
{:file-id (:id file)
:library-file-id (:id victim)})
_ (th/db-insert! :file-library-sync
{:file-id (:id file)
:library-file-id (:id victim)})
_ (th/db-insert! :file-library-rel
{:file-id (:id file)
:library-file-id (:id file)})
_ (th/db-insert! :file-library-sync
{:file-id (:id file)
:library-file-id (:id file)})]
(t/is (= 3 (count (th/db-query :file-library-rel {:file-id (:id file)}))))
(th/db-exec! [sql:delete-forbidden-library-sync])
(th/db-exec! [sql:delete-forbidden-library-rel])
(t/testing "same-team relation and its sync row survive"
(t/is (= [{:file-id (:id file) :library-file-id (:id library)}]
(map #(select-keys % [:file-id :library-file-id])
(th/db-query :file-library-rel {:file-id (:id file)}))))
(t/is (= [{:file-id (:id file) :library-file-id (:id library)}]
(map #(select-keys % [:file-id :library-file-id])
(th/db-query :file-library-sync {:file-id (:id file)})))))
(t/testing "cross-team and self relations and their sync rows are gone"
(t/is (empty? (th/db-query :file-library-rel {:library-file-id (:id victim)})))
(t/is (empty? (th/db-query :file-library-sync {:library-file-id (:id victim)})))
(t/is (empty? (th/db-query :file-library-rel {:library-file-id (:id file)})))
(t/is (empty? (th/db-query :file-library-sync {:library-file-id (:id file)}))))))