penpot/backend/test/backend_tests/http_middleware_test.clj
Andrey Antukh dd4a163217
🐛 Remove internal error details from HTTP error responses (#11288)
* 🚑 Remove internal error details from HTTP error responses

PostgreSQL exceptions, I/O exceptions, and unhandled errors were
leaking raw database messages (table names, constraint names,
SQLSTATE codes), filesystem paths, and internal exception details
to API clients via :hint, :state, and :path response fields.

Remove these fields from server-error responses while keeping
full error context in server-side logs for operators.

Closes #11287

AI-assisted-by: mimo-v2.5-pro

* 🚑 Strip internal fields and map PG errors to safe messages

Complete the security fix for GHSA-r8wx-23q6-w3gf by addressing
the incomplete redaction found in code review.

Add strip-internal-fields helper to dissoc :hint, :state, :path,
and :context from error response data in three handlers that
previously passed raw ex-data through to clients:

- handle-error :internal
- handle-exception :default (else branch)
- handle-error :assertion (else branch)

Add pgsql-state->message to map PostgreSQL SQLSTATE codes to safe,
client-facing messages (e.g. 23505 → "A conflicting entry already
exists") instead of returning raw PG error text. Include :message
in all PSQLException response branches.

Add regression tests asserting :hint, :state, :path, :context are
absent from responses for :internal and unhandled ex-info errors.

Closes #11287

AI-assisted-by: mimo-v2.5-pro

* 🚑 Keep :hint in error protocol, fix unsafe sources

Refine the security fix based on code review feedback.

Keep :hint as part of the error protocol — it is essential for
controlled error communication. Remove it from strip-internal-fields
(which now only strips :state, :path, :context).

Fix the actual sources of unsafe :hint values:

- http/middleware.clj: replace (ex-message cause) with safe static
  strings for IllegalArgumentException, RequestTooBigException, and
  EOFException. These :validation errors return ex-data verbatim
  to clients, so raw exception messages were leaking internals.

- PSQLException handler: use :hint instead of :message for the
  SQLSTATE-mapped messages, staying consistent with the error
  protocol.

Update tests to assert :hint is present (with safe static values)
in :internal and unhandled ex-info responses, and absent only from
bare RuntimeException and IOException responses.

Closes #11287

AI-assisted-by: mimo-v2.5-pro
2026-08-21 12:43:23 +02:00

444 lines
19 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 INC Sucursal en España SL
(ns backend-tests.http-middleware-test
(:require
[app.common.exceptions :as ex]
[app.common.time :as ct]
[app.db :as db]
[app.http :as-alias http]
[app.http.access-token]
[app.http.errors :as http-errors]
[app.http.middleware :as mw]
[app.http.session :as session]
[app.main :as-alias main]
[app.rpc :as-alias rpc]
[app.rpc.commands.access-token]
[app.tokens :as tokens]
[backend-tests.helpers :as th]
[clojure.string :as str]
[clojure.test :as t]
[mockery.core :refer [with-mocks]]
[yetti.request :as yreq]
[yetti.response :as yres])
(:import
io.undertow.server.RequestTooBigException))
(t/use-fixtures :once th/state-init)
(t/use-fixtures :each th/database-reset)
(defrecord DummyRequest [headers cookies method body-stream
remote-addr server-name server-port
scheme protocol path query ssl-client-cert]
yreq/IRequestCookies
(get-cookie [_ name]
{:value (get cookies name)})
yreq/IRequest
(get-header [_ name]
(get headers name))
(method [_] method)
(body [_] body-stream)
(path [_] path)
(query [_] query)
(server-port [_] server-port)
(server-name [_] server-name)
(remote-addr [_] remote-addr)
(ssl-client-cert [_] ssl-client-cert)
(scheme [_] scheme)
(protocol [_] protocol))
(defn- make-dummy-request
"Constructs a DummyRequest from an options map. Every key is
optional; missing values fall back to sensible defaults. New
fields added to DummyRequest won't break existing call sites
as long as this constructor keeps its `:or` defaults in sync.
Recognized keys:
:headers — map of header name → value
:cookies — map of cookie name → value
:method — HTTP method keyword (default :get)
:body-stream — InputStream for the body (used directly)
:body-bytes — bytes or string for the body; wrapped in a
ByteArrayInputStream if :body-stream is not
given
:remote-addr — string (default \"127.0.0.1\")
:server-name — string (default \"test\")
:server-port — long (default 0)
:scheme — keyword (default :http)
:protocol — string (default \"HTTP/1.1\")
:path — string (default \"/test\")
:query — string or nil (default nil)
:ssl-client-cert — X509Certificate or nil (default nil)"
[{:keys [headers cookies method body-stream body-bytes
remote-addr server-name server-port scheme protocol
path query ssl-client-cert]
:or {headers {} cookies {} method :get
body-stream nil
remote-addr "127.0.0.1" server-name "test" server-port 0
scheme :http protocol "HTTP/1.1" path "/test" query nil
ssl-client-cert nil}}]
(let [body-stream (or body-stream
(when body-bytes
(java.io.ByteArrayInputStream.
(if (string? body-bytes)
(.getBytes ^String body-bytes "UTF-8")
body-bytes))))]
(->DummyRequest headers cookies method body-stream
remote-addr server-name server-port
scheme protocol path query ssl-client-cert)))
(t/deftest auth-middleware-1
(let [request (volatile! nil)
handler (#'app.http.middleware/wrap-auth
(fn [req] (vreset! request req))
{})]
(handler (make-dummy-request {}))
(t/is (nil? (::http/auth-data @request)))
(handler (make-dummy-request {:headers {"authorization" "Token aaaa"}}))
(let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)]
(t/is (= :token token-type))
(t/is (= "aaaa" token))
(t/is (nil? claims)))))
(t/deftest auth-middleware-2
(let [request (volatile! nil)
handler (#'app.http.middleware/wrap-auth
(fn [req] (vreset! request req))
{})]
(handler (make-dummy-request {}))
(t/is (nil? (::http/auth-data @request)))
(handler (make-dummy-request {:headers {"authorization" "Bearer aaaa"}}))
(let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)]
(t/is (= :bearer token-type))
(t/is (= "aaaa" token))
(t/is (nil? claims)))))
(t/deftest auth-middleware-3
(let [request (volatile! nil)
handler (#'app.http.middleware/wrap-auth
(fn [req] (vreset! request req))
{})]
(handler (make-dummy-request {}))
(t/is (nil? (::http/auth-data @request)))
(handler (make-dummy-request {:cookies {"auth-token" "foobar"}}))
(let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)]
(t/is (= :cookie token-type))
(t/is (= "foobar" token))
(t/is (nil? claims)))))
(t/deftest shared-key-auth
(let [handler (#'app.http.middleware/wrap-shared-key-auth
(fn [req] {::yres/status 200})
{:test1 "secret-key"})]
(let [response (handler (make-dummy-request {}))]
(t/is (= 403 (::yres/status response))))
(let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))]
(t/is (= 403 (::yres/status response))))
(let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key"}}))]
(t/is (= 403 (::yres/status response))))
(let [response (handler (make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))]
(t/is (= 200 (::yres/status response))))))
(t/deftest access-token-authz
(let [profile (th/create-profile* 1)
token (db/tx-run! th/*system* app.rpc.commands.access-token/create-access-token (:id profile) "test" nil nil)
handler (#'app.http.access-token/wrap-authz identity th/*system*)]
(let [response (handler nil)]
(t/is (nil? response)))
(let [response (handler {::http/auth-data {:type :token :token "foobar" :claims {:tid (:id token)}}})]
(t/is (= #{} (:app.http.access-token/perms response)))
(t/is (= (:id profile) (:app.http.access-token/profile-id response))))))
(t/deftest access-token-authz-sets-token-id-and-type
(let [profile (th/create-profile* 1)
token (db/tx-run! th/*system* app.rpc.commands.access-token/create-access-token
(:id profile) "test" nil "mcp")
handler (#'app.http.access-token/wrap-authz identity th/*system*)
request {::http/auth-data {:type :token :token "foobar" :claims {:tid (:id token)}}}
response (handler request)]
;; Must set ::actoken/id from claims :tid
(t/is (= (:id token) (:app.http.access-token/id response)))
;; Must set ::actoken/type from database
(t/is (= "mcp" (:app.http.access-token/type response)))
;; Existing assertions still pass
(t/is (= #{} (:app.http.access-token/perms response)))
(t/is (= (:id profile) (:app.http.access-token/profile-id response)))))
(defrecord MethodAwareDummyRequest [req-method headers]
yreq/IRequest
(method [_] req-method)
(get-header [_ name] (get headers name)))
(t/deftest cors-middleware-allowlisted-origin
(let [handler (#'app.http.middleware/wrap-cors
(fn [_] {::yres/status 200 ::yres/headers {}})
#{"https://trusted.example"})
resp (handler (->MethodAwareDummyRequest :get {"origin" "https://trusted.example"}))
headers (::yres/headers resp)]
(t/is (= 200 (::yres/status resp)))
(t/is (= "https://trusted.example" (get headers "access-control-allow-origin")))
(t/is (= "true" (get headers "access-control-allow-credentials")))
(t/is (= "Origin" (get headers "vary")))
(t/is (= "content-type" (get headers "access-control-expose-headers")))
(t/is (not (str/includes?
(get headers "access-control-allow-headers" "")
"cookie")))))
(t/deftest cors-middleware-non-allowlisted-origin
(let [handler (#'app.http.middleware/wrap-cors
(fn [_] {::yres/status 200 ::yres/headers {}})
#{"https://trusted.example"})
resp (handler (->MethodAwareDummyRequest :get {"origin" "https://attacker.example"}))
headers (::yres/headers resp)]
(t/is (= 200 (::yres/status resp)))
(t/is (nil? (get headers "access-control-allow-origin")))
(t/is (nil? (get headers "access-control-allow-credentials")))
(t/is (nil? (get headers "access-control-allow-headers")))
(t/is (nil? (get headers "access-control-expose-headers")))
(t/is (= "Origin" (get headers "vary")))))
(t/deftest cors-middleware-preflight-allowlisted
(let [handler (#'app.http.middleware/wrap-cors
(fn [_] {::yres/status 200 ::yres/headers {}})
#{"https://trusted.example"})
resp (handler (->MethodAwareDummyRequest :options {"origin" "https://trusted.example"}))
headers (::yres/headers resp)]
(t/is (= 204 (::yres/status resp)))
(t/is (= "https://trusted.example" (get headers "access-control-allow-origin")))
(t/is (= "true" (get headers "access-control-allow-credentials")))))
(t/deftest cors-middleware-preflight-non-allowlisted
(let [handler (#'app.http.middleware/wrap-cors
(fn [_] {::yres/status 200 ::yres/headers {}})
#{"https://trusted.example"})
resp (handler (->MethodAwareDummyRequest :options {"origin" "https://attacker.example"}))
headers (::yres/headers resp)]
(t/is (= 204 (::yres/status resp)))
(t/is (nil? (get headers "access-control-allow-origin")))
(t/is (nil? (get headers "access-control-allow-credentials")))))
(t/deftest cors-middleware-missing-origin
(let [handler (#'app.http.middleware/wrap-cors
(fn [_] {::yres/status 200 ::yres/headers {}})
#{"https://trusted.example"})
resp (handler (->MethodAwareDummyRequest :get {}))
headers (::yres/headers resp)]
(t/is (= 200 (::yres/status resp)))
(t/is (nil? (get headers "access-control-allow-origin")))
(t/is (nil? (get headers "access-control-allow-credentials")))))
(t/deftest session-authz
(let [cfg th/*system*
manager (session/inmemory-manager)
profile (th/create-profile* 1)
handler (-> (fn [req] req)
(#'session/wrap-authz {::session/manager manager})
(#'mw/wrap-auth {:bearer (partial session/decode-token cfg)
:cookie (partial session/decode-token cfg)}))
session (->> (session/create-session manager {:profile-id (:id profile)
:user-agent "user agent"})
(#'session/assign-token cfg))
response (handler (make-dummy-request {:cookies {"auth-token" (:token session)}}))
{:keys [token claims] token-type :type}
(get response ::http/auth-data)]
(t/is (= :cookie token-type))
(t/is (= (:token session) token))
(t/is (= "authentication" (:iss claims)))
(t/is (= "penpot" (:aud claims)))
(t/is (= (:id session) (:sid claims)))
(t/is (= (:id profile) (:uid claims)))))
(t/deftest parse-request-illegal-argument-exception
;; clojure.data.json raises IllegalArgumentException (case
;; fall-through) on several kinds of malformed input. The
;; parse-request middleware should convert any such IAE into a
;; 400 :malformed-json validation error rather than letting it
;; surface as a 500 internal error. Because the conversion is
;; done by raising an ex-info (caught by the top-level error
;; 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}))
;; Body contains the bytes for: {"x": "\}"} -- a string
;; value with a backslash followed by '}', which
;; clojure.data.json v0.5.x cannot handle.
body (.getBytes "{\"x\": \"\\}\"}" "UTF-8")
request (make-dummy-request
{:method :post
:headers {"content-type" "application/json"}
:body-bytes body})
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 (= "invalid JSON in request body" (-> ex ex-data :hint)))))
(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"))))
request (make-dummy-request
{:method :post
:headers {"content-type" "application/json"}
:body-bytes (.getBytes "{}" "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 (= :request-body-too-large (-> ex ex-data :code)))
(t/is (= "request body exceeds size limit" (-> ex ex-data :hint)))))
(t/deftest parse-request-eof-exception
;; When java.io.EOFException is raised (e.g. the body stream
;; 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"))))
request (make-dummy-request
{:method :post
:headers {"content-type" "application/json"}
:body-bytes (.getBytes "{}" "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 (= "unexpected end of request body" (-> ex ex-data :hint)))))
(t/deftest parse-request-runtime-exception-with-cause
;; When a RuntimeException with a non-nil ex-cause is raised,
;; the middleware should recurse on the cause and dispatch
;; through the specific-exception branches. Here we wrap an
;; IllegalArgumentException in a RuntimeException and verify
;; it surfaces as :malformed-json.
(let [iae (IllegalArgumentException. "No matching clause: 99")
wrapped (doto (RuntimeException. "wrapped")
(.initCause iae))
handler (#'app.http.middleware/wrap-parse-request
(fn [_] (throw wrapped)))
request (make-dummy-request
{:method :post
:headers {"content-type" "application/json"}
:body-bytes (.getBytes "{}" "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/deftest parse-request-runtime-exception-without-cause
;; When a bare RuntimeException (no ex-cause) is raised, the
;; middleware should fall through to errors/handle's :default
;; 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"))))
request (make-dummy-request
{:method :post
:headers {"content-type" "application/json"}
:body-bytes (.getBytes "{}" "UTF-8")})
response (handler request)
body (::yres/body response)]
(t/is (= 500 (::yres/status response)))
(t/is (= :server-error (:type body)))
(t/is (= :unexpected (:code body)))
(t/is (nil? (:hint body)))))
(t/deftest parse-request-non-runtime-throwable
;; When a non-RuntimeException Throwable is raised (e.g. an
;; Error subclass or a non-RuntimeException checked-style
;; exception), the middleware should fall through to the
;; :else branch and call errors/handle. java.io.IOException
;; 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"))))
request (make-dummy-request
{:method :post
:headers {"content-type" "application/json"}
:body-bytes (.getBytes "{}" "UTF-8")})
response (handler request)
body (::yres/body response)]
(t/is (= 500 (::yres/status response)))
(t/is (= :server-error (:type body)))
(t/is (= :io-exception (:code body)))
(t/is (nil? (:hint body)))))
(t/deftest internal-error-strips-sensitive-fields
;; When an :internal error is raised with :state, :path, and
;; :context, those fields must not appear in the response body.
;; :hint is part of the error protocol and is preserved.
(let [cause (ex-info "internal error"
{:type :internal
:code :test-error
:hint "safe user-facing hint"
:state "XX000"
:path "/data/penpot/storage"
:context {:backend :s3 :bucket "prod"}})
response (http-errors/handle cause {})
body (::yres/body response)]
(t/is (= 500 (::yres/status response)))
(t/is (= :server-error (:type body)))
(t/is (= :test-error (:code body)))
(t/is (= "safe user-facing hint" (:hint body)))
(t/is (nil? (:state body)))
(t/is (nil? (:path body)))
(t/is (nil? (:context body)))))
(t/deftest unhandled-exinfo-strips-sensitive-fields
;; When an ex-info with an unregistered :type (dispatches through
;; handle-exception :default :else) carries :state and :path,
;; those fields must not appear in the response body.
;; :hint is part of the error protocol and is preserved.
(let [cause (ex-info "something broke"
{:type :unregistered-type
:code :custom-code
:hint "safe user-facing hint"
:state "internal-state"
:path "/internal/path"})
response (http-errors/handle cause {})
body (::yres/body response)]
(t/is (= 500 (::yres/status response)))
(t/is (= :server-error (:type body)))
(t/is (= :custom-code (:code body)))
(t/is (= "safe user-facing hint" (:hint body)))
(t/is (nil? (:state body)))
(t/is (nil? (:path body)))))