mirror of
https://github.com/penpot/penpot.git
synced 2026-08-28 07:38:42 +00:00
🐛 Enforce ownership check on tempfile bucket access (#11270)
* 🐛 Enforce ownership check on tempfile bucket access The upload-tempfile RPC stores profile-id with tempfile objects, but objects-handler never verified the requester was the owner. Any authenticated user who knew the UUID could access the tempfile. Add ownership check: tempfile bucket now requires the request's profile-id to match the stored profile-id. Returns 404 on mismatch (not 403) to avoid leaking object existence. Legacy tempfiles without stored profile-id remain accessible to any authenticated user for backward compatibility. Closes #11269 AI-assisted-by: qwen3.7-plus * ♻️ Extract tempfile-bucket constant and fix docstring indentation Extract the 'tempfile' bucket string literal into a named constant (sto/tempfile-bucket) to prevent typos and make future bucket renames trivial. Updated 9 occurrences across 7 files. Also fixed minor docstring indentation inconsistency in authenticated? function. AI-assisted-by: qwen3.7-plus * ♻️ Refactor process-bucket! and authenticated? helpers Replace case with cond in process-bucket! to properly resolve sto/tempfile-bucket var from another namespace (case does not evaluate qualified vars at compile time). Redefine authenticated? in terms of request-profile-id to remove duplicated lookup logic. Closes #11269 AI-assisted-by: mimo-v2.5-pro
This commit is contained in:
parent
b33213787e
commit
7079d33ae1
@ -95,17 +95,32 @@
|
|||||||
(let [bucket (-> obj meta :bucket)]
|
(let [bucket (-> obj meta :bucket)]
|
||||||
(not (contains? public-buckets bucket))))
|
(not (contains? public-buckets bucket))))
|
||||||
|
|
||||||
|
(defn- request-profile-id
|
||||||
|
"Extract the authenticated profile-id from the request."
|
||||||
|
[request]
|
||||||
|
(or (::session/profile-id request)
|
||||||
|
(::actoken/profile-id request)))
|
||||||
|
|
||||||
(defn- authenticated?
|
(defn- authenticated?
|
||||||
"Check if the request has an authenticated profile, either via session
|
"Check if the request has an authenticated profile, either via session
|
||||||
or access token."
|
or access token."
|
||||||
[request]
|
[request]
|
||||||
(or (some? (::session/profile-id request))
|
(some? (request-profile-id request)))
|
||||||
(some? (::actoken/profile-id request))))
|
|
||||||
|
(defn- tempfile-owner-match?
|
||||||
|
"Check if the request's profile-id matches the tempfile's stored owner.
|
||||||
|
Returns true if no profile-id was stored (legacy objects)."
|
||||||
|
[obj request]
|
||||||
|
(let [stored-profile-id (:profile-id (meta obj))
|
||||||
|
request-profile-id (request-profile-id request)]
|
||||||
|
(or (nil? stored-profile-id)
|
||||||
|
(= stored-profile-id request-profile-id))))
|
||||||
|
|
||||||
(defn objects-handler
|
(defn objects-handler
|
||||||
"Handler that serves storage objects by id.
|
"Handler that serves storage objects by id.
|
||||||
For non-public buckets (e.g. profile), requires authentication
|
For non-public buckets (e.g. profile), requires authentication
|
||||||
via session cookie or access token."
|
via session cookie or access token.
|
||||||
|
For tempfile bucket, also requires ownership (profile-id match)."
|
||||||
[{:keys [::sto/storage] :as cfg} request]
|
[{:keys [::sto/storage] :as cfg} request]
|
||||||
(let [id (get-id request)
|
(let [id (get-id request)
|
||||||
obj (sto/get-object storage id)]
|
obj (sto/get-object storage id)]
|
||||||
@ -117,6 +132,10 @@
|
|||||||
(not (authenticated? request)))
|
(not (authenticated? request)))
|
||||||
{::yres/status 401}
|
{::yres/status 401}
|
||||||
|
|
||||||
|
(and (= (-> obj meta :bucket) sto/tempfile-bucket)
|
||||||
|
(not (tempfile-owner-match? obj request)))
|
||||||
|
{::yres/status 404}
|
||||||
|
|
||||||
:else
|
:else
|
||||||
(serve-object cfg obj))))
|
(serve-object cfg obj))))
|
||||||
|
|
||||||
|
|||||||
@ -60,7 +60,7 @@
|
|||||||
{::sto/content data
|
{::sto/content data
|
||||||
::sto/touched-at (ct/in-future {:minutes 60})
|
::sto/touched-at (ct/in-future {:minutes 60})
|
||||||
:content-type "application/zip"
|
:content-type "application/zip"
|
||||||
:bucket "tempfile"})]
|
:bucket sto/tempfile-bucket})]
|
||||||
|
|
||||||
(-> (cf/get :public-uri)
|
(-> (cf/get :public-uri)
|
||||||
(u/join "/assets/by-id/")
|
(u/join "/assets/by-id/")
|
||||||
|
|||||||
@ -353,7 +353,7 @@
|
|||||||
::sto/touched-at (ct/in-future {:minutes 30})
|
::sto/touched-at (ct/in-future {:minutes 30})
|
||||||
:profile-id profile-id
|
:profile-id profile-id
|
||||||
:content-type mtype
|
:content-type mtype
|
||||||
:bucket "tempfile"}]
|
:bucket sto/tempfile-bucket}]
|
||||||
|
|
||||||
(sto/put-object! storage content)))
|
(sto/put-object! storage content)))
|
||||||
|
|
||||||
|
|||||||
@ -379,7 +379,7 @@
|
|||||||
::sto/deduplicate? false
|
::sto/deduplicate? false
|
||||||
::sto/touch true
|
::sto/touch true
|
||||||
:content-type (:mtype content)
|
:content-type (:mtype content)
|
||||||
:bucket "tempfile"
|
:bucket sto/tempfile-bucket
|
||||||
:upload-id (str session-id)
|
:upload-id (str session-id)
|
||||||
:chunk-index index}))
|
:chunk-index index}))
|
||||||
|
|
||||||
|
|||||||
@ -43,7 +43,7 @@
|
|||||||
::sto/touched-at (ct/in-future {:minutes 10})
|
::sto/touched-at (ct/in-future {:minutes 10})
|
||||||
:profile-id profile-id
|
:profile-id profile-id
|
||||||
:content-type (:mtype content)
|
:content-type (:mtype content)
|
||||||
:bucket "tempfile"}
|
:bucket sto/tempfile-bucket}
|
||||||
object (sto/put-object! storage content)]
|
object (sto/put-object! storage content)]
|
||||||
{:id (:id object)
|
{:id (:id object)
|
||||||
:uri (-> (cf/get :public-uri)
|
:uri (-> (cf/get :public-uri)
|
||||||
|
|||||||
@ -38,6 +38,10 @@
|
|||||||
(def default-bucket
|
(def default-bucket
|
||||||
"file-media-object")
|
"file-media-object")
|
||||||
|
|
||||||
|
(def tempfile-bucket
|
||||||
|
"Bucket name for temporary file uploads (10-minute expiry)."
|
||||||
|
"tempfile")
|
||||||
|
|
||||||
(def valid-buckets
|
(def valid-buckets
|
||||||
#{"file-media-object"
|
#{"file-media-object"
|
||||||
"team-font-variant"
|
"team-font-variant"
|
||||||
@ -45,7 +49,7 @@
|
|||||||
"file-thumbnail"
|
"file-thumbnail"
|
||||||
"profile"
|
"profile"
|
||||||
"organization"
|
"organization"
|
||||||
"tempfile"
|
tempfile-bucket
|
||||||
"file-data"
|
"file-data"
|
||||||
"file-data-fragment"
|
"file-data-fragment"
|
||||||
"file-change"})
|
"file-change"})
|
||||||
@ -136,7 +140,7 @@
|
|||||||
result (when (and (::deduplicate? params)
|
result (when (and (::deduplicate? params)
|
||||||
(:hash mdata)
|
(:hash mdata)
|
||||||
(:bucket mdata)
|
(:bucket mdata)
|
||||||
(not= "tempfile" (:bucket mdata)))
|
(not= tempfile-bucket (:bucket mdata)))
|
||||||
(let [result (get-database-object-by-hash connectable backend
|
(let [result (get-database-object-by-hash connectable backend
|
||||||
(:bucket mdata)
|
(:bucket mdata)
|
||||||
(:hash mdata))]
|
(:hash mdata))]
|
||||||
|
|||||||
@ -149,7 +149,7 @@
|
|||||||
:status "delete"
|
:status "delete"
|
||||||
:bucket bucket)
|
:bucket bucket)
|
||||||
(recur to-freeze (conj to-delete id) (rest objects))))
|
(recur to-freeze (conj to-delete id) (rest objects))))
|
||||||
(let [deletion-delay (if (= "tempfile" bucket)
|
(let [deletion-delay (if (= sto/tempfile-bucket bucket)
|
||||||
(ct/duration {:hours 2})
|
(ct/duration {:hours 2})
|
||||||
(cf/get-deletion-delay))]
|
(cf/get-deletion-delay))]
|
||||||
(some->> (seq to-freeze) (mark-freeze-in-bulk! conn))
|
(some->> (seq to-freeze) (mark-freeze-in-bulk! conn))
|
||||||
@ -158,15 +158,16 @@
|
|||||||
|
|
||||||
(defn- process-bucket!
|
(defn- process-bucket!
|
||||||
[conn bucket objects]
|
[conn bucket objects]
|
||||||
(case bucket
|
(cond
|
||||||
"file-media-object" (process-objects! conn has-file-media-object-refs? bucket objects)
|
(= bucket "file-media-object") (process-objects! conn has-file-media-object-refs? bucket objects)
|
||||||
"team-font-variant" (process-objects! conn has-team-font-variant-refs? bucket objects)
|
(= bucket "team-font-variant") (process-objects! conn has-team-font-variant-refs? bucket objects)
|
||||||
"file-object-thumbnail" (process-objects! conn has-file-object-thumbnails-refs? bucket objects)
|
(= bucket "file-object-thumbnail") (process-objects! conn has-file-object-thumbnails-refs? bucket objects)
|
||||||
"file-thumbnail" (process-objects! conn has-file-thumbnails-refs? bucket objects)
|
(= bucket "file-thumbnail") (process-objects! conn has-file-thumbnails-refs? bucket objects)
|
||||||
"profile" (process-objects! conn has-profile-refs? bucket objects)
|
(= bucket "profile") (process-objects! conn has-profile-refs? bucket objects)
|
||||||
"file-data" (process-objects! conn has-file-data-refs? bucket objects)
|
(= bucket "file-data") (process-objects! conn has-file-data-refs? bucket objects)
|
||||||
"tempfile" (process-objects! conn (constantly false) bucket objects)
|
(= bucket sto/tempfile-bucket) (process-objects! conn (constantly false) sto/tempfile-bucket objects)
|
||||||
"organization" (process-objects! conn (constantly false) bucket objects)
|
(= bucket "organization") (process-objects! conn (constantly false) bucket objects)
|
||||||
|
:else
|
||||||
(ex/raise :type :internal
|
(ex/raise :type :internal
|
||||||
:code :unexpected-unknown-reference
|
:code :unexpected-unknown-reference
|
||||||
:hint (dm/fmt "unknown reference '%'" bucket))))
|
:hint (dm/fmt "unknown reference '%'" bucket))))
|
||||||
|
|||||||
@ -37,11 +37,16 @@
|
|||||||
(assoc storage ::sto/backend :fs))
|
(assoc storage ::sto/backend :fs))
|
||||||
|
|
||||||
(defn- create-storage-object!
|
(defn- create-storage-object!
|
||||||
"Create a storage object with the given bucket and content."
|
"Create a storage object with the given bucket and content.
|
||||||
[storage bucket content]
|
Optional opts map can include :profile-id to set the owner."
|
||||||
(sto/put-object! storage {::sto/content (sto/content content)
|
([storage bucket content]
|
||||||
:bucket bucket
|
(create-storage-object! storage bucket content {}))
|
||||||
:content-type "text/plain"}))
|
([storage bucket content {:keys [profile-id]}]
|
||||||
|
(sto/put-object! storage (cond-> {::sto/content (sto/content content)
|
||||||
|
:bucket bucket
|
||||||
|
:content-type "text/plain"}
|
||||||
|
(some? profile-id)
|
||||||
|
(assoc :profile-id profile-id)))))
|
||||||
|
|
||||||
(defn- make-handler-cfg
|
(defn- make-handler-cfg
|
||||||
"Build a minimal cfg map for the assets handlers."
|
"Build a minimal cfg map for the assets handlers."
|
||||||
@ -708,3 +713,70 @@
|
|||||||
::session/profile-id (:id profile)}
|
::session/profile-id (:id profile)}
|
||||||
response (assets/objects-handler cfg request)]
|
response (assets/objects-handler cfg request)]
|
||||||
(t/is (= 404 (::yres/status response)))))
|
(t/is (= 404 (::yres/status response)))))
|
||||||
|
|
||||||
|
;; ----------------------------------------------------------------
|
||||||
|
;; Tests: objects-handler — tempfile bucket ownership (T9-F-10)
|
||||||
|
;; ----------------------------------------------------------------
|
||||||
|
|
||||||
|
(t/deftest objects-handler-tempfile-owner-can-access
|
||||||
|
;; Owner of a tempfile should be able to access it via session auth.
|
||||||
|
(let [storage (-> (:app.storage/storage th/*system*)
|
||||||
|
(configure-storage-backend))
|
||||||
|
cfg (make-handler-cfg storage)
|
||||||
|
owner (th/create-profile* 1)
|
||||||
|
object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)})
|
||||||
|
request {:path-params {:id (str (:id object))}
|
||||||
|
::session/profile-id (:id owner)}
|
||||||
|
response (assets/objects-handler cfg request)]
|
||||||
|
(t/is (= 204 (::yres/status response)))))
|
||||||
|
|
||||||
|
(t/deftest objects-handler-tempfile-non-owner-gets-404
|
||||||
|
;; Non-owner accessing a tempfile should get 404 (not 403, to avoid leaking existence).
|
||||||
|
(let [storage (-> (:app.storage/storage th/*system*)
|
||||||
|
(configure-storage-backend))
|
||||||
|
cfg (make-handler-cfg storage)
|
||||||
|
owner (th/create-profile* 1)
|
||||||
|
stranger (th/create-profile* 2)
|
||||||
|
object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)})
|
||||||
|
request {:path-params {:id (str (:id object))}
|
||||||
|
::session/profile-id (:id stranger)}
|
||||||
|
response (assets/objects-handler cfg request)]
|
||||||
|
(t/is (= 404 (::yres/status response)))))
|
||||||
|
|
||||||
|
(t/deftest objects-handler-tempfile-access-token-owner-can-access
|
||||||
|
;; Owner of a tempfile should be able to access it via access token auth.
|
||||||
|
(let [storage (-> (:app.storage/storage th/*system*)
|
||||||
|
(configure-storage-backend))
|
||||||
|
cfg (make-handler-cfg storage)
|
||||||
|
owner (th/create-profile* 1)
|
||||||
|
object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)})
|
||||||
|
request {:path-params {:id (str (:id object))}
|
||||||
|
::actoken/profile-id (:id owner)}
|
||||||
|
response (assets/objects-handler cfg request)]
|
||||||
|
(t/is (= 204 (::yres/status response)))))
|
||||||
|
|
||||||
|
(t/deftest objects-handler-tempfile-access-token-non-owner-gets-404
|
||||||
|
;; Non-owner accessing a tempfile via access token should get 404.
|
||||||
|
(let [storage (-> (:app.storage/storage th/*system*)
|
||||||
|
(configure-storage-backend))
|
||||||
|
cfg (make-handler-cfg storage)
|
||||||
|
owner (th/create-profile* 1)
|
||||||
|
stranger (th/create-profile* 2)
|
||||||
|
object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)})
|
||||||
|
request {:path-params {:id (str (:id object))}
|
||||||
|
::actoken/profile-id (:id stranger)}
|
||||||
|
response (assets/objects-handler cfg request)]
|
||||||
|
(t/is (= 404 (::yres/status response)))))
|
||||||
|
|
||||||
|
(t/deftest objects-handler-tempfile-no-stored-profile-id-serves
|
||||||
|
;; Legacy tempfile objects without stored profile-id should be accessible
|
||||||
|
;; to any authenticated user (backward compatibility).
|
||||||
|
(let [storage (-> (:app.storage/storage th/*system*)
|
||||||
|
(configure-storage-backend))
|
||||||
|
cfg (make-handler-cfg storage)
|
||||||
|
stranger (th/create-profile* 1)
|
||||||
|
object (create-storage-object! storage "tempfile" "legacy temp data")
|
||||||
|
request {:path-params {:id (str (:id object))}
|
||||||
|
::session/profile-id (:id stranger)}
|
||||||
|
response (assets/objects-handler cfg request)]
|
||||||
|
(t/is (= 204 (::yres/status response)))))
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user