github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-08-22 12:49:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Add proper ownership check on managing/deleting shared link on a file (#11290)

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

* 🐛 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
This commit is contained in:
Andrey Antukh 2026-08-19 18:26:35 +02:00 committed by GitHub
parent c200a4d777
commit 209aea8365
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 140 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
backend/src/app/rpc/commands/files_share.clj
View File

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

127
backend/test/backend_tests/rpc_file_test.clj
View File

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