From e05747b54694b1ab5f60c16ca352bc61fcf4625f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Sep 2026 09:45:50 +0200 Subject: [PATCH] :sparkles: Restrict optional RPC ids to user-provided UUIDs (#11777) * :sparkles: Restrict optional RPC ids to user-provided UUIDs Add ::sm/user-provided-uuid, backed by a version and variant aware regex that only accepts v4, v7 and v8 instances. Use it for the optional :id of the creation RPC commands so reserved versions such as v3 are rejected at validation time. Reads such as get-team keep the lax ::sm/uuid. Cover the predicate and the schema on both JVM and JS runtimes. AI-assisted-by: muse-spark-1.3-contributor * :sparkles: Cover id version restriction at the RPC boundary Add backend regression tests proving the seven creation commands reject reserved-version ids (v3) with :params-validation and accept v4 ids (plus v7/v8 on create-team) through the real decode and validate path. Also drop two duplicated assertions and document the version and variant of every fixture UUID in user-provided-test. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/rpc/commands/files_create.clj | 2 +- backend/src/app/rpc/commands/media.clj | 6 +- backend/src/app/rpc/commands/projects.clj | 2 +- backend/src/app/rpc/commands/teams.clj | 2 +- .../app/rpc/commands/teams_invitations.clj | 2 +- backend/test/backend_tests/rpc_file_test.clj | 27 +++++ backend/test/backend_tests/rpc_media_test.clj | 105 ++++++++++++++++++ .../test/backend_tests/rpc_project_test.clj | 27 +++++ backend/test/backend_tests/rpc_team_test.clj | 56 ++++++++++ common/src/app/common/schema.cljc | 15 +++ common/src/app/common/uuid.cljc | 14 +++ common/test/common_tests/schema_test.cljc | 38 +++++++ common/test/common_tests/uuid_test.cljc | 43 +++++++ 13 files changed, 332 insertions(+), 7 deletions(-) diff --git a/backend/src/app/rpc/commands/files_create.clj b/backend/src/app/rpc/commands/files_create.clj index dd297e223b..2e2bbebea6 100644 --- a/backend/src/app/rpc/commands/files_create.clj +++ b/backend/src/app/rpc/commands/files_create.clj @@ -75,7 +75,7 @@ [:map {:title "create-file"} [:name [:string {:max 250}]] [:project-id ::sm/uuid] - [:id {:optional true} ::sm/uuid] + [:id {:optional true} ::sm/user-provided-uuid] [:is-shared {:optional true} ::sm/boolean] [:features {:optional true} ::cfeat/features]]) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 51f87c320c..99d5daea5a 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -48,7 +48,7 @@ (def ^:private schema:upload-file-media-object [:map {:title "upload-file-media-object"} - [:id {:optional true} ::sm/uuid] + [:id {:optional true} ::sm/user-provided-uuid] [:file-id ::sm/uuid] [:is-local ::sm/boolean] [:name [:string {:max 250}]] @@ -225,7 +225,7 @@ [:file-id ::sm/uuid] [:is-local ::sm/boolean] [:url ::sm/uri] - [:id {:optional true} ::sm/uuid] + [:id {:optional true} ::sm/user-provided-uuid] [:name {:optional true} [:string {:max 250}]]]) (sv/defmethod ::create-file-media-object-from-url @@ -489,7 +489,7 @@ [:is-local ::sm/boolean] [:name [:string {:max 250}]] [:mtype :string] - [:id {:optional true} ::sm/uuid]]) + [:id {:optional true} ::sm/user-provided-uuid]]) (sv/defmethod ::assemble-file-media-object {::doc/added "2.17" diff --git a/backend/src/app/rpc/commands/projects.clj b/backend/src/app/rpc/commands/projects.clj index cfb03a2f0d..05d5ae79aa 100644 --- a/backend/src/app/rpc/commands/projects.clj +++ b/backend/src/app/rpc/commands/projects.clj @@ -199,7 +199,7 @@ [:map {:title "create-project"} [:team-id ::sm/uuid] [:name [:string {:max 250 :min 1}]] - [:id {:optional true} ::sm/uuid]]) + [:id {:optional true} ::sm/user-provided-uuid]]) (sv/defmethod ::create-project {::doc/added "1.18" diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index 728e97bef1..9c13d6b37d 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -523,7 +523,7 @@ [:map {:title "create-team"} [:name types.team/schema:team-name] [:features {:optional true} ::cfeat/features] - [:id {:optional true} ::sm/uuid] + [:id {:optional true} ::sm/user-provided-uuid] [:organization-id {:optional true} ::sm/uuid] [:is-default {:optional true} :boolean]]) diff --git a/backend/src/app/rpc/commands/teams_invitations.clj b/backend/src/app/rpc/commands/teams_invitations.clj index 7051c2b594..dae96b1bf5 100644 --- a/backend/src/app/rpc/commands/teams_invitations.clj +++ b/backend/src/app/rpc/commands/teams_invitations.clj @@ -538,7 +538,7 @@ [:map {:title "create-team-with-invitations"} [:name [:string {:max 250}]] [:features {:optional true} ::cfeat/features] - [:id {:optional true} ::sm/uuid] + [:id {:optional true} ::sm/user-provided-uuid] [:emails [::sm/set ::sm/email]] [:role types.team/schema:role]]) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index da8fbbb1f7..b0076da05c 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -166,6 +166,33 @@ (t/is (th/ex-info? err)) (t/is (th/ex-of-type? err :not-found)))))) +(t/deftest create-file-id-version + (let [prof (th/create-profile* 1 {:is-active true}) + proj-id (:default-project-id prof) + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e" + v4-id "550e8400-e29b-41d4-a716-446655440000"] + + ;; reserved version (v3) must be rejected at the RPC boundary + (let [data {::th/type :create-file + ::rpc/profile-id (:id prof) + :project-id proj-id + :id v3-id + :name "file with v3 id"} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation))) + + ;; v4 id is accepted + (let [data {::th/type :create-file + ::rpc/profile-id (:id prof) + :project-id proj-id + :id v4-id + :name "file with v4 id"} + out (th/command! data)] + (t/is (th/success? out)) + (t/is (= v4-id (str (:id (:result out)))))))) + (t/deftest file-gc-with-fragments (let [profile (th/create-profile* 1) file (th/create-file* 1 {:profile-id (:id profile) diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index e22ddb5afd..958b425b32 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -137,6 +137,66 @@ (t/is (uuid? (:thumbnail-id result)))))) +(t/deftest upload-file-media-object-id-version + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + mfile {:filename "sample.jpg" + :path (th/tempfile "backend_tests/test_files/sample.jpg") + :mtype "image/jpeg" + :size 312043} + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e" + v4-id "550e8400-e29b-41d4-a716-446655440000"] + + ;; reserved version (v3) must be rejected at the RPC boundary + (let [params {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile + :id v3-id} + out (th/command! params)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation))) + + ;; v4 id is accepted + (let [params {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile + :id v4-id} + out (th/command! params)] + (t/is (th/success? out)) + (t/is (= v4-id (str (:id (:result out)))))))) + +(t/deftest create-file-media-object-from-url-id-version + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e"] + + ;; reserved version (v3) must be rejected before any download happens + (let [params {::th/type :create-file-media-object-from-url + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :url "https://example.com/sample.jpg" + :id v3-id} + out (th/command! params)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation))))) + (t/deftest media-object-from-url-command (let [prof (th/create-profile* 1) proj (th/create-project* 1 {:profile-id (:id prof) @@ -1078,3 +1138,48 @@ (t/is (th/ex-info? error)) (t/is (= :not-found (:type error-data))) (t/is (= :object-not-found (:code error-data)))))) + +(t/deftest assemble-file-media-object-id-version + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e" + v4-id "550e8400-e29b-41d4-a716-446655440000" + mtype "image/jpeg"] + + ;; reserved version (v3) must be rejected without touching any session + (let [out (th/command! {::th/type :assemble-file-media-object + ::rpc/profile-id (:id prof) + :session-id (uuid/next) + :file-id (:id file) + :is-local true + :name "assembled-image" + :mtype mtype + :id v3-id})] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation))) + + ;; v4 id is accepted through the full chunked flow + (let [source-path (th/tempfile "backend_tests/test_files/sample.jpg") + chunks (split-file-into-chunks source-path 312043) + session-id (create-session! prof 1) + mfile (make-chunk-mfile (first chunks) mtype) + _ (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) + out (th/command! {::th/type :assemble-file-media-object + ::rpc/profile-id (:id prof) + :session-id session-id + :file-id (:id file) + :is-local true + :name "assembled-image" + :mtype mtype + :id v4-id})] + (t/is (th/success? out)) + (t/is (= v4-id (str (:id (:result out)))))))) diff --git a/backend/test/backend_tests/rpc_project_test.clj b/backend/test/backend_tests/rpc_project_test.clj index 3f80f06f6b..01cfb0a853 100644 --- a/backend/test/backend_tests/rpc_project_test.clj +++ b/backend/test/backend_tests/rpc_project_test.clj @@ -123,6 +123,33 @@ (t/is (th/ex-info? error)) (t/is (th/ex-of-type? error :not-found)))) +(t/deftest create-project-id-version + (let [profile (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id profile)}) + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e" + v4-id "550e8400-e29b-41d4-a716-446655440000"] + + ;; reserved version (v3) must be rejected at the RPC boundary + (let [data {::th/type :create-project + ::rpc/profile-id (:id profile) + :team-id (:id team) + :id v3-id + :name "project with v3 id"} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation))) + + ;; v4 id is accepted + (let [data {::th/type :create-project + ::rpc/profile-id (:id profile) + :team-id (:id team) + :id v4-id + :name "project with v4 id"} + out (th/command! data)] + (t/is (th/success? out)) + (t/is (= v4-id (str (:id (:result out)))))))) + (t/deftest permissions-checks-rename-project (let [profile1 (th/create-profile* 1) profile2 (th/create-profile* 2) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index b087eb6819..61acf23cb8 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -1116,6 +1116,62 @@ out (th/command! data)] (t/is (th/success? out))))) +(t/deftest create-team-id-version + (let [profile (th/create-profile* 1 {:is-active true}) + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e" + v4-id "550e8400-e29b-41d4-a716-446655440000" + v7-id "0191062e-3f50-7a5e-9f5a-1a2b3c4d5e6f"] + + ;; reserved version (v3) must be rejected at the RPC boundary + (let [data {::th/type :create-team + ::rpc/profile-id (:id profile) + :name "team with v3 id" + :id v3-id} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation))) + + ;; v4, v7 and v8 ids are accepted + (doseq [id [v4-id v7-id (str (uuid/next))]] + (let [data {::th/type :create-team + ::rpc/profile-id (:id profile) + :name (str "team with id " id) + :id id} + out (th/command! data)] + (t/is (th/success? out)) + (t/is (= id (str (:id (:result out))))))))) + +(t/deftest create-team-with-invitations-id-version + (with-mocks [mock {:target 'app.email/send! :return nil}] + (let [profile (th/create-profile* 1 {:is-active true}) + v3-id "6fa459ea-ee8a-3ca4-894e-db77e160355e" + v4-id "550e8400-e29b-41d4-a716-446655440000"] + + ;; reserved version (v3) must be rejected before any invitation is sent + (let [data {::th/type :create-team-with-invitations + ::rpc/profile-id (:id profile) + :name "team with v3 id" + :id v3-id + :emails #{"invitee@example.com"} + :role :editor} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :params-validation)) + (t/is (= 0 (:call-count @mock)))) + + ;; v4 id is accepted + (let [data {::th/type :create-team-with-invitations + ::rpc/profile-id (:id profile) + :name "team with v4 id" + :id v4-id + :emails #{"invitee@example.com"} + :role :editor} + out (th/command! data)] + (t/is (th/success? out)) + (t/is (= v4-id (str (:id (:result out))))))))) + (t/deftest create-team-invitations-email-cooldown (with-mocks [mock {:target 'app.email/send! :return nil}] (let [profile1 (th/create-profile* 1 {:is-active true}) diff --git a/common/src/app/common/schema.cljc b/common/src/app/common/schema.cljc index fba8169bcd..9014bf1cf6 100644 --- a/common/src/app/common/schema.cljc +++ b/common/src/app/common/schema.cljc @@ -448,6 +448,21 @@ ::oapi/type "string" ::oapi/format "uuid"}}) +(register! + {:type ::user-provided-uuid + :pred uuid/user-provided? + :type-properties + {:title "user-provided-uuid" + :description "UUID provided by the user (v4, v7 or v8)" + :error/message "should be a user provided uuid (v4, v7 or v8)" + :gen/gen (sg/uuid) + :decode/string parse-uuid + :decode/json parse-uuid + :encode/string encode-uuid + :encode/json encode-uuid + ::oapi/type "string" + ::oapi/format "uuid"}}) + ;; Strict email regex aligned with app.common.spec/email-re. ;; Local part: valid RFC chars, no leading/trailing dot, no consecutive dots. ;; Domain: labels can't start/end with hyphen, no empty labels. diff --git a/common/src/app/common/uuid.cljc b/common/src/app/common/uuid.cljc index d094d4c06d..fce87bcd7d 100644 --- a/common/src/app/common/uuid.cljc +++ b/common/src/app/common/uuid.cljc @@ -70,6 +70,20 @@ [v] (= zero v)) +(def ^:private user-provided-regex + #"(?i)^[0-9a-f]{8}-[0-9a-f]{4}-[478][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$") + +(defn user-provided? + "Returns true when `v` is a UUID instance penpot can accept when + it is provided by the user (v4 random, v7 time-ordered or v8 penpot + time-ordered). Anything else, including UUID strings, returns false; + strings are meant to be decoded into UUID instances first, mirroring + how ::sm/uuid behaves." + [v] + (boolean + (and (uuid? v) + (re-matches user-provided-regex (str v))))) + (defn get-word-high [id] #?(:clj (.getMostSignificantBits ^UUID id) diff --git a/common/test/common_tests/schema_test.cljc b/common/test/common_tests/schema_test.cljc index b14f1df0f5..9dc98b118e 100644 --- a/common/test/common_tests/schema_test.cljc +++ b/common/test/common_tests/schema_test.cljc @@ -9,6 +9,7 @@ [app.common.data :as d] [app.common.schema :as sm] [app.common.schema.generators :as sg] + [app.common.uuid :as uuid] [clojure.test :as t])) (t/deftest test-set-of-email @@ -245,3 +246,40 @@ (t/is (false? (sm/validate ::sm/email "user@"))) (t/is (false? (sm/validate ::sm/email "userdomain.com"))) (t/is (false? (sm/validate ::sm/email "user@@domain.com"))))) + +(t/deftest test-user-provided-uuid + (let [v4 (uuid/uuid "550e8400-e29b-41d4-a716-446655440000") + v7 (uuid/uuid "0191062e-3f50-7a5e-9f5a-1a2b3c4d5e6f") + v8 (uuid/uuid "0227df82-63d7-8016-8005-48d9c0f33011") + v1 (uuid/uuid "6ba7b810-9dad-11d1-80b4-00c04fd430c8") + v3 (uuid/uuid "6fa459ea-ee8a-3ca4-894e-db77e160355e") + v5 (uuid/uuid "886313e1-3b8a-5372-9b90-0c9aee199e5d")] + + (t/testing "accepts v4, v7 and v8 instances" + (t/is (true? (sm/validate ::sm/user-provided-uuid v4))) + (t/is (true? (sm/validate ::sm/user-provided-uuid v7))) + (t/is (true? (sm/validate ::sm/user-provided-uuid v8))) + (t/is (true? (sm/validate ::sm/user-provided-uuid (uuid/random)))) + (t/is (true? (sm/validate ::sm/user-provided-uuid (uuid/next))))) + + (t/testing "rejects reserved and other versions" + (t/is (false? (sm/validate ::sm/user-provided-uuid v1))) + (t/is (false? (sm/validate ::sm/user-provided-uuid v3))) + (t/is (false? (sm/validate ::sm/user-provided-uuid v5))) + (t/is (false? (sm/validate ::sm/user-provided-uuid uuid/zero))) + (t/is (false? (sm/validate ::sm/user-provided-uuid nil))) + (t/is (false? (sm/validate ::sm/user-provided-uuid "not-an-uuid")))) + + (t/testing "decodes strings like the RPC layer does" + (let [decode (sm/decoder ::sm/user-provided-uuid sm/json-transformer)] + (t/is (= v4 (decode (str v4)))) + (t/is (= v3 (decode (str v3)))) + (t/is (true? (sm/validate ::sm/user-provided-uuid (decode (str v4))))) + (t/is (false? (sm/validate ::sm/user-provided-uuid (decode (str v3))))))) + + (t/testing "rejects raw strings without decoding, like ::sm/uuid does" + (t/is (false? (sm/validate ::sm/user-provided-uuid (str v4)))) + (t/is (false? (sm/validate ::sm/user-provided-uuid (str v3))))) + + (t/testing "generate" + (t/is (true? (sm/validate ::sm/user-provided-uuid (sg/generate ::sm/user-provided-uuid))))))) diff --git a/common/test/common_tests/uuid_test.cljc b/common/test/common_tests/uuid_test.cljc index ec6f14ceee..6d6d982e5d 100644 --- a/common/test/common_tests/uuid_test.cljc +++ b/common/test/common_tests/uuid_test.cljc @@ -95,3 +95,46 @@ (t/is (= (nth expected 1) (aget parts 1))) (t/is (= (nth expected 2) (aget parts 2))) (t/is (= (nth expected 3) (aget parts 3)))))))) + +(t/deftest user-provided-test + ;; The version is the first nibble of the 3rd group, the variant is + ;; the first nibble of the 4th group (8, 9, a or b means RFC 4122). + (let [v4 (uuid/uuid "550e8400-e29b-41d4-a716-446655440000") ; v4, variant a: valid + v7 (uuid/uuid "0191062e-3f50-7a5e-9f5a-1a2b3c4d5e6f") ; v7, variant 9: valid + v8 (uuid/uuid "0227df82-63d7-8016-8005-48d9c0f33011") ; v8, variant 8: valid + v4-upper (uuid/uuid "550E8400-E29B-41D4-A716-446655440000") ; v4 uppercase, variant A: valid + v1 (uuid/uuid "6ba7b810-9dad-11d1-80b4-00c04fd430c8") ; v1 time-based: rejected + v3 (uuid/uuid "6fa459ea-ee8a-3ca4-894e-db77e160355e") ; v3 MD5 name-based (reserved): rejected + v5 (uuid/uuid "886313e1-3b8a-5372-9b90-0c9aee199e5d") ; v5 SHA-1 name-based: rejected + v6 (uuid/uuid "1e29effc-7a68-6d2c-9b6f-7f6a5f9b5b5b") ; v6 time-ordered: rejected + v4-bad-variant (uuid/uuid "550e8400-e29b-41d4-0716-446655440000")] ; v4 but variant 0 (NCS reserved): rejected + + (t/testing "accepts v4, v7 and v8 instances" + (t/is (true? (uuid/user-provided? v4))) + (t/is (true? (uuid/user-provided? v7))) + (t/is (true? (uuid/user-provided? v8))) + (t/is (true? (uuid/user-provided? v4-upper))) + (t/is (true? (uuid/user-provided? (uuid/random)))) ; generates v4 + (t/is (true? (uuid/user-provided? (uuid/next))))) ; generates v8 + + (t/testing "rejects strings, they must be decoded first" + (t/is (false? (uuid/user-provided? (str v4)))) + (t/is (false? (uuid/user-provided? (str v7)))) + (t/is (false? (uuid/user-provided? (str v8)))) + (t/is (false? (uuid/user-provided? (str v3))))) + + (t/testing "rejects nil uuid and reserved and other versions" + (t/is (false? (uuid/user-provided? uuid/zero))) ; version 0, variant 0 + (t/is (false? (uuid/user-provided? v1))) + (t/is (false? (uuid/user-provided? v3))) + (t/is (false? (uuid/user-provided? v5))) + (t/is (false? (uuid/user-provided? v6))) + (t/is (false? (uuid/user-provided? (uuid/custom 1))))) ; version 0, variant 0 + + (t/testing "rejects right version with wrong variant" + (t/is (false? (uuid/user-provided? v4-bad-variant)))) + + (t/testing "rejects non-uuid values" + (t/is (false? (uuid/user-provided? nil))) + (t/is (false? (uuid/user-provided? 42))) + (t/is (false? (uuid/user-provided? "not-an-uuid"))))))