🐛 Require thread access when marking threads as read (#12894)

* 🐛 Require thread access when marking threads as read

mark-all-threads-as-read took any list of thread ids and wrote the read
status of the caller on each of them, without checking that the caller
can access the file that owns the thread. It was the only comment
mutation that did not check permissions, and it took the ids as an
unbounded array, so a single request could keep a database connection
and a transaction open for a long time.

The rpc now resolves the threads in one query and checks the comment
permissions of every distinct file, answering not-found when a thread
does not exist or is not accessible. Both cases answer the same error
on purpose, so the response cannot be used to tell existing threads
apart.

The status rows are now written with a single insert, keeping the
modified-at date one second in the future that makes the thread count
as read. Repeated ids are deduped, the command returns nil again like
the sibling commands with no result, and the schema limits the number
of threads per call to 500 while the dashboard sends the ids in batches
of that size.

Tests cover access rejection, unknown threads, mixed batches that write
nothing, repeated ids, empty lists, marking twice, and the frontend
batch split.

Relates to GHSA-j4vc-cmf6-jhr7

AI-assisted-by: minimax-m3.1-flash

* 🐛 Refresh the unread list when a batch fails

The dashboard sends the thread ids in batches of 500 and each batch
commits on the server. A failure in a later batch left the unread list
stale: the re-read only ran when every batch succeeded, so the threads
already marked as read stayed visible as unread.

The re-read now runs in both paths. On failure the event is emitted
before the error is propagated, so the error toast still shows and the
success toast stays limited to a full success.

The backend also answers an empty list without opening a transaction,
since there is nothing to check and nothing to write.

Relates to GHSA-j4vc-cmf6-jhr7

AI-assisted-by: deepseek-v4.1-flash

backend/src/app/rpc/commands/comments.clj

@@ -845,17 +845,69 @@
                 {::db/return-keys false})
     nil))
 
+;; --- COMMAND: Mark all comment threads as read
+
+;; The dashboard sends the thread ids in batches of this same size
+;; (frontend/src/app/main/data/comments.cljs), so the batch has to fit in
+;; this limit.
+(def ^:private max-mark-all-threads 500)
+
 (def ^:private
   schema:mark-all-threads-as-read
   [:map {:title "mark-all-threads-as-read"}
-   [:threads [:vector ::sm/uuid]]])
+   [:threads [:vector {:max max-mark-all-threads} ::sm/uuid]]])
+
+(def ^:private sql:comment-threads-files
+  "SELECT ct.id, ct.file_id
+     FROM comment_thread ct
+    WHERE ct.id = ANY(?::uuid[])")
+
+(def ^:private sql:upsert-comment-thread-status-batch
+  "insert into comment_thread_status (thread_id, profile_id, modified_at)
+   select t.id, ?::uuid, ?::timestamptz
+     from unnest(?::uuid[]) as t(id)
+       on conflict (thread_id, profile_id)
+       do update set modified_at = excluded.modified_at;")
+
+(defn- get-comment-threads-files
+  [conn thread-ids]
+  (db/exec! conn [sql:comment-threads-files
+                  (db/create-array conn "uuid" thread-ids)]))
+
+(defn- check-threads-access
+  "Raises a :not-found error when any of the threads does not exist or the
+   caller has no access to the file that owns it."
+  [cfg profile-id thread-ids]
+  (let [rows  (get-comment-threads-files (::db/conn cfg) thread-ids)
+        found (into #{} (map :id) rows)]
+    (when-not (= (count thread-ids) (count found))
+      (ex/raise :type :not-found
+                :code :object-not-found
+                :hint "not found"))
+    (doseq [file-id (distinct (map :file-id rows))]
+      (files/check-comment-permissions! cfg profile-id file-id nil))))
+
+(defn- upsert-comment-thread-status-batch
+  [conn profile-id thread-ids]
+  (db/exec! conn [sql:upsert-comment-thread-status-batch
+                  profile-id
+                  (ct/in-future "1s")
+                  (db/create-array conn "uuid" thread-ids)]))
 
 (sv/defmethod ::mark-all-threads-as-read
+  "Marks the given comment threads as read for the current profile. The
+  caller needs to have access to the file that owns every thread."
   {::doc/added "1.15"
+   ::doc/changes [["2.19" "The caller needs access to the file that owns every thread and the number of threads is limited to 500"]]
    ::sm/params schema:mark-all-threads-as-read}
-  [cfg {:keys [::rpc/profile-id threads] :as params}]
-  (db/tx-run!
-   cfg
-   (fn [{:keys [::db/conn]}]
-     (doseq [thread-id threads]
-       (upsert-comment-thread-status! conn profile-id thread-id)))))
+  [cfg {:keys [::rpc/profile-id threads]}]
+  (let [thread-ids (distinct threads)]
+    ;; Nothing to check and nothing to write: an empty list must not open a
+    ;; transaction nor run the queries with an empty array.
+    (when (seq thread-ids)
+      (db/tx-run!
+       cfg
+       (fn [{:keys [::db/conn] :as cfg}]
+         (check-threads-access cfg profile-id thread-ids)
+         (upsert-comment-thread-status-batch conn profile-id thread-ids)
+         nil)))))

backend/test/backend_tests/rpc_comment_test.clj

@@ -848,3 +848,213 @@
                (get-in (meta (:result thread)) [:app.loggers.audit/props :team-id])))
       (t/is (= (:id team)
                (get-in (meta (:result comment)) [:app.loggers.audit/props :team-id]))))))
+
+(t/deftest mark-all-threads-as-read-requires-access-to-the-threads
+  (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 {:team-id (:id team)
+                                        :profile-id (:id owner)})
+        file     (th/create-file* 1 {:profile-id (:id owner)
+                                     :project-id (:id project)})
+        page-id  (get-in file [:data :pages 0])
+
+        thread   (th/command! {::th/type :create-comment-thread
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :page-id page-id
+                               :position (gpt/point 0)
+                               :content "private thread"
+                               :frame-id uuid/zero})
+        thread-id (get-in thread [:result :id])]
+
+    (t/testing "the sibling rpc already rejects a profile without access"
+      (let [out (th/command! {::th/type :update-comment-thread-status
+                              ::rpc/profile-id (:id stranger)
+                              :id thread-id})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))
+
+    (t/testing "mark-all-threads-as-read rejects a profile without access"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id stranger)
+                              :threads [thread-id]})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out)))))
+
+      (t/is (nil? (th/db-get :comment-thread-status
+                             {:thread-id thread-id
+                              :profile-id (:id stranger)}))))))
+
+(t/deftest mark-all-threads-as-read-behaviour
+  (let [owner    (th/create-profile* 1 {:is-active true})
+        member   (th/create-profile* 2 {:is-active true})
+        viewer   (th/create-profile* 3 {:is-active true})
+        outsider (th/create-profile* 4 {:is-active true})
+
+        team     (th/create-team* 1 {:profile-id (:id owner)})
+        _        (th/create-team-role* {:team-id (:id team)
+                                        :profile-id (:id member)
+                                        :role :editor})
+        _        (th/create-team-role* {:team-id (:id team)
+                                        :profile-id (:id viewer)
+                                        :role :viewer})
+        project  (th/create-project* 1 {:team-id (:id team)
+                                        :profile-id (:id owner)})
+        file     (th/create-file* 1 {:profile-id (:id owner)
+                                     :project-id (:id project)})
+        page-id  (get-in file [:data :pages 0])
+
+        thread   (th/command! {::th/type :create-comment-thread
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :page-id page-id
+                               :position (gpt/point 0)
+                               :content "hello"
+                               :frame-id uuid/zero})
+        thread-id (get-in thread [:result :id])]
+
+    (t/testing "the thread is unread for a team member"
+      (let [out (th/command! {::th/type :get-unread-comment-threads
+                              ::rpc/profile-id (:id member)
+                              :team-id (:id team)})]
+        (t/is (th/success? out))
+        (t/is (= 1 (count (:result out))))))
+
+    (t/testing "a team member can mark the threads as read"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id member)
+                              :threads [thread-id]})]
+        (t/is (th/success? out)))
+
+      (let [out (th/command! {::th/type :get-unread-comment-threads
+                              ::rpc/profile-id (:id member)
+                              :team-id (:id team)})]
+        (t/is (th/success? out))
+        (t/is (= 0 (count (:result out))))))
+
+    (t/testing "a viewer of the team can mark the threads as read"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id viewer)
+                              :threads [thread-id]})]
+        (t/is (th/success? out))))
+
+    (t/testing "an unknown thread is rejected"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id member)
+                              :threads [(uuid/random)]})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))
+
+    (t/testing "more threads than the allowed maximum are rejected"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id member)
+                              :threads (vec (repeatedly 501 uuid/random))})]
+        (t/is (not (th/success? out)))
+        (t/is (= :validation (th/ex-type (:error out))))))
+
+    (t/testing "a list with an inaccessible thread writes nothing"
+      (let [other-team    (th/create-team* 2 {:profile-id (:id owner)})
+            other-project (th/create-project* 2 {:team-id (:id other-team)
+                                                 :profile-id (:id owner)})
+            other-file    (th/create-file* 2 {:profile-id (:id owner)
+                                              :project-id (:id other-project)})
+            other-thread  (th/command! {::th/type :create-comment-thread
+                                        ::rpc/profile-id (:id owner)
+                                        :file-id (:id other-file)
+                                        :page-id (get-in other-file [:data :pages 0])
+                                        :position (gpt/point 0)
+                                        :content "other team"
+                                        :frame-id uuid/zero})
+            other-thread-id (get-in other-thread [:result :id])
+
+            accessible-thread (th/command! {::th/type :create-comment-thread
+                                            ::rpc/profile-id (:id owner)
+                                            :file-id (:id file)
+                                            :page-id page-id
+                                            :position (gpt/point 0)
+                                            :content "accessible"
+                                            :frame-id uuid/zero})
+            accessible-thread-id (get-in accessible-thread [:result :id])
+
+            out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id member)
+                              :threads [accessible-thread-id other-thread-id]})]
+
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))
+        (t/is (nil? (th/db-get :comment-thread-status
+                               {:thread-id accessible-thread-id
+                                :profile-id (:id member)})))
+        (t/is (nil? (th/db-get :comment-thread-status
+                               {:thread-id other-thread-id
+                                :profile-id (:id member)})))))
+
+    (t/testing "a profile without any relation with the team is rejected"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id outsider)
+                              :threads [thread-id]})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))))
+
+(t/deftest mark-all-threads-as-read-edge-cases
+  (let [owner   (th/create-profile* 1 {:is-active true})
+        member  (th/create-profile* 2 {:is-active true})
+        viewer  (th/create-profile* 3 {:is-active true})
+
+        team    (th/create-team* 1 {:profile-id (:id owner)})
+        _       (th/create-team-role* {:team-id (:id team)
+                                       :profile-id (:id member)
+                                       :role :editor})
+        _       (th/create-team-role* {:team-id (:id team)
+                                       :profile-id (:id viewer)
+                                       :role :viewer})
+        project (th/create-project* 1 {:team-id (:id team)
+                                       :profile-id (:id owner)})
+        file    (th/create-file* 1 {:profile-id (:id owner)
+                                    :project-id (:id project)})
+        page-id (get-in file [:data :pages 0])
+
+        thread  (th/command! {::th/type :create-comment-thread
+                              ::rpc/profile-id (:id owner)
+                              :file-id (:id file)
+                              :page-id page-id
+                              :position (gpt/point 0)
+                              :content "hello"
+                              :frame-id uuid/zero})
+        thread-id (get-in thread [:result :id])]
+
+    (t/testing "repeated thread ids are accepted and written once"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id member)
+                              :threads [thread-id thread-id]})]
+        (t/is (th/success? out))
+        (t/is (= 1 (count (th/db-query :comment-thread-status
+                                       {:thread-id thread-id
+                                        :profile-id (:id member)}))))))
+
+    (t/testing "an empty list of threads is accepted and writes nothing"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id viewer)
+                              :threads []})]
+        (t/is (th/success? out))
+        (t/is (= 0 (count (th/db-query :comment-thread-status
+                                       {:thread-id thread-id
+                                        :profile-id (:id viewer)}))))))
+
+    (t/testing "marking a thread as read twice keeps a single status row"
+      (let [out (th/command! {::th/type :mark-all-threads-as-read
+                              ::rpc/profile-id (:id member)
+                              :threads [thread-id]})]
+        (t/is (th/success? out)))
+
+      (t/is (= 1 (count (th/db-query :comment-thread-status
+                                     {:thread-id thread-id
+                                      :profile-id (:id member)}))))
+
+      (let [out (th/command! {::th/type :get-unread-comment-threads
+                              ::rpc/profile-id (:id member)
+                              :team-id (:id team)})]
+        (t/is (th/success? out))
+        (t/is (= 0 (count (:result out))))))))

frontend/src/app/main/data/comments.cljs

@@ -462,6 +462,18 @@
              (rx/map #(partial fetched-comments %))
              (rx/catch #(rx/throw {:type :comment-error})))))))
 
+;; Has to fit in the limit of the backend rpc
+;; (backend/src/app/rpc/commands/comments.clj).
+(def ^:private mark-all-threads-batch-size 500)
+
+(defn mark-all-threads-batches
+  "Splits the thread ids into the batches that the backend accepts on a
+  single call."
+  [thread-ids]
+  (->> thread-ids
+       (partition-all mark-all-threads-batch-size)
+       (mapv vec)))
+
 (defn mark-all-threads-as-read
   "Mark all threads as read"
   [team-id]
@@ -470,11 +482,23 @@
     (-data [_] {})
     ptk/WatchEvent
     (watch [_ state _]
-      (let [threads (-> state :comment-threads vals)]
+      (let [threads       (-> state :comment-threads vals)
+            batches       (mark-all-threads-batches (mapv :id threads))
+            refresh-event (retrieve-unread-comment-threads team-id)]
         (rx/concat
-         (->> (rp/cmd! :mark-all-threads-as-read {:threads (mapv :id threads)})
-              (rx/map #(retrieve-unread-comment-threads team-id))
-              (rx/catch #(rx/throw {:type :comment-error})))
+         (->> (rx/from batches)
+              (rx/mapcat (fn [batch]
+                           (rp/cmd! :mark-all-threads-as-read {:threads batch})))
+              (rx/reduce (fn [_ _] nil) nil)
+              (rx/map (fn [_] refresh-event))
+              ;; Each batch commits on the server on its own, so a failure in
+              ;; a later batch leaves the previous ones already marked as
+              ;; read. The unread list is re-read anyway and the error is
+              ;; propagated afterwards, so the success toast below stays
+              ;; limited to a full success.
+              (rx/catch (fn [_]
+                          (rx/concat (rx/of refresh-event)
+                                     (rx/throw {:type :comment-error})))))
          (rx/of (ntf/show {:level :info
                            :type :toast
                            :content (tr "dashboard.mark-all-as-read.success")

frontend/test/frontend_tests/data/comments_batches_test.cljs

@@ -0,0 +1,95 @@
+;; 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/.
+
+(ns frontend-tests.data.comments-batches-test
+  (:require
+   [app.common.uuid :as uuid]
+   [app.main.data.comments :as dcmt]
+   [app.main.data.notifications :as ntf]
+   [app.main.repo :as rp]
+   [beicon.v2.core :as rx]
+   [cljs.test :as t :include-macros true]
+   [frontend-tests.helpers.async :as async]
+   [frontend-tests.helpers.mock :as mock]
+   [potok.v2.core :as ptk]))
+
+(defn- comment-threads-state
+  [n]
+  {:comment-threads (into {} (map (fn [_] [(uuid/next) {:id (uuid/next)}]) (range n)))})
+
+(t/deftest mark-all-threads-batches-splits-the-ids
+  (t/is (= [] (dcmt/mark-all-threads-batches [])))
+  (t/is (= [[1 2 3]] (dcmt/mark-all-threads-batches [1 2 3])))
+  (t/is (= [500 1] (mapv count (dcmt/mark-all-threads-batches (range 501))))))
+
+(t/deftest ^:async mark-all-threads-as-read-sends-the-ids-in-batches
+  (let [calls (atom [])
+        state (comment-threads-state 1200)
+        event (dcmt/mark-all-threads-as-read (uuid/next))]
+
+    (await
+     (mock/with-mocks*
+       {rp/cmd! (mock/stub
+                 (fn [cmd params]
+                   (swap! calls conj [cmd params])
+                   (rx/of nil)))}
+       (await (async/observe (ptk/watch event state nil)))))
+
+    (t/is (= 3 (count @calls)))
+    (t/is (every? #(= :mark-all-threads-as-read (first %)) @calls))
+    (t/is (= [500 500 200] (mapv (comp count :threads second) @calls)))))
+
+(t/deftest ^:async mark-all-threads-as-read-sends-a-single-batch
+  (let [calls (atom [])
+        state (comment-threads-state 10)
+        event (dcmt/mark-all-threads-as-read (uuid/next))]
+
+    (await
+     (mock/with-mocks*
+       {rp/cmd! (mock/stub
+                 (fn [cmd params]
+                   (swap! calls conj [cmd params])
+                   (rx/of nil)))}
+       (await (async/observe (ptk/watch event state nil)))))
+
+    (t/is (= 1 (count @calls)))
+    (t/is (= 10 (count (:threads (second (first @calls))))))))
+
+(t/deftest ^:async mark-all-threads-as-read-refreshes-the-unread-list-after-a-batch-fails
+  (let [calls  (atom [])
+        state  (comment-threads-state 1200)
+        event  (dcmt/mark-all-threads-as-read (uuid/next))
+        seen   (atom [])
+        errors (atom [])]
+
+    (await
+     (mock/with-mocks*
+       {rp/cmd! (mock/stub
+                 (fn [cmd params]
+                   (swap! calls conj [cmd params])
+                   (if (= 2 (count @calls))
+                     (rx/throw (ex-info "boom" {}))
+                     (rx/of nil))))}
+       (await (async/observe (ptk/watch event state nil)
+                             :on-next #(swap! seen conj (ptk/type %))
+                             :on-error #(swap! errors conj %)))))
+
+    (t/is (= 2 (count @calls))
+          "the pipeline stops at the batch that fails")
+    (t/is (= [::dcmt/retrieve-unread-comment-threads] @seen)
+          "the unread list is re-read and no success toast is emitted")
+    (t/is (= [{:type :comment-error}] @errors))))
+
+(t/deftest ^:async mark-all-threads-as-read-refreshes-and-shows-the-success-toast
+  (let [state (comment-threads-state 600)
+        event (dcmt/mark-all-threads-as-read (uuid/next))
+        seen  (atom [])]
+
+    (await
+     (mock/with-mocks*
+       {rp/cmd! (mock/stub (fn [_ _] (rx/of nil)))}
+       (await (async/observe (ptk/watch event state nil)
+                             :on-next #(swap! seen conj (ptk/type %))))))
+
+    (t/is (= [::dcmt/retrieve-unread-comment-threads ::ntf/show] @seen))))

frontend/test/frontend_tests/runner.cljs

@@ -10,6 +10,7 @@
    [frontend-tests.composable-tests.comp.sync-test]
    [frontend-tests.copy-as-svg-test]
    [frontend-tests.data.auth-test]
+   [frontend-tests.data.comments-batches-test]
    [frontend-tests.data.comments-filters-test]
    [frontend-tests.data.dashboard-test]
    [frontend-tests.data.event-test]
@@ -165,6 +166,7 @@
    'frontend-tests.composable-tests.comp.sync-test
    'frontend-tests.copy-as-svg-test
    'frontend-tests.data.auth-test
+   'frontend-tests.data.comments-batches-test
    'frontend-tests.data.comments-filters-test
    'frontend-tests.data.dashboard-test
    'frontend-tests.data.event-test