From f1cae19ed2143d4c19db6b37a5a0fc321e5ed28b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 10 Sep 2026 13:48:17 +0000 Subject: [PATCH] :recycle: Uniform binfile import limits behind init-limits Move the binfile import limits to a single source of truth in app.binfile.common (default-* vars) and drop the duplicated entries from config/default; env overrides keep working through the schema. Resolve all limits once per job with init-limits (::max-size, ::total-max, ::current-size, ::max-object-size, ::max-zip-entries) instead of rebuilding the map per zip entry. Thread cfg as the first arg through the v3 readers, collapse read-plain-entry into read-entry, and give size-limiting-stream a single explicit-counter arity. v1 keeps using the compiled default (mechanical rename only). No behavior change. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/binfile/common.clj | 19 ++- backend/src/app/binfile/v1.clj | 4 +- backend/src/app/binfile/v3.clj | 176 ++++++++++---------- backend/src/app/config.clj | 8 +- backend/test/backend_tests/binfile_test.clj | 8 +- 5 files changed, 104 insertions(+), 111 deletions(-) diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 5eee3d320e..c755630799 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -52,24 +52,29 @@ (* 1024 1024 2)) ;; A maximum (storage) object size allowed: 100MiB -(def ^:const max-object-size +(def ^:const default-max-object-size (* 1024 1024 100)) ;; Maximum decompressed size allowed for a single JSON/text zip entry ;; (manifest, files, pages, shapes, colors, components, typographies, ;; tokens, plugin-data) during binfile import: 20MiB. Legitimate entries -;; are KB-sized, so this is deliberately much lower than max-object-size -;; and bounds the DEFLATE amplification of any single entry. -(def ^:const max-entry-text-size +;; are KB-sized, so this is deliberately much lower than +;; default-max-object-size and bounds the DEFLATE amplification of any +;; single entry. +(def ^:const default-max-entry-text-size (* 1024 1024 20)) ;; Maximum total decompressed size allowed for all JSON/text zip entries ;; combined within a single import job: 200MiB. Bounds the case where many -;; entries, each individually under max-entry-text-size, still sum to an -;; unreasonable total. -(def ^:const max-text-total-size +;; entries, each individually under default-max-entry-text-size, still sum +;; to an unreasonable total. +(def ^:const default-max-text-total-size (* 1024 1024 200)) +;; Maximum number of entries allowed in the import zip: 500,000. +(def ^:const default-max-zip-entries + (* 500 1000)) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (declare get-resolved-file-libraries) diff --git a/backend/src/app/binfile/v1.clj b/backend/src/app/binfile/v1.clj index 5f1834cb74..5a1c24b93e 100644 --- a/backend/src/app/binfile/v1.clj +++ b/backend/src/app/binfile/v1.clj @@ -174,7 +174,7 @@ (assert-mark m :obj) (let [size (read-long! input)] (assert (pos? size) "incorrect header size found on reading header") - (when (> size bfc/max-object-size) + (when (> size bfc/default-max-object-size) (ex/raise :type :validation :code :max-file-size-reached :hint (dm/str "unable to import object with size " size " bytes"))) @@ -249,7 +249,7 @@ p (tmp/tempfile :prefix "penpot.binfile.")] (assert-mark m :stream) - (when (> s bfc/max-object-size) + (when (> s bfc/default-max-object-size) (ex/raise :type :validation :code :max-file-size-reached :hint (str/ffmt "unable to import storage object with size % bytes" s))) diff --git a/backend/src/app/binfile/v3.clj b/backend/src/app/binfile/v3.clj index c952135c15..e633f2a6bb 100644 --- a/backend/src/app/binfile/v3.clj +++ b/backend/src/app/binfile/v3.clj @@ -434,54 +434,57 @@ (defn- size-limiting-stream "Wraps an InputStream to enforce a maximum number of decompressed bytes. Raises :validation :max-file-size-reached when the limit is exceeded. - Accepts an optional shared counter atom so several streams can account - against the same cumulative budget." - (^InputStream - [input max-size] - (size-limiting-stream input max-size (atom 0))) - (^InputStream - [^InputStream input ^long max-size counter] - (let [on-read (fn [n] - (when (pos? n) - (when (> (swap! counter + (long n)) max-size) - (ex/raise :type :validation - :code :max-file-size-reached - :hint (str "stream exceeded max size: " max-size)))) - n)] - (proxy [FilterInputStream] [input] - (read - ([] - (let [b (.read input)] - (when (pos? b) (on-read 1)) - b)) - ([^bytes buf] - (on-read (.read input buf 0 (alength buf)))) - ([^bytes buf off] - (on-read (.read input buf (int off) (- (alength buf) (int off))))) - ([^bytes buf off len] - (on-read (.read input buf (int off) (int len))))))))) + `counter` holds the bytes accounted so far: pass a fresh atom for a + per-entry cap, or the shared job-wide atom for the cumulative budget." + ^InputStream + [^InputStream input ^long max-size counter] + (let [on-read (fn [n] + (when (pos? n) + (when (> (swap! counter + (long n)) max-size) + (ex/raise :type :validation + :code :max-file-size-reached + :hint (str "stream exceeded max size: " max-size)))) + n)] + (proxy [FilterInputStream] [input] + (read + ([] + (let [b (.read input)] + (when (pos? b) (on-read 1)) + b)) + ([^bytes buf] + (on-read (.read input buf 0 (alength buf)))) + ([^bytes buf off] + (on-read (.read input buf (int off) (- (alength buf) (int off))))) + ([^bytes buf off len] + (on-read (.read input buf (int off) (int len)))))))) -(defn- text-entry-limits - "Resolve the decompressed-size limits for JSON/text zip entries from cfg, - falling back to safe defaults when the keys are absent (e.g. callers that - predate the limits). Returns a map with `:max-size` (per-entry cap), - `:cumulative` (shared atom or nil) and `:total-max` (cumulative cap)." +(defn- init-limits + "Resolve the binfile import limits once per job from cfg, falling back + to the namespace defaults when the keys are absent. Returns cfg with + `::max-size` (per JSON/text entry cap), `::total-max` (cumulative + JSON/text budget), `::current-size` (shared atom holding the cumulative + bytes read so far), `::max-object-size` (storage blob cap) and + `::max-zip-entries` (zip entry count cap)." [cfg] - {:max-size (or (::bfc/import-max-entry-text-size cfg) bfc/max-entry-text-size) - :cumulative (::bfc/cumulative-text-bytes cfg) - :total-max (or (::bfc/import-max-text-total-size cfg) bfc/max-text-total-size)}) + (assoc cfg + ::max-size (or (::bfc/import-max-entry-text-size cfg) bfc/default-max-entry-text-size) + ::total-max (or (::bfc/import-max-text-total-size cfg) bfc/default-max-text-total-size) + ::current-size (atom 0) + ::max-object-size (or (::bfc/import-max-object-size cfg) bfc/default-max-object-size) + ::max-zip-entries (or (::bfc/import-max-zip-entries cfg) bfc/default-max-zip-entries))) (defn- zip-entry-reader - "Opens a UTF-8 reader over a zip entry, enforcing `max-size` on the actual - decompressed bytes streamed through it and, when `cumulative` is provided, - accounting the same bytes against the shared job-wide `total-max` budget. + "Opens a UTF-8 reader over a zip entry, enforcing the per-entry + decompressed-size cap on the actual bytes streamed through it and + accounting the same bytes against the shared job-wide budget. A cheap pre-check on the entry-declared size rejects obvious bombs without opening the stream; the streaming counters remain the authoritative guard because the declared size is attacker-controlled metadata." - [^ZipFile input ^ZipEntry entry & {:keys [max-size cumulative total-max]}] + [cfg ^ZipFile input ^ZipEntry entry] (let [entry-name (zip-entry-name entry) - declared (get-zip-entry-size entry)] - (when (and max-size (not (neg? declared)) (> declared (long max-size))) + declared (get-zip-entry-size entry) + max-size (::max-size cfg)] + (when (and (not (neg? declared)) (> declared (long max-size))) (ex/raise :type :validation :code :max-file-size-reached :hint (str "zip entry exceeds maximum size: " entry-name) @@ -489,8 +492,8 @@ :max max-size :found declared)) (-> (zip-entry-stream input entry) - (cond-> max-size (size-limiting-stream max-size)) - (cond-> cumulative (size-limiting-stream total-max cumulative)) + (size-limiting-stream max-size (atom 0)) + (size-limiting-stream (::total-max cfg) (::current-size cfg)) (io/reader :encoding "UTF-8")))) (defn- zip-entry-storage-content @@ -499,7 +502,7 @@ [input entry & {:keys [max-size]}] (let [stream-fn (fn [] (cond-> (zip-entry-stream input entry) - max-size (size-limiting-stream max-size))) + max-size (size-limiting-stream max-size (atom 0)))) hash (delay (->> (stream-fn) (sto.impl/calculate-hash)))] (reify @@ -523,12 +526,9 @@ (throw (UnsupportedOperationException. "not implemented")))))) (defn- read-manifest - [^ZipFile input & [{:keys [max-size cumulative total-max]}]] + [cfg ^ZipFile input] (let [entry (get-zip-entry input "manifest.json")] - (with-open [^AutoCloseable reader (zip-entry-reader input entry - :max-size (or max-size bfc/max-entry-text-size) - :cumulative cumulative - :total-max total-max)] + (with-open [^AutoCloseable reader (zip-entry-reader cfg input entry)] (let [manifest (json/read reader :key-fn json/read-kebab-key)] (decode-manifest manifest))))) @@ -617,26 +617,17 @@ :id (parse-uuid id)})))) (defn- read-entry - [^ZipFile input entry & [{:keys [max-size cumulative total-max]}]] - (with-open [^AutoCloseable reader (zip-entry-reader input entry - :max-size (or max-size bfc/max-entry-text-size) - :cumulative cumulative - :total-max total-max)] - (json/read reader :key-fn json/read-kebab-key))) - -(defn- read-plain-entry - [^ZipFile input entry & [{:keys [max-size cumulative total-max]}]] - (with-open [^AutoCloseable reader (zip-entry-reader input entry - :max-size (or max-size bfc/max-entry-text-size) - :cumulative cumulative - :total-max total-max)] - (json/read reader))) + ([cfg ^ZipFile input entry] + (read-entry cfg input entry json/read-kebab-key)) + ([cfg ^ZipFile input entry key-fn] + (with-open [^AutoCloseable reader (zip-entry-reader cfg input entry)] + (json/read reader :key-fn key-fn)))) (defn- read-file [{:keys [::bfc/input ::bfc/timestamp] :as cfg} file-id] (let [path (str "files/" file-id ".json") entry (get-zip-entry input path)] - (-> (read-entry input entry (text-entry-limits cfg)) + (-> (read-entry cfg input entry) (decode-file) (update :revn d/nilv 1) (update :created-at d/nilv timestamp) @@ -648,7 +639,7 @@ (let [path (str "files/" file-id "/plugin-data.json") entry (get-zip-entry* input path)] (when entry - (-> (read-entry input entry (text-entry-limits cfg)) + (-> (read-entry cfg input entry) (decode-plugin-data) (validate-plugin-data))))) @@ -656,7 +647,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-media-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry (text-entry-limits cfg)) + (let [object (->> (read-entry cfg input entry) (decode-media) (validate-media)) object (-> object @@ -676,7 +667,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-color-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry (text-entry-limits cfg)) + (let [object (->> (read-entry cfg input entry) (decode-color) (validate-color))] (events/tap :progress {:section :color :id id :file-id file-id}) @@ -707,7 +698,7 @@ (->> (keep (match-component-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry (text-entry-limits cfg)) + (let [object (->> (read-entry cfg input entry) (clean-component-pre-decode) (decode-component) (clean-component-post-decode))] @@ -722,7 +713,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-typography-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry (text-entry-limits cfg)) + (let [object (->> (read-entry cfg input entry) (decode-typography) (validate-typography))] (events/tap :progress {:section :typography :id id :file-id file-id}) @@ -736,7 +727,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (when-let [entry (d/seek (match-tokens-lib-entry-fn file-id) entries)] (events/tap :progress {:section :tokens-lib :file-id file-id}) - (->> (read-plain-entry input entry (text-entry-limits cfg)) + (->> (read-entry cfg input entry nil) (decode-tokens-lib) (validate-tokens-lib)))) @@ -744,7 +735,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id page-id] (->> (keep (match-shape-entry-fn file-id page-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry (text-entry-limits cfg)) + (let [object (->> (read-entry cfg input entry) (bfl/clean-shape-pre-decode) (decode-shape) (bfl/clean-shape-post-decode))] @@ -758,7 +749,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-page-entry-fn file-id) entries) (keep (fn [{:keys [id entry]}] - (let [page (->> (read-entry input entry (text-entry-limits cfg)) + (let [page (->> (read-entry cfg input entry) (decode-page)) page (dissoc page :options)] (events/tap :progress {:section :page :id id :file-id file-id}) @@ -774,7 +765,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-thumbnail-entry-fn file-id) entries) (reduce (fn [result {:keys [page-id frame-id tag entry]}] - (let [object (->> (read-entry input entry (text-entry-limits cfg)) + (let [object (->> (read-entry cfg input entry) (decode-file-thumbnail) (validate-file-thumbnail))] @@ -908,7 +899,7 @@ entries (keep (match-storage-entry-fn) entries)] (doseq [{:keys [id entry]} entries] - (let [object (-> (read-entry input entry (text-entry-limits cfg)) + (let [object (-> (read-entry cfg input entry) (decode-storage-object) (update :bucket d/nilv sto/default-bucket) (validate-storage-object)) @@ -917,7 +908,7 @@ path (str "objects/" id ext) content (zip-entry-storage-content input (get-zip-entry input path) - :max-size (::bfc/import-max-object-size cfg))] + :max-size (::max-object-size cfg))] (when (not= (:size object) (sto/get-size content)) (ex/raise :type :validation @@ -927,7 +918,7 @@ :expected-size (:size object) :found-size (sto/get-size content))) - (when-let [max (::bfc/import-max-object-size cfg)] + (let [max (::max-object-size cfg)] (when (> (sto/get-size content) max) (ex/raise :type :validation :code :max-file-size-reached @@ -1015,21 +1006,22 @@ (assert (instance? ZipFile input) "expected zip file") (assert (ct/inst? timestamp) "expected valid instant") - ;; Shared job-wide budget for all JSON/text entries: every bounded read - ;; below accounts its actual decompressed bytes here, so many entries - ;; each under the per-entry cap cannot sum to an unreasonable total. - (let [cfg (assoc cfg ::bfc/cumulative-text-bytes (atom 0)) - manifest (-> (read-manifest input (text-entry-limits cfg)) + ;; Resolve all import limits once per job (see `init-limits`); every + ;; bounded read below accounts its actual decompressed bytes against the + ;; shared job-wide budget, so many entries each under the per-entry cap + ;; cannot sum to an unreasonable total. + (let [cfg (init-limits cfg) + manifest (-> (read-manifest cfg input) (validate-manifest)) entries (read-zip-entries input) - _ (when-let [max (::bfc/import-max-zip-entries cfg)] - (when (> (count entries) max) - (ex/raise :type :validation - :code :too-many-zip-entries - :hint (str "zip file has too many entries: " (count entries)) - :max max - :found (count entries)))) + max (::max-zip-entries cfg) + _ (when (> (count entries) max) + (ex/raise :type :validation + :code :too-many-zip-entries + :hint (str "zip file has too many entries: " (count entries)) + :max max + :found (count entries))) cfg (-> cfg (assoc ::entries entries) @@ -1154,8 +1146,10 @@ "Reads and validates the manifest of a `.penpot` file at `path`. Runs synchronously on the RPC request thread (before the background import job exists), so the read is bounded by the configured - per-entry decompressed-size limit." + decompressed-size limits." [path] - (with-open [^AutoCloseable input (ZipFile. ^File (fs/file path))] - (-> (read-manifest input {:max-size (cf/get :binfile-import-max-entry-text-size)}) - (validate-manifest)))) + (let [cfg (init-limits {::bfc/import-max-entry-text-size (cf/get :binfile-import-max-entry-text-size) + ::bfc/import-max-text-total-size (cf/get :binfile-import-max-text-total-size)})] + (with-open [^AutoCloseable input (ZipFile. ^File (fs/file path))] + (-> (read-manifest cfg input) + (validate-manifest))))) diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index a022c0a28e..55be5383c2 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -95,13 +95,7 @@ ;; SSRF protection :ssrf-allowed-hosts #{} - :ssrf-extra-blocked-cidrs #{} - - ;; Binfile import limits - :binfile-import-max-object-size (* 1024 1024 100) ;; 100 MiB - :binfile-import-max-entry-text-size (* 1024 1024 20) ;; 20 MiB - :binfile-import-max-text-total-size (* 1024 1024 200) ;; 200 MiB - :binfile-import-max-zip-entries (* 500 1000)}) ;; 500,000 + :ssrf-extra-blocked-cidrs #{}}) (def schema:config (do #_sm/optional-keys diff --git a/backend/test/backend_tests/binfile_test.clj b/backend/test/backend_tests/binfile_test.clj index 50c9ff598a..0473204ac3 100644 --- a/backend/test/backend_tests/binfile_test.clj +++ b/backend/test/backend_tests/binfile_test.clj @@ -238,9 +238,9 @@ (t/is (= "penpot" (get-in imported [:metadata :referer])))))) (t/deftest read-obj-rejects-oversized-buffer - ;; N1-07: read-obj! must reject objects exceeding max-object-size + ;; N1-07: read-obj! must reject objects exceeding default-max-object-size ;; before attempting to allocate the buffer - (let [size (+ bfc/max-object-size 1) + (let [size (+ bfc/default-max-object-size 1) baos (java.io.ByteArrayOutputStream. 17) dos (java.io.DataOutputStream. baos)] (.writeByte dos 5) @@ -261,7 +261,7 @@ (t/is (= :max-file-size-reached (:code out)))))))) (t/deftest import-rejects-too-many-zip-entries - ;; import must reject ZIP files exceeding max-zip-entries + ;; import must reject ZIP files exceeding max-zip-entries (default-max-zip-entries) (let [profile (th/create-profile* 1) file (prepare-simple-file profile) output (tmp/tempfile :suffix ".zip")] @@ -318,7 +318,7 @@ (dissoc file :data))) (t/deftest import-rejects-oversized-object - ;; import must reject storage objects exceeding max-object-size + ;; import must reject storage objects exceeding default-max-object-size (let [profile (th/create-profile* 1) file (prepare-file-with-media profile) output (tmp/tempfile :suffix ".zip")]