From 7079d33ae197e4d9a7f62dca0ec6fee9365217a8 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 26 Aug 2026 13:05:36 +0200 Subject: [PATCH] :bug: Enforce ownership check on tempfile bucket access (#11270) * :bug: 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 * :recycle: 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 * :recycle: 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 --- backend/src/app/http/assets.clj | 25 +++++- backend/src/app/rpc/commands/binfile.clj | 2 +- backend/src/app/rpc/commands/fonts.clj | 2 +- backend/src/app/rpc/commands/media.clj | 2 +- backend/src/app/rpc/management/exporter.clj | 2 +- backend/src/app/storage.clj | 8 +- backend/src/app/storage/gc_touched.clj | 21 ++--- .../test/backend_tests/http_assets_test.clj | 82 +++++++++++++++++-- 8 files changed, 120 insertions(+), 24 deletions(-) diff --git a/backend/src/app/http/assets.clj b/backend/src/app/http/assets.clj index 22783be1e2..b0adb45b13 100644 --- a/backend/src/app/http/assets.clj +++ b/backend/src/app/http/assets.clj @@ -95,17 +95,32 @@ (let [bucket (-> obj meta :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? "Check if the request has an authenticated profile, either via session or access token." [request] - (or (some? (::session/profile-id request)) - (some? (::actoken/profile-id request)))) + (some? (request-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 "Handler that serves storage objects by id. 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] (let [id (get-id request) obj (sto/get-object storage id)] @@ -117,6 +132,10 @@ (not (authenticated? request))) {::yres/status 401} + (and (= (-> obj meta :bucket) sto/tempfile-bucket) + (not (tempfile-owner-match? obj request))) + {::yres/status 404} + :else (serve-object cfg obj)))) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index ec4510200d..44b7014968 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -60,7 +60,7 @@ {::sto/content data ::sto/touched-at (ct/in-future {:minutes 60}) :content-type "application/zip" - :bucket "tempfile"})] + :bucket sto/tempfile-bucket})] (-> (cf/get :public-uri) (u/join "/assets/by-id/") diff --git a/backend/src/app/rpc/commands/fonts.clj b/backend/src/app/rpc/commands/fonts.clj index 7b5ac6ac4e..4ab02627bc 100644 --- a/backend/src/app/rpc/commands/fonts.clj +++ b/backend/src/app/rpc/commands/fonts.clj @@ -353,7 +353,7 @@ ::sto/touched-at (ct/in-future {:minutes 30}) :profile-id profile-id :content-type mtype - :bucket "tempfile"}] + :bucket sto/tempfile-bucket}] (sto/put-object! storage content))) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index a5e1b9672a..ffa94d5a6b 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -379,7 +379,7 @@ ::sto/deduplicate? false ::sto/touch true :content-type (:mtype content) - :bucket "tempfile" + :bucket sto/tempfile-bucket :upload-id (str session-id) :chunk-index index})) diff --git a/backend/src/app/rpc/management/exporter.clj b/backend/src/app/rpc/management/exporter.clj index f4b7d9547f..f6cdbdd820 100644 --- a/backend/src/app/rpc/management/exporter.clj +++ b/backend/src/app/rpc/management/exporter.clj @@ -43,7 +43,7 @@ ::sto/touched-at (ct/in-future {:minutes 10}) :profile-id profile-id :content-type (:mtype content) - :bucket "tempfile"} + :bucket sto/tempfile-bucket} object (sto/put-object! storage content)] {:id (:id object) :uri (-> (cf/get :public-uri) diff --git a/backend/src/app/storage.clj b/backend/src/app/storage.clj index f30d8762ec..019536e233 100644 --- a/backend/src/app/storage.clj +++ b/backend/src/app/storage.clj @@ -38,6 +38,10 @@ (def default-bucket "file-media-object") +(def tempfile-bucket + "Bucket name for temporary file uploads (10-minute expiry)." + "tempfile") + (def valid-buckets #{"file-media-object" "team-font-variant" @@ -45,7 +49,7 @@ "file-thumbnail" "profile" "organization" - "tempfile" + tempfile-bucket "file-data" "file-data-fragment" "file-change"}) @@ -136,7 +140,7 @@ result (when (and (::deduplicate? params) (:hash mdata) (:bucket mdata) - (not= "tempfile" (:bucket mdata))) + (not= tempfile-bucket (:bucket mdata))) (let [result (get-database-object-by-hash connectable backend (:bucket mdata) (:hash mdata))] diff --git a/backend/src/app/storage/gc_touched.clj b/backend/src/app/storage/gc_touched.clj index b7ace59ef3..49bce333ed 100644 --- a/backend/src/app/storage/gc_touched.clj +++ b/backend/src/app/storage/gc_touched.clj @@ -149,7 +149,7 @@ :status "delete" :bucket bucket) (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}) (cf/get-deletion-delay))] (some->> (seq to-freeze) (mark-freeze-in-bulk! conn)) @@ -158,15 +158,16 @@ (defn- process-bucket! [conn bucket objects] - (case 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) - "file-object-thumbnail" (process-objects! conn has-file-object-thumbnails-refs? bucket objects) - "file-thumbnail" (process-objects! conn has-file-thumbnails-refs? bucket objects) - "profile" (process-objects! conn has-profile-refs? bucket objects) - "file-data" (process-objects! conn has-file-data-refs? bucket objects) - "tempfile" (process-objects! conn (constantly false) bucket objects) - "organization" (process-objects! conn (constantly false) bucket objects) + (cond + (= bucket "file-media-object") (process-objects! conn has-file-media-object-refs? bucket objects) + (= bucket "team-font-variant") (process-objects! conn has-team-font-variant-refs? bucket objects) + (= bucket "file-object-thumbnail") (process-objects! conn has-file-object-thumbnails-refs? bucket objects) + (= bucket "file-thumbnail") (process-objects! conn has-file-thumbnails-refs? bucket objects) + (= bucket "profile") (process-objects! conn has-profile-refs? bucket objects) + (= bucket "file-data") (process-objects! conn has-file-data-refs? bucket objects) + (= bucket sto/tempfile-bucket) (process-objects! conn (constantly false) sto/tempfile-bucket objects) + (= bucket "organization") (process-objects! conn (constantly false) bucket objects) + :else (ex/raise :type :internal :code :unexpected-unknown-reference :hint (dm/fmt "unknown reference '%'" bucket)))) diff --git a/backend/test/backend_tests/http_assets_test.clj b/backend/test/backend_tests/http_assets_test.clj index e4f5ebff43..d689e9d2a6 100644 --- a/backend/test/backend_tests/http_assets_test.clj +++ b/backend/test/backend_tests/http_assets_test.clj @@ -37,11 +37,16 @@ (assoc storage ::sto/backend :fs)) (defn- create-storage-object! - "Create a storage object with the given bucket and content." - [storage bucket content] - (sto/put-object! storage {::sto/content (sto/content content) - :bucket bucket - :content-type "text/plain"})) + "Create a storage object with the given bucket and content. + Optional opts map can include :profile-id to set the owner." + ([storage bucket content] + (create-storage-object! storage bucket content {})) + ([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 "Build a minimal cfg map for the assets handlers." @@ -708,3 +713,70 @@ ::session/profile-id (:id profile)} response (assets/objects-handler cfg request)] (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)))))