🐛 Enforce share-link comment permissions and page scope (#11371)

Fix two security vulnerabilities in comment RPCs when accessed
via share-links:

- GHSA-4p97-v4wg-jxfx: Share-link holders with who-comment=team
  could bypass the restriction and comment. The check-comment-permissions!
  function treated can-read as sufficient, but share-links always set
  can-read=true.

- GHSA-fwm4-hm9f-rmcp: Comment query RPCs returned threads from all
  pages, ignoring the share-link's :pages restriction.

Changes:
- files.clj: Differentiate :membership vs :share-link in
  check-comment-permissions!. For share-links, require
  has-comment-permissions? only (who-comment=all).
- comments.clj: Filter threads by (:pages perms) for share-link
  access in get-comment-threads, get-comment-thread, and get-comments.

Closes #11370

AI-assisted-by: qwen3.7-plus

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

@@ -231,8 +231,11 @@
    ::sm/params schema:get-comment-threads}
   [cfg {:keys [::rpc/profile-id file-id share-id] :as params}]
   (db/run! cfg (fn [{:keys [::db/conn] :as cfg}]
-                 (files/check-comment-permissions! cfg profile-id file-id share-id)
+                 (let [perms (files/check-comment-permissions! cfg profile-id file-id share-id)
-                 (get-comment-threads conn profile-id file-id))))
+                       threads (get-comment-threads conn profile-id file-id)]
+                   (if (= :share-link (:type perms))
+                     (filterv #(contains? (:pages perms) (:page-id %)) threads)
+                     threads)))))
 
 (defn- get-comment-threads-sql
   [where]
@@ -329,9 +332,15 @@
    ::sm/params schema:get-comment-thread}
   [cfg {:keys [::rpc/profile-id file-id id share-id] :as params}]
   (db/run! cfg (fn [{:keys [::db/conn] :as cfg}]
-                 (files/check-comment-permissions! cfg profile-id file-id share-id)
+                 (let [perms  (files/check-comment-permissions! cfg profile-id file-id share-id)
-                 (some-> (db/exec-one! conn [sql:get-comment-thread profile-id file-id id])
+                       thread (some-> (db/exec-one! conn [sql:get-comment-thread profile-id file-id id])
-                         (decode-row)))))
+                                      (decode-row))]
+                   (when (and thread (= :share-link (:type perms)))
+                     (when-not (contains? (:pages perms) (:page-id thread))
+                       (ex/raise :type :not-found
+                                 :code :object-not-found
+                                 :hint "not found")))
+                   thread))))
 
 ;; --- COMMAND: Retrieve Comments
 
@@ -348,8 +357,13 @@
    ::sm/params schema:get-comments}
   [cfg {:keys [::rpc/profile-id thread-id share-id]}]
   (db/run! cfg (fn [{:keys [::db/conn] :as cfg}]
-                 (let [{:keys [file-id]} (get-comment-thread conn thread-id)]
+                 (let [{:keys [file-id page-id]} (get-comment-thread conn thread-id)
-                   (files/check-comment-permissions! cfg profile-id file-id share-id)
+                       perms (files/check-comment-permissions! cfg profile-id file-id share-id)]
+                   (when (and (= :share-link (:type perms))
+                              (not (contains? (:pages perms) page-id)))
+                     (ex/raise :type :not-found
+                               :code :object-not-found
+                               :hint "not found"))
                    (get-comments conn thread-id)))))
 
 (def sql:get-comments

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

@@ -95,18 +95,23 @@
 (def check-read-permissions!
   (perms/make-check-fn has-read-permissions?))
 
-;; A user has comment permissions if she has read permissions, or
+;; A user has comment permissions if:
-;; explicit comment permissions through the share-id
+;; - For :membership type: they have read permissions OR explicit comment permissions
+;; - For :share-link type: they must have explicit comment permissions (who-comment=all)
+;;   This prevents share-link holders with who-comment=team from bypassing the restriction
 
 (defn check-comment-permissions!
   [cfg profile-id file-id share-id]
-  (let [perms       (perms/get-file-read-permissions cfg profile-id file-id share-id)
+  (let [perms    (perms/get-file-read-permissions cfg profile-id file-id share-id)
-        can-read    (has-read-permissions? perms)
+        allowed? (if (= :share-link (:type perms))
-        can-comment (has-comment-permissions? perms)]
+                   (has-comment-permissions? perms)
-    (when-not (or can-read can-comment)
+                   (or (has-read-permissions? perms)
+                       (has-comment-permissions? perms)))]
+    (when-not allowed?
       (ex/raise :type :not-found
                 :code :object-not-found
-                :hint "not found"))))
+                :hint "not found"))
+    perms))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; QUERY COMMANDS

backend/test/backend_tests/rpc_comment_test.clj

@@ -285,3 +285,196 @@
 
           (let [threads (th/db-query :comment-thread {:file-id (:id file-1)})]
             (t/is (= 0 (count threads)))))))))
+
+(t/deftest share-link-who-comment-team-cannot-comment
+  (let [owner    (th/create-profile* 1 {:is-active true})
+        outsider (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])
+
+        share    (th/command! {::th/type :create-share-link
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :pages #{page-id}
+                               :who-comment "team"
+                               :who-inspect "all"})
+        share-id (get-in share [:result :id])]
+
+    (t/testing "outsider with who-comment=team share-link cannot get-comment-threads"
+      (let [out (th/command! {::th/type :get-comment-threads
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :share-id share-id})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))
+
+    (t/testing "outsider with who-comment=team share-link cannot create-comment-thread"
+      (let [out (th/command! {::th/type :create-comment-thread
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :page-id page-id
+                              :position (gpt/point 0)
+                              :content "outsider comment"
+                              :frame-id uuid/zero
+                              :share-id share-id})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))))
+
+(t/deftest share-link-who-comment-all-can-comment
+  (let [owner    (th/create-profile* 1 {:is-active true})
+        outsider (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])
+
+        share    (th/command! {::th/type :create-share-link
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :pages #{page-id}
+                               :who-comment "all"
+                               :who-inspect "all"})
+        share-id (get-in share [:result :id])]
+
+    (t/testing "outsider with who-comment=all share-link can get-comment-threads"
+      (let [out (th/command! {::th/type :get-comment-threads
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :share-id share-id})]
+        (t/is (th/success? out))))
+
+    (t/testing "outsider with who-comment=all share-link can create-comment-thread"
+      (let [out (th/command! {::th/type :create-comment-thread
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :page-id page-id
+                              :position (gpt/point 0)
+                              :content "outsider comment"
+                              :frame-id uuid/zero
+                              :share-id share-id})]
+        (t/is (th/success? out))))))
+
+(t/deftest share-link-page-scope-enforced
+  (let [owner    (th/create-profile* 1 {:is-active true})
+        outsider (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-a   (get-in file [:data :pages 0])
+        page-b   (uuid/random)
+
+        _        (th/command! {::th/type :update-file
+                               ::rpc/profile-id (:id owner)
+                               :id (:id file)
+                               :session-id (uuid/random)
+                               :revn 0
+                               :vern 0
+                               :changes [{:type :add-page
+                                          :id page-b
+                                          :page {:id page-b
+                                                 :name "Page B"
+                                                 :options {}
+                                                 :objects {}}}]})
+
+        thread-a (th/command! {::th/type :create-comment-thread
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :page-id page-a
+                               :position (gpt/point 0)
+                               :content "comment on page A"
+                               :frame-id uuid/zero})
+        thread-b (th/command! {::th/type :create-comment-thread
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :page-id page-b
+                               :position (gpt/point 0)
+                               :content "comment on page B"
+                               :frame-id uuid/zero})
+
+        thread-a-id (get-in thread-a [:result :id])
+        thread-b-id (get-in thread-b [:result :id])
+
+        share    (th/command! {::th/type :create-share-link
+                               ::rpc/profile-id (:id owner)
+                               :file-id (:id file)
+                               :pages #{page-a}
+                               :who-comment "all"
+                               :who-inspect "all"})
+        share-id (get-in share [:result :id])]
+
+    (t/testing "share-link holder can get-comment-threads for shared page only"
+      (let [out (th/command! {::th/type :get-comment-threads
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :share-id share-id})
+            result (:result out)]
+        (t/is (th/success? out))
+        (t/is (= 1 (count result)))
+        (t/is (= page-a (:page-id (first result))))))
+
+    (t/testing "share-link holder cannot get-comment-thread for unshared page"
+      (let [out (th/command! {::th/type :get-comment-thread
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :id thread-b-id
+                              :share-id share-id})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))
+
+    (t/testing "share-link holder can get-comment-thread for shared page"
+      (let [out (th/command! {::th/type :get-comment-thread
+                              ::rpc/profile-id (:id outsider)
+                              :file-id (:id file)
+                              :id thread-a-id
+                              :share-id share-id})]
+        (t/is (th/success? out))))
+
+    (t/testing "share-link holder cannot get-comments for thread on unshared page"
+      (let [out (th/command! {::th/type :get-comments
+                              ::rpc/profile-id (:id outsider)
+                              :thread-id thread-b-id
+                              :share-id share-id})]
+        (t/is (not (th/success? out)))
+        (t/is (= :not-found (th/ex-type (:error out))))))))
+
+(t/deftest membership-can-still-comment
+  (let [owner   (th/create-profile* 1 {:is-active true})
+        member  (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 member)
+                                       :role :editor})
+        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])]
+
+    (t/testing "team member can get-comment-threads without share-id"
+      (let [out (th/command! {::th/type :get-comment-threads
+                              ::rpc/profile-id (:id member)
+                              :file-id (:id file)})]
+        (t/is (th/success? out))))
+
+    (t/testing "team member can create-comment-thread without share-id"
+      (let [out (th/command! {::th/type :create-comment-thread
+                              ::rpc/profile-id (:id member)
+                              :file-id (:id file)
+                              :page-id page-id
+                              :position (gpt/point 0)
+                              :content "member comment"
+                              :frame-id uuid/zero})]
+        (t/is (th/success? out))))))