From 764b62906b868f2d860b54fd73ea6b31f2624b33 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 31 Jul 2026 12:06:19 +0200 Subject: [PATCH] :bug: Handle unrecognized JSON escape sequences as malformed-json (#10808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :paperclip: Update serena documentation about creating-prs workflow * :bug: Handle unrecognized JSON escape sequences as malformed-json When clojure.data.json's read-escaped-char encounters an unrecognized escape sequence (e.g. a backslash followed by '}', or other case fall-throughs in the parser) in a JSON request body, it throws a bare IllegalArgumentException. Previously this fell through to the generic RuntimeException branch in wrap-parse-request's handle-error, which unwrapped and recurred without matching, eventually reaching the internal-error handler and producing HTTP 500 + an error report — even though the root cause was malformed client input, not a server bug. The fix converts any IllegalArgumentException raised in the JSON parse path into a `:validation`/`:malformed-json` error by raising a new ex-info (which is caught by the top-level error handler in `app.http/router-handler`). The result is an HTTP 400 response with a descriptive hint, and no error report is generated. This addresses ~10% of all error reports received. The new IAE branch is placed before the RuntimeException branch in the cond (since IllegalArgumentException IS-A RuntimeException) and uses the throw-style (ex/raise) to match the existing RequestTooBigException / EOFException branches. A comment above the handle-error cond documents why raising is intentional and is caught by the top-level app.http error handler, not by the per-route wrap-errors middleware. Test suite changes: - Extend the existing `DummyRequest` defrecord in `http_middleware_test.clj` from 2 fields to 12 fields, implementing every IRequest method, and add a private `make-dummy-request` constructor that accepts an options map with every key optional and sensible `:or` defaults. Future fields added to DummyRequest won't break existing call sites as long as the `:or` defaults are kept in sync. - Remove the now-redundant `JsonRequest` defrecord and migrate all 11 `->DummyRequest` call sites to `make-dummy-request`. - Add 6 new deftest cases: - parse-request-illegal-argument-exception: malformed JSON body (containing `\}`) is converted to `:malformed-json`. - parse-request-request-too-big-exception: RequestTooBigException is converted to `:request-body-too-large`. - parse-request-eof-exception: java.io.EOFException is converted to `:malformed-json`. - parse-request-runtime-exception-with-cause: a wrapped RuntimeException recurses on ex-cause and dispatches to the matching specific branch. - parse-request-runtime-exception-without-cause: a bare RuntimeException falls through to errors/handle, returning 500 with :type :server-error :code :unexpected. - parse-request-non-runtime-throwable: java.io.IOException (a non-RuntimeException Throwable) is handled by the dedicated handle-exception method, returning 500 with :code :io-exception. Together, the new tests cover all 6 branches of wrap-parse-request's handle-error cond. Refs #10804. AI-assisted-by: minimax-m3 --- .serena/memories/workflow/creating-prs.md | 2 +- backend/src/app/http/middleware.clj | 26 ++- .../backend_tests/http_middleware_test.clj | 206 ++++++++++++++++-- 3 files changed, 215 insertions(+), 19 deletions(-) diff --git a/.serena/memories/workflow/creating-prs.md b/.serena/memories/workflow/creating-prs.md index 00252a7661..13bde56149 100644 --- a/.serena/memories/workflow/creating-prs.md +++ b/.serena/memories/workflow/creating-prs.md @@ -30,7 +30,7 @@ See `mem:workflow/creating-commits` for emoji codes. Squash merge uses the PR ti Include concise sections covering: - what changed and why; -- related GitHub issues or Taiga stories (`Fixes #NNNN`, `Relates to #NNNN`, `Taiga #NNNN`); +- related GitHub issues or Taiga stories (`Closes #NNNN`, `Relates to #NNNN`, `Taiga #NNNN`); - screenshots or recordings for UI-visible changes; - testing performed and residual risk; - breaking changes or migration notes, if any. diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 81b1ef13ec..fa2faa8a55 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -65,12 +65,25 @@ :else request))) + ;; The specific-exception branches below (IAE, + ;; RequestTooBigException, EOFException) raise with + ;; `ex/raise` rather than calling `errors/handle` directly. + ;; This is intentional: the throw is caught by the + ;; top-level error handler in `app.http/router-handler` + ;; (`backend/src/app/http.clj`), which routes every + ;; uncaught exception through `errors/handle`. The + ;; per-route `wrap-errors` middleware in the route list + ;; is a defensive layer; correctness does not depend on + ;; it. Raising here keeps the cond uniform with the + ;; existing RequestTooBigException / EOFException + ;; branches. (handle-error [cause request] (cond - (instance? RuntimeException cause) - (if-let [cause (ex-cause cause)] - (handle-error cause request) - (errors/handle cause request)) + (instance? IllegalArgumentException cause) + (ex/raise :type :validation + :code :malformed-json + :hint (ex-message cause) + :cause cause) (instance? RequestTooBigException cause) (ex/raise :type :validation @@ -83,6 +96,11 @@ :hint (ex-message cause) :cause cause) + (instance? RuntimeException cause) + (if-let [cause (ex-cause cause)] + (handle-error cause request) + (errors/handle cause request)) + :else (errors/handle cause request)))] diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index f751d8aca6..bd986fc031 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -21,19 +21,73 @@ [clojure.test :as t] [mockery.core :refer [with-mocks]] [yetti.request :as yreq] - [yetti.response :as yres])) + [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] +(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))) + (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) @@ -41,11 +95,11 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {"authorization" "Token aaaa"} {})) + (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)) @@ -58,10 +112,10 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {"authorization" "Bearer aaaa"} {})) + (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)) @@ -74,10 +128,10 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {} {"auth-token" "foobar"})) + (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)) @@ -89,16 +143,16 @@ (fn [req] {::yres/status 200}) {:test1 "secret-key"})] - (let [response (handler (->DummyRequest {} {}))] + (let [response (handler (make-dummy-request {}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "secret-key2"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "secret-key"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "test1 secret-key"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))] (t/is (= 200 (::yres/status response)))))) (t/deftest access-token-authz @@ -209,7 +263,7 @@ :user-agent "user agent"}) (#'session/assign-token cfg)) - response (handler (->DummyRequest {} {"auth-token" (:token session)})) + response (handler (make-dummy-request {:cookies {"auth-token" (:token session)}})) {:keys [token claims] token-type :type} (get response ::http/auth-data)] @@ -220,3 +274,127 @@ (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 (string? (-> 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 (string? (-> 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 (string? (-> 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 (= "boom" (: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 (= "network gone" (:hint body)))))