From afa735a9c1fc2a45940a6a84c50dfdbc798b5d0b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 29 Nov 2023 17:13:54 +0100 Subject: [PATCH] :sparkles: Add protection for version inconsistency on opening or editing file --- backend/src/app/rpc/commands/files.clj | 29 +++++++-- backend/src/app/rpc/commands/files_update.clj | 60 +++++++++---------- common/src/app/common/files/migrations.cljc | 4 +- frontend/src/app/main/errors.cljs | 42 +++++++------ frontend/translations/en.po | 5 ++ 5 files changed, 80 insertions(+), 60 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index a74bb0290..38d4b1497 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -11,7 +11,7 @@ [app.common.exceptions :as ex] [app.common.features :as cfeat] [app.common.files.helpers :as cfh] - [app.common.files.migrations :as pmg] + [app.common.files.migrations :as fmg] [app.common.schema :as sm] [app.common.schema.desc-js-like :as-alias smdj] [app.common.spec :as us] @@ -68,6 +68,22 @@ changes (assoc :changes (blob/decode changes)) data (assoc :data (blob/decode data))))) +(defn check-version! + [{:keys [data] :as file}] + (dm/assert! + "expect data to be decoded" + (map? data)) + + (let [version (:version data 0)] + (when (> version fmg/version) + (ex/raise :type :restriction + :code :file-version-not-supported + :hint "file version is greated that the maximum" + :file-version version + :max-version fmg/version)) + + file)) + ;; --- FILE PERMISSIONS (def ^:private sql:file-permissions @@ -283,7 +299,7 @@ file (-> (db/get conn :file params) (decode-row) - (pmg/migrate-file))] + (fmg/migrate-file))] ;; NOTE: when file is migrated, we break the rule of no perform ;; mutations on get operations and update the file with all @@ -292,7 +308,7 @@ ;; NOTE: the following code will not work on read-only mode, it ;; is a known issue; we keep is not implemented until we really ;; need this - (if (pmg/migrated? file) + (if (fmg/migrated? file) (let [file (update file :features cfeat/migrate-legacy-features) features (set/union (deref cfeat/*new*) (:features file))] (db/update! conn :file @@ -328,7 +344,8 @@ :file-id id) file (-> (get-file conn id project-id) - (assoc :permissions perms)) + (assoc :permissions perms) + (check-version!)) _ (-> (cfeat/get-team-enabled-features cf/flags team) (cfeat/check-client-features! (:features params)) @@ -824,7 +841,7 @@ into the file local libraries" [conn {:keys [id] :as library}] (let [ldata (binding [pmap/*load-fn* (partial load-pointer conn id)] - (-> library decode-row (process-pointers deref) pmg/migrate-file :data)) + (-> library decode-row (process-pointers deref) fmg/migrate-file :data)) rows (db/exec! conn [sql:get-referenced-files id])] (doseq [file-id (map :id rows)] (binding [pmap/*load-fn* (partial load-pointer conn file-id) @@ -834,7 +851,7 @@ ::db/remove-deleted? false) (decode-row) (load-all-pointers!) - (pmg/migrate-file)) + (fmg/migrate-file)) data (ctf/absorb-assets (:data file) ldata)] (db/update! conn :file {:revn (inc (:revn file)) diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index c67447d7f..6026db5c1 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -10,7 +10,7 @@ [app.common.exceptions :as ex] [app.common.features :as cfeat] [app.common.files.changes :as cpc] - [app.common.files.migrations :as pmg] + [app.common.files.migrations :as fmg] [app.common.files.validate :as val] [app.common.logging :as l] [app.common.schema :as sm] @@ -39,34 +39,32 @@ ;; --- SCHEMA -(def ^:private schema:changes - [:vector ::cpc/change]) +(def ^:private + schema:update-file + (sm/define + [:map {:title "update-file"} + [:id ::sm/uuid] + [:session-id ::sm/uuid] + [:revn {:min 0} :int] + [:features {:optional true} ::cfeat/features] + [:changes {:optional true} [:vector ::cpc/change]] + [:changes-with-metadata {:optional true} + [:vector [:map + [:changes [:vector ::cpc/change]] + [:hint-origin {:optional true} :keyword] + [:hint-events {:optional true} [:vector :string]]]]] + [:skip-validate {:optional true} :boolean]])) -(def ^:private schema:change-with-metadata - [:map {:title "ChangeWithMetadata"} - [:changes schema:changes] - [:hint-origin {:optional true} :keyword] - [:hint-events {:optional true} [:vector :string]]]) - -(def ^:private schema:update-file - [:map {:title "update-file"} - [:id ::sm/uuid] - [:session-id ::sm/uuid] - [:revn {:min 0} :int] - [:features {:optional true} ::cfeat/features] - [:changes {:optional true} schema:changes] - [:changes-with-metadata {:optional true} - [:vector schema:change-with-metadata]] - [:skip-validate {:optional true} :boolean]]) - -(def ^:private schema:update-file-result - [:vector {:title "update-file-result"} - [:map - [:changes schema:changes] - [:file-id ::sm/uuid] - [:id ::sm/uuid] - [:revn {:min 0} :int] - [:session-id ::sm/uuid]]]) +(def ^:private + schema:update-file-result + (sm/define + [:vector {:title "update-file-result"} + [:map + [:changes [:vector ::cpc/change]] + [:file-id ::sm/uuid] + [:id ::sm/uuid] + [:revn {:min 0} :int] + [:session-id ::sm/uuid]]])) ;; --- HELPERS @@ -297,7 +295,7 @@ (-> data (blob/decode) (assoc :id (:id file)) - (pmg/migrate-data) + (fmg/migrate-data) (d/without-nils)))) ;; WARNING: this ruins performance; maybe we need to find @@ -310,10 +308,10 @@ (-> (db/get conn :file {:id id}) (files/decode-row) (files/process-pointers deref) ; ensure all pointers resolved - (pmg/migrate-file)))))) + (fmg/migrate-file)))))) (d/index-by :id)))] - (-> file + (-> (files/check-version! file) (update :revn inc) (update :data cpc/process-changes changes) diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index 46e5a1110..c461cb9fc 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -10,7 +10,7 @@ [app.common.data.macros :as dm] [app.common.features :as cfeat] [app.common.files.changes :as cpc] - [app.common.files.defaults :refer [version]] + [app.common.files.defaults :as cfd] [app.common.files.helpers :as cfh] [app.common.geom.matrix :as gmt] [app.common.geom.rect :as grc] @@ -27,6 +27,8 @@ #?(:cljs (l/set-level! :info)) +(def version cfd/version) + (defmulti migrate :version) (defn migrate-data diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index f1c85b9f2..58d1dc7e3 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -199,6 +199,15 @@ (ts/schedule #(st/emit! (rt/assign-exception error)))) + +(defn- redirect-to-dashboard + [] + (let [team-id (:current-team-id @st/state) + project-id (:current-project-id @st/state)] + (if (and project-id team-id) + (st/emit! (rt/nav :dashboard-files {:team-id team-id :project-id project-id})) + (set! (.-href glob/location) "")))) + (defmethod ptk/handle-error :restriction [{:keys [code] :as error}] (cond @@ -213,31 +222,20 @@ (st/emit! (modal/show {:type :alert :message message :on-accept on-accept}))) (= :file-feature-mismatch code) - (let [message (tr "errors.file-feature-mismatch" (:feature error)) - team-id (:current-team-id @st/state) - project-id (:current-project-id @st/state) - on-accept #(if (and project-id team-id) - (st/emit! (rt/nav :dashboard-files {:team-id team-id :project-id project-id})) - (set! (.-href glob/location) ""))] - (st/emit! (modal/show {:type :alert :message message :on-accept on-accept}))) + (let [message (tr "errors.file-feature-mismatch" (:feature error))] + (st/emit! (modal/show {:type :alert :message message :on-accept redirect-to-dashboard}))) (= :feature-mismatch code) - (let [message (tr "errors.feature-mismatch" (:feature error)) - team-id (:current-team-id @st/state) - project-id (:current-project-id @st/state) - on-accept #(if (and project-id team-id) - (st/emit! (rt/nav :dashboard-files {:team-id team-id :project-id project-id})) - (set! (.-href glob/location) ""))] - (st/emit! (modal/show {:type :alert :message message :on-accept on-accept}))) + (let [message (tr "errors.feature-mismatch" (:feature error))] + (st/emit! (modal/show {:type :alert :message message :on-accept redirect-to-dashboard}))) (= :feature-not-supported code) - (let [message (tr "errors.feature-not-supported" (:feature error)) - team-id (:current-team-id @st/state) - project-id (:current-project-id @st/state) - on-accept #(if (and project-id team-id) - (st/emit! (rt/nav :dashboard-files {:team-id team-id :project-id project-id})) - (set! (.-href glob/location) ""))] - (st/emit! (modal/show {:type :alert :message message :on-accept on-accept}))) + (let [message (tr "errors.feature-not-supported" (:feature error))] + (st/emit! (modal/show {:type :alert :message message :on-accept redirect-to-dashboard}))) + + (= :file-version-not-supported code) + (let [message (tr "errors.version-not-supported")] + (st/emit! (modal/show {:type :alert :message message :on-accept redirect-to-dashboard}))) (= :max-quote-reached code) (let [message (tr "errors.max-quote-reached" (:target error))] @@ -250,7 +248,7 @@ (st/emit! (modal/show {:type :alert :message message}))) :else - (ptk/handle-error {:type :server-error :data error}))) + (print-cause! "Restriction Error" error))) ;; This happens when the backed server fails to process the ;; request. This can be caused by an internal assertion or any other diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 1df309459..1c5e97d90 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -883,6 +883,11 @@ msgstr "" "Looks like you are opening a file that has the feature '%s' enabled but " "the current penpot version does not supports it or has it disabled." +#: src/app/main/errors.cljs +msgid "errors.version-not-supported" +msgstr "" +"File has an incompatible version number" + #: src/app/main/errors.cljs msgid "errors.file-feature-mismatch" msgstr ""