From 9336962a773280a96ce14294995db7b3a1b07240 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Sat, 17 Oct 2026 14:54:23 +0200 Subject: [PATCH 1/4] :rewind: Backport docker changes from develop --- docker/devenv/Dockerfile | 7 +++++-- docker/imagemagick/Dockerfile | 6 +++--- docker/images/Dockerfile.backend | 6 +++--- docker/images/Dockerfile.exporter | 4 ++-- docker/images/Dockerfile.frontend | 2 +- docker/images/Dockerfile.mcp | 4 ++-- docker/images/Dockerfile.media-processor | 2 +- docker/images/Dockerfile.storybook | 2 +- 8 files changed, 18 insertions(+), 15 deletions(-) diff --git a/docker/devenv/Dockerfile b/docker/devenv/Dockerfile index 4cc0e0817e..ba65456579 100644 --- a/docker/devenv/Dockerfile +++ b/docker/devenv/Dockerfile @@ -1,4 +1,4 @@ -FROM dhi.io/debian-base:trixie-debian13-dev AS base +FROM dhi.io/debian-base:trixie-debian13-dev@sha256:7b1d6522b9a4e72e2ca3537c031ca3650c654ec81c181447bf952069eb827226 AS base ENV LANG='C.UTF-8' \ LC_ALL='C.UTF-8' \ @@ -76,12 +76,14 @@ RUN set -eux; \ aarch64|arm64) \ OPENSSL_ARCH='linux-aarch64'; \ BINARY_URL="https://nodejs.org/dist/${NODE_VERSION}/node-${NODE_VERSION}-linux-arm64.tar.gz"; \ + NODE_SHA256='b7e7991d5407815adfa74a2a9cd4db683552c8ce32ff4a295c22e972849c3f5d'; \ PNPM_ARCH='arm64'; \ PNPM_SHA256='cfc780b9e690549e6633e1045fb1fb6ef84e390711e2099e6ccbae91c32456ea'; \ ;; \ amd64|x86_64) \ OPENSSL_ARCH='linux-x86_64'; \ BINARY_URL="https://nodejs.org/dist/${NODE_VERSION}/node-${NODE_VERSION}-linux-x64.tar.gz"; \ + NODE_SHA256='28ea0a438de9a13bd7891fd758eee6c7819ee8af5442dfd8d45bd65e56d4228c'; \ PNPM_ARCH='x64'; \ PNPM_SHA256='3910c3852136bed5bc1511b9137a45fd4357ed6b3970d9d08893641e9005867e'; \ ;; \ @@ -91,6 +93,7 @@ RUN set -eux; \ ;; \ esac; \ curl -LfsSo /tmp/nodejs.tar.gz ${BINARY_URL}; \ + echo "${NODE_SHA256} */tmp/nodejs.tar.gz" | sha256sum -c -; \ mkdir -p /opt/node; \ cd /opt/node; \ tar -xf /tmp/nodejs.tar.gz --strip-components=1; \ @@ -516,7 +519,7 @@ ENV LANG='C.UTF-8' \ SERENA_CONTEXT="claude-code" \ PATH="/opt/jdk/bin:/opt/gh/bin:/opt/utils/bin:/opt/clojure/bin:/opt/node/bin:/opt/imagick/bin:/opt/cargo/bin:$PATH" -COPY --from=penpotapp/imagemagick:7.1.2-27-1 /opt/imagick /opt/imagick +COPY --from=penpotapp/imagemagick:7.1.2-32 /opt/imagick /opt/imagick COPY --from=setup-jvm /opt/jdk /opt/jdk COPY --from=setup-jvm /opt/clojure /opt/clojure COPY --from=setup-node /opt/node /opt/node diff --git a/docker/imagemagick/Dockerfile b/docker/imagemagick/Dockerfile index 0ab72b37af..a0323a1c63 100644 --- a/docker/imagemagick/Dockerfile +++ b/docker/imagemagick/Dockerfile @@ -1,4 +1,4 @@ -FROM dhi.io/debian-base:trixie-debian13-dev AS build +FROM dhi.io/debian-base:trixie-debian13-dev@sha256:7b1d6522b9a4e72e2ca3537c031ca3650c654ec81c181447bf952069eb827226 AS build LABEL maintainer="Penpot " ENV LANG='C.UTF-8' \ @@ -6,7 +6,7 @@ ENV LANG='C.UTF-8' \ DEBIAN_FRONTEND=noninteractive \ TZ=Etc/UTC -ARG IMAGEMAGICK_VERSION=7.1.2-27 +ARG IMAGEMAGICK_VERSION=7.1.2-32 RUN set -e; \ apt-get -qq update; \ @@ -74,7 +74,7 @@ RUN set -ex; \ rm -rf /var/lib/apt/lists/* -FROM dhi.io/debian-base:trixie-debian13 AS image +FROM dhi.io/debian-base:trixie-debian13@sha256:20079b51710f0397da5e056bfc7156b6aafc7ff3aa4ef4cbcb0b0f1df99cd8c4 AS image LABEL maintainer="Penpot " ENV LANG='C.UTF-8' \ diff --git a/docker/images/Dockerfile.backend b/docker/images/Dockerfile.backend index e2f1f15179..3d74f8be43 100644 --- a/docker/images/Dockerfile.backend +++ b/docker/images/Dockerfile.backend @@ -1,4 +1,4 @@ -FROM dhi.io/debian-base:trixie-debian13-dev AS build +FROM dhi.io/debian-base:trixie-debian13-dev@sha256:7b1d6522b9a4e72e2ca3537c031ca3650c654ec81c181447bf952069eb827226 AS build LABEL maintainer="Penpot " ENV LANG='C.UTF-8' \ @@ -53,7 +53,7 @@ RUN set -eux; \ --output /opt/jre; -FROM dhi.io/debian-base:trixie-debian13-dev AS image +FROM dhi.io/debian-base:trixie-debian13-dev@sha256:7b1d6522b9a4e72e2ca3537c031ca3650c654ec81c181447bf952069eb827226 AS image LABEL maintainer="Penpot " ENV LANG='C.UTF-8' \ @@ -106,7 +106,7 @@ RUN set -ex; \ chown -R penpot:penpot /opt/data; COPY --from=build /opt/jre /opt/jre -COPY --from=penpotapp/imagemagick:7.1.2-27-1 /opt/imagick /opt/imagick +COPY --from=penpotapp/imagemagick:7.1.2-32 /opt/imagick /opt/imagick COPY files/imagemagick-policy.xml /opt/imagick/etc/ImageMagick-7/policy.xml diff --git a/docker/images/Dockerfile.exporter b/docker/images/Dockerfile.exporter index dd28f7cb43..4bbb65594f 100644 --- a/docker/images/Dockerfile.exporter +++ b/docker/images/Dockerfile.exporter @@ -1,4 +1,4 @@ -FROM dhi.io/node:24.22.0-debian13-dev +FROM dhi.io/node:24.22.0-debian13-dev@sha256:39f55475c45417de6a7be329834ce5b19bdda6c347ecd98e42b89d76bf4afa82 LABEL maintainer="Penpot " ENV LANG=en_US.UTF-8 \ @@ -88,7 +88,7 @@ RUN set -ex; \ ARG BUNDLE_PATH="./bundle-exporter/" COPY --chown=penpot:penpot $BUNDLE_PATH /opt/penpot/exporter/ -COPY --from=penpotapp/imagemagick:7.1.2-27-1 /opt/imagick /opt/imagick +COPY --from=penpotapp/imagemagick:7.1.2-32 /opt/imagick /opt/imagick WORKDIR /opt/penpot/exporter diff --git a/docker/images/Dockerfile.frontend b/docker/images/Dockerfile.frontend index 7c6547779e..8213d88490 100644 --- a/docker/images/Dockerfile.frontend +++ b/docker/images/Dockerfile.frontend @@ -1,4 +1,4 @@ -FROM dhi.io/nginx:1.31.6-debian13-dev +FROM dhi.io/nginx:1.31.7-debian13-dev@sha256:10c0037149e51fec759fb6006d5ab8bc8343f0d718c86841bd510848cc5eeef4 LABEL maintainer="Penpot " ENV DEBIAN_FRONTEND=noninteractive diff --git a/docker/images/Dockerfile.mcp b/docker/images/Dockerfile.mcp index 6f91c42991..f73a35696b 100644 --- a/docker/images/Dockerfile.mcp +++ b/docker/images/Dockerfile.mcp @@ -1,4 +1,4 @@ -FROM dhi.io/node:24.22.0-debian13-dev AS build +FROM dhi.io/node:24.22.0-debian13-dev@sha256:39f55475c45417de6a7be329834ce5b19bdda6c347ecd98e42b89d76bf4afa82 AS build LABEL maintainer="Penpot " ENV DEBIAN_FRONTEND=noninteractive @@ -42,7 +42,7 @@ RUN set -eux; \ RUN ./setup -FROM dhi.io/node:24.22.0-debian13 AS image +FROM dhi.io/node:24.22.0-debian13@sha256:c6ddacf90691f33734423b06682a63b27abeeafee294830455e91c79614ece8b AS image LABEL maintainer="Penpot " ENV LANG=en_US.UTF-8 \ diff --git a/docker/images/Dockerfile.media-processor b/docker/images/Dockerfile.media-processor index eefa694905..1590dcf6a1 100644 --- a/docker/images/Dockerfile.media-processor +++ b/docker/images/Dockerfile.media-processor @@ -1,5 +1,5 @@ # syntax=docker/dockerfile:1 -FROM dhi.io/node:24.22.0-debian13-dev +FROM dhi.io/node:24.22.0-debian13-dev@sha256:39f55475c45417de6a7be329834ce5b19bdda6c347ecd98e42b89d76bf4afa82 LABEL maintainer="Penpot " ENV LANG=en_US.UTF-8 \ diff --git a/docker/images/Dockerfile.storybook b/docker/images/Dockerfile.storybook index 7428803510..2b259ffb61 100644 --- a/docker/images/Dockerfile.storybook +++ b/docker/images/Dockerfile.storybook @@ -1,4 +1,4 @@ -FROM dhi.io/nginx:1.31.6-alpine3.24 +FROM dhi.io/nginx:1.31.7-alpine3.24@sha256:6d3811fbc76c5d0652752b39a003769759ccfeb2c1e874bacf99e3d44e5da3b9 LABEL maintainer="Penpot " ARG BUNDLE_PATH="./bundle-storybook/" From 07e085c2cf4c8b323b0a47535d4ae74e9144b48a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Sat, 17 Oct 2026 14:55:22 +0200 Subject: [PATCH 2/4] :revind: Backport several updates for .github/workflows from develop --- .github/workflows/commit-checker.yml | 2 +- .github/workflows/tests-e2e.yml | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/commit-checker.yml b/.github/workflows/commit-checker.yml index 0de7fe8975..15790e72a9 100644 --- a/.github/workflows/commit-checker.yml +++ b/.github/workflows/commit-checker.yml @@ -56,7 +56,7 @@ jobs: # always the trusted version: PR commits are only read, never # executed. On push events this resolves to the pushed sha. - name: Checkout trusted code - uses: actions/checkout@v6 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: fetch-depth: 0 ref: ${{ github.event.pull_request.base.sha || github.sha }} diff --git a/.github/workflows/tests-e2e.yml b/.github/workflows/tests-e2e.yml index 84db8d144d..167f00ddf8 100644 --- a/.github/workflows/tests-e2e.yml +++ b/.github/workflows/tests-e2e.yml @@ -206,7 +206,7 @@ jobs: --reporter=blob - name: Upload blob report - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 if: always() with: name: integration-blob-report-${{ matrix.shard }} @@ -215,7 +215,7 @@ jobs: retention-days: 3 - name: Upload test result - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 if: always() with: name: integration-tests-result-${{ matrix.shard }} @@ -338,7 +338,7 @@ jobs: pnpm install --frozen-lockfile; - name: Download blob reports - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: path: frontend/all-blob-reports pattern: integration-blob-report-* @@ -367,7 +367,7 @@ jobs: >> "$GITHUB_STEP_SUMMARY" - name: Upload JSON report - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 if: always() with: name: integration-json-report @@ -377,7 +377,7 @@ jobs: retention-days: 7 - name: Upload HTML report - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 if: always() with: name: integration-html-report From 15606a3f85b6ad82ff7a753e5b4d7dfe7b3a2242 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Sat, 17 Oct 2026 13:46:22 +0000 Subject: [PATCH 3/4] :bug: Answer 400 on a request body the parser cannot read A body declared as JSON that clojure.data.json cannot read ended as a 500 :unexpected with the whole stack trace in the log: wrap-parse-request classified the failure by exception class, and the library signals its syntax errors with a bare java.lang.Exception. Classify it where it happens instead: read-body-params reads the body and raises :validation :malformed-json (400) for anything it cannot parse, keeps RequestTooBigException at 413 and re-raises a structured error from deeper untouched. The body is read outside the handler try, so the error goes out as a throw like the other specific-exception branches. The failure is now one log line (path, peer address, content-type, parser message) without the stack trace, and it is counted: the middleware takes the metrics instance as a middleware argument, so the backend exports penpot_http_malformed_requests_total and nexus declares its own nexus_http_malformed_requests_total. Tests: parse-request-malformed-json-body reproduces the production body and asserts the 400 and the counter; the existing parse-request tests now pass a metrics stub. AI-assisted-by: deepseek-v4.1-flash --- .serena/memories/audit-log.md | 2 +- .serena/memories/backend/subtleties.md | 3 +- backend/src/app/http.clj | 3 +- backend/src/app/http/middleware.clj | 106 ++++++++++++++---- backend/src/app/main.clj | 6 + .../backend_tests/http_middleware_test.clj | 52 +++++++-- 6 files changed, 139 insertions(+), 33 deletions(-) diff --git a/.serena/memories/audit-log.md b/.serena/memories/audit-log.md index 3739612214..898e95e413 100644 --- a/.serena/memories/audit-log.md +++ b/.serena/memories/audit-log.md @@ -269,7 +269,7 @@ Nexus dispatches on `:type`: `"telemetry-events"` batches are coerced and insert - **Anonymization happens in Penpot, before sending.** By the time events leave, times are day-truncated, IPs are already `0.0.0.0`, props/context are filtered (see above), and emails are gone (only derived `email-domain` survives). Nexus additionally forces the outcome: it prefixes any non-`telemetry:` source, sets `received-at = tracked-at` (already truncated), pins IP to `0.0.0.0`, and stamps the sending `instance-id` into every event context. - **No personal data is expected.** Profile ids are random UUIDs with no PII, so the telemetry consumer needs no login guard; grouping is by `instance`, not by person (see posthog-telemetry below). -- **The endpoint always answers 200.** Failures are swallowed and only counted (`nexus_telemetry_errors_total`), so a bad batch never breaks the sending instance; successes are counted (`nexus_telemetry_events_total`, `nexus_telemetry_reports_total`). Old instances may send a fressian+zstd base64 blob instead of a plain vector; Nexus decodes both (50 MiB cap against decompression bombs). +- **The endpoint always answers 200.** Failures are swallowed and only counted (`nexus_telemetry_errors_total`), so a bad batch never breaks the sending instance; successes are counted (`nexus_telemetry_events_total`, `nexus_telemetry_reports_total`). A body the parser cannot read never reaches the handler: it answers 400 and counts in `nexus_http_malformed_requests_total`. Old instances may send a fressian+zstd base64 blob instead of a plain vector; Nexus decodes both (50 MiB cap against decompression bombs). ## 12. Archive to Nexus (today) and to Nitrate (future) diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index 996b2c19d2..7e6c6ba54b 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -42,7 +42,7 @@ - `app.config/config` and `flags` are dynamic `defonce` vars populated from `PENPOT_*` env vars through the shared schema string transformer. Tests and tooling can bind them. - `parse-flags` automatically adds `:disable-secure-session-cookies` when `public-uri` is plain HTTP and not localhost. This changes cookie defaults without an explicit env flag. - The backend sets Clojure `*assert*` globally from the `:backend-asserts` feature flag. Assertion-dependent checks can therefore differ by runtime flags. -- Request body parsing is mostly POST-oriented and supports Transit JSON plus plain JSON. Plain JSON request keys are kebab-decoded before being merged into `:params`. +- Request body parsing is mostly POST-oriented and supports Transit JSON plus plain JSON. Plain JSON request keys are kebab-decoded before being merged into `:params`. A body the parser cannot read is a client error, classified at the parse site and never by exception class (`clojure.data.json` signals its syntax errors with a bare `java.lang.Exception`): 400 `:validation`/`:malformed-json`, one `warn` line (`malformed request body`) carrying path, peer address and content-type instead of a stack trace, and `penpot_http_malformed_requests_total`. `RequestTooBigException` keeps its 413. - Response formatting negotiates with `Accept` or `_fmt=json`. Transit is the default for collection/boolean bodies; JSON encoding has special pointer-map handling. - Auth prefers the session cookie token before the `Authorization` header. Headers may be `Token` or `Bearer`. Only `kid=1`/`ver=1` tokens are decoded as session tokens; anything else is left unauthenticated (legacy v1 tokens were removed). - Shared-key auth requires `x-shared-key` as ` ` and stores the lowercased key id on the request. If no shared keys are configured it always rejects. @@ -60,6 +60,7 @@ ## Metrics recording - `app.metrics/run!` is safe by default: a recording failure never throws (a metrics bug must not change the behavior of the measured operation). The first failure per metric id logs at `warn`, later ones at `debug`. The `instance` precondition is a plain assert (the backend enables `:backend-asserts`), and the collector lookup sits outside the recording guard, so a missing instance fails hard even when asserts are disabled. `::mtx/metrics` is required by the storage, s3-backend, and db-pool schemas; `app.db` wires the prometheus `MetricsTrackerFactory` unconditionally. Storage-specific metric contracts: `mem:backend/storage`. +- `penpot_http_malformed_requests_total` (counter, no labels) counts requests whose body the parser could not read. `app.http.middleware/wrap-parse-request` emits it where it classifies the error, so the instance travels as a middleware argument (`[mw/parse-request (::mtx/metrics cfg)]`) and each service declares the definition with its own prefix (nexus: `nexus_http_malformed_requests_total`). The per-endpoint request counters of nexus do not count these requests: they sit inside the parse middleware, so the failure happens before them. ## Log-derived metrics diff --git a/backend/src/app/http.clj b/backend/src/app/http.clj index 30efa426e4..aa6950fdeb 100644 --- a/backend/src/app/http.clj +++ b/backend/src/app/http.clj @@ -244,6 +244,7 @@ [::mgmt/routes schema:routes] ::session/manager ::setup/props + ::mtx/metrics ::db/pool]) (defmethod ig/assert-key ::router @@ -260,7 +261,7 @@ [mw/auth {:bearer (partial session/decode-token cfg) :cookie (partial session/decode-token cfg) :token (partial actoken/decode-token cfg)}] - [mw/parse-request] + [mw/parse-request (::mtx/metrics cfg)] [mw/errors errors/handle] [mw/restrict-methods]]} diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 45f8abecca..ad80bfe9c4 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -14,7 +14,9 @@ [app.config :as cf] [app.http :as-alias http] [app.http.errors :as errors] + [app.metrics :as mtx] [app.tokens :as tokens] + [app.util.inet :as inet] [app.util.pointer-map :as pmap] [cuerdas.core :as str] [yetti.adapter :as yt] @@ -44,27 +46,81 @@ (java.io.BufferedReader. (java.io.InputStreamReader. body)))) +(def ^:private not-a-body + "Marker returned by `read-body-params` when the `content-type` header names + neither transit nor json; the request is left untouched. + + A fresh object and not a keyword: a keyword is a legal value for a decoded + body, so identity is the only safe comparison." + (Object.)) + +(defn- raise-malformed-body + "Signals a body that the parser could not read. + + A failure while reading the body means the body does not match what the + client declared, so it is a client error: the exception class says nothing + here, because `clojure.data.json` signals its syntax errors with a bare + `java.lang.Exception`." + [request metrics content-type cause] + ;; A single line, without the stack trace: the trace of a parse failure + ;; hides the fields (path, peer address, content-type) that identify the + ;; client sending the broken body. + (l/wrn :hint "malformed request body" + :path (yreq/path request) + :remote-addr (inet/parse-request request) + :content-type content-type + :error (ex/get-hint cause)) + + (mtx/run! metrics + :id :http-malformed-requests-total + :inc 1) + + (ex/raise :type :validation + :code :malformed-json + :hint "invalid JSON in request body" + :cause cause)) + +(defn- read-body-params + "Reads the request body according to the `content-type` header. + + `RequestTooBigException` keeps its own code (413) and a structured error + raised from deeper (an `ex-info`) travels untouched; anything else is a + malformed body." + [request metrics content-type] + (try + (cond + (str/starts-with? content-type "application/transit+json") + (with-open [^InputStream is (yreq/body request)] + (t/read! (t/reader is))) + + (str/starts-with? content-type "application/json") + (with-open [reader (get-reader request)] + (json/read reader :key-fn json/read-kebab-key)) + + :else + not-a-body) + + (catch RequestTooBigException cause + (ex/raise :type :validation + :code :request-body-too-large + :hint "request body exceeds size limit" + :cause cause)) + + (catch Throwable cause + (if (ex/error? cause) + (throw cause) + (raise-malformed-body request metrics content-type cause))))) + (defn wrap-parse-request - [handler] + [handler metrics] (letfn [(process-request [request] - (let [header (yreq/get-header request "content-type")] - (cond - (str/starts-with? header "application/transit+json") - (with-open [^InputStream is (yreq/body request)] - (let [params (t/read! (t/reader is))] - (-> request - (assoc :body-params params) - (update :params merge params)))) - - (str/starts-with? header "application/json") - (with-open [reader (get-reader request)] - (let [params (json/read reader :key-fn json/read-kebab-key)] - (-> request - (assoc :body-params params) - (update :params merge params)))) - - :else - request))) + (let [header (yreq/get-header request "content-type") + params (read-body-params request metrics header)] + (if (identical? params not-a-body) + request + (-> request + (assoc :body-params params) + (update :params merge params))))) ;; The specific-exception branches below (IAE, ;; RequestTooBigException, EOFException) raise with @@ -107,10 +163,14 @@ (fn [request] (if (= (yreq/method request) :post) - (try - (-> request process-request handler) - (catch Throwable cause - (handle-error cause request))) + ;; The body is read outside the `try`: a failure there is always + ;; raised already classified by `read-body-params`, and goes out as a + ;; throw like the rest of the specific-exception branches above. + (let [request (process-request request)] + (try + (handler request) + (catch Throwable cause + (handle-error cause request)))) (handler request))))) (def parse-request diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index 9fc93eb26e..43039cdee4 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -226,6 +226,11 @@ :http-connector-errors-total {::mdef/name "penpot_http_connector_errors_total" ::mdef/help "Total number of handler errors in the http listener." + ::mdef/type :counter} + + :http-malformed-requests-total + {::mdef/name "penpot_http_malformed_requests_total" + ::mdef/help "Total number of requests whose body the parser could not read." ::mdef/type :counter}}) (def system-config @@ -360,6 +365,7 @@ ::rpc/routes (ig/ref ::rpc/routes) ::setup/props (ig/ref ::setup/props) ::mtx/routes (ig/ref ::mtx/routes) + ::mtx/metrics (ig/ref ::mtx/metrics) ::oidc/routes (ig/ref ::oidc/routes) ::mgmt/routes (ig/ref ::mgmt/routes) ::http.debug/routes (ig/ref ::http.debug/routes) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 1651d92e0a..c572e20c09 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -16,7 +16,8 @@ [app.http.errors :as http-errors] [app.http.middleware :as mw] [app.http.session :as session] - [app.main :as-alias main] + [app.main :as main] + [app.metrics :as mtx] [app.rpc :as-alias rpc] [app.rpc.commands.access-token] [app.tokens :as tokens] @@ -28,6 +29,7 @@ [yetti.request :as yreq] [yetti.response :as yres]) (:import + io.prometheus.client.CollectorRegistry io.undertow.server.RequestTooBigException)) (t/use-fixtures :once th/state-init) @@ -515,7 +517,8 @@ ;; handler in app.http/router-handler), this test asserts on ;; the ex-info thrown by wrap-parse-request directly. (let [handler (#'app.http.middleware/wrap-parse-request - (fn [_] {::yres/status 200 ::yres/body :ok})) + (fn [_] {::yres/status 200 ::yres/body :ok}) + (th/metrics-stub)) ;; Body contains the bytes for: {"x": "\}"} -- a string ;; value with a backslash followed by '}', which ;; clojure.data.json v0.5.x cannot handle. @@ -532,13 +535,44 @@ (t/is (= :malformed-json (-> ex ex-data :code))) (t/is (= "invalid JSON in request body" (-> ex ex-data :hint))))) +(defn- malformed-requests-count + "Value of the malformed-body counter of a metrics instance." + [metrics] + (.getSampleValue ^CollectorRegistry (mtx/get-registry metrics) + "penpot_http_malformed_requests_total" + (make-array String 0) + (make-array String 0))) + +(t/deftest parse-request-malformed-json-body + ;; The body shape seen in production: an object where the parser expects a + ;; key. clojure.data.json signals it with a bare java.lang.Exception, which + ;; the class-based classification did not recognize, so it surfaced as a 500 + ;; :unexpected with the whole stack trace in the log. It is a client error: + ;; 400 :malformed-json, and it is counted. + (let [metrics (ig/init-key :app.metrics/metrics {:default main/default-metrics}) + handler (#'app.http.middleware/wrap-parse-request + (fn [_] {::yres/status 200 ::yres/body :ok}) + metrics) + request (th/make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{\"events\":[{{\"a\":1}:\"b\"}]}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))) + (t/is (= 1.0 (malformed-requests-count metrics))))) + (t/deftest parse-request-request-too-big-exception ;; When RequestTooBigException is raised (e.g. the request body ;; exceeded the configured size limit), the middleware should ;; convert it to a 413 :request-body-too-large validation ;; error. (let [handler (#'app.http.middleware/wrap-parse-request - (fn [_] (throw (RequestTooBigException. "too large")))) + (fn [_] (throw (RequestTooBigException. "too large"))) + (th/metrics-stub)) request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} @@ -556,7 +590,8 @@ ;; was closed before the parser could read it), the middleware ;; should convert it to a 400 :malformed-json validation error. (let [handler (#'app.http.middleware/wrap-parse-request - (fn [_] (throw (java.io.EOFException. "stream closed")))) + (fn [_] (throw (java.io.EOFException. "stream closed"))) + (th/metrics-stub)) request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} @@ -579,7 +614,8 @@ wrapped (doto (RuntimeException. "wrapped") (.initCause iae)) handler (#'app.http.middleware/wrap-parse-request - (fn [_] (throw wrapped))) + (fn [_] (throw wrapped)) + (th/metrics-stub)) request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} @@ -597,7 +633,8 @@ ;; path and return a 500 with :type :server-error :code ;; :unexpected. This is the "true internal error" path. (let [handler (#'app.http.middleware/wrap-parse-request - (fn [_] (throw (RuntimeException. "boom")))) + (fn [_] (throw (RuntimeException. "boom"))) + (th/metrics-stub)) request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} @@ -617,7 +654,8 @@ ;; has a dedicated handle-exception method that returns 500 ;; with :code :io-exception. (let [handler (#'app.http.middleware/wrap-parse-request - (fn [_] (throw (java.io.IOException. "network gone")))) + (fn [_] (throw (java.io.IOException. "network gone"))) + (th/metrics-stub)) request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} From 70b317a976e0bc1c68a47882f23244cecc1e710a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Sat, 17 Oct 2026 16:01:04 +0200 Subject: [PATCH 4/4] :bug: Harden upload path handling and validation error bodies (#12067) * :ambulance: Reject forged upload paths outside multipart Transit bodies can smuggle upload maps whose path points at any server file; the schema only checks the shape and handlers open the path directly. app.rpc now injects a server-side multipart flag that bodies cannot forge, and methods declare their upload keys with the new multipart-only-params metadata, enforced centrally after schema validation with a params-validation error. Covers all nine upload methods, including chunked and management flows. AI-assisted-by: muse-spark-1.3-contributor * :bug: Sanitize raw explain in generic validation error bodies The generic :validation branch returned ex-data untouched, so errors carrying the raw malli explain (live schema objects, e.g. storage invalid-storage-metadata) broke JSON encoding mid-stream and clients got truncated bodies. Apply the same clean treatment the known codes already use: drop the raw explain and attach its humanized text. AI-assisted-by: muse-spark-1.3-contributor * :bug: Add e2e path traversal tests for binfile and media upload Add two e2e suites that hand-write transit bodies with a forged ["~#path", ...] upload path (no transit library needed) and expect 400 validation/params-validation. They cover import-binfile :file, upload-file-media-object :content and upload-chunk :content, plus a plain-JSON string-path control case. Before the multipart-path fix they failed (500 io-exception, invalid-image, 200 stored chunk); after it they pass. AI-assisted-by: muse-spark-1.3-contributor * :bug: Use multipart helper in binfile SVG test setup The SVG import setup uploaded through plain command!, so the new multipart-only guard rejected it with params-validation. Switch the setup to multipart-command! like the other upload tests. AI-assisted-by: muse-spark-1.3-contributor * :bug: Use multipart helper in remaining upload tests The multipart-only upload guard rejects plain RPC calls carrying upload maps. Five tests with real files still used command! and failed with params-validation. Switch them to multipart-command! while keeping the forged-content tests on plain command!. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/http/errors.clj | 6 +- backend/src/app/rpc.clj | 31 +- backend/src/app/rpc/commands/binfile.clj | 1 + .../src/app/rpc/commands/files_thumbnails.clj | 6 +- backend/src/app/rpc/commands/media.clj | 2 + backend/src/app/rpc/commands/profile.clj | 1 + backend/src/app/rpc/commands/teams.clj | 3 +- backend/src/app/rpc/management/exporter.clj | 1 + backend/src/app/rpc/management/nitrate.clj | 1 + .../test/backend_tests/binfile_svg_test.clj | 18 +- backend/test/backend_tests/helpers.clj | 34 ++ backend/test/backend_tests/helpers_test.clj | 29 ++ .../backend_tests/http_middleware_test.clj | 22 ++ .../test/backend_tests/rpc_binfile_test.clj | 44 +++ backend/test/backend_tests/rpc_file_test.clj | 18 +- .../rpc_file_thumbnails_test.clj | 110 ++++-- backend/test/backend_tests/rpc_font_test.clj | 20 +- .../rpc_management_nitrate_test.clj | 16 + .../backend_tests/rpc_management_test.clj | 19 +- backend/test/backend_tests/rpc_media_test.clj | 315 ++++++++++-------- .../test/backend_tests/rpc_profile_test.clj | 25 +- .../test/backend_tests/rpc_quotes_test.clj | 2 +- backend/test/backend_tests/rpc_team_test.clj | 16 +- backend/test/backend_tests/storage_test.clj | 104 +++--- .../import-binfile-path-traversal.test.mjs | 110 ++++++ .../upload-file-media-path-traversal.test.mjs | 189 +++++++++++ 26 files changed, 865 insertions(+), 278 deletions(-) create mode 100644 backend/test/e2e/import-binfile-path-traversal.test.mjs create mode 100644 backend/test/e2e/upload-file-media-path-traversal.test.mjs diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index e8b103c417..b8ef12eb74 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -122,7 +122,11 @@ {::yres/status 400 ::yres/body data})) :else - {::yres/status 400 ::yres/body data}))) + (let [explain (ex/explain data)] + {::yres/status 400 + ::yres/body (-> data + (dissoc ::sm/explain) + (cond-> explain (assoc :explain explain)))})))) (defmethod handle-error :assertion [error request parent-cause] diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index e28485181f..c5e464c75e 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -127,7 +127,15 @@ (-> (assoc ::auth-type :token) (assoc ::token-perms (set (::actoken/perms request #{}))))) (cond-> key-id - (assoc ::auth-key-id key-id))) + (assoc ::auth-key-id key-id)) + ;; Server-side only: request bodies cannot forge + ;; it (qualified keys are stripped before the + ;; merge). Upload maps are only trusted when they + ;; arrive over multipart. + (assoc ::is-multipart + (str/starts-with? + (str (yreq/get-header request "content-type")) + "multipart/"))) data (with-meta data {::http/request request}) @@ -228,10 +236,23 @@ (fn [cfg params] (let [request-params (-> params meta ::http/request :params decode)] (if (validate request-params) - (let [result (f cfg (merge params (d/without-qualified request-params)))] - (if (instance? clojure.lang.IObj result) - (vary-meta result assoc :encode/json encode) - result)) + (let [merged (merge params (d/without-qualified request-params))] + ;; Upload temp files are created server-side by the multipart + ;; parser, so an upload map smuggled in a transit/JSON body can + ;; point :path at any internal server file. Methods declare + ;; their upload keys with ::multipart-only-params and they are + ;; only accepted when ::is-multipart arrived server-side with + ;; the request (request bodies cannot forge qualified keys). + (when (some #(and (contains? merged %) + (not (::is-multipart merged))) + (::multipart-only-params mdata)) + (ex/raise :type :validation + :code :params-validation + :hint "upload requires multipart")) + (let [result (f cfg merged)] + (if (instance? clojure.lang.IObj result) + (vary-meta result assoc :encode/json encode) + result))) (ex/raise :type :validation :code :params-validation ::sm/explain (explain request-params)))))) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index c4fc37cb99..e842fd4c60 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -170,6 +170,7 @@ ::webhooks/event? true ::sse/stream? true ::sm/params schema:import-binfile + ::rpc/multipart-only-params [:file] ::climit/id [[:import-binfile/by-profile ::rpc/profile-id] [:import-binfile/global]]} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id project-id version upload-id] :as params}] diff --git a/backend/src/app/rpc/commands/files_thumbnails.clj b/backend/src/app/rpc/commands/files_thumbnails.clj index 1a821c4027..964e3be645 100644 --- a/backend/src/app/rpc/commands/files_thumbnails.clj +++ b/backend/src/app/rpc/commands/files_thumbnails.clj @@ -286,7 +286,8 @@ ::rtry/enabled true ::rtry/when rtry/conflict-exception? ::audit/skip true - ::sm/params schema:create-file-object-thumbnail} + ::sm/params schema:create-file-object-thumbnail + ::rpc/multipart-only-params [:media]} [cfg {:keys [::rpc/profile-id file-id object-id media tag]}] (media.v/validate-media-type! media) @@ -387,7 +388,8 @@ [:file-thumbnail-ops/global]] ::rtry/enabled true ::rtry/when rtry/conflict-exception? - ::sm/params schema:create-file-thumbnail} + ::sm/params schema:create-file-thumbnail + ::rpc/multipart-only-params [:media]} [cfg {:keys [::rpc/profile-id file-id] :as params}] (media.v/validate-media-type! (:media params)) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 1c9e07ed01..fa2199d24c 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -57,6 +57,7 @@ {::doc/added "1.17" ::doc/changes [["2.19" "The optional :id param is rejected with a params-validation error; the server always generates the identifier"]] ::sm/params schema:upload-file-media-object + ::rpc/multipart-only-params [:content] ::climit/id [[:process-image/by-profile ::rpc/profile-id] [:process-image/global]]} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id content] :as params}] @@ -371,6 +372,7 @@ (sv/defmethod ::upload-chunk {::doc/added "2.17" ::sm/params schema:upload-chunk + ::rpc/multipart-only-params [:content] ::sm/result schema:upload-chunk-result} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id session-id index content] :as _params}] diff --git a/backend/src/app/rpc/commands/profile.clj b/backend/src/app/rpc/commands/profile.clj index 82d307bb46..c0cdf56971 100644 --- a/backend/src/app/rpc/commands/profile.clj +++ b/backend/src/app/rpc/commands/profile.clj @@ -307,6 +307,7 @@ (sv/defmethod ::update-profile-photo {:doc/added "1.1" ::sm/params schema:update-profile-photo + ::rpc/multipart-only-params [:file] ::sm/result :nil} [cfg {:keys [::rpc/profile-id file] :as params}] ;; Validate incoming mime type diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index 0cfb0f2baf..f8b85ac0eb 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -1084,7 +1084,8 @@ (sv/defmethod ::update-team-photo {::doc/added "1.17" - ::sm/params schema:update-team-photo} + ::sm/params schema:update-team-photo + ::rpc/multipart-only-params [:file]} [cfg {:keys [::rpc/profile-id file] :as params}] ;; Validate incoming mime type diff --git a/backend/src/app/rpc/management/exporter.clj b/backend/src/app/rpc/management/exporter.clj index dabca00835..332fe23984 100644 --- a/backend/src/app/rpc/management/exporter.clj +++ b/backend/src/app/rpc/management/exporter.clj @@ -31,6 +31,7 @@ (sv/defmethod ::upload-tempfile {::doc/added "2.12" ::sm/params schema:upload-tempfile-params + ::rpc/multipart-only-params [:content] ::sm/result schema:upload-tempfile-result} [cfg {:keys [::rpc/profile-id content]}] (media.v/validate-media-type! content cm/tempfile-types) diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index 36e37aca9d..bb97a66f37 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -135,6 +135,7 @@ collection when replacing an existing one." {::doc/added "2.18" ::sm/params schema:upload-organization-logo + ::rpc/multipart-only-params [:content] ::sm/result schema:upload-organization-logo-result ::nitrate/sso false} [{:keys [::sto/storage]} {:keys [content organization-id previous-id]}] diff --git a/backend/test/backend_tests/binfile_svg_test.clj b/backend/test/backend_tests/binfile_svg_test.clj index 92fe3f2a0e..446abca995 100644 --- a/backend/test/backend_tests/binfile_svg_test.clj +++ b/backend/test/backend_tests/binfile_svg_test.clj @@ -195,15 +195,15 @@ :project-id (:default-project-id profile) :is-shared false}) [svg-path svg-size] (write-temp-svg clean-svg) - out (th/command! {::th/type :upload-file-media-object - ::rpc/profile-id (:id profile) - :file-id (:id file) - :is-local true - :name "benign.svg" - :content {:filename "benign.svg" - :path svg-path - :mtype "image/svg+xml" - :size svg-size}}) + out (th/multipart-command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id profile) + :file-id (:id file) + :is-local true + :name "benign.svg" + :content {:filename "benign.svg" + :path svg-path + :mtype "image/svg+xml" + :size svg-size}}) _ (t/is (nil? (:error out)) (str "upload failed: " (pr-str (:error out)))) uploaded (:result out) page-id (first (get-in file [:data :pages])) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 4882827b65..8aa3b684fc 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -471,6 +471,13 @@ (-> params (assoc :app.rpc/request-at (ct/now)) + ;; Mirror of the production flag in `app.rpc/make-rpc-handler`: + ;; upload maps are only trusted over multipart. Tests declare it + ;; with a content-type header on the supplied request map. + (assoc ::rpc/is-multipart + (str/starts-with? + (str (yreq/get-header request "content-type")) + "multipart/")) (with-meta {:app.http/request request})))) (defn command! @@ -508,6 +515,33 @@ (let [params (prepare-rpc-params data)] (try-on! (method-fn params))))) +(def multipart-headers + {"content-type" "multipart/form-data; boundary=test"}) + +(defn with-multipart + "Attach a multipart content-type to RPC test params, declaring that the + upload maps they carry arrived over multipart (as the production + multipart parser would deliver them). Preserves any request metadata + already present on the data." + [data] + (let [request (-> data meta :app.http/request)] + (vary-meta data assoc :app.http/request + (assoc (if (map? request) request {}) + :headers (merge multipart-headers + (when (map? request) + (:headers request))))))) + +(defn multipart-command! + "Like `command!` but declares the request as multipart, for calls that + carry upload maps (`:file`, `:content`, `:media`)." + [data] + (command! (with-multipart data))) + +(defn multipart-management-command! + "Like `management-command!` but declares the request as multipart." + [data] + (management-command! (with-multipart data))) + (defn run-task! ([name] (run-task! name {})) diff --git a/backend/test/backend_tests/helpers_test.clj b/backend/test/backend_tests/helpers_test.clj index d8e1bb2dc2..2473f86115 100644 --- a/backend/test/backend_tests/helpers_test.clj +++ b/backend/test/backend_tests/helpers_test.clj @@ -91,6 +91,35 @@ (t/deftest make-dummy-request-missing-cookie-returns-nil-value (t/is (= {:value nil} (yreq/get-cookie (th/make-dummy-request) "missing")))) +;; --- RPC PARAMS: MULTIPART FLAG + +(t/deftest prepare-rpc-params-marks-multipart-request + (let [params (#'th/prepare-rpc-params + (with-meta {::th/type :dummy + :events [{:name "event"}]} + {:app.http/request {:headers {"content-type" "multipart/form-data; boundary=xxx"}}}))] + (t/is (true? (:app.rpc/is-multipart params))))) + +(t/deftest prepare-rpc-params-leaves-non-multipart-request-unmarked + (let [params (#'th/prepare-rpc-params + (with-meta {::th/type :dummy + :events [{:name "event"}]} + {:app.http/request {:headers {"content-type" "application/transit+json"}}}))] + (t/is (false? (:app.rpc/is-multipart params))))) + +(t/deftest prepare-rpc-params-leaves-missing-content-type-unmarked + (let [params (#'th/prepare-rpc-params {::th/type :dummy + :events [{:name "event"}]})] + (t/is (false? (:app.rpc/is-multipart params))))) + +(t/deftest prepare-rpc-params-body-cannot-forge-multipart-flag + (let [params (#'th/prepare-rpc-params + (with-meta {::th/type :dummy + :app.rpc/is-multipart true + :events [{:name "event"}]} + {:app.http/request {:headers {"content-type" "application/json"}}}))] + (t/is (false? (:app.rpc/is-multipart params))))) + ;; --- RPC PARAMS: REQUEST METADATA (t/deftest prepare-rpc-params-preserves-supplied-request diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index c572e20c09..44042a4b2f 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -7,6 +7,7 @@ (ns backend-tests.http-middleware-test (:require [app.common.exceptions :as ex] + [app.common.json :as json] [app.common.time :as ct] [app.common.uuid :as uuid] [app.config :as cf] @@ -20,6 +21,7 @@ [app.metrics :as mtx] [app.rpc :as-alias rpc] [app.rpc.commands.access-token] + [app.storage.schema :as stsch] [app.tokens :as tokens] [backend-tests.helpers :as th] [clojure.string :as str] @@ -707,3 +709,23 @@ (t/is (= "safe user-facing hint" (:hint body))) (t/is (nil? (:state body))) (t/is (nil? (:path body))))) + +(t/deftest validation-error-body-with-raw-explain-is-json-serializable + ;; A :validation error whose ex-data carries the raw malli explain + ;; (storage invalid-storage-metadata) must produce a response body + ;; that survives JSON encoding. Before the fix the live schema + ;; object inside the explain broke the writer mid-stream and the + ;; client received a truncated body. + (let [cause (try + (stsch/encode-metadata {}) + (catch clojure.lang.ExceptionInfo cause cause)) + response (http-errors/handle cause {}) + body (::yres/body response)] + (t/is (= 400 (::yres/status response))) + (t/is (= :validation (:type body))) + (t/is (= :invalid-storage-metadata (:code body))) + (t/is (nil? (:app.common.schema/explain body))) + (let [encoded (json/encode body :key-fn json/write-camel-key) + decoded (json/decode encoded :key-fn json/read-kebab-key)] + (t/is (str/includes? encoded "invalid-storage-metadata")) + (t/is (string? (:explain decoded)))))) diff --git a/backend/test/backend_tests/rpc_binfile_test.clj b/backend/test/backend_tests/rpc_binfile_test.clj index ccdcc7fafa..0447fe135d 100644 --- a/backend/test/backend_tests/rpc_binfile_test.clj +++ b/backend/test/backend_tests/rpc_binfile_test.clj @@ -45,6 +45,50 @@ (t/is (false? (validator params-with-file-id)) "params with file-id should be rejected"))) +(t/deftest import-binfile-schema-accepts-path-object + ;; Documents the schema hole: a server-side Path object in :file passes + ;; validation on shape alone. The multipart guard in the handler (not the + ;; schema) is what rejects client-forged paths. + (let [schema @#'binfile/schema:import-binfile + validator (sm/lazy-validator schema)] + (t/is (true? (validator {:name "pwned" + :project-id (uuid/random) + :version 3 + :file {:filename "evil.zip" + :size 123 + :path (fs/path "/etc/passwd")}}))))) + +(t/deftest import-binfile-rejects-forged-file-without-multipart + ;; Path-traversal PoC at command level: a transit body like + ;; {"~:file": {"~:path": ["~#path", "/etc/passwd"]}} decodes to this exact + ;; map. Without a multipart request it must fail at params validation, + ;; before any file is read. + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :import-binfile + ::rpc/profile-id (:id prof) + :name "pwned" + :version 3 + :project-id (:default-project-id prof) + :file {:filename "evil.zip" + :size 123 + :path (fs/path "/etc/passwd")}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + +(t/deftest import-binfile-rejects-forged-file-with-forged-flag + ;; The multipart flag is server-side: smuggling it in the body must not + ;; bypass the guard (prepare-rpc-params overwrites it from the request). + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :import-binfile + ::rpc/profile-id (:id prof) + :app.rpc/is-multipart true + :name "pwned" + :version 3 + :project-id (:default-project-id prof) + :file {:filename "evil.zip" + :size 123 + :path (fs/path "/etc/passwd")}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + (t/deftest import-binfile-schema-rejects-unsupported-version ;; T1-N2-03: version parameter should be restricted to supported values (1 or 3) (let [schema @#'binfile/schema:import-binfile diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index 1bb3a0a09e..34adcae63e 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -264,7 +264,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -413,7 +413,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -604,7 +604,7 @@ :object-id object-id :tag "frame" :media mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -1264,7 +1264,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] (t/is (nil? error)) (t/is (map? result))) @@ -1321,7 +1321,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] (t/is (nil? error)) (t/is (map? result))) @@ -1345,7 +1345,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] (t/is (nil? error)) (t/is (map? result))) @@ -1396,7 +1396,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] ;; (th/print-result! out) (t/is (nil? error)) @@ -1411,7 +1411,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - {:keys [error result] :as out} (th/command! data)] + {:keys [error result] :as out} (th/multipart-command! data)] ;; (th/print-result! out) (t/is (nil? error)) @@ -1909,7 +1909,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) diff --git a/backend/test/backend_tests/rpc_file_thumbnails_test.clj b/backend/test/backend_tests/rpc_file_thumbnails_test.clj index 5273180c15..49345f4ba9 100644 --- a/backend/test/backend_tests/rpc_file_thumbnails_test.clj +++ b/backend/test/backend_tests/rpc_file_thumbnails_test.clj @@ -76,11 +76,11 @@ :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data1)] + (let [out (th/multipart-command! data1)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) - (let [out (th/command! data2)] + (let [out (th/multipart-command! data2)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -184,12 +184,12 @@ :path (th/tempfile "backend_tests/test_files/sample.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data1)] + (let [out (th/multipart-command! data1)] ;; (th/print-result! out) (t/is (nil? (:error out))) (t/is (contains? (:result out) :uri))) - (let [out (th/command! data2)] + (let [out (th/multipart-command! data2)] (t/is (nil? (:error out))) (t/is (contains? (:result out) :uri))) @@ -272,7 +272,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data) + out (th/multipart-command! data) error (:error out)] (t/is (nil? (:result out))) @@ -298,7 +298,7 @@ :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data1)] + (let [out (th/multipart-command! data1)] ;; (th/print-result! out) (t/is (nil? (:error out))) (t/is (contains? (:result out) :uri))) @@ -325,7 +325,7 @@ :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}}] - (let [out (th/command! data)] + (let [out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -341,7 +341,7 @@ (let [params {::th/type :get-file-object-thumbnails ::rpc/profile-id (:id profile) :file-id (:id file)} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) @@ -367,7 +367,7 @@ (-> media (assoc :path (th/tempfile "backend_tests/test_files/sample2.jpg")) (assoc :size 7923)))) - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -376,7 +376,7 @@ (-> media (assoc :path (th/tempfile "backend_tests/test_files/sample.jpg")) (assoc :size 312043)))) - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))))) @@ -402,7 +402,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -419,7 +419,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2 oid3]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -440,7 +440,7 @@ data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids []} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out))))) @@ -457,7 +457,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))))) @@ -480,7 +480,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -488,7 +488,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2 oid3]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -517,7 +517,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -525,7 +525,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -533,7 +533,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -563,7 +563,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -571,7 +571,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile2) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (some? (:error out))) (t/is (th/ex-info? (:error out))) (t/is (= :not-found (th/ex-type (:error out))))) @@ -606,7 +606,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -614,7 +614,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -653,7 +653,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -665,7 +665,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -674,7 +674,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile1) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (some? (:error out))) (t/is (th/ex-info? (:error out))) (t/is (= :not-found (th/ex-type (:error out))))) @@ -709,7 +709,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -721,7 +721,7 @@ :size 312043 :path (th/tempfile "backend_tests/test_files/sample.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -740,7 +740,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1 oid2]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -770,7 +770,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -784,7 +784,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids oids} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -813,7 +813,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -821,7 +821,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -849,7 +849,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out)))) @@ -857,7 +857,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid oid]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -891,7 +891,7 @@ :size 7923 :path (th/tempfile "backend_tests/test_files/sample2.jpg") :mtype "image/jpeg"}} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (map? (:result out))))) @@ -899,7 +899,7 @@ (let [data {::th/type :delete-file-object-thumbnails ::rpc/profile-id (:id profile) :object-ids [oid1]} - out (th/command! data)] + out (th/multipart-command! data)] (t/is (nil? (:error out))) (t/is (nil? (:result out)))) @@ -915,3 +915,39 @@ (t/is (= 1 (count rows2))) (t/is (nil? (:deleted-at (first rows2))))))) + +(t/deftest create-file-thumbnail-rejects-forged-media + ;; A transit body like {"~:media": {"~:path": ["~#path", "/etc/passwd"]}} + ;; decodes to this exact map. Without a multipart request it must fail at + ;; params validation, before any file is read. NOTE: plain command! on + ;; purpose, a multipart header would make it legitimate. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + out (th/command! {::th/type :create-file-thumbnail + ::rpc/profile-id (:id profile) + :file-id (:id file) + :revn 0 + :media {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + +(t/deftest create-file-object-thumbnail-rejects-forged-media + ;; Same transit-forgery vector through object thumbnails. NOTE: plain + ;; command! on purpose, a multipart header would make it legitimate. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + out (th/command! {::th/type :create-file-object-thumbnail + ::rpc/profile-id (:id profile) + :file-id (:id file) + :object-id "obj" + :media {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_font_test.clj b/backend/test/backend_tests/rpc_font_test.clj index bd81112bcb..4ef4533f7a 100644 --- a/backend/test/backend_tests/rpc_font_test.clj +++ b/backend/test/backend_tests/rpc_font_test.clj @@ -76,11 +76,11 @@ (when (uuid? session-id) (doseq [[idx chunk-data] (map-indexed vector chunks)] (let [mfile (make-chunk-mfile chunk-data mtype) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index idx - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index idx + :content mfile})] (t/is (nil? (:error out)))))) session-id)) @@ -438,11 +438,11 @@ ;; Upload all real chunks except the last one (omit it so the session is incomplete) (doseq [[idx chunk-data] (map-indexed vector (butlast chunks))] (let [mfile (make-chunk-mfile chunk-data "font/ttf") - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index idx - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index idx + :content mfile})] (t/is (nil? (:error out))))) (let [out (th/command! {::th/type :create-font-variant diff --git a/backend/test/backend_tests/rpc_management_nitrate_test.clj b/backend/test/backend_tests/rpc_management_nitrate_test.clj index 30ff6a3823..5be56ece30 100644 --- a/backend/test/backend_tests/rpc_management_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_management_nitrate_test.clj @@ -23,6 +23,7 @@ [clojure.set :as set] [clojure.test :as t] [cuerdas.core :as str] + [datoteka.fs :as fs] [mockery.core :refer [with-mocks]])) (t/use-fixtures :once (t/compose-fixtures @@ -2070,3 +2071,18 @@ (t/is (th/success? out)) (let [[params] (:call-args @email-mock)] (t/is (= "" (:user-name params))))))) + +(t/deftest upload-organization-logo-rejects-forged-content + ;; Same transit-forgery vector through the management API: without a + ;; multipart request the wrapper must reject it at params validation. + (with-mocks [nitrate-mock {:target 'app.nitrate/call :return nil}] + (let [profile (th/create-profile* 1 {:is-active true}) + params {::th/type :upload-organization-logo + ::rpc/profile-id (:id profile) + :organization-id (uuid/random) + :content {:filename "evil.png" + :path (fs/path "/etc/passwd") + :mtype "image/png" + :size 123}} + out (th/management-command! params)] + (t/is (th/ex-of-code? (:error out) :params-validation))))) diff --git a/backend/test/backend_tests/rpc_management_test.clj b/backend/test/backend_tests/rpc_management_test.clj index e1f6e81950..a55dcf95a4 100644 --- a/backend/test/backend_tests/rpc_management_test.clj +++ b/backend/test/backend_tests/rpc_management_test.clj @@ -56,9 +56,9 @@ :size 7}} config (assoc cf/config :public-uri "https://example.com/penpot") out1 (binding [cf/config config] - (th/management-command! params)) + (th/multipart-management-command! params)) out2 (binding [cf/config config] - (th/management-command! params))] + (th/multipart-management-command! params))] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) (t/is (str/starts-with? (str (get-in out1 [:result :uri])) @@ -77,7 +77,7 @@ :path path :mtype "text/html" :size 27}} - out (th/management-command! params)] + out (th/multipart-management-command! params)] (t/is (some? (:error out))) (t/is (= :validation (th/ex-type (:error out)))) (t/is (= :media-type-not-allowed (th/ex-code (:error out)))))) @@ -720,3 +720,16 @@ (ct/inst? (get-in % [:props :modified-at]))) events)) (t/is (every? #(= (:id team) (get-in % [:props :team-id])) events)))))) + +(t/deftest upload-tempfile-rejects-forged-content + ;; Same transit-forgery vector through the management API: without a + ;; multipart request the wrapper must reject it at params validation. + (let [profile (th/create-profile* 1 {:is-active true}) + params {::th/type :upload-tempfile + ::rpc/profile-id (:id profile) + :content {:filename "evil.png" + :path (fs/path "/etc/passwd") + :mtype "image/png" + :size 123}} + out (th/management-command! params)] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 985fb68fc2..3b42685b4d 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -73,7 +73,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -144,7 +144,7 @@ :is-local true :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -189,7 +189,7 @@ :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] (let [error (:error out) error-data (ex-data error)] @@ -391,11 +391,11 @@ ;; --- 1. Upload chunks --- (doseq [[idx chunk-data] (map-indexed vector chunks)] (let [mfile (make-chunk-mfile chunk-data mtype) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index idx - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index idx + :content mfile})] (t/is (nil? (:error out))) (t/is (= session-id (:session-id (:result out)))) (t/is (= idx (:index (:result out)))))) @@ -438,11 +438,11 @@ mfile (make-chunk-mfile (first chunks) mtype) session-id (create-session! prof 1)] - (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) ;; First assemble succeeds; session row is marked as consumed afterwards (let [out1 (th/command! {::th/type :assemble-file-media-object @@ -482,11 +482,11 @@ :size 312043} ;; prof2 tries to upload a chunk into prof1's session - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof2) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof2) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out))) (t/is (= :not-found (-> out :error ex-data :type))))) @@ -505,11 +505,11 @@ :size 312043}] ;; prof1 uploads a chunk into their own session - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof1) - :session-id session-id - :index 0 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof1) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out)))) ;; prof2 tries to assemble prof1's session via create-font-variant @@ -540,11 +540,11 @@ :mtype "image/jpeg" :size 312043}] - (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) ;; Assemble with a wrong mtype should fail validation (let [out (th/command! {::th/type :assemble-file-media-object @@ -573,11 +573,11 @@ :size 312043}] ;; Upload only 1 chunk - (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) ;; Assemble: session says 3 expected, only 1 stored → :missing-chunks (let [out (th/command! {::th/type :assemble-file-media-object @@ -606,18 +606,18 @@ mtype "image/jpeg" size (alength (first chunks))] - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (first chunks) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] (t/is (nil? (:error out)))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (first chunks) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-already-exists (-> out :error ex-data :code)))) @@ -650,27 +650,27 @@ (t/is (= 3 (count chunks))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (nth chunks 0) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (nth chunks 0) mtype)})] (t/is (nil? (:error out)))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (nth chunks 0) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (nth chunks 0) mtype)})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-already-exists (-> out :error ex-data :code)))) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 1 - :content (make-chunk-mfile (nth chunks 1) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 1 + :content (make-chunk-mfile (nth chunks 1) mtype)})] (t/is (nil? (:error out)))) ;; The mapping table holds exactly the two distinct indices: the @@ -735,11 +735,11 @@ :mtype "image/jpeg" :size 312043} session-id (create-session! prof 1) - upload-out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + upload-out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error upload-out))) (let [assemble-out (th/command! {::th/type :assemble-file-media-object @@ -788,21 +788,21 @@ :size 312043}] ;; index == total-chunks (out of range) - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 2 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 2 + :content mfile})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :invalid-chunk-index (-> out :error ex-data :code)))) ;; negative index - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index -1 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index -1 + :content mfile})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :invalid-chunk-index (-> out :error ex-data :code)))))) @@ -821,11 +821,11 @@ mtype "image/jpeg"] ;; 312043 bytes exceeds the mocked 1024-byte cap: rejected - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (first chunks) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-too-large (-> out :error ex-data :code)))) @@ -835,11 +835,11 @@ session-id])))) ;; A chunk exactly at the cap still uploads fine - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content (make-chunk-mfile (byte-array 1024 (byte 1)) mtype)})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (byte-array 1024 (byte 1)) mtype)})] (t/is (nil? (:error out))))))) (t/deftest chunked-upload-sessions-per-profile-quota @@ -875,11 +875,11 @@ :path source-path :mtype "image/jpeg" :size 312043} - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (let [row (th/db-exec-one! ["select session_id, object_id, chunk_index from upload_session_chunk where session_id = ?" @@ -904,19 +904,19 @@ :path source-path :mtype "image/jpeg" :size 312043} - out1 (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out1 (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out1))) (let [before (:count (th/db-exec-one! ["select count(*) from storage_object"])) - out2 (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out2 (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out2))) (t/is (= :validation (-> out2 :error ex-data :type))) (t/is (= :chunk-already-exists (-> out2 :error ex-data :code))) @@ -937,11 +937,11 @@ (th/db-exec! ["insert into upload_session_chunk (session_id, chunk_index, object_id) values (?, ?, null)" session-id 0]) (let [before (:count (th/db-exec-one! ["select count(*) from storage_object"])) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out))) (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :chunk-already-exists (-> out :error ex-data :code))) @@ -965,21 +965,21 @@ (if (compare-and-set! failed? false true) (throw (ex-info "link boom" {})) (orig pool object-id session-id index)))}] - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out)))) ;; the failed link left no reservation behind (t/is (= 0 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" session-id])))) ;; retrying the same index in the same session succeeds - (let [out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + (let [out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (t/is (= 1 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" session-id])))))))) @@ -999,11 +999,11 @@ :path source-path :mtype "image/jpeg" :size 312043} - out1 (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out1 (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out1))) (let [assemble-out (th/command! {::th/type :assemble-file-media-object @@ -1024,11 +1024,11 @@ ;; uploading to the consumed session fails without creating an object (let [before (:count (th/db-exec-one! ["select count(*) from storage_object"])) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (some? (:error out))) (t/is (= :not-found (-> out :error ex-data :type))) (t/is (= :object-not-found (-> out :error ex-data :code))) @@ -1054,11 +1054,11 @@ :path source-path :mtype "image/jpeg" :size 312043} - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (let [object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" @@ -1182,13 +1182,13 @@ :mtype "image/jpeg" :size 312043} sent-id (uuid/next) - out (th/command! {::th/type :upload-file-media-object - ::rpc/profile-id (:id prof) - :file-id (:id file) - :is-local true - :name "testfile" - :content mfile - :id sent-id})] + out (th/multipart-command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile + :id sent-id})] (t/is (th/ex-info? (:error out))) (t/is (th/ex-of-type? (:error out) :validation)) (t/is (th/ex-of-code? (:error out) :params-validation)))) @@ -1241,12 +1241,45 @@ :path (th/tempfile "backend_tests/test_files/sample.jpg") :mtype "image/jpeg" :size 312043} - out (th/command! {::th/type :upload-file-media-object - ::rpc/profile-id (:id prof) - :file-id (:id file) - :is-local true - :name "testfile" - :content mfile})] + out (th/multipart-command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile})] (t/is (th/success? out)) (t/is (= (:id team) (get-in (meta (:result out)) [:app.loggers.audit/replace-props :team-id]))))) + +(t/deftest upload-file-media-object-rejects-forged-content + ;; A transit body like {"~:content": {"~:path": ["~#path", "/etc/passwd"]}} + ;; decodes to this exact map. Without a multipart request it must fail at + ;; params validation, before any file is read. + (let [prof (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + out (th/command! {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + +(t/deftest upload-chunk-rejects-forged-content + ;; Same transit-forgery vector through the chunk primitive: without a + ;; multipart request the wrapper must reject it at params validation. + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id (uuid/random) + :index 0 + :content {:filename "chunk" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_profile_test.clj b/backend/test/backend_tests/rpc_profile_test.clj index 6200261e86..11a13addbe 100644 --- a/backend/test/backend_tests/rpc_profile_test.clj +++ b/backend/test/backend_tests/rpc_profile_test.clj @@ -140,12 +140,12 @@ (t/is (= "dark" (get-in out [:result :theme]))))) (t/testing "update photo" - (let [data {::th/type :update-profile-photo - ::rpc/profile-id (:id profile) - :file {:filename "sample.jpg" - :size 123123 - :path (th/tempfile "backend_tests/test_files/sample.jpg") - :mtype "image/jpeg"}} + (let [data (th/with-multipart {::th/type :update-profile-photo + ::rpc/profile-id (:id profile) + :file {:filename "sample.jpg" + :size 123123 + :path (th/tempfile "backend_tests/test_files/sample.jpg") + :mtype "image/jpeg"}}) out (th/command! data)] ;; (th/print-result! out) @@ -1829,3 +1829,16 @@ (t/is (th/success? second-out)) (t/is (= (:id profile) (get-in (meta second-out) [:app.loggers.audit/props :id])))))) + +(t/deftest update-profile-photo-rejects-forged-file + ;; A transit body like {"~:file": {"~:path": ["~#path", "/etc/passwd"]}} + ;; decodes to this exact map. Without a multipart request it must fail at + ;; params validation, before any file is read. + (let [prof (th/create-profile* 1) + out (th/command! {::th/type :update-profile-photo + ::rpc/profile-id (:id prof) + :file {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) diff --git a/backend/test/backend_tests/rpc_quotes_test.clj b/backend/test/backend_tests/rpc_quotes_test.clj index c691ea678b..7c42451418 100644 --- a/backend/test/backend_tests/rpc_quotes_test.clj +++ b/backend/test/backend_tests/rpc_quotes_test.clj @@ -464,7 +464,7 @@ :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] ;; 312043 bytes > 100 byte limit → should be rejected (t/is (not (th/success? out))) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 991b380548..10638cee1d 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -911,7 +911,7 @@ :name "testfile" :content mfile} - out (th/command! params)] + out (th/multipart-command! params)] (t/is (nil? (:error out)))) (let [params {::th/type :delete-team @@ -1443,6 +1443,20 @@ (t/is (th/ex-of-type? (:error out) :validation)) (t/is (th/ex-of-code? (:error out) :params-validation)))) +(t/deftest update-team-photo-rejects-forged-file + ;; Same transit-forgery vector as profile photos: without a multipart + ;; request the wrapper must reject it at params validation. + (let [prof (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id prof)}) + out (th/command! {::th/type :update-team-photo + ::rpc/profile-id (:id prof) + :team-id (:id team) + :file {:filename "evil.jpg" + :size 123 + :path (fs/path "/etc/passwd") + :mtype "image/jpeg"}})] + (t/is (th/ex-of-code? (:error out) :params-validation)))) + (t/deftest leaver-loses-file-access ;; GHSA-v9r9-h77c-55m2: leaving the team must revoke the file and ;; project roles granted in that team. diff --git a/backend/test/backend_tests/storage_test.clj b/backend/test/backend_tests/storage_test.clj index f85319bc47..0cc5ba990f 100644 --- a/backend/test/backend_tests/storage_test.clj +++ b/backend/test/backend_tests/storage_test.clj @@ -211,8 +211,8 @@ :name "testfile" :content mfile} - out1 (th/command! params) - out2 (th/command! params)] + out1 (th/multipart-command! params) + out2 (th/multipart-command! params)] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) @@ -269,11 +269,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (assert (nil? (:error out))) session-id)) @@ -318,7 +318,7 @@ :font-style "normal" :uploads {"font/ttf" session-id}} - out1 (th/command! params1) + out1 (th/multipart-command! params1) out2 (th/command! params2)] ;; (th/print-result! out) @@ -385,8 +385,8 @@ :name "testfile" :content mfile} - out1 (th/command! params) - out2 (th/command! params)] + out1 (th/multipart-command! params) + out2 (th/multipart-command! params)] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) @@ -661,8 +661,8 @@ :is-local true :name "testfile" :content mfile} - out1 (th/command! params) - out2 (th/command! params)] + out1 (th/multipart-command! params) + out2 (th/multipart-command! params)] (t/is (nil? (:error out1))) (t/is (nil? (:error out2))) @@ -1035,11 +1035,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) @@ -1076,11 +1076,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (t/is (= 1 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" @@ -1128,11 +1128,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) (t/is (= 1 (:count (th/db-exec-one! ["select count(*) from upload_session_chunk where session_id = ?" session-id])))) @@ -1167,11 +1167,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" session-id])) check @#'gc-touched/has-upload-session-chunk-refs? @@ -1212,11 +1212,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" session-id]))] (t/is (nil? (:error out))) @@ -1250,11 +1250,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile}) + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile}) object-id (:object-id (th/db-exec-one! ["select object_id from upload_session_chunk where session_id = ?" session-id]))] (t/is (nil? (:error out))) @@ -1290,11 +1290,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) @@ -1335,11 +1335,11 @@ ::rpc/profile-id (:id prof) :total-chunks 1}) :result :session-id) - out (th/command! {::th/type :upload-chunk - ::rpc/profile-id (:id prof) - :session-id session-id - :index 0 - :content mfile})] + out (th/multipart-command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile})] (t/is (nil? (:error out))) ;; soft-delete the profile; the live session is neither consumed nor stalled diff --git a/backend/test/e2e/import-binfile-path-traversal.test.mjs b/backend/test/e2e/import-binfile-path-traversal.test.mjs new file mode 100644 index 0000000000..df5598aa2f --- /dev/null +++ b/backend/test/e2e/import-binfile-path-traversal.test.mjs @@ -0,0 +1,110 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { setupTestProfile } from "./helpers/auth.mjs"; +import { rpcPost } from "./helpers/client.mjs"; + +function errorCode(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorCode(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.code ?? body["~:code"]; +} + +function errorType(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorType(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.type ?? body["~:type"]; +} + +describe("import-binfile path traversal", () => { + let profile; + let cookie; + + before(async () => { + const setup = await setupTestProfile(); + profile = setup.profile; + cookie = setup.cookie; + }); + + it("rejects a hand-forged :file path in a transit body", async () => { + // ATTACK SCENARIO: + // 1. The attacker has a valid session and a valid project-id. + // 2. Instead of a real multipart upload, they hand-write a transit + // body (plain JSON, no transit library needed) with a forged + // :file map that points to an internal server file. + // The transit tag ["~#path", "/etc/passwd"] becomes a real + // java.nio.file.Path on the server (see app.common.transit + // read handlers) and passes media.v/schema:upload, which only + // checks the shape, not where the path came from. + // 3. import-binfile then uses (:path file) directly as input-path + // (parse-file-format / get-manifest / import-files!). + // + // EXPECTED BEHAVIOR AFTER FIX: 400 validation/params-validation, + // without touching the file. While vulnerable, the request passes + // validation and fails later (zip/manifest error) or hangs in SSE, + // proving the file was opened. + const transitBody = JSON.stringify({ + "~:project-id": `~u${profile.defaultProjectId}`, + "~:name": "pwned", + "~:version": 3, + "~:file": { + "~:filename": "evil.zip", + "~:size": 123, + "~:path": ["~#path", "/etc/passwd"], + }, + }); + + const res = await rpcPost("import-binfile", transitBody, { + cookieToken: cookie, + contentType: "application/transit+json", + accept: "application/json", + }); + + assert.equal( + res.status, + 400, + `forged :file path must be rejected with 400, got ${res.status}: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorType(res.body), + "validation", + `expected validation error, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorCode(res.body), + "params-validation", + `forged :file path must fail at params validation, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + }); + + it("plain JSON string path stays rejected", async () => { + // Control case: without the transit "~#path" tag the path arrives + // as a plain string and ::fs/path already rejects it. This test + // documents that only the transit vector bypasses the check. + const res = await rpcPost( + "import-binfile", + { + projectId: profile.defaultProjectId, + name: "pwned", + version: 3, + file: { filename: "evil.zip", size: 123, path: "/etc/passwd" }, + }, + { cookieToken: cookie } + ); + + assert.equal(res.status, 400); + assert.equal(errorType(res.body), "validation"); + assert.equal(errorCode(res.body), "params-validation"); + }); +}); diff --git a/backend/test/e2e/upload-file-media-path-traversal.test.mjs b/backend/test/e2e/upload-file-media-path-traversal.test.mjs new file mode 100644 index 0000000000..7b98f820d2 --- /dev/null +++ b/backend/test/e2e/upload-file-media-path-traversal.test.mjs @@ -0,0 +1,189 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { setupTestProfile } from "./helpers/auth.mjs"; +import { rpcPost } from "./helpers/client.mjs"; + +function errorCode(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorCode(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.code ?? body["~:code"]; +} + +function errorType(body) { + if (body == null) return undefined; + if (typeof body === "string") { + try { + return errorType(JSON.parse(body)); + } catch { + return undefined; + } + } + return body.type ?? body["~:type"]; +} + +async function createFile(cookie, projectId, name = "E2E Media Test File") { + const res = await rpcPost( + "create-file", + { name, projectId }, + { cookieToken: cookie } + ); + assert.equal(res.status, 200, `create-file failed: ${JSON.stringify(res.body)}`); + return res.body; +} + +async function createUploadSession(cookie) { + const res = await rpcPost( + "create-upload-session", + { totalChunks: 1 }, + { cookieToken: cookie } + ); + assert.equal( + res.status, + 200, + `create-upload-session failed: ${JSON.stringify(res.body)}` + ); + return res.body.sessionId ?? res.body["~:session-id"]; +} + +describe("upload-file-media path traversal", () => { + let profile; + let cookie; + let fileId; + + before(async () => { + const setup = await setupTestProfile(); + profile = setup.profile; + cookie = setup.cookie; + const file = await createFile(cookie, profile.defaultProjectId); + fileId = file.id; + }); + + it("rejects a hand-forged :content path in a transit body", async () => { + // ATTACK SCENARIO: + // 1. The attacker has a valid session and a file they can edit. + // 2. Instead of a real multipart upload, they hand-write a transit + // body (plain JSON, no transit library needed) with a forged + // :content map that points to an internal server file. + // The transit tag ["~#path", "/etc/passwd"] becomes a real + // java.nio.file.Path on the server (see app.common.transit + // read handlers) and passes media.v/schema:upload, which only + // checks the shape, not where the path came from. The attacker + // also controls :size and :mtype, so validate-media-type! and + // validate-media-size! pass with "image/png" and a small size. + // 3. upload-file-media-object then reads (:path content) in + // process-image (media/run :info) and stores the bytes as a + // media object, which is later downloadable by URL. + // + // EXPECTED BEHAVIOR AFTER FIX: 400 validation/params-validation, + // without touching the file. While vulnerable, the request passes + // validation and fails later in media processing, proving the + // file was opened. + const transitBody = JSON.stringify({ + "~:file-id": `~u${fileId}`, + "~:is-local": true, + "~:name": "pwned", + "~:content": { + "~:filename": "evil.png", + "~:size": 123, + "~:mtype": "image/png", + "~:path": ["~#path", "/etc/passwd"], + }, + }); + + const res = await rpcPost("upload-file-media-object", transitBody, { + cookieToken: cookie, + contentType: "application/transit+json", + accept: "application/json", + }); + + assert.equal( + res.status, + 400, + `forged :content path must be rejected with 400, got ${res.status}: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorType(res.body), + "validation", + `expected validation error, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorCode(res.body), + "params-validation", + `forged :content path must fail at params validation, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + }); + + it("rejects a hand-forged chunk :content path in a transit body", async () => { + // Same vector through upload-chunk: the forged path is read with + // sto/content and stored as a chunk blob. While vulnerable this + // returns 200 (the internal file bytes are stored under the + // attacker session, retrievable via assemble) — proving the file + // was read and kept. After the fix it must be 400 + // validation/params-validation. + const sessionId = await createUploadSession(cookie); + + const transitBody = JSON.stringify({ + "~:session-id": `~u${sessionId}`, + "~:index": 0, + "~:content": { + "~:filename": "c", + "~:size": 123, + "~:mtype": "application/octet-stream", + "~:path": ["~#path", "/etc/passwd"], + }, + }); + + const res = await rpcPost("upload-chunk", transitBody, { + cookieToken: cookie, + contentType: "application/transit+json", + accept: "application/json", + }); + + assert.equal( + res.status, + 400, + `forged chunk :content path must be rejected with 400, got ${res.status}: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorType(res.body), + "validation", + `expected validation error, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + assert.equal( + errorCode(res.body), + "params-validation", + `forged chunk :content path must fail at params validation, got: ${JSON.stringify(res.body).slice(0, 500)}` + ); + }); + + it("plain JSON string path stays rejected", async () => { + // Control case: without the transit "~#path" tag the path arrives + // as a plain string and ::fs/path already rejects it. This test + // documents that only the transit vector bypasses the check. + const res = await rpcPost( + "upload-file-media-object", + { + fileId, + isLocal: true, + name: "pwned", + content: { + filename: "evil.png", + size: 123, + mtype: "image/png", + path: "/etc/passwd", + }, + }, + { cookieToken: cookie } + ); + + assert.equal(res.status, 400); + assert.equal(errorType(res.body), "validation"); + assert.equal(errorCode(res.body), "params-validation"); + }); +});