From dd4a16321796a534c1457508bd6de195aa83568f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 12:43:23 +0200 Subject: [PATCH] :bug: Remove internal error details from HTTP error responses (#11288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :ambulance: 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 * :ambulance: 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 * :ambulance: 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 --- backend/src/app/http/errors.clj | 45 +++++++++++----- backend/src/app/http/middleware.clj | 6 +-- .../backend_tests/http_middleware_test.clj | 53 +++++++++++++++++-- 3 files changed, 82 insertions(+), 22 deletions(-) diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index f1eaea621c..2393abf129 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -34,6 +34,12 @@ (assoc :request/auth-data (dissoc auth :token)) (assoc :frontend/version (or (yreq/get-header request "x-frontend-version") "unknown"))))) +(defn- strip-internal-fields + "Remove fields that leak internal implementation details from error + response data. Full context is preserved in server-side logs." + [data] + (dissoc data :state :path :context)) + (defmulti handle-error (fn [cause _ _] (-> cause ex-data :type))) @@ -136,6 +142,7 @@ (l/error :hint "assertion error" :cause cause) {::yres/status 500 ::yres/body (-> data + (strip-internal-fields) (assoc :type :server-error) (assoc :code :assertion))}))))) @@ -161,9 +168,9 @@ (l/error :hint "internal error" :cause cause) {::yres/status 500 ::yres/body (-> data + (strip-internal-fields) (assoc :type :server-error) - (update :code #(or % :unhandled)) - (assoc :hint (ex-message error)))}))) + (update :code #(or % :unhandled)))}))) (defmethod handle-error :default [error request parent-cause] @@ -178,6 +185,20 @@ (handle-exception (:handling edata) request error) (handle-exception error request parent-cause)))) +(defn- pgsql-state->message + "Map PostgreSQL SQLSTATE codes to safe, client-facing messages. + Returns a user-friendly string that conveys the nature of the error + without exposing table names, constraint names, or other internals." + [state] + (case state + "23505" "A conflicting entry already exists" + "23503" "The referenced item does not exist" + "23502" "A required field is missing" + "23514" "The value violates a data integrity constraint" + "57014" "The operation took too long and was cancelled" + "25P03" "The transaction was idle too long and was cancelled" + "A database error occurred")) + (defmethod handle-exception org.postgresql.util.PSQLException [error request parent-cause] (let [state (.getSQLState ^java.sql.SQLException error) @@ -190,20 +211,19 @@ {::yres/status 504 ::yres/body {:type :server-error :code :statement-timeout - :hint (ex-message error)}} + :hint (pgsql-state->message state)}} (= state "25P03") {::yres/status 504 ::yres/body {:type :server-error :code :idle-in-transaction-timeout - :hint (ex-message error)}} + :hint (pgsql-state->message state)}} :else {::yres/status 500 ::yres/body {:type :server-error - :code :unexpected - :hint (ex-message error) - :state state}})))) + :code :database-error + :hint (pgsql-state->message state)}})))) (defmethod handle-exception :default [error request parent-cause] @@ -216,17 +236,16 @@ (l/error :hint "unexpected error" :cause cause) {::yres/status 500 ::yres/body {:type :server-error - :code :unexpected - :hint (ex-message error)}}) + :code :unexpected}}) :else (binding [l/*context* (request->context request)] (l/error :hint "unhandled error" :cause cause) {::yres/status 500 ::yres/body (-> edata + (strip-internal-fields) (assoc :type :server-error) - (update :code #(or % :unhandled)) - (assoc :hint (ex-message error)))})))) + (update :code #(or % :unhandled)))})))) (defmethod handle-exception java.io.IOException [cause request _] @@ -234,9 +253,7 @@ (l/wrn :hint "io exception" :cause cause) {::yres/status 500 ::yres/body {:type :server-error - :code :io-exception - :hint (ex-message cause) - :path (:path request)}})) + :code :io-exception}})) (defmethod handle-exception java.util.concurrent.CompletionException [cause request _] diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 31b96927a6..6cb8e6b8b7 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -83,18 +83,18 @@ (instance? IllegalArgumentException cause) (ex/raise :type :validation :code :malformed-json - :hint (ex-message cause) + :hint "invalid JSON in request body" :cause cause) (instance? RequestTooBigException cause) (ex/raise :type :validation :code :request-body-too-large - :hint (ex-message cause)) + :hint "request body exceeds size limit") (instance? java.io.EOFException cause) (ex/raise :type :validation :code :malformed-json - :hint (ex-message cause) + :hint "unexpected end of request body" :cause cause) (instance? RuntimeException cause) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index bd986fc031..bca962d3fc 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -6,10 +6,12 @@ (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] @@ -300,7 +302,7 @@ (t/is (instance? clojure.lang.ExceptionInfo ex)) (t/is (= :validation (-> ex ex-data :type))) (t/is (= :malformed-json (-> ex ex-data :code))) - (t/is (string? (-> ex ex-data :hint))))) + (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 @@ -319,7 +321,7 @@ (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 (string? (-> ex ex-data :hint))))) + (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 @@ -337,7 +339,7 @@ (t/is (instance? clojure.lang.ExceptionInfo ex)) (t/is (= :validation (-> ex ex-data :type))) (t/is (= :malformed-json (-> ex ex-data :code))) - (t/is (string? (-> ex ex-data :hint))))) + (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, @@ -377,7 +379,7 @@ (t/is (= 500 (::yres/status response))) (t/is (= :server-error (:type body))) (t/is (= :unexpected (:code body))) - (t/is (= "boom" (:hint body))))) + (t/is (nil? (:hint body))))) (t/deftest parse-request-non-runtime-throwable ;; When a non-RuntimeException Throwable is raised (e.g. an @@ -397,4 +399,45 @@ (t/is (= 500 (::yres/status response))) (t/is (= :server-error (:type body))) (t/is (= :io-exception (:code body))) - (t/is (= "network gone" (:hint 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)))))