From 6c2b61e1ad6912f556b0e297c13a3a8a262b182a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 28 Jul 2026 10:59:16 +0200 Subject: [PATCH] :bug: Fix several issues in RPC command handlers (#10670) * :bug: Fix several issues in RPC command handlers - Reject circular library references in link-file-to-library - Add explicit team permission check in search-files - Constrain search-term max length to 250 chars - Include :deleted-at in file ETag for COND caching - Move storage I/O outside DB transaction in create-file-thumbnail AI-assisted-by: deepseek-v4-pro * :paperclip: Check perms before circular link checks * :bug: Handle circular library reference error Catch :circular-library-reference error from backend when linking files to libraries. Show user-friendly toast notification instead of propagating unhandled error. Add English and Spanish translations. AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/files.clj | 13 +- .../src/app/rpc/commands/files_thumbnails.clj | 116 ++++++++---------- backend/src/app/rpc/commands/search.clj | 8 +- backend/test/backend_tests/rpc_file_test.clj | 72 +++++++++++ .../app/main/data/workspace/libraries.cljs | 7 +- frontend/translations/en.po | 3 + frontend/translations/es.po | 3 + 7 files changed, 154 insertions(+), 68 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index 3851ea577b..4daa2dd32a 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -156,11 +156,13 @@ (assoc mfile :permissions perms))) (defn get-file-etag - [{:keys [::rpc/profile-id]} {:keys [modified-at revn vern permissions]}] + [{:keys [::rpc/profile-id]} {:keys [modified-at revn vern deleted-at permissions]}] (str profile-id "/" revn "/" vern "/" (hash fmg/available-migrations) "/" (ct/format-inst modified-at :iso) "/" - (uri/map->query-string permissions))) + (uri/map->query-string permissions) + "/" + (some-> deleted-at (ct/format-inst :iso)))) (sv/defmethod ::get-file "Retrieve a file by its ID. Only authenticated users." @@ -1102,6 +1104,13 @@ (check-edition-permissions! conn profile-id file-id) (check-edition-permissions! conn profile-id library-id) + + (let [transitive-deps (bfc/get-libraries cfg [library-id])] + (when (contains? transitive-deps file-id) + (ex/raise :type :validation + :code :circular-library-reference + :hint "linking this library would create a circular dependency"))) + (link-file-to-library conn params) (bfc/get-libraries cfg [library-id])) diff --git a/backend/src/app/rpc/commands/files_thumbnails.clj b/backend/src/app/rpc/commands/files_thumbnails.clj index 130d9a86be..024bce17e7 100644 --- a/backend/src/app/rpc/commands/files_thumbnails.clj +++ b/backend/src/app/rpc/commands/files_thumbnails.clj @@ -374,61 +374,6 @@ ;; --- MUTATION COMMAND: create-file-thumbnail -(defn- create-file-thumbnail - [{:keys [::db/conn ::sto/storage] :as cfg} {:keys [file-id revn props media] :as params}] - (media/validate-media-type! media) - (media/validate-media-size! media) - - (let [file (bfc/get-file cfg file-id - :include-deleted? true - :load-data? false) - - props (db/tjson (or props {})) - path (:path media) - mtype (:mtype media) - hash (sto/calculate-hash path) - data (-> (sto/content path) - (sto/wrap-with-hash hash)) - tnow (ct/now) - - media (sto/put-object! storage - {::sto/content data - ::sto/deduplicate? true - ::sto/touched-at tnow - :content-type mtype - :bucket "file-thumbnail"}) - - thumb (db/get* conn :file-thumbnail - {:file-id file-id - :revn revn} - {::db/remove-deleted false - ::sql/for-update true})] - - (if (some? thumb) - (do - ;; We mark the old media id as touched if it does not match - (when (not= (:id media) (:media-id thumb)) - (sto/touch-object! storage (:media-id thumb))) - - (db/update! conn :file-thumbnail - {:media-id (:id media) - :deleted-at (:deleted-at file) - :updated-at tnow - :props props} - {:file-id file-id - :revn revn})) - - (db/insert! conn :file-thumbnail - {:file-id file-id - :revn revn - :created-at tnow - :updated-at tnow - :deleted-at (:deleted-at file) - :props props - :media-id (:id media)})) - - media)) - (def ^:private schema:create-file-thumbnail [:map {:title "create-file-thumbnail"} @@ -448,12 +393,57 @@ ::rtry/when rtry/conflict-exception? ::sm/params schema:create-file-thumbnail} - ;; FIXME: do not run the thumbnail upload inside a transaction - [cfg {:keys [::rpc/profile-id file-id] :as params}] - (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] - (files/check-edition-permissions! conn profile-id file-id) - (when-not (db/read-only? conn) - (let [media (create-file-thumbnail cfg params)] - {:uri (files/resolve-public-uri (:id media)) - :id (:id media)}))))) + (media/validate-media-type! (:media params)) + (media/validate-media-size! (:media params)) + + (db/run! cfg files/check-edition-permissions! profile-id file-id) + + (when-not (db/read-only? (::db/pool cfg)) + (let [storage (::sto/storage cfg) + file (bfc/get-file cfg file-id :include-deleted? true :load-data? false) + props (db/tjson (or (:props params) {})) + {:keys [path mtype]} (:media params) + hash (sto/calculate-hash path) + data (-> (sto/content path) + (sto/wrap-with-hash hash)) + tnow (ct/now) + + media (sto/put-object! storage + {::sto/content data + ::sto/deduplicate? true + ::sto/touched-at tnow + :content-type mtype + :bucket "file-thumbnail"}) + + revn (:revn params) + + result (db/tx-run! cfg + (fn [{:keys [::db/conn]}] + (let [thumb (db/get* conn :file-thumbnail + {:file-id file-id :revn revn} + {::db/remove-deleted false + ::sql/for-update true})] + (if (some? thumb) + (do + (when (not= (:id media) (:media-id thumb)) + (sto/touch-object! storage (:media-id thumb))) + (db/update! conn :file-thumbnail + {:media-id (:id media) + :deleted-at (:deleted-at file) + :updated-at tnow + :props props} + {:file-id file-id :revn revn})) + (db/insert! conn :file-thumbnail + {:file-id file-id + :revn revn + :created-at tnow + :updated-at tnow + :deleted-at (:deleted-at file) + :props props + :media-id (:id media)})) + media)))] + + (when result + {:uri (files/resolve-public-uri (:id result)) + :id (:id result)})))) diff --git a/backend/src/app/rpc/commands/search.clj b/backend/src/app/rpc/commands/search.clj index 796ddf4810..7b60e6db30 100644 --- a/backend/src/app/rpc/commands/search.clj +++ b/backend/src/app/rpc/commands/search.clj @@ -6,9 +6,11 @@ (ns app.rpc.commands.search (:require + [app.common.data.macros :as dm] [app.common.schema :as sm] [app.db :as db] [app.rpc :as-alias rpc] + [app.rpc.commands.teams :as teams] [app.rpc.doc :as-alias doc] [app.util.services :as sv])) @@ -66,11 +68,13 @@ (def ^:private schema:search-files [:map {:title "search-files"} [:team-id ::sm/uuid] - [:search-term {:optional true} :string]]) + [:search-term {:optional true} [:string {:max 250}]]]) (sv/defmethod ::search-files {::doc/added "1.17" ::doc/module :files ::sm/params schema:search-files} [{:keys [::db/pool]} {:keys [::rpc/profile-id team-id search-term]}] - (some->> search-term (search-files pool profile-id team-id))) + (dm/with-open [conn (db/open pool)] + (teams/check-read-permissions! conn profile-id team-id) + (some->> search-term (search-files conn profile-id team-id)))) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index 5460b4143f..1c07f35971 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -2319,3 +2319,75 @@ (t/is (not (nil? (:error out)))) (let [edata (-> out :error ex-data)] (t/is (= :not-found (:type edata)))))) + +;; --- Security Fix Tests --- + +(t/deftest link-file-to-library-circular-reference + (let [profile (th/create-profile* 1) + file1 (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared true}) + file2 (th/create-file* 2 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared true}) + file3 (th/create-file* 3 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false})] + (th/link-file-to-library* {:file-id (:id file3) :library-id (:id file2)}) + (th/link-file-to-library* {:file-id (:id file2) :library-id (:id file1)}) + (let [data {::th/type :link-file-to-library + ::rpc/profile-id (:id profile) + :file-id (:id file1) + :library-id (:id file3)} + out (th/command! data)] + (t/is (th/ex-info? (:error out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (let [edata (-> out :error ex-data)] + (t/is (= :circular-library-reference (:code edata))))))) + +(t/deftest get-file-etag-includes-deleted-at + (let [profile-id (uuid/random) + file1 {:modified-at (ct/now) + :revn 1 + :vern 0 + :deleted-at nil + :permissions {:can-edit true}} + file2 (assoc file1 :deleted-at (ct/now))] + (t/is (not= (files/get-file-etag {::rpc/profile-id profile-id} file1) + (files/get-file-etag {::rpc/profile-id profile-id} file2))))) + +(t/deftest search-files-with-permission + (let [profile (th/create-profile* 1) + _ (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + data {::th/type :search-files + ::rpc/profile-id (:id profile) + :team-id (:default-team-id profile) + :search-term "test"} + out (th/command! data)] + (t/is (nil? (:error out))) + (t/is (vector? (:result out))))) + +(t/deftest search-files-forbidden + (let [profile (th/create-profile* 1) + other (th/create-profile* 2) + data {::th/type :search-files + ::rpc/profile-id (:id other) + :team-id (:default-team-id profile) + :search-term "test"} + out (th/command! data)] + (t/is (th/ex-info? (:error out))) + (let [edata (-> out :error ex-data)] + (t/is (= :not-found (:type edata)))))) + +(t/deftest search-files-term-too-long + (let [profile (th/create-profile* 1) + data {::th/type :search-files + ::rpc/profile-id (:id profile) + :team-id (:default-team-id profile) + :search-term (apply str (repeat 300 "x"))} + out (th/command! data)] + (t/is (th/ex-info? (:error out))) + (let [edata (-> out :error ex-data)] + (t/is (= :validation (:type edata)))))) diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index 667f0b2b61..5c09149bad 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -1571,7 +1571,12 @@ (as-> libraries-to-load $ (remove loaded-libraries $) (conj $ library-id) - (map #(load-library-file file-id %) $)))))) + (map #(load-library-file file-id %) $)))) + (rx/catch (fn [cause] + (let [error (ex-data cause)] + (if (= (:code error) :circular-library-reference) + (rx/of (ntf/error (tr "errors.circular-library-reference"))) + (rx/throw cause))))))) (rx/of (ptk/reify ::attach-library-finished)) (when (pos? variants-count) (->> (rp/cmd! :get-library-usage {:file-id library-id}) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 2155b71895..1f0017d87f 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1490,6 +1490,9 @@ msgstr "The fonts %s could not be loaded" msgid "errors.cannot-upload" msgstr "Cannot upload the media file." +msgid "errors.circular-library-reference" +msgstr "Cannot add library: this would create a circular dependency" + #: src/app/main/ui/comments.cljs:737, src/app/main/ui/comments.cljs:767, src/app/main/ui/comments.cljs:864 msgid "errors.character-limit-exceeded" msgstr "Character limit exceeded" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index ef0ef18989..3c2655035b 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -1501,6 +1501,9 @@ msgstr "No se han podido cargar las fuentes %s" msgid "errors.cannot-upload" msgstr "No se puede cargar el archivo multimedia." +msgid "errors.circular-library-reference" +msgstr "No se puede añadir la biblioteca: crearía una dependencia circular" + #: src/app/main/ui/comments.cljs:737, src/app/main/ui/comments.cljs:767, src/app/main/ui/comments.cljs:864 msgid "errors.character-limit-exceeded" msgstr "Se ha superado el límite de caracteres"