penpot/backend/test/backend_tests/rpc_binfile_test.clj
Andrey Antukh 70b317a976
🐛 Harden upload path handling and validation error bodies (#12067)
* 🚑 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

* 🐛 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

* 🐛 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

* 🐛 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

* 🐛 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
2026-10-17 16:01:04 +02:00

158 lines
6.7 KiB
Clojure

;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS SUBSIDIARY SL
(ns backend-tests.rpc-binfile-test
(:require
[app.binfile.v3 :as bf.v3]
[app.common.schema :as sm]
[app.common.uuid :as uuid]
[app.config :as cf]
[app.db :as db]
[app.rpc :as-alias rpc]
[app.rpc.commands.binfile :as binfile]
[backend-tests.helpers :as th]
[clojure.test :as t]
[datoteka.fs :as fs]
[mockery.core :refer [with-mocks]]))
(t/use-fixtures :once th/state-init)
(t/use-fixtures :each th/database-reset)
(t/deftest import-binfile-schema-omits-file-id
;; N1-06: file-id parameter must be removed from schema for security
(let [schema @#'binfile/schema:import-binfile
validator (sm/lazy-validator schema)
valid-params
{:name "test"
:project-id (uuid/random)
:version 3
:upload-id (uuid/random)}
params-with-file-id
(assoc valid-params :file-id (uuid/random))]
(t/is (true? (validator valid-params))
"params without file-id should be valid")
(t/is (not (contains? (sm/keys (second schema)) :file-id))
"file-id should not be a declared parameter")
;; Params with file-id should fail (schema closed)
(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
validator (sm/lazy-validator schema)
base-params {:name "test"
:project-id (uuid/random)
:upload-id (uuid/random)}]
;; Version 1 should be accepted
(t/is (true? (validator (assoc base-params :version 1)))
"version 1 should be valid")
;; Version 3 should be accepted
(t/is (true? (validator (assoc base-params :version 3)))
"version 3 should be valid")
;; Version 2 should be rejected
(t/is (false? (validator (assoc base-params :version 2)))
"version 2 should be rejected")
;; Version 0 should be rejected
(t/is (false? (validator (assoc base-params :version 0)))
"version 0 should be rejected")
;; Negative version should be rejected
(t/is (false? (validator (assoc base-params :version -1)))
"negative version should be rejected")
;; Version 4 should be rejected
(t/is (false? (validator (assoc base-params :version 4)))
"version 4 should be rejected")))
(t/deftest import-binfile-audit-events-carry-entity-ids
;; The imported files are created inside the importer, so the command emits a
;; create-file event per file for a local projection.
(with-mocks [audit-mock {:target 'app.loggers.audit/submit :return nil}]
(with-redefs [cf/flags (conj cf/flags :audit-log)]
(let [profile (th/create-profile* 1 {:is-active true})
team (th/create-team* 51 {:profile-id (:id profile)})
project (th/create-project* 51 {:profile-id (:id profile)
:team-id (:id team)})
;; A real row: `submit-file-created-event` reads the light
;; columns back, so a phantom id would emit no event.
file (th/create-file* 99 {:profile-id (:id profile)
:project-id (:id project)})
file-id (:id file)]
(with-redefs [bf.v3/import-files! (fn [_cfg] {:file-ids [file-id]})]
(#'binfile/import-binfile
th/*system*
{::rpc/profile-id (:id profile)
:profile-id (:id profile)
:project-id (:id project)
:version 3
:name "imported"
:file {:filename "package.penpot"
:path (th/tempfile "backend_tests/test_files/svg-attrs-camel-case.penpot")
:mtype "application/zip"
:size 1}}))
(let [events (->> (:call-args-list @audit-mock)
(map second)
(filter #(= "create-file" (:name %))))]
(t/is (= 1 (count events)))
(t/is (= file-id (get-in (first events) [:props :id])))
(t/is (= (:id project) (get-in (first events) [:props :project-id])))
(t/is (= (:id team) (get-in (first events) [:props :team-id]))))))))