From fc500d1102f21f17b462befae50326ae3a394c33 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 7 Oct 2026 17:49:38 +0000 Subject: [PATCH] :bug: Harden SVG sanitization: workers, limits and error mapping Address the review findings on the DOMPurify SVG sanitization. F1: sanitizeSvg was synchronous and blocked the service's only event loop: a ~30 MiB SVG (the old cap) froze media-processor for ~20 s and peaked at ~3.4 GiB. The sanitizer now runs in a worker_threads pool (sanitizeSvgSync is the CPU-bound core), each worker has a resourceLimits heap cap so an OOM kills the worker and not the process, and a stuck job is terminated by a timeout and respawned. The default SVG cap drops to 2 MiB and is enforced in both modes (the backend validates the declared and the real size on upload, and the real bytes on import), so a file is accepted or rejected the same way with and without media-processor. F2: an unexpected sanitizer failure is logged and reported as 503 svg-sanitization-failed instead of 400 invalid-svg-file, so a service fault is not blamed on the user's file and no internal detail leaks in the hint. F4: the remote backend rejects a 2xx response that is not an SVG instead of storing it. Also pin the backend/service wire contract with a test that posts the exact multipart the JVM builds, and pin the data: URI behavior. AI-assisted-by: deepseek-v4.1-flash --- .../memories/backend/media-sanitization.md | 12 + .serena/memories/media-processor/core.md | 32 ++- backend/src/app/binfile/common.clj | 12 + backend/src/app/config.clj | 6 + backend/src/app/media/remote.clj | 10 +- backend/src/app/media/validation.clj | 11 +- backend/src/app/rpc/commands/media.clj | 18 +- .../test/backend_tests/binfile_svg_test.clj | 28 ++- .../test/backend_tests/media_remote_test.clj | 12 + backend/test/backend_tests/media_test.clj | 14 ++ backend/test/backend_tests/rpc_media_test.clj | 45 ++++ media-processor/esbuild.config.mjs | 4 +- media-processor/src/config.ts | 8 +- media-processor/src/index.ts | 15 ++ media-processor/src/routes/svg.ts | 2 +- media-processor/src/services/svg.ts | 49 +++- media-processor/src/svg-pool.ts | 228 ++++++++++++++++++ media-processor/src/svg-worker.ts | 27 +++ media-processor/src/types.ts | 3 + media-processor/test/config.test.ts | 20 ++ media-processor/test/svg-pool.test.ts | 170 +++++++++++++ media-processor/test/svg.test.ts | 108 ++++++++- 22 files changed, 793 insertions(+), 41 deletions(-) create mode 100644 media-processor/src/svg-pool.ts create mode 100644 media-processor/src/svg-worker.ts create mode 100644 media-processor/test/svg-pool.test.ts diff --git a/.serena/memories/backend/media-sanitization.md b/.serena/memories/backend/media-sanitization.md index 9716183a6e..b6e91de9c5 100644 --- a/.serena/memories/backend/media-sanitization.md +++ b/.serena/memories/backend/media-sanitization.md @@ -24,6 +24,18 @@ Rules: - Fail closed: when the remote backend is enabled but the service fails, the exception propagates and the upload/import fails; there is no silent fallback to the weaker local filter. +- SVG has its own size cap, `:media-svg-max-file-size` (default 2MiB, env + `PENPOT_MEDIA_SVG_MAX_FILE_SIZE`), lower than `:media-max-file-size`. It is + enforced in both modes and reports `:svg-too-large`, the same code + media-processor returns, so the same file is accepted or rejected regardless of + the mode. The upload path checks the declared `:size` early + (`app.media.validation/validate-media-size!`) and the real bytes later + (`process-main-image`), so a client cannot lie about the size; the import path + checks the real bytes (`sanitize-imported-svg`). Keep it aligned with + `PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE` (`mem:media-processor/core`). +- The remote defmethod rejects a 2xx response that is not an SVG (it must start + with `= threshold) │ ├── logger.ts # Pino logger setup +│ ├── svg-pool.ts # worker_threads pool for SVG sanitization +│ ├── svg-worker.ts # worker entry point (runs sanitizeSvgSync) │ ├── middleware/ │ │ ├── auth.ts # Timing-safe shared key authentication │ │ ├── error-handler.ts # ProcessingError class, centralized error handling @@ -57,7 +59,8 @@ media-processor/ ### Resource Limits - Image: max pixels, max width/height enforced before processing -- SVG: max input size enforced before parsing (default 30MB, `PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE`); an oversized input is rejected with 413, never truncated +- SVG: max input size enforced before parsing (default 2MB, `PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE`); an oversized input is rejected with 413, never truncated +- SVG workers: `PENPOT_MEDIA_PROCESSOR_SVG_WORKERS` (default 2; 0 runs inline), `PENPOT_MEDIA_PROCESSOR_SVG_WORKER_MAX_OLD_MB` (default 512, per-worker V8 old-generation cap) and `PENPOT_MEDIA_PROCESSOR_SVG_TIMEOUT` (default 30000 ms) - Font: prlimit wraps FontForge processes with memory (AS) and CPU time limits - Concurrency: p-queue limits concurrent requests (default 10) - Upload: hybrid storage — memory for files < 10MB, disk for larger; configurable via `PENPOT_MEDIA_PROCESSOR_MEMORY_THRESHOLD` @@ -82,6 +85,18 @@ media-processor/ - `POST /api/svg/sanitize` (multipart field `file`) returns the sanitized SVG bytes with `Content-Type: image/svg+xml`; it is the remote backend of `app.media/sanitize-svg` in the JVM. +- The route never runs the sanitizer on the main thread: `sanitizeSvg` delegates + to a pool of `worker_threads` workers (`svg-pool.ts` / `svg-worker.ts`), so a + heavy SVG cannot block the service's event loop (`/api/health` included). + `sanitizeSvgSync` is the CPU-bound core and stays synchronous. With + `PENPOT_MEDIA_PROCESSOR_SVG_WORKERS=0` the pool is disabled and the sanitizer + runs inline (tests and an escape hatch). +- The pool is created at boot, queues when every worker is busy, and applies + `resourceLimits: { maxOldGenerationSizeMb }` per worker: a worker that runs out + of memory dies with an error instead of the kernel killing the process, and the + pool maps that to a 503 and respawns it. A stuck job is terminated by a timeout + and its worker respawned. Worst-case memory is roughly workers × + maxOldGenerationSizeMb. - DOMPurify is given `USE_PROFILES: {svg: true, svgFilters: true}` and `NAMESPACE: "http://www.w3.org/2000/svg"`, so it parses the document as XML/SVG the way a browser parses a standalone `.svg`. Its default allowlist fails closed @@ -95,7 +110,20 @@ media-processor/ bails on them; stripping the DOCTYPE also keeps its entities undeclared). An input that does not produce an ` size max-size) + (ex/raise :type :restriction + :code :svg-too-large + :hint (str/ffmt "the imported svg size % is greater than the maximum %" + size + max-size)))) (let [sanitized (media/sanitize-svg system (String. ^bytes raw "UTF-8")) bytes (.getBytes ^String sanitized "UTF-8")] {:bytes bytes diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index a4375c2184..3ab07d865e 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -83,6 +83,11 @@ :jobs-request-timeout (ct/duration {:minutes 2}) :media-max-file-size (* 1024 1024 30) ; 30MiB + ;; SVG has its own, lower cap: parsing it with jsdom in media-processor + ;; costs memory and time out of proportion to its byte size, so a large SVG + ;; is a denial-of-service vector for the service. Keep it aligned with + ;; PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE (see mem:backend/media-sanitization). + :media-svg-max-file-size (* 1024 1024 2) ; 2MiB :font-max-file-size (* 1024 1024 30) ; 30MiB :font-process-mem 512 ;; 512 MiB address space ceiling @@ -163,6 +168,7 @@ [:auto-file-snapshot-timeout {:optional true} ::ct/duration] [:media-max-file-size {:optional true} ::sm/int] + [:media-svg-max-file-size {:optional true} ::sm/int] [:font-max-file-size {:optional true} ::sm/int] ;; Font processing resource limits (PENPOT_FONT_PROCESS_*) diff --git a/backend/src/app/media/remote.clj b/backend/src/app/media/remote.clj index 506cb81e71..66d4431196 100644 --- a/backend/src/app/media/remote.clj +++ b/backend/src/app/media/remote.clj @@ -226,7 +226,15 @@ :mtype "image/svg+xml"}) body (:body resp)] (try - (assoc params :content (slurp body :encoding "UTF-8")) + ;; Defense in depth: the service guarantees an ` (:size upload) max-size) (ex/raise :type :restriction - :code :media-max-file-size-reached + :code code :hint (str/ffmt "the uploaded file size % is greater than the maximum %" (:size upload) max-size))) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 29e49c634c..dddb576240 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -129,11 +129,19 @@ (let [path (:path info) mtype (:mtype info) path (if (= mtype "image/svg+xml") - (let [content (slurp path) - sanitized (media/sanitize-svg cfg content) - temp-path (tmp/tempfile :prefix "penpot-svg-" :suffix ".svg" :min-age "5m")] - (spit (str temp-path) sanitized) - temp-path) + (let [content (slurp path) + size (alength (.getBytes ^String content "UTF-8")) + max-size (cf/get :media-svg-max-file-size)] + ;; The declared size is checked earlier; this is the real + ;; one, so a client cannot smuggle a bigger SVG past the cap. + (when (> size max-size) + (ex/raise :type :restriction + :code :svg-too-large + :hint (str "the svg size " size " is greater than the maximum " max-size))) + (let [sanitized (media/sanitize-svg cfg content) + temp-path (tmp/tempfile :prefix "penpot-svg-" :suffix ".svg" :min-age "5m")] + (spit (str temp-path) sanitized) + temp-path)) path) hash (sto/calculate-hash path) data (-> (sto/content path) diff --git a/backend/test/backend_tests/binfile_svg_test.clj b/backend/test/backend_tests/binfile_svg_test.clj index 07270b588a..ff4d28d3d6 100644 --- a/backend/test/backend_tests/binfile_svg_test.clj +++ b/backend/test/backend_tests/binfile_svg_test.clj @@ -118,6 +118,18 @@ (bfc/sanitize-imported-svg th/*system* {:content-type "image/svg+xml" :bucket "file-media-object"} (utf8bytes "")))))) +(t/deftest sanitize-imported-svg-rejects-over-cap + (t/testing "an imported SVG over the cap is rejected before parsing" + (let [raw (byte-array (inc (cf/get :media-svg-max-file-size))) + err (try + (bfc/sanitize-imported-svg th/*system* + {:content-type "image/svg+xml" :bucket "file-media-object"} + raw) + nil + (catch clojure.lang.ExceptionInfo e (ex-data e)))] + (t/is (= :restriction (:type err))) + (t/is (= :svg-too-large (:code err)))))) + (t/deftest sanitize-imported-svg-routes-through-media-processor (t/testing "with remote media processing on, the import sanitizes through the service" (let [served ""] @@ -433,14 +445,18 @@ (t/is (= [(:id file)] (mapv :id (th/db-query :file {:project-id (:default-project-id profile)})))))) -(def ^:private large-clean-svg - (let [pad (apply str (repeat 110000 ""))] +;; Small on purpose: the test lowers `bfc/temp-file-threshold` with +;; `with-redefs` to reach the tempfile branch, because the SVG size cap (2MiB) +;; is now the same as the real threshold and an SVG cannot exceed it. +(def ^:private tempfile-branch-svg + (let [pad (apply str (repeat 100 ""))] (str "" pad ""))) (t/deftest import-binfile-v1-roundtrips-large-svg (t/testing "objects over the tempfile threshold (tempfile branch) import fine" - (t/is (> (alength (utf8bytes large-clean-svg)) bfc/temp-file-threshold)) (let [profile (th/create-profile* 5) - [file _seed] (seed-file-with-svg-media profile 5 large-clean-svg "image/svg+xml") - [served _stored] (export-import-v1 profile file)] - (t/is (str/includes? served "circle"))))) + [file _seed] (seed-file-with-svg-media profile 5 tempfile-branch-svg "image/svg+xml")] + (with-redefs [bfc/temp-file-threshold 1024] + (t/is (> (alength (utf8bytes tempfile-branch-svg)) bfc/temp-file-threshold)) + (let [[served _stored] (export-import-v1 profile file)] + (t/is (str/includes? served "circle"))))))) diff --git a/backend/test/backend_tests/media_remote_test.clj b/backend/test/backend_tests/media_remote_test.clj index 253875f077..e46373ab91 100644 --- a/backend/test/backend_tests/media_remote_test.clj +++ b/backend/test/backend_tests/media_remote_test.clj @@ -662,3 +662,15 @@ (with-redefs [cf/flags #{:remote-media-processing} cf/get (th/config-get-mock config-mock)] (t/is (= "REMOTE" (media/sanitize-svg (mk-system) ""))))))) + +(t/deftest sanitize-svg-rejects-non-svg-response + (t/testing "a 2xx response that is not an SVG is rejected" + (with-mocks [mock {:target 'app.media.remote/service-request + :return {:status 200 + :body (ByteArrayInputStream. (.getBytes "nope" "UTF-8"))}}] + (with-redefs [cf/get (th/config-get-mock config-mock)] + (let [err (ex/try! (media.remote/process (mk-system) + {:cmd :sanitize-svg :content ""}))] + (t/is (ex/error? err)) + (t/is (= :internal (:type (ex-data err)))) + (t/is (= :invalid-sanitized-svg (:code (ex-data err))))))))) diff --git a/backend/test/backend_tests/media_test.clj b/backend/test/backend_tests/media_test.clj index 107317488e..38222a5df1 100644 --- a/backend/test/backend_tests/media_test.clj +++ b/backend/test/backend_tests/media_test.clj @@ -10,6 +10,7 @@ [app.config :as cf] [app.media :as media] [app.media.svg :as svg] + [app.media.validation :as media.v] [backend-tests.helpers :as th] [clojure.test :as t] [datoteka.fs :as fs])) @@ -221,6 +222,19 @@ (t/is (= :validation (:type (ex-data err)))) (t/is (= :invalid-svg-file (:code (ex-data err)))))))) +(t/deftest validate-media-size-svg-cap + (t/testing "SVG uses its own cap and code; other types use the general cap" + (let [cap (cf/get :media-svg-max-file-size)] + (t/is (pos? cap)) + (let [err (ex/try! (media.v/validate-media-size! {:mtype "image/svg+xml" :size (inc cap)}))] + (t/is (ex/error? err)) + (t/is (= :restriction (:type (ex-data err)))) + (t/is (= :svg-too-large (:code (ex-data err))))) + (t/is (= {:mtype "image/svg+xml" :size cap} + (media.v/validate-media-size! {:mtype "image/svg+xml" :size cap}))) + (t/is (= {:mtype "image/jpeg" :size (inc cap)} + (media.v/validate-media-size! {:mtype "image/jpeg" :size (inc cap)})))))) + (t/deftest info-invalid-image (t/testing "info on invalid image raises error" (let [path (fs/create-tempfile :prefix "penpot-test-" :suffix ".jpg")] diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 74c5ff318c..b7ede96c6d 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -1238,6 +1238,51 @@ (t/is (= served stored)) (t/is (= 1 (:call-count @mock)))))))))) +(def ^:private big-upload-svg + (str "" + (apply str (repeat 90000 "")) + "")) + +(t/deftest upload-svg-over-cap-is-rejected + (t/testing "an SVG over the svg cap is rejected on upload" + (let [prof (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + path (write-svg-tempfile big-upload-svg) + out (th/command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "big.svg" + :content {:filename "big.svg" + :path path + :mtype "image/svg+xml" + :size (alength (.getBytes big-upload-svg "UTF-8"))}})] + (t/is (th/ex-info? (:error out))) + (t/is (th/ex-of-type? (:error out) :restriction)) + (t/is (th/ex-of-code? (:error out) :svg-too-large))))) + +(t/deftest upload-svg-over-cap-with-lying-size-is-rejected + (t/testing "the real size is checked even when the declared size lies" + (let [prof (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + path (write-svg-tempfile big-upload-svg) + out (th/command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "big.svg" + :content {:filename "big.svg" + :path path + :mtype "image/svg+xml" + :size 10}})] + (t/is (th/ex-info? (:error out))) + (t/is (th/ex-of-type? (:error out) :restriction)) + (t/is (th/ex-of-code? (:error out) :svg-too-large))))) + (t/deftest upload-file-media-object-rejects-client-id (let [prof (th/create-profile* 1) file (th/create-file* 1 {:profile-id (:id prof) diff --git a/media-processor/esbuild.config.mjs b/media-processor/esbuild.config.mjs index c3534fbf02..5690b23b6e 100644 --- a/media-processor/esbuild.config.mjs +++ b/media-processor/esbuild.config.mjs @@ -1,12 +1,12 @@ import { build } from "esbuild"; await build({ - entryPoints: ["src/index.ts"], + entryPoints: ["src/index.ts", "src/svg-worker.ts"], bundle: true, platform: "node", target: "node24", format: "esm", - outfile: "dist/index.js", + outdir: "dist", external: ["sharp", "pino", "pino-pretty", "pino-loki", "dompurify", "jsdom"], banner: { js: ` diff --git a/media-processor/src/config.ts b/media-processor/src/config.ts index 8d3218fffe..657af5fe2a 100644 --- a/media-processor/src/config.ts +++ b/media-processor/src/config.ts @@ -12,7 +12,10 @@ const envSchema = z.object({ PENPOT_MEDIA_PROCESSOR_IMAGE_MAX_PIXELS: z.coerce.number().int().positive().default(128_000_000), PENPOT_MEDIA_PROCESSOR_IMAGE_MAX_WIDTH: z.coerce.number().int().positive().default(16384), PENPOT_MEDIA_PROCESSOR_IMAGE_MAX_HEIGHT: z.coerce.number().int().positive().default(16384), - PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE: z.coerce.number().int().positive().default(31457280), // 30 MB + PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE: z.coerce.number().int().positive().default(2097152), // 2 MB — see mem:media-processor/core + PENPOT_MEDIA_PROCESSOR_SVG_WORKERS: z.coerce.number().int().nonnegative().default(2), // 0 = run inline (tests/escape hatch) + PENPOT_MEDIA_PROCESSOR_SVG_WORKER_MAX_OLD_MB: z.coerce.number().int().positive().default(512), + PENPOT_MEDIA_PROCESSOR_SVG_TIMEOUT: z.coerce.number().int().positive().default(30000), PENPOT_MEDIA_PROCESSOR_FONT_PROCESS_MEM: z.coerce.number().int().positive().default(512), PENPOT_MEDIA_PROCESSOR_FONT_PROCESS_CPU_TIME: z.coerce.number().int().positive().default(30), PENPOT_MEDIA_PROCESSOR_FONT_TIMEOUT: z.coerce.number().int().positive().default(120000), @@ -53,6 +56,9 @@ export function loadConfig(): AppConfig { imageMaxWidth: parsed.PENPOT_MEDIA_PROCESSOR_IMAGE_MAX_WIDTH, imageMaxHeight: parsed.PENPOT_MEDIA_PROCESSOR_IMAGE_MAX_HEIGHT, svgMaxSize: parsed.PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE, + svgWorkers: parsed.PENPOT_MEDIA_PROCESSOR_SVG_WORKERS, + svgWorkerMaxOldMb: parsed.PENPOT_MEDIA_PROCESSOR_SVG_WORKER_MAX_OLD_MB, + svgTimeout: parsed.PENPOT_MEDIA_PROCESSOR_SVG_TIMEOUT, fontProcessMem: parsed.PENPOT_MEDIA_PROCESSOR_FONT_PROCESS_MEM, fontProcessCpuTime: parsed.PENPOT_MEDIA_PROCESSOR_FONT_PROCESS_CPU_TIME, fontTimeout: parsed.PENPOT_MEDIA_PROCESSOR_FONT_TIMEOUT, diff --git a/media-processor/src/index.ts b/media-processor/src/index.ts index 484ef67818..867fe33078 100644 --- a/media-processor/src/index.ts +++ b/media-processor/src/index.ts @@ -13,6 +13,7 @@ import { loggingMiddleware } from "./middleware/logging.js"; import { configureImageLimits } from "./services/image.js"; import { configureFontLimits } from "./services/font.js"; import { configureSvgLimits } from "./services/svg.js"; +import { configureSvgPool, shutdownSvgPool } from "./svg-pool.js"; import { configureUploadLimits } from "./upload.js"; import sharp from "sharp"; @@ -43,6 +44,12 @@ configureFontLimits({ configureSvgLimits({ maxSize: config.svgMaxSize }); +configureSvgPool({ + workers: config.svgWorkers, + maxOldGenerationSizeMb: config.svgWorkerMaxOldMb, + timeout: config.svgTimeout, +}); + configureUploadLimits({ maxFileSize: config.maxFileSize, memoryThreshold: config.memoryThreshold }); const queueMiddleware = createQueueMiddleware(config.maxConcurrentRequests); @@ -61,4 +68,12 @@ app.listen(config.port, config.host, () => { logger.info(`media-processor listening on ${config.host}:${config.port}`); }); +// Terminate the SVG workers on shutdown so in-flight sanitizations fail fast +// instead of keeping the pool's timers alive until the process dies. +for (const signal of ["SIGTERM", "SIGINT"] as const) { + process.on(signal, () => { + void shutdownSvgPool().finally(() => process.exit(0)); + }); +} + export { app }; diff --git a/media-processor/src/routes/svg.ts b/media-processor/src/routes/svg.ts index dd83fb9489..a267395f62 100644 --- a/media-processor/src/routes/svg.ts +++ b/media-processor/src/routes/svg.ts @@ -27,7 +27,7 @@ export function createSvgRoutes(): IRouter { } res.locals.opMeta = `size=${req.file!.size}`; - const clean = sanitizeSvg((await getFileBuffer(req.file!)).toString("utf8")); + const clean = await sanitizeSvg((await getFileBuffer(req.file!)).toString("utf8")); res.setHeader("Content-Type", "image/svg+xml"); res.send(Buffer.from(clean, "utf8")); diff --git a/media-processor/src/services/svg.ts b/media-processor/src/services/svg.ts index a0f3a304d8..fd9f7c386d 100644 --- a/media-processor/src/services/svg.ts +++ b/media-processor/src/services/svg.ts @@ -1,11 +1,15 @@ +import { isMainThread } from "node:worker_threads"; import createDOMPurify, { type WindowLike } from "dompurify"; import { JSDOM } from "jsdom"; -import { throwValidation, throwRestriction } from "./errors.js"; +import { throwValidation, throwRestriction, throwProcessing } from "./errors.js"; import { ProcessingError } from "../middleware/error-handler.js"; +import { createLogger } from "../logger.js"; +import { getSvgPoolOptions, runInSvgPool } from "../svg-pool.js"; +const logger = createLogger("svg"); const SVG_NAMESPACE = "http://www.w3.org/2000/svg"; -let svgMaxSize = 30 * 1024 * 1024; +let svgMaxSize = 2 * 1024 * 1024; export function configureSvgLimits(opts: { maxSize: number }): void { svgMaxSize = opts.maxSize; @@ -20,7 +24,7 @@ export function getSvgMaxSize(): number { // the sanitizer return nothing, so they are stripped first. Stripping the // DOCTYPE is also what keeps its entities from ever being declared. A prolog // the stripper misses still fails closed: DOMPurify returns an empty document -// and `sanitizeSvg` rejects it. +// and `sanitizeSvgSync` rejects it. function stripProlog(svg: string): string { return svg .replace(/^\uFEFF/, "") @@ -32,8 +36,8 @@ function stripProlog(svg: string): string { const SVG_ROOT = /^]/i; /** - * Sanitize an SVG document with DOMPurify over jsdom and return the clean - * SVG as UTF-8 text. + * Sanitize an SVG document synchronously with DOMPurify over jsdom and return + * the clean SVG as UTF-8 text. * * DOMPurify works from an allowlist: every element and attribute it does not * explicitly allow is removed, so it fails closed. It is given the SVG profile @@ -44,11 +48,16 @@ const SVG_ROOT = /^]/i; * DOMPurify keeps state on its window, and reusing one window in a long-lived * process leaks memory and degrades latency without bound. * + * This is the CPU-bound core. The HTTP route does not call it directly: it runs + * in a `worker_threads` worker through `sanitizeSvg`, so the service's event + * loop is never blocked by it. + * * Throws a `ProcessingError` (413 `svg-too-large`) when the input exceeds the - * configured limit, and (400 `invalid-svg-file`) when the input is not a - * well-formed SVG document. + * configured limit, (400 `invalid-svg-file`) when the input is not a + * well-formed SVG document, and (503 `svg-sanitization-failed`) when anything + * else goes wrong. */ -export function sanitizeSvg(svg: string): string { +export function sanitizeSvgSync(svg: string): string { const size = Buffer.byteLength(svg, "utf8"); if (size > svgMaxSize) { throwRestriction("svg-too-large", `SVG size ${size} exceeds the maximum of ${svgMaxSize} bytes`); @@ -71,8 +80,30 @@ export function sanitizeSvg(svg: string): string { if (err instanceof ProcessingError) { throw err; } - throwValidation("invalid-svg-file", `Failed to sanitize svg: ${(err as Error).message}`); + // A failure of the sanitizer itself (a bug, an OOM, a DOMPurify regression) + // is not the user's file being invalid: report it as a service failure and + // keep the technical detail out of the hint. Inside a worker this is not + // logged: the pool logs it on the main thread when the result comes back, + // so a worker never initializes the (transport-spawning) logger. + if (isMainThread) { + logger.error({ err }, "unexpected svg sanitization failure"); + } + throwProcessing("svg-sanitization-failed", "SVG sanitization failed unexpectedly"); } finally { dom.window.close(); } } + +/** + * Sanitize an SVG document, off the event loop when a worker pool is + * configured. Falls back to running `sanitizeSvgSync` inline when the pool is + * disabled (`PENPOT_MEDIA_PROCESSOR_SVG_WORKERS=0`) or not configured, which is + * what tests use. + */ +export function sanitizeSvg(svg: string): Promise { + const options = getSvgPoolOptions(); + if (options === null || options.workers <= 0) { + return Promise.resolve().then(() => sanitizeSvgSync(svg)); + } + return runInSvgPool(svg); +} diff --git a/media-processor/src/svg-pool.ts b/media-processor/src/svg-pool.ts new file mode 100644 index 0000000000..dd383e66ea --- /dev/null +++ b/media-processor/src/svg-pool.ts @@ -0,0 +1,228 @@ +import { Worker } from "node:worker_threads"; +import { ProcessingError } from "./middleware/error-handler.js"; +import { createLogger } from "./logger.js"; +import type { AppError } from "./types.js"; + +const logger = createLogger("svg-pool"); + +/** + * The slice of `worker_threads.Worker` the pool uses. Keeping it narrow lets + * tests inject a fake worker and exercise the queue, the timeout and the + * failure handling without real threads. + */ +export interface SvgWorker { + postMessage(value: unknown): void; + terminate(): Promise; + on(event: "message", listener: (value: unknown) => void): this; + on(event: "error", listener: (err: Error) => void): this; + on(event: "exit", listener: (code: number) => void): this; +} + +export interface SvgPoolOptions { + workers: number; + maxOldGenerationSizeMb: number; + timeout: number; +} + +interface Job { + svg: string; + resolve: (clean: string) => void; + reject: (err: Error) => void; + timer: NodeJS.Timeout | null; +} + +interface PooledWorker { + worker: SvgWorker; + job: Job | null; + terminating: boolean; +} + +interface PoolState { + options: SvgPoolOptions; + workers: PooledWorker[]; + queue: Job[]; + stopped: boolean; +} + +type SvgWorkerResult = { ok: true; clean: string } | { ok: false; statusCode: number; body: AppError }; + +let state: PoolState | null = null; +let createWorker: () => SvgWorker = defaultWorkerFactory; + +function workerUrl(): URL { + // Under `tsx` the module is a `.ts` source and the worker must be loaded as + // `.ts`; in the bundle it is the emitted `.js`. `import.meta.url` tells them + // apart. + const extension = import.meta.url.endsWith(".ts") ? "ts" : "js"; + return new URL(`./svg-worker.${extension}`, import.meta.url); +} + +function defaultWorkerFactory(): SvgWorker { + const options = state!.options; + return new Worker(workerUrl(), { + // Node inherits the parent flags by default; setting it explicitly keeps + // the dev loader (`tsx`) available in the worker. + execArgv: process.execArgv, + resourceLimits: { maxOldGenerationSizeMb: options.maxOldGenerationSizeMb }, + }) as unknown as SvgWorker; +} + +/** Replace the worker factory. Tests use it to inject a fake; `null` restores the real one. */ +export function setSvgWorkerFactory(factory: (() => SvgWorker) | null): void { + createWorker = factory ?? defaultWorkerFactory; +} + +/** Create the pool. `workers <= 0` disables it (the sanitizer runs inline). */ +export function configureSvgPool(options: SvgPoolOptions): void { + if (state !== null) { + throw new Error("svg pool is already configured"); + } + state = { options, workers: [], queue: [], stopped: false }; + for (let i = 0; i < options.workers; i++) { + state.workers.push(spawnWorker()); + } +} + +export function getSvgPoolOptions(): SvgPoolOptions | null { + return state?.options ?? null; +} + +/** Queue an SVG for sanitization in a worker. */ +export function runInSvgPool(svg: string): Promise { + const current = state; + if (current === null || current.stopped) { + return Promise.reject(stoppedError()); + } + return new Promise((resolve, reject) => { + current.queue.push({ svg, resolve, reject, timer: null }); + dispatch(); + }); +} + +/** Terminate every worker and reject whatever is pending. */ +export async function shutdownSvgPool(): Promise { + const current = state; + if (current === null) { + return; + } + current.stopped = true; + state = null; + + for (const job of current.queue.splice(0)) { + job.reject(stoppedError()); + } + + const workers = current.workers; + current.workers = []; + await Promise.all( + workers.map((pooled) => { + pooled.terminating = true; + const job = pooled.job; + pooled.job = null; + if (job !== null) { + clearJobTimer(job); + job.reject(stoppedError()); + } + return pooled.worker.terminate().catch(() => 0); + }) + ); +} + +function spawnWorker(): PooledWorker { + const worker = createWorker(); + const pooled: PooledWorker = { worker, job: null, terminating: false }; + worker.on("message", (value) => onMessage(pooled, value)); + worker.on("error", (err) => onWorkerError(pooled, err)); + worker.on("exit", (code) => onWorkerExit(pooled, code)); + return pooled; +} + +function dispatch(): void { + const current = state; + if (current === null || current.stopped) { + return; + } + while (current.queue.length > 0) { + const pooled = current.workers.find((w) => w.job === null && !w.terminating); + if (pooled === undefined) { + return; + } + const job = current.queue.shift()!; + pooled.job = job; + job.timer = setTimeout(() => { + job.timer = null; + failWorker(pooled, new ProcessingError(503, { type: "internal", code: "svg-timeout" })); + }, current.options.timeout); + pooled.worker.postMessage(job.svg); + } +} + +function onMessage(pooled: PooledWorker, value: unknown): void { + const job = pooled.job; + if (job === null) { + return; + } + clearJobTimer(job); + pooled.job = null; + + const result = value as SvgWorkerResult; + if (result.ok) { + job.resolve(result.clean); + } else { + if (result.body.code === "svg-sanitization-failed") { + logger.error("svg worker reported an unexpected sanitization failure"); + } + job.reject(new ProcessingError(result.statusCode, result.body)); + } + dispatch(); +} + +function onWorkerError(pooled: PooledWorker, err: Error): void { + logger.error({ err }, "svg worker error"); + failWorker(pooled, new ProcessingError(503, { type: "internal", code: "svg-worker-failed" })); +} + +function onWorkerExit(pooled: PooledWorker, code: number): void { + if (pooled.terminating) { + return; + } + logger.error({ code }, "svg worker exited unexpectedly"); + failWorker(pooled, new ProcessingError(503, { type: "internal", code: "svg-worker-exited" })); +} + +/** Reject the worker's in-flight job (if any) and replace the worker. */ +function failWorker(pooled: PooledWorker, err: ProcessingError): void { + if (pooled.terminating) { + return; + } + pooled.terminating = true; + + const job = pooled.job; + pooled.job = null; + if (job !== null) { + clearJobTimer(job); + job.reject(err); + } + + void pooled.worker.terminate().catch(() => 0); + + const current = state; + if (current !== null && !current.stopped) { + const index = current.workers.indexOf(pooled); + if (index !== -1) { + current.workers[index] = spawnWorker(); + } + dispatch(); + } +} + +function clearJobTimer(job: Job): void { + if (job.timer !== null) { + clearTimeout(job.timer); + job.timer = null; + } +} + +function stoppedError(): ProcessingError { + return new ProcessingError(503, { type: "internal", code: "svg-pool-stopped" }); +} diff --git a/media-processor/src/svg-worker.ts b/media-processor/src/svg-worker.ts new file mode 100644 index 0000000000..22dbf267b6 --- /dev/null +++ b/media-processor/src/svg-worker.ts @@ -0,0 +1,27 @@ +import { parentPort } from "node:worker_threads"; +import { sanitizeSvgSync } from "./services/svg.js"; +import { ProcessingError } from "./middleware/error-handler.js"; + +// Entry point of the SVG sanitization worker. `svg-pool` spawns this file and +// sends the SVG to sanitize through `postMessage`; the answer is always a +// plain, structured-cloneable object, never a class instance. +const port = parentPort; +if (port === null) { + throw new Error("svg-worker must be started as a worker thread"); +} + +port.on("message", (svg: string) => { + try { + port.postMessage({ ok: true, clean: sanitizeSvgSync(svg) }); + } catch (err) { + if (err instanceof ProcessingError) { + port.postMessage({ ok: false, statusCode: err.statusCode, body: err.errorBody }); + } else { + port.postMessage({ + ok: false, + statusCode: 503, + body: { type: "internal", code: "svg-sanitization-failed" }, + }); + } + } +}); diff --git a/media-processor/src/types.ts b/media-processor/src/types.ts index 681af6a619..f421de2b4c 100644 --- a/media-processor/src/types.ts +++ b/media-processor/src/types.ts @@ -11,6 +11,9 @@ export interface AppConfig { imageMaxWidth: number; imageMaxHeight: number; svgMaxSize: number; + svgWorkers: number; + svgWorkerMaxOldMb: number; + svgTimeout: number; fontProcessMem: number; fontProcessCpuTime: number; fontTimeout: number; diff --git a/media-processor/test/config.test.ts b/media-processor/test/config.test.ts index 019cbd4947..7b17161b56 100644 --- a/media-processor/test/config.test.ts +++ b/media-processor/test/config.test.ts @@ -20,6 +20,10 @@ describe("loadConfig", () => { expect(config.requestTimeout).toBe(180000); expect(config.maxFileSize).toBe(367001600); expect(config.memoryThreshold).toBe(10485760); + expect(config.svgMaxSize).toBe(2097152); + expect(config.svgWorkers).toBe(2); + expect(config.svgWorkerMaxOldMb).toBe(512); + expect(config.svgTimeout).toBe(30000); }); it("accepts valid config with all fields set", () => { @@ -101,6 +105,22 @@ describe("loadConfig", () => { expect(() => loadConfig()).toThrow(); }); + it("rejects negative svg max size", () => { + process.env.PENPOT_MEDIA_PROCESSOR_SVG_MAX_SIZE = "-1"; + expect(() => loadConfig()).toThrow(); + }); + + it("accepts svg workers=0 (inline mode)", () => { + process.env.PENPOT_MEDIA_PROCESSOR_SVG_WORKERS = "0"; + const config = loadConfig(); + expect(config.svgWorkers).toBe(0); + }); + + it("rejects negative svg worker heap", () => { + process.env.PENPOT_MEDIA_PROCESSOR_SVG_WORKER_MAX_OLD_MB = "-1"; + expect(() => loadConfig()).toThrow(); + }); + it("rejects negative font process mem", () => { process.env.PENPOT_MEDIA_PROCESSOR_FONT_PROCESS_MEM = "-512"; expect(() => loadConfig()).toThrow(); diff --git a/media-processor/test/svg-pool.test.ts b/media-processor/test/svg-pool.test.ts new file mode 100644 index 0000000000..6afc52f102 --- /dev/null +++ b/media-processor/test/svg-pool.test.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, afterEach, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import { + configureSvgPool, + shutdownSvgPool, + runInSvgPool, + setSvgWorkerFactory, + type SvgWorker, +} from "../src/svg-pool.js"; +import { ProcessingError } from "../src/middleware/error-handler.js"; + +class FakeWorker extends EventEmitter { + public readonly posted: unknown[] = []; + public terminated = false; + + public postMessage(value: unknown): void { + this.posted.push(value); + } + + public async terminate(): Promise { + this.terminated = true; + this.emit("exit", 0); + return 0; + } + + public reply(value: unknown): void { + this.emit("message", value); + } + + public fail(err: Error): void { + this.emit("error", err); + } + + public exitUnexpectedly(code = 1): void { + this.emit("exit", code); + } +} + +let created: FakeWorker[] = []; + +function installFakeFactory(): void { + created = []; + setSvgWorkerFactory(() => { + const worker = new FakeWorker(); + created.push(worker); + return worker as unknown as SvgWorker; + }); +} + +async function rejection(promise: Promise): Promise { + try { + await promise; + } catch (err) { + return err as ProcessingError; + } + throw new Error("expected the promise to reject"); +} + +afterEach(async () => { + await shutdownSvgPool(); + setSvgWorkerFactory(null); +}); + +describe("svg pool: dispatch", () => { + it("runs jobs on idle workers and queues the rest", async () => { + installFakeFactory(); + configureSvgPool({ workers: 2, maxOldGenerationSizeMb: 512, timeout: 1000 }); + + const first = runInSvgPool("1"); + const second = runInSvgPool("2"); + expect(created).toHaveLength(2); + expect(created[0].posted).toEqual(["1"]); + expect(created[1].posted).toEqual(["2"]); + + const third = runInSvgPool("3"); + expect(created[0].posted).toHaveLength(1); + + created[0].reply({ ok: true, clean: "A" }); + await expect(first).resolves.toBe("A"); + expect(created[0].posted).toEqual(["1", "3"]); + + created[1].reply({ ok: true, clean: "B" }); + created[0].reply({ ok: true, clean: "C" }); + await expect(second).resolves.toBe("B"); + await expect(third).resolves.toBe("C"); + }); + + it("rejects with the error body the worker reports", async () => { + installFakeFactory(); + configureSvgPool({ workers: 1, maxOldGenerationSizeMb: 512, timeout: 1000 }); + + const job = runInSvgPool(""); + created[0].reply({ + ok: false, + statusCode: 400, + body: { type: "validation", code: "invalid-svg-file" }, + }); + + const err = await rejection(job); + expect(err).toBeInstanceOf(ProcessingError); + expect(err.statusCode).toBe(400); + expect(err.errorBody.code).toBe("invalid-svg-file"); + }); +}); + +describe("svg pool: worker failure", () => { + it("rejects the job and respawns when a worker errors", async () => { + installFakeFactory(); + configureSvgPool({ workers: 1, maxOldGenerationSizeMb: 512, timeout: 1000 }); + + const job = runInSvgPool(""); + created[0].fail(new Error("boom")); + + const err = await rejection(job); + expect(err.statusCode).toBe(503); + expect(err.errorBody.code).toBe("svg-worker-failed"); + expect(created).toHaveLength(2); + }); + + it("rejects the job and respawns when a worker exits unexpectedly", async () => { + installFakeFactory(); + configureSvgPool({ workers: 1, maxOldGenerationSizeMb: 512, timeout: 1000 }); + + const job = runInSvgPool(""); + created[0].exitUnexpectedly(); + + const err = await rejection(job); + expect(err.statusCode).toBe(503); + expect(err.errorBody.code).toBe("svg-worker-exited"); + expect(created).toHaveLength(2); + }); +}); + +describe("svg pool: timeout", () => { + it("terminates and respawns a worker that does not answer in time", async () => { + vi.useFakeTimers(); + try { + installFakeFactory(); + configureSvgPool({ workers: 1, maxOldGenerationSizeMb: 512, timeout: 1000 }); + + const job = runInSvgPool(""); + const settled = rejection(job); + await vi.advanceTimersByTimeAsync(1000); + + const err = await settled; + expect(err.statusCode).toBe(503); + expect(err.errorBody.code).toBe("svg-timeout"); + expect(created[0].terminated).toBe(true); + expect(created).toHaveLength(2); + } finally { + vi.useRealTimers(); + } + }); +}); + +describe("svg pool: shutdown", () => { + it("rejects pending jobs and terminates the workers", async () => { + installFakeFactory(); + configureSvgPool({ workers: 1, maxOldGenerationSizeMb: 512, timeout: 1000 }); + + const job = runInSvgPool(""); + const settled = rejection(job); + await shutdownSvgPool(); + + const err = await settled; + expect(err.statusCode).toBe(503); + expect(err.errorBody.code).toBe("svg-pool-stopped"); + expect(created[0].terminated).toBe(true); + }); +}); diff --git a/media-processor/test/svg.test.ts b/media-processor/test/svg.test.ts index c376e75a80..7c6db5ae87 100644 --- a/media-processor/test/svg.test.ts +++ b/media-processor/test/svg.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, beforeAll, afterAll, afterEach } from "vitest"; import request from "supertest"; import express from "express"; -import { sanitizeSvg, configureSvgLimits } from "../src/services/svg.js"; +import { sanitizeSvg, sanitizeSvgSync, configureSvgLimits } from "../src/services/svg.js"; import { createSvgRoutes } from "../src/routes/svg.js"; import { errorHandler, ProcessingError } from "../src/middleware/error-handler.js"; import { timeoutMiddleware } from "../src/middleware/timeout.js"; @@ -10,7 +10,7 @@ import { createQueueMiddleware } from "../src/middleware/queue.js"; import { configureUploadLimits } from "../src/upload.js"; const SVG_NS = "http://www.w3.org/2000/svg"; -const DEFAULT_MAX_SIZE = 30 * 1024 * 1024; +const DEFAULT_MAX_SIZE = 2 * 1024 * 1024; function svg(inner: string, attrs = `xmlns="${SVG_NS}" width="100" height="100"`): string { return `${inner}`; @@ -105,7 +105,7 @@ const MALICIOUS: Array<{ name: string; svg: string; forbidden: RegExp }> = [ describe("sanitizeSvg: removes dangerous content", () => { it.each(MALICIOUS)("removes $name", ({ svg: dirty, forbidden }) => { - const clean = sanitizeSvg(dirty); + const clean = sanitizeSvgSync(dirty); expect(clean).toMatch(/^]/i); expect(clean).not.toMatch(forbidden); expectNoDangerousOutput(clean); @@ -130,7 +130,7 @@ describe("sanitizeSvg: keeps legitimate SVG", () => { `hi`, `xmlns="${SVG_NS}" viewBox="0 0 10 10"` ); - const clean = sanitizeSvg(dirty); + const clean = sanitizeSvgSync(dirty); expect(clean).toMatch(/^]/i); expect(clean).toContain(" { }); it("keeps a safe relative/absolute href", () => { - const clean = sanitizeSvg(svg(``)); + const clean = sanitizeSvgSync(svg(``)); expect(clean).toContain('href="https://example.com/a.png"'); }); it("accepts a document with a leading UTF-8 BOM", () => { - const clean = sanitizeSvg(`\uFEFF${svg(``)}`); + const clean = sanitizeSvgSync(`\uFEFF${svg(``)}`); expect(clean).toMatch(/^]/i); }); it("accepts a document with an XML declaration and leading whitespace", () => { - const clean = sanitizeSvg( + const clean = sanitizeSvgSync( `\n` ); expect(clean).toMatch(/^]/i); @@ -162,14 +162,14 @@ describe("sanitizeSvg: keeps legitimate SVG", () => { it("accepts a document with a DOCTYPE", () => { const dirty = `${svg(``)}`; - const clean = sanitizeSvg(dirty); + const clean = sanitizeSvgSync(dirty); expect(clean).toMatch(/^]/i); expect(clean).toContain(" { const dirty = `]>${svg(``)}`; - const clean = sanitizeSvg(dirty); + const clean = sanitizeSvgSync(dirty); expect(clean).not.toContain("passwd"); expect(clean).not.toContain("&xxe;"); }); @@ -190,7 +190,7 @@ describe("sanitizeSvg: rejects invalid input", () => { ]; it.each(INVALID)("rejects $name with invalid-svg-file", ({ svg: dirty }) => { - expectProcessingError(() => sanitizeSvg(dirty), 400, "invalid-svg-file"); + expectProcessingError(() => sanitizeSvgSync(dirty), 400, "invalid-svg-file"); }); }); @@ -204,21 +204,74 @@ describe("sanitizeSvg: limits and stability", () => { it("rejects an input larger than the configured maximum", () => { configureSvgLimits({ maxSize: 100 }); const big = svg(`${"".repeat(20)}`); - expectProcessingError(() => sanitizeSvg(big), 413, "svg-too-large"); + expectProcessingError(() => sanitizeSvgSync(big), 413, "svg-too-large"); }); it("stays stable over many consecutive calls", () => { const dirty = svg(``); for (let i = 0; i < 150; i++) { - expect(sanitizeSvg(dirty)).toMatch(/^]/i); + expect(sanitizeSvgSync(dirty)).toMatch(/^]/i); } }); }); +// --------------------------------------------------------------------------- +// sanitizeSvg: async wrapper and internal failures +// --------------------------------------------------------------------------- + +describe("sanitizeSvg: async wrapper", () => { + it("runs inline when no pool is configured", async () => { + const clean = await sanitizeSvg(svg(``)); + expect(clean).toMatch(/^]/i); + expect(clean).not.toMatch(/