🐛 Fix several issues in RPC command handlers (#10670)

* 🐛 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

* 📎 Check perms before circular link checks

* 🐛 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

@@ -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]))
 

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)}))))

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))))

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))))))

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})

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"

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"