From 209aea83658f209c4189b531eeda0f3a638a0294 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 18:26:35 +0200 Subject: [PATCH] :bug: Add proper ownership check on managing/deleting shared link on a file (#11290) * :bug: Add ownership check to share-link deletion The delete-share-link RPC command only verified file-level edit permission but did not check if the caller owned the share-link. This allowed any file editor to delete share-links created by other users, disrupting collaborative workflows. The fix adds an ownership check that allows deletion only by: - The share-link creator (owner-id matches profile-id) - File admins (is-admin permission) - File owners (is-owner permission) Implemented using TDD: - RED: Test demonstrates IDOR vulnerability (editor can delete) - GREEN: Ownership check prevents unauthorized deletion - All existing tests continue to pass Closes #11289 AI-assisted-by: qwen3.7-plus * :bug: Add test coverage for share-link deletion escape hatches Address code review feedback for PR #11290: - Add test for editor deleting their own share-link - Add test for admin deleting editor's share-link - Add test for owner deleting editor's share-link - Remove redundant :is-owner check (already included in :is-admin) - Add clarifying comment about :is-admin including :is-owner Closes #11289 AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/files_share.clj | 13 ++ backend/test/backend_tests/rpc_file_test.clj | 127 +++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/backend/src/app/rpc/commands/files_share.clj b/backend/src/app/rpc/commands/files_share.clj index 9a8326d06d..0e8c184ede 100644 --- a/backend/src/app/rpc/commands/files_share.clj +++ b/backend/src/app/rpc/commands/files_share.clj @@ -7,6 +7,8 @@ (ns app.rpc.commands.files-share "Share link related rpc mutation methods." (:require + [app.binfile.common :as bfc] + [app.common.exceptions :as ex] [app.common.schema :as sm] [app.common.uuid :as uuid] [app.db :as db] @@ -66,5 +68,16 @@ [{:keys [::db/conn]} {:keys [::rpc/profile-id id] :as params}] (let [slink (db/get-by-id conn :share-link id)] (files/check-edition-permissions! conn profile-id (:file-id slink)) + + ;; Verify caller owns this specific share-link, OR has admin access. + ;; Note: :is-admin already includes :is-owner (see bfc/get-file-permissions), + ;; so we only need to check :is-admin here. + (let [perms (bfc/get-file-permissions conn profile-id (:file-id slink))] + (when-not (or (= (:owner-id slink) profile-id) + (:is-admin perms)) + (ex/raise :type :authorization + :code :not-share-link-owner + :hint "You can only delete share-links you created"))) + (db/delete! conn :share-link {:id id}) nil)) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index d1ec0eb233..27e881dc45 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -2467,3 +2467,130 @@ err (:error out)] (t/is (th/ex-info? err)) (t/is (th/ex-of-type? err :not-found)))) + +(t/deftest share-link-deletion-idor + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + admin (th/create-profile* 3 {:is-active true}) + proj-id (:default-project-id owner) + team-id (:default-team-id owner) + + file (th/create-file* 1 {:profile-id (:id owner) + :project-id proj-id + :is-shared false}) + + ;; Invite editor to the team with edit permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id editor) + :role :editor}) + + ;; Invite admin to the team with admin permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id admin) + :role :admin}) + + ;; Owner creates a share-link + slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{(get-in file [:data :pages 0])} + :who-comment "team" + :who-inspect "all"}) + slink-id (get-in slink [:result :id])] + + (t/testing "owner can delete their own share-link" + (let [out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id owner) + :id slink-id})] + (t/is (nil? (:error out))))) + + (t/testing "editor CANNOT delete owner's share-link (IDOR)" + ;; Recreate the share-link for this test + (let [slink2 (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink2-id (get-in slink2 [:result :id]) + + ;; Editor tries to delete owner's share-link + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id editor) + :id slink2-id}) + err (:error out) + edata (ex-data err)] + + ;; Should be denied with authorization error + (t/is (th/ex-info? err)) + (t/is (= :authorization (:type edata))) + + ;; Verify the share-link still exists + (let [check (th/command! {::th/type :get-view-only-bundle + ::rpc/profile-id (:id owner) + :file-id (:id file)}) + share-links (:share-links (:result check))] + (t/is (some #(= slink2-id (:id %)) share-links))))))) + +(t/deftest share-link-deletion-escape-hatches + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + admin (th/create-profile* 3 {:is-active true}) + proj-id (:default-project-id owner) + team-id (:default-team-id owner) + + file (th/create-file* 1 {:profile-id (:id owner) + :project-id proj-id + :is-shared false}) + + ;; Invite editor to the team with edit permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id editor) + :role :editor}) + + ;; Invite admin to the team with admin permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id admin) + :role :admin})] + + (t/testing "editor CAN delete their own share-link" + (let [slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id editor) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink-id (get-in slink [:result :id]) + + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id editor) + :id slink-id})] + (t/is (nil? (:error out))))) + + (t/testing "admin CAN delete editor's share-link" + (let [slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id editor) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink-id (get-in slink [:result :id]) + + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id admin) + :id slink-id})] + (t/is (nil? (:error out))))) + + (t/testing "owner CAN delete editor's share-link" + (let [slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id editor) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink-id (get-in slink [:result :id]) + + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id owner) + :id slink-id})] + (t/is (nil? (:error out)))))))