diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index e8b103c417..b8ef12eb74 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -122,7 +122,11 @@ {::yres/status 400 ::yres/body data})) :else - {::yres/status 400 ::yres/body data}))) + (let [explain (ex/explain data)] + {::yres/status 400 + ::yres/body (-> data + (dissoc ::sm/explain) + (cond-> explain (assoc :explain explain)))})))) (defmethod handle-error :assertion [error request parent-cause] diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index e28485181f..c5e464c75e 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -127,7 +127,15 @@ (-> (assoc ::auth-type :token) (assoc ::token-perms (set (::actoken/perms request #{}))))) (cond-> key-id - (assoc ::auth-key-id key-id))) + (assoc ::auth-key-id key-id)) + ;; Server-side only: request bodies cannot forge + ;; it (qualified keys are stripped before the + ;; merge). Upload maps are only trusted when they + ;; arrive over multipart. + (assoc ::is-multipart + (str/starts-with? + (str (yreq/get-header request "content-type")) + "multipart/"))) data (with-meta data {::http/request request}) @@ -228,10 +236,23 @@ (fn [cfg params] (let [request-params (-> params meta ::http/request :params decode)] (if (validate request-params) - (let [result (f cfg (merge params (d/without-qualified request-params)))] - (if (instance? clojure.lang.IObj result) - (vary-meta result assoc :encode/json encode) - result)) + (let [merged (merge params (d/without-qualified request-params))] + ;; Upload temp files are created server-side by the multipart + ;; parser, so an upload map smuggled in a transit/JSON body can + ;; point :path at any internal server file. Methods declare + ;; their upload keys with ::multipart-only-params and they are + ;; only accepted when ::is-multipart arrived server-side with + ;; the request (request bodies cannot forge qualified keys). + (when (some #(and (contains? merged %) + (not (::is-multipart merged))) + (::multipart-only-params mdata)) + (ex/raise :type :validation + :code :params-validation + :hint "upload requires multipart")) + (let [result (f cfg merged)] + (if (instance? clojure.lang.IObj result) + (vary-meta result assoc :encode/json encode) + result))) (ex/raise :type :validation :code :params-validation ::sm/explain (explain request-params)))))) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index c4fc37cb99..e842fd4c60 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -170,6 +170,7 @@ ::webhooks/event? true ::sse/stream? true ::sm/params schema:import-binfile + ::rpc/multipart-only-params [:file] ::climit/id [[:import-binfile/by-profile ::rpc/profile-id] [:import-binfile/global]]} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id project-id version upload-id] :as params}] diff --git a/backend/src/app/rpc/commands/files_thumbnails.clj b/backend/src/app/rpc/commands/files_thumbnails.clj index 1a821c4027..964e3be645 100644 --- a/backend/src/app/rpc/commands/files_thumbnails.clj +++ b/backend/src/app/rpc/commands/files_thumbnails.clj @@ -286,7 +286,8 @@ ::rtry/enabled true ::rtry/when rtry/conflict-exception? ::audit/skip true - ::sm/params schema:create-file-object-thumbnail} + ::sm/params schema:create-file-object-thumbnail + ::rpc/multipart-only-params [:media]} [cfg {:keys [::rpc/profile-id file-id object-id media tag]}] (media.v/validate-media-type! media) @@ -387,7 +388,8 @@ [:file-thumbnail-ops/global]] ::rtry/enabled true ::rtry/when rtry/conflict-exception? - ::sm/params schema:create-file-thumbnail} + ::sm/params schema:create-file-thumbnail + ::rpc/multipart-only-params [:media]} [cfg {:keys [::rpc/profile-id file-id] :as params}] (media.v/validate-media-type! (:media params)) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 1c9e07ed01..fa2199d24c 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -57,6 +57,7 @@ {::doc/added "1.17" ::doc/changes [["2.19" "The optional :id param is rejected with a params-validation error; the server always generates the identifier"]] ::sm/params schema:upload-file-media-object + ::rpc/multipart-only-params [:content] ::climit/id [[:process-image/by-profile ::rpc/profile-id] [:process-image/global]]} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id content] :as params}] @@ -371,6 +372,7 @@ (sv/defmethod ::upload-chunk {::doc/added "2.17" ::sm/params schema:upload-chunk + ::rpc/multipart-only-params [:content] ::sm/result schema:upload-chunk-result} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id session-id index content] :as _params}] diff --git a/backend/src/app/rpc/commands/profile.clj b/backend/src/app/rpc/commands/profile.clj index 82d307bb46..c0cdf56971 100644 --- a/backend/src/app/rpc/commands/profile.clj +++ b/backend/src/app/rpc/commands/profile.clj @@ -307,6 +307,7 @@ (sv/defmethod ::update-profile-photo {:doc/added "1.1" ::sm/params schema:update-profile-photo + ::rpc/multipart-only-params [:file] ::sm/result :nil} [cfg {:keys [::rpc/profile-id file] :as params}] ;; Validate incoming mime type diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index 0cfb0f2baf..f8b85ac0eb 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -1084,7 +1084,8 @@ (sv/defmethod ::update-team-photo {::doc/added "1.17" - ::sm/params schema:update-team-photo} + ::sm/params schema:update-team-photo + ::rpc/multipart-only-params [:file]} [cfg {:keys [::rpc/profile-id file] :as params}] ;; Validate incoming mime type diff --git a/backend/src/app/rpc/management/exporter.clj b/backend/src/app/rpc/management/exporter.clj index dabca00835..332fe23984 100644 --- a/backend/src/app/rpc/management/exporter.clj +++ b/backend/src/app/rpc/management/exporter.clj @@ -31,6 +31,7 @@ (sv/defmethod ::upload-tempfile {::doc/added "2.12" ::sm/params schema:upload-tempfile-params + ::rpc/multipart-only-params [:content] ::sm/result schema:upload-tempfile-result} [cfg {:keys [::rpc/profile-id content]}] (media.v/validate-media-type! content cm/tempfile-types) diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index 36e37aca9d..bb97a66f37 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -135,6 +135,7 @@ collection when replacing an existing one." {::doc/added "2.18" ::sm/params schema:upload-organization-logo + ::rpc/multipart-only-params [:content] ::sm/result schema:upload-organization-logo-result ::nitrate/sso false} [{:keys [::sto/storage]} {:keys [content organization-id previous-id]}] diff --git a/backend/test/backend_tests/binfile_svg_test.clj b/backend/test/backend_tests/binfile_svg_test.clj index 92fe3f2a0e..446abca995 100644 --- a/backend/test/backend_tests/binfile_svg_test.clj +++ b/backend/test/backend_tests/binfile_svg_test.clj @@ -195,15 +195,15 @@ :project-id (:default-project-id profile) :is-shared false}) [svg-path svg-size] (write-temp-svg clean-svg) - out (th/command! {::th/type :upload-file-media-object - ::rpc/profile-id (:id profile) - :file-id (:id file) - :is-local true - :name "benign.svg" - :content {:filename "benign.svg" - :path svg-path - :mtype "image/svg+xml" - :size svg-size}}) + out (th/multipart-command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id profile) + :file-id (:id file) + :is-local true + :name "benign.svg" + :content {:filename "benign.svg" + :path svg-path + :mtype "image/svg+xml" + :size svg-size}}) _ (t/is (nil? (:error out)) (str "upload failed: " (pr-str (:error out)))) uploaded (:result out) page-id (first (get-in file [:data :pages])) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 4882827b65..8aa3b684fc 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -471,6 +471,13 @@ (-> params (assoc :app.rpc/request-at (ct/now)) + ;; Mirror of the production flag in `app.rpc/make-rpc-handler`: + ;; upload maps are only trusted over multipart. Tests declare it + ;; with a content-type header on the supplied request map. + (assoc ::rpc/is-multipart + (str/starts-with? + (str (yreq/get-header request "content-type")) + "multipart/")) (with-meta {:app.http/request request})))) (defn command! @@ -508,6 +515,33 @@ (let [params (prepare-rpc-params data)] (try-on! (method-fn params))))) +(def multipart-headers + {"content-type" "multipart/form-data; boundary=test"}) + +(defn with-multipart + "Attach a multipart content-type to RPC test params, declaring that the + upload maps they carry arrived over multipart (as the production + multipart parser would deliver them). Preserves any request metadata + already present on the data." + [data] + (let [request (-> data meta :app.http/request)] + (vary-meta data assoc :app.http/request + (assoc (if (map? request) request {}) + :headers (merge multipart-headers + (when (map? request) + (:headers request))))))) + +(defn multipart-command! + "Like `command!` but declares the request as multipart, for calls that + carry upload maps (`:file`, `:content`, `:media`)." + [data] + (command! (with-multipart data))) + +(defn multipart-management-command! + "Like `management-command!` but declares the request as multipart." + [data] + (management-command! (with-multipart data))) + (defn run-task! ([name] (run-task! name {})) diff --git a/backend/test/backend_tests/helpers_test.clj b/backend/test/backend_tests/helpers_test.clj index d8e1bb2dc2..2473f86115 100644 --- a/backend/test/backend_tests/helpers_test.clj +++ b/backend/test/backend_tests/helpers_test.clj @@ -91,6 +91,35 @@ (t/deftest make-dummy-request-missing-cookie-returns-nil-value (t/is (= {:value nil} (yreq/get-cookie (th/make-dummy-request) "missing")))) +;; --- RPC PARAMS: MULTIPART FLAG + +(t/deftest prepare-rpc-params-marks-multipart-request + (let [params (#'th/prepare-rpc-params + (with-meta {::th/type :dummy + :events [{:name "event"}]} + {:app.http/request {:headers {"content-type" "multipart/form-data; boundary=xxx"}}}))] + (t/is (true? (:app.rpc/is-multipart params))))) + +(t/deftest prepare-rpc-params-leaves-non-multipart-request-unmarked + (let [params (#'th/prepare-rpc-params + (with-meta {::th/type :dummy + :events [{:name "event"}]} + {:app.http/request {:headers {"content-type" "application/transit+json"}}}))] + (t/is (false? (:app.rpc/is-multipart params))))) + +(t/deftest prepare-rpc-params-leaves-missing-content-type-unmarked + (let [params (#'th/prepare-rpc-params {::th/type :dummy + :events [{:name "event"}]})] + (t/is (false? (:app.rpc/is-multipart params))))) + +(t/deftest prepare-rpc-params-body-cannot-forge-multipart-flag + (let [params (#'th/prepare-rpc-params + (with-meta {::th/type :dummy + :app.rpc/is-multipart true + :events [{:name "event"}]} + {:app.http/request {:headers {"content-type" "application/json"}}}))] + (t/is (false? (:app.rpc/is-multipart params))))) + ;; --- RPC PARAMS: REQUEST METADATA (t/deftest prepare-rpc-params-preserves-supplied-request diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index c572e20c09..44042a4b2f 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -7,6 +7,7 @@ (ns backend-tests.http-middleware-test (:require [app.common.exceptions :as ex] + [app.common.json :as json] [app.common.time :as ct] [app.common.uuid :as uuid] [app.config :as cf] @@ -20,6 +21,7 @@ [app.metrics :as mtx] [app.rpc :as-alias rpc] [app.rpc.commands.access-token] + [app.storage.schema :as stsch] [app.tokens :as tokens] [backend-tests.helpers :as th] [clojure.string :as str] @@ -707,3 +709,23 @@ (t/is (= "safe user-facing hint" (:hint body))) (t/is (nil? (:state body))) (t/is (nil? (:path body))))) + +(t/deftest validation-error-body-with-raw-explain-is-json-serializable + ;; A :validation error whose ex-data carries the raw malli explain + ;; (storage invalid-storage-metadata) must produce a response body + ;; that survives JSON encoding. Before the fix the live schema + ;; object inside the explain broke the writer mid-stream and the + ;; client received a truncated body. + (let [cause (try + (stsch/encode-metadata {}) + (catch clojure.lang.ExceptionInfo cause cause)) + response (http-errors/handle cause {}) + body (::yres/body response)] + (t/is (= 400 (::yres/status response))) + (t/is (= :validation (:type body))) + (t/is (= :invalid-storage-metadata (:code body))) + (t/is (nil? (:app.common.schema/explain body))) + (let [encoded (json/encode body :key-fn json/write-camel-key) + decoded (json/decode encoded :key-fn json/read-kebab-key)] + (t/is (str/includes? encoded "invalid-storage-metadata")) + (t/is (string? (:explain decoded)))))) diff --git a/backend/test/backend_tests/rpc_binfile_test.clj b/backend/test/backend_tests/rpc_binfile_test.clj index ccdcc7fafa..0447fe135d 100644 --- a/backend/test/backend_tests/rpc_binfile_test.clj +++ b/backend/test/backend_tests/rpc_binfile_test.clj @@ -45,6 +45,50 @@ (t/is (false? (validator params-with-file-id)) "params with file-id should be rejected"))) +(t/deftest import-binfile-schema-accepts-path-object + ;; Documents the schema hole: a server-side Path object in :file passes + ;; validation on shape alone. The multipart guard in the handler (not the + ;; schema) is what rejects client-forged paths. + (let [schema @#'binfile/schema:import-binfile + validator (sm/lazy-validator schema)] + (t/is (true? (validator {:name "pwned" + :project-id (uuid/random) + :version 3 + :file {:filename "evil.zip" + :size 123 + :path (fs/path "/etc/passwd")}}))))) + +(t/deftest import-binfile-rejects-forged-file-without-multipart + ;; Path-traversal PoC at command level: a transit body like + ;; {"~:file": {"~:path": ["~#path", "/etc/passwd"]}} decodes to this exact + ;; map. Without a multipart request it must fail at params validation, + ;; before any file is read. + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :import-binfile + ::rpc/profile-id (:id prof) + :name "pwned" + :version 3 + :project-id (:default-project-id prof) + :file {:filename "evil.zip" + :size 123 + :path (fs/path "/etc/passwd")}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + +(t/deftest import-binfile-rejects-forged-file-with-forged-flag + ;; The multipart flag is server-side: smuggling it in the body must not + ;; bypass the guard (prepare-rpc-params overwrites it from the request). + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :import-binfile + ::rpc/profile-id (:id prof) + :app.rpc/is-multipart true + :name "pwned" + :version 3 + :project-id (:default-project-id prof) + :file {:filename "evil.zip" + :size 123 + :path (fs/path "/etc/passwd")}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + (t/deftest import-binfile-schema-rejects-unsupported-version ;; T1-N2-03: version parameter should be restricted to supported values (1 or 3) (let [schema @#'binfile/schema:import-binfile diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index 1bb3a0a09e..34adcae63e 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -264,7 +264,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -413,7 +413,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -604,7 +604,7 @@ :object-id object-id :tag "frame" :media mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -1264,7 +1264,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] (t/is (nil? error)) (t/is (map? result))) @@ -1321,7 +1321,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] (t/is (nil? error)) (t/is (map? result))) @@ -1345,7 +1345,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] (t/is (nil? error)) (t/is (map? result))) @@ -1396,7 +1396,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] ;; (th/print-result! out) (t/is (nil? error)) @@ -1411,7 +1411,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] ;; (th/print-result! out) (t/is (nil? error)) @@ -1909,7 +1909,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) diff --git a/backend/test/backend_tests/rpc_file_thumbnails_test.clj b/backend/test/backend_tests/rpc_file_thumbnails_test.clj index 5273180c15..49345f4ba9 100644 --- a/backend/test/backend_tests/rpc_file_thumbnails_test.clj +++ b/backend/test/backend_tests/rpc_file_thumbnails_test.clj @@ -76,11 +76,11 @@ :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data1)] + (let [out (th/multipart-command! data1)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) - (let [out (th/command! data2)] + (let [out (th/multipart-command! data2)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -184,12 +184,12 @@ :path (th/tempfile "backend_tests/test_files/sample.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data1)] + (let [out (th/multipart-command! data1)] ;; (th/print-result! out) (t/is (nil? (:error out))) (t/is (contains? (:result out) :uri))) - (let [out (th/command! data2)] + (let [out (th/multipart-command! data2)] (t/is (nil? (:error out))) (t/is (contains? (:result out) :uri))) @@ -272,7 +272,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data) + out (th/multipart-command! data) error (:error out)] (t/is (nil? (:result out))) @@ -298,7 +298,7 @@ :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data1)] + (let [out (th/multipart-command! data1)] ;; (th/print-result! out) (t/is (nil? (:error out))) (t/is (contains? (:result out) :uri))) @@ -325,7 +325,7 @@ :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data)] + (let [out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -341,7 +341,7 @@ (let [params {::th/type :get-file-object-thumbnails ::rpc/profile-id (:id profile) :file-id (:id file)} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) @@ -367,7 +367,7 @@ (-> media (assoc :path (th/tempfile "backend_tests/test_files/sample2.jpg")) (assoc :size 7923)))) - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -376,7 +376,7 @@ (-> media (assoc :path (th/tempfile "backend_tests/test_files/sample.jpg")) (assoc :size 312043)))) - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))))) @@ -402,7 +402,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -419,7 +419,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2 oid3]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -440,7 +440,7 @@ data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids []} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out))))) @@ -457,7 +457,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))))) @@ -480,7 +480,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -488,7 +488,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2 oid3]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -517,7 +517,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -525,7 +525,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -533,7 +533,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -563,7 +563,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -571,7 +571,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile2) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (some? (:error out))) (t/is (th/ex-info? (:error out))) (t/is (= :not-found (th/ex-type (:error out))))) @@ -606,7 +606,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -614,7 +614,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -653,7 +653,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -665,7 +665,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -674,7 +674,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile1) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (some? (:error out))) (t/is (th/ex-info? (:error out))) (t/is (= :not-found (th/ex-type (:error out))))) @@ -709,7 +709,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -721,7 +721,7 @@ :size 312043 :path (th/tempfile "backend_tests/test_files/sample.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -740,7 +740,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -770,7 +770,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -784,7 +784,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids oids} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -813,7 +813,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -821,7 +821,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -849,7 +849,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -857,7 +857,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -891,7 +891,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -899,7 +899,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -915,3 +915,39 @@ (t/is (= 1 (count rows2))) (t/is (nil? (:deleted-at (first rows2))))))) + +(t/deftest create-file-thumbnail-rejects-forged-media + ;; A transit body like {"~:media": {"~:path": ["~#path", "/etc/passwd"]}} + ;; decodes to this exact map. Without a multipart request it must fail at + ;; params validation, before any file is read. NOTE: plain command! on + ;; purpose, a multipart header would make it legitimate. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + out (th/command! {::th/type :create-file-thumbnail + ::rpc/profile-id (:id profile) + :file-id (:id file) + :revn 0 + :media {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + +(t/deftest create-file-object-thumbnail-rejects-forged-media + ;; Same transit-forgery vector through object thumbnails. NOTE: plain + ;; command! on purpose, a multipart header would make it legitimate. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + out (th/command! {::th/type :create-file-object-thumbnail + ::rpc/profile-id (:id profile) + :file-id (:id file) + :object-id "obj" + :media {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_font_test.clj b/backend/test/backend_tests/rpc_font_test.clj index bd81112bcb..4ef4533f7a 100644 --- a/backend/test/backend_tests/rpc_font_test.clj +++ b/backend/test/backend_tests/rpc_font_test.clj @@ -76,11 +76,11 @@ (when (uuid? session-id) (doseq [[idx chunk-data] (map-indexed vector chunks)] (let [mfile (make-chunk-mfile chunk-data mtype) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index idx - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index idx + :content mfile})] (t/is (nil? (:error out)))))) session-id)) @@ -438,11 +438,11 @@ ;; Upload all real chunks except the last one (omit it so the session is incomplete) (doseq [[idx chunk-data] (map-indexed vector (butlast chunks))] (let [mfile (make-chunk-mfile chunk-data "font/ttf") - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index idx - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index idx + :content mfile})] (t/is (nil? (:error out))))) (let [out (th/command! {::th/type :create-font-variant diff --git a/backend/test/backend_tests/rpc_management_nitrate_test.clj b/backend/test/backend_tests/rpc_management_nitrate_test.clj index 30ff6a3823..5be56ece30 100644 --- a/backend/test/backend_tests/rpc_management_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_management_nitrate_test.clj @@ -23,6 +23,7 @@ [clojure.set :as set] [clojure.test :as t] [cuerdas.core :as str] + [datoteka.fs :as fs] [mockery.core :refer [with-mocks]])) (t/use-fixtures :once (t/compose-fixtures @@ -2070,3 +2071,18 @@ (t/is (th/success? out)) (let [[params] (:call-args @email-mock)] (t/is (= "" (:user-name params))))))) + +(t/deftest upload-organization-logo-rejects-forged-content + ;; Same transit-forgery vector through the management API: without a + ;; multipart request the wrapper must reject it at params validation. + (with-mocks [nitrate-mock {:target 'app.nitrate/call :return nil}] + (let [profile (th/create-profile* 1 {:is-active true}) + params {::th/type :upload-organization-logo + ::rpc/profile-id (:id profile) + :organization-id (uuid/random) + :content {:filename "evil.png" + :path (fs/path "/etc/passwd") + :mtype "image/png" + :size 123}} + out (th/management-command! params)] + (t/is (th/ex-of-code? (:error out) :params-validation))))) diff --git a/backend/test/backend_tests/rpc_management_test.clj b/backend/test/backend_tests/rpc_management_test.clj index e1f6e81950..a55dcf95a4 100644 --- a/backend/test/backend_tests/rpc_management_test.clj +++ b/backend/test/backend_tests/rpc_management_test.clj @@ -56,9 +56,9 @@ :size 7}} config (assoc cf/config :public-uri "https://example.com/penpot") out1 (binding [cf/config config] - (th/management-command! params)) + (th/multipart-management-command! params)) out2 (binding [cf/config config] - (th/management-command! params))] + (th/multipart-management-command! params))] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) (t/is (str/starts-with? (str (get-in out1 [:result :uri])) @@ -77,7 +77,7 @@ :path path :mtype "text/html" :size 27}} - out (th/management-command! params)] + out (th/multipart-management-command! params)] (t/is (some? (:error out))) (t/is (= :validation (th/ex-type (:error out)))) (t/is (= :media-type-not-allowed (th/ex-code (:error out)))))) @@ -720,3 +720,16 @@ (ct/inst? (get-in % [:props :modified-at]))) events)) (t/is (every? #(= (:id team) (get-in % [:props :team-id])) events)))))) + +(t/deftest upload-tempfile-rejects-forged-content + ;; Same transit-forgery vector through the management API: without a + ;; multipart request the wrapper must reject it at params validation. + (let [profile (th/create-profile* 1 {:is-active true}) + params {::th/type :upload-tempfile + ::rpc/profile-id (:id profile) + :content {:filename "evil.png" + :path (fs/path "/etc/passwd") + :mtype "image/png" + :size 123}} + out (th/management-command! params)] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 985fb68fc2..3b42685b4d 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -73,7 +73,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -144,7 +144,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -189,7 +189,7 @@ :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] (let [error (:error out) error-data (ex-data error)] @@ -391,11 +391,11 @@ ;; --- 1. Upload chunks --- (doseq [[idx chunk-data] (map-indexed vector chunks)] (let [mfile (make-chunk-mfile chunk-data mtype) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index idx - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index idx + :content mfile})] (t/is (nil? (:error out))) (t/is (= session-id (:session-id (:result out)))) (t/is (= idx (:index (:result out)))))) @@ -438,11 +438,11 @@ mfile (make-chunk-mfile (first chunks) mtype) session-id (create-session! prof 1)] - (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) ;; First assemble succeeds; session row is marked as consumed afterwards (let [out1 (th/command! {::th/type :assemble-file-media-object @@ -482,11 +482,11 @@ :size 312043} ;; prof2 tries to upload a chunk into prof1's session - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof2) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof2) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out))) (t/is (= :not-found (-> out :error ex-data :type))))) @@ -505,11 +505,11 @@ :size 312043}] ;; prof1 uploads a chunk into their own session - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof1) - :session-id session-id - :index 0 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof1) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out)))) ;; prof2 tries to assemble prof1's session via create-font-variant @@ -540,11 +540,11 @@ :mtype "image/jpeg" :size 312043}] - (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) ;; Assemble with a wrong mtype should fail validation (let [out (th/command! {::th/type :assemble-file-media-object @@ -573,11 +573,11 @@ :size 312043}] ;; Upload only 1 chunk - (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) ;; Assemble: session says 3 expected, only 1 stored → :missing-chunks (let [out (th/command! {::th/type :assemble-file-media-object @@ -606,18 +606,18 @@ mtype "image/jpeg" size (alength (first chunks))] - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (first chunks) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] (t/is (nil? (:error out)))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (first chunks) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-already-exists (-> out :error ex-data :code)))) @@ -650,27 +650,27 @@ (t/is (= 3 (count chunks))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (nth chunks 0) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (nth chunks 0) mtype)})] (t/is (nil? (:error out)))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (nth chunks 0) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (nth chunks 0) mtype)})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-already-exists (-> out :error ex-data :code)))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 1 - :content (make-chunk-mfile (nth chunks 1) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 1 + :content (make-chunk-mfile (nth chunks 1) mtype)})] (t/is (nil? (:error out)))) ;; The mapping table holds exactly the two distinct indices: the @@ -735,11 +735,11 @@ :mtype "image/jpeg" :size 312043} session-id (create-session! prof 1) - upload-out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + upload-out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error upload-out))) (let [assemble-out (th/command! {::th/type :assemble-file-media-object @@ -788,21 +788,21 @@ :size 312043}] ;; index == total-chunks (out of range) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 2 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 2 + :content mfile})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :invalid-chunk-index (-> out :error ex-data :code)))) ;; negative index - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index -1 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index -1 + :content mfile})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :invalid-chunk-index (-> out :error ex-data :code)))))) @@ -821,11 +821,11 @@ mtype "image/jpeg"] ;; 312043 bytes exceeds the mocked 1024-byte cap: rejected - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (first chunks) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-too-large (-> out :error ex-data :code)))) @@ -835,11 +835,11 @@ session-id])))) ;; A chunk exactly at the cap still uploads fine - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (byte-array 1024 (byte 1)) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (byte-array 1024 (byte 1)) mtype)})] (t/is (nil? (:error out))))))) (t/deftest chunked-upload-sessions-per-profile-quota @@ -875,11 +875,11 @@ :path source-path :mtype "image/jpeg" :size 312043} - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (let [row (th/db-exec-one! ["select session_id, object_id, chunk_index from upload_session_chunk where session_id = ?" @@ -904,19 +904,19 @@ :path source-path :mtype "image/jpeg" :size 312043} - out1 (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out1 (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out1))) (let [before (:count (th/db-exec-one! ["select count(*) from storage_object"])) - out2 (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out2 (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out2))) (t/is (= :validation (-> out2 :error ex-data :type))) (t/is (= :chunk-already-exists (-> out2 :error ex-data :code))) @@ -937,11 +937,11 @@ (th/db-exec! ["insert into upload_session_chunk (session_id, chunk_index, object_id) values (?, ?, null)" session-id 0]) (let [before (:count (th/db-exec-one! ["select count(*) from storage_object"])) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-already-exists (-> out :error ex-data :code))) @@ -965,21 +965,21 @@ (if (compare-and-set! failed? false true) (throw (ex-info "link boom" {})) (orig pool object-id session-id index)))}] - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out)))) ;; the failed link left no reservation behind (t/is (= 0 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" session-id])))) ;; retrying the same index in the same session succeeds - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (t/is (= 1 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" session-id])))))))) @@ -999,11 +999,11 @@ :path source-path :mtype "image/jpeg" :size 312043} - out1 (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out1 (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out1))) (let [assemble-out (th/command! {::th/type :assemble-file-media-object @@ -1024,11 +1024,11 @@ ;; uploading to the consumed session fails without creating an object (let [before (:count (th/db-exec-one! ["select count(*) from storage_object"])) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out))) (t/is (= :not-found (-> out :error ex-data :type))) (t/is (= :object-not-found (-> out :error ex-data :code))) @@ -1054,11 +1054,11 @@ :path source-path :mtype "image/jpeg" :size 312043} - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (let [object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" @@ -1182,13 +1182,13 @@ :mtype "image/jpeg" :size 312043} sent-id (uuid/next) - out (th/command! {::th/type :upload-file-media-object - ::rpc/profile-id (:id prof) - :file-id (:id file) - :is-local true - :name "testfile" - :content mfile - :id sent-id})] + out (th/multipart-command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile + :id sent-id})] (t/is (th/ex-info? (:error out))) (t/is (th/ex-of-type? (:error out) :validation)) (t/is (th/ex-of-code? (:error out) :params-validation)))) @@ -1241,12 +1241,45 @@ :path (th/tempfile "backend_tests/test_files/sample.jpg") :mtype "image/jpeg" :size 312043} - out (th/command! {::th/type :upload-file-media-object - ::rpc/profile-id (:id prof) - :file-id (:id file) - :is-local true - :name "testfile" - :content mfile})] + out (th/multipart-command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile})] (t/is (th/success? out)) (t/is (= (:id team) (get-in (meta (:result out)) [:app.loggers.audit/replace-props :team-id]))))) + +(t/deftest upload-file-media-object-rejects-forged-content + ;; A transit body like {"~:content": {"~:path": ["~#path", "/etc/passwd"]}} + ;; decodes to this exact map. Without a multipart request it must fail at + ;; params validation, before any file is read. + (let [prof (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + out (th/command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + +(t/deftest upload-chunk-rejects-forged-content + ;; Same transit-forgery vector through the chunk primitive: without a + ;; multipart request the wrapper must reject it at params validation. + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id (uuid/random) + :index 0 + :content {:filename "chunk" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_profile_test.clj b/backend/test/backend_tests/rpc_profile_test.clj index 6200261e86..11a13addbe 100644 --- a/backend/test/backend_tests/rpc_profile_test.clj +++ b/backend/test/backend_tests/rpc_profile_test.clj @@ -140,12 +140,12 @@ (t/is (= "dark" (get-in out [:result :theme]))))) (t/testing "update photo" - (let [data {::th/type :update-profile-photo - ::rpc/profile-id (:id profile) - :file {:filename "sample.jpg" - :size 123123 - :path (th/tempfile "backend_tests/test_files/sample.jpg") - :mtype "image/jpeg"}} + (let [data (th/with-multipart {::th/type :update-profile-photo + ::rpc/profile-id (:id profile) + :file {:filename "sample.jpg" + :size 123123 + :path (th/tempfile "backend_tests/test_files/sample.jpg") + :mtype "image/jpeg"}}) out (th/command! data)] ;; (th/print-result! out) @@ -1829,3 +1829,16 @@ (t/is (th/success? second-out)) (t/is (= (:id profile) (get-in (meta second-out) [:app.loggers.audit/props :id])))))) + +(t/deftest update-profile-photo-rejects-forged-file + ;; A transit body like {"~:file": {"~:path": ["~#path", "/etc/passwd"]}} + ;; decodes to this exact map. Without a multipart request it must fail at + ;; params validation, before any file is read. + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :update-profile-photo + ::rpc/profile-id (:id prof) + :file {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_quotes_test.clj b/backend/test/backend_tests/rpc_quotes_test.clj index c691ea678b..7c42451418 100644 --- a/backend/test/backend_tests/rpc_quotes_test.clj +++ b/backend/test/backend_tests/rpc_quotes_test.clj @@ -464,7 +464,7 @@ :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; 312043 bytes > 100 byte limit → should be rejected (t/is (not (th/success? out))) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 991b380548..10638cee1d 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -911,7 +911,7 @@ :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] (t/is (nil? (:error out)))) (let [params {::th/type :delete-team @@ -1443,6 +1443,20 @@ (t/is (th/ex-of-type? (:error out) :validation)) (t/is (th/ex-of-code? (:error out) :params-validation)))) +(t/deftest update-team-photo-rejects-forged-file + ;; Same transit-forgery vector as profile photos: without a multipart + ;; request the wrapper must reject it at params validation. + (let [prof (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id prof)}) + out (th/command! {::th/type :update-team-photo + ::rpc/profile-id (:id prof) + :team-id (:id team) + :file {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + (t/deftest leaver-loses-file-access ;; GHSA-v9r9-h77c-55m2: leaving the team must revoke the file and ;; project roles granted in that team. diff --git a/backend/test/backend_tests/storage_test.clj b/backend/test/backend_tests/storage_test.clj index f85319bc47..0cc5ba990f 100644 --- a/backend/test/backend_tests/storage_test.clj +++ b/backend/test/backend_tests/storage_test.clj @@ -211,8 +211,8 @@ :name "testfile" :content mfile} - out1 (th/command! params) - out2 (th/command! params)] + out1 (th/multipart-command! params) + out2 (th/multipart-command! params)] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) @@ -269,11 +269,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (assert (nil? (:error out))) session-id)) @@ -318,7 +318,7 @@ :font-style "normal" :uploads {"font/ttf" session-id}} - out1 (th/command! params1) + out1 (th/multipart-command! params1) out2 (th/command! params2)] ;; (th/print-result! out) @@ -385,8 +385,8 @@ :name "testfile" :content mfile} - out1 (th/command! params) - out2 (th/command! params)] + out1 (th/multipart-command! params) + out2 (th/multipart-command! params)] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) @@ -661,8 +661,8 @@ :is-local true :name "testfile" :content mfile} - out1 (th/command! params) - out2 (th/command! params)] + out1 (th/multipart-command! params) + out2 (th/multipart-command! params)] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) @@ -1035,11 +1035,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) @@ -1076,11 +1076,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (t/is (= 1 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" @@ -1128,11 +1128,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (t/is (= 1 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" session-id])))) @@ -1167,11 +1167,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" session-id])) check @#'gc-touched/has-upload-session-chunk-refs? @@ -1212,11 +1212,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" session-id]))] (t/is (nil? (:error out))) @@ -1250,11 +1250,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" session-id]))] (t/is (nil? (:error out))) @@ -1290,11 +1290,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) @@ -1335,11 +1335,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) ;; soft-delete the profile; the live session is neither consumed nor stalled diff --git a/backend/test/e2e/import-binfile-path-traversal.test.mjs b/backend/test/e2e/import-binfile-path-traversal.test.mjs new file mode 100644 index 0000000000..df5598aa2f --- /dev/null +++ b/backend/test/e2e/import-binfile-path-traversal.test.mjs @@ -0,0 +1,110 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { setupTestProfile } from "./helpers/auth.mjs"; +import { rpcPost } from "./helpers/client.mjs"; + +function errorCode(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorCode(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.code ?? body["~:code"]; +} + +function errorType(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorType(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.type ?? body["~:type"]; +} + +describe("import-binfile path traversal", () => { + let profile; + let cookie; + + before(async () => { + const setup = await setupTestProfile(); + profile = setup.profile; + cookie = setup.cookie; + }); + + it("rejects a hand-forged :file path in a transit body", async () => { + // ATTACK SCENARIO: + // 1. The attacker has a valid session and a valid project-id. + // 2. Instead of a real multipart upload, they hand-write a transit + // body (plain JSON, no transit library needed) with a forged + // :file map that points to an internal server file. + // The transit tag ["~#path", "/etc/passwd"] becomes a real + // java.nio.file.Path on the server (see app.common.transit + // read handlers) and passes media.v/schema:upload, which only + // checks the shape, not where the path came from. + // 3. import-binfile then uses (:path file) directly as input-path + // (parse-file-format / get-manifest / import-files!). + // + // EXPECTED BEHAVIOR AFTER FIX: 400 validation/params-validation, + // without touching the file. While vulnerable, the request passes + // validation and fails later (zip/manifest error) or hangs in SSE, + // proving the file was opened. + const transitBody = JSON.stringify({ + "~:project-id": `~u${profile.defaultProjectId}`, + "~:name": "pwned", + "~:version": 3, + "~:file": { + "~:filename": "evil.zip", + "~:size": 123, + "~:path": ["~#path", "/etc/passwd"], + }, + }); + + const res = await rpcPost("import-binfile", transitBody, { + cookieToken: cookie, + contentType: "application/transit+json", + accept: "application/json", + }); + + assert.equal( + res.status, + 400, + `forged :file path must be rejected with 400, got ${res.status}: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorType(res.body), + "validation", + `expected validation error, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorCode(res.body), + "params-validation", + `forged :file path must fail at params validation, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + }); + + it("plain JSON string path stays rejected", async () => { + // Control case: without the transit "~#path" tag the path arrives + // as a plain string and ::fs/path already rejects it. This test + // documents that only the transit vector bypasses the check. + const res = await rpcPost( + "import-binfile", + { + projectId: profile.defaultProjectId, + name: "pwned", + version: 3, + file: { filename: "evil.zip", size: 123, path: "/etc/passwd" }, + }, + { cookieToken: cookie } + ); + + assert.equal(res.status, 400); + assert.equal(errorType(res.body), "validation"); + assert.equal(errorCode(res.body), "params-validation"); + }); +}); diff --git a/backend/test/e2e/upload-file-media-path-traversal.test.mjs b/backend/test/e2e/upload-file-media-path-traversal.test.mjs new file mode 100644 index 0000000000..7b98f820d2 --- /dev/null +++ b/backend/test/e2e/upload-file-media-path-traversal.test.mjs @@ -0,0 +1,189 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { setupTestProfile } from "./helpers/auth.mjs"; +import { rpcPost } from "./helpers/client.mjs"; + +function errorCode(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorCode(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.code ?? body["~:code"]; +} + +function errorType(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorType(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.type ?? body["~:type"]; +} + +async function createFile(cookie, projectId, name = "E2E Media Test File") { + const res = await rpcPost( + "create-file", + { name, projectId }, + { cookieToken: cookie } + ); + assert.equal(res.status, 200, `create-file failed: ${JSON.stringify(res.body)}`); + return res.body; +} + +async function createUploadSession(cookie) { + const res = await rpcPost( + "create-upload-session", + { totalChunks: 1 }, + { cookieToken: cookie } + ); + assert.equal( + res.status, + 200, + `create-upload-session failed: ${JSON.stringify(res.body)}` + ); + return res.body.sessionId ?? res.body["~:session-id"]; +} + +describe("upload-file-media path traversal", () => { + let profile; + let cookie; + let fileId; + + before(async () => { + const setup = await setupTestProfile(); + profile = setup.profile; + cookie = setup.cookie; + const file = await createFile(cookie, profile.defaultProjectId); + fileId = file.id; + }); + + it("rejects a hand-forged :content path in a transit body", async () => { + // ATTACK SCENARIO: + // 1. The attacker has a valid session and a file they can edit. + // 2. Instead of a real multipart upload, they hand-write a transit + // body (plain JSON, no transit library needed) with a forged + // :content map that points to an internal server file. + // The transit tag ["~#path", "/etc/passwd"] becomes a real + // java.nio.file.Path on the server (see app.common.transit + // read handlers) and passes media.v/schema:upload, which only + // checks the shape, not where the path came from. The attacker + // also controls :size and :mtype, so validate-media-type! and + // validate-media-size! pass with "image/png" and a small size. + // 3. upload-file-media-object then reads (:path content) in + // process-image (media/run :info) and stores the bytes as a + // media object, which is later downloadable by URL. + // + // EXPECTED BEHAVIOR AFTER FIX: 400 validation/params-validation, + // without touching the file. While vulnerable, the request passes + // validation and fails later in media processing, proving the + // file was opened. + const transitBody = JSON.stringify({ + "~:file-id": `~u${fileId}`, + "~:is-local": true, + "~:name": "pwned", + "~:content": { + "~:filename": "evil.png", + "~:size": 123, + "~:mtype": "image/png", + "~:path": ["~#path", "/etc/passwd"], + }, + }); + + const res = await rpcPost("upload-file-media-object", transitBody, { + cookieToken: cookie, + contentType: "application/transit+json", + accept: "application/json", + }); + + assert.equal( + res.status, + 400, + `forged :content path must be rejected with 400, got ${res.status}: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorType(res.body), + "validation", + `expected validation error, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorCode(res.body), + "params-validation", + `forged :content path must fail at params validation, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + }); + + it("rejects a hand-forged chunk :content path in a transit body", async () => { + // Same vector through upload-chunk: the forged path is read with + // sto/content and stored as a chunk blob. While vulnerable this + // returns 200 (the internal file bytes are stored under the + // attacker session, retrievable via assemble) — proving the file + // was read and kept. After the fix it must be 400 + // validation/params-validation. + const sessionId = await createUploadSession(cookie); + + const transitBody = JSON.stringify({ + "~:session-id": `~u${sessionId}`, + "~:index": 0, + "~:content": { + "~:filename": "c", + "~:size": 123, + "~:mtype": "application/octet-stream", + "~:path": ["~#path", "/etc/passwd"], + }, + }); + + const res = await rpcPost("upload-chunk", transitBody, { + cookieToken: cookie, + contentType: "application/transit+json", + accept: "application/json", + }); + + assert.equal( + res.status, + 400, + `forged chunk :content path must be rejected with 400, got ${res.status}: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorType(res.body), + "validation", + `expected validation error, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorCode(res.body), + "params-validation", + `forged chunk :content path must fail at params validation, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + }); + + it("plain JSON string path stays rejected", async () => { + // Control case: without the transit "~#path" tag the path arrives + // as a plain string and ::fs/path already rejects it. This test + // documents that only the transit vector bypasses the check. + const res = await rpcPost( + "upload-file-media-object", + { + fileId, + isLocal: true, + name: "pwned", + content: { + filename: "evil.png", + size: 123, + mtype: "image/png", + path: "/etc/passwd", + }, + }, + { cookieToken: cookie } + ); + + assert.equal(res.status, 400); + assert.equal(errorType(res.body), "validation"); + assert.equal(errorCode(res.body), "params-validation"); + }); +});