🐛 Handle unrecognized JSON escape sequences as malformed-json (#10808)

* 📎 Update serena documentation about creating-prs workflow

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

@@ -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.

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)))]
 

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)))))