mirror of
https://github.com/penpot/penpot.git
synced 2026-10-28 21:40:34 +00:00
🐛 Stop oversized audit events from wedging the queue
Port staging c74d415 (#12252) to main, adapted to this branch. The :export-shapes modal event carried the full :exports vector, each export embedding its shape, which could reach megabytes and wedge the audit archive queue past the proxy body limit. Build a compact body instead: totals, distinct shapes and per-type counts, plus the page name. Other modals keep their props. Move the modal :origin prop to ::ev/origin so make-proto-event records it as :event-origin; the UI still reads the plain prop. Bound exception messages at the source: get-hint clamps to max-message-length (1000 chars) and the CLJS format-throwable clamps the trace message line and honors :trace-length. The Context Hint header in generate-report prints get-hint instead of the raw hint. Also fix first-line on the JVM, where cuerdas/index-of returns nil and (pos? nil) throws. Adaptations to main: skip audit-log.md (absent here), skip the generate-report huge-message test (staging async helpers are absent), and fit the errors.cljs hunk to main's generate-report. AI-assisted-by: muse-spark-1.3-contributor
This commit is contained in:
parent
d57cdb4588
commit
334f727e87
@ -18,3 +18,4 @@ assumption; a wrong assumption here already cost a review round.
|
||||
- `^:async` fns never throw synchronously (rejected promises instead);
|
||||
`try/catch/finally` supported; continuations are microtasks, timers and
|
||||
RxJS schedulers are macrotasks (FIFO).
|
||||
- `cuerdas.core/index-of` returns `nil` (not `-1`) when the substring is absent, like `clojure.string/index-of`. `(pos? nil)` **throws on the JVM** but evaluates to `false` in CLJS (JS coercion), so `(if (pos? (str/index-of s "\n")) …)` works in the browser and crashes the backend. Guard with `some?` before `pos?` (this was `app.common.exceptions/first-line`).
|
||||
|
||||
@ -16,10 +16,11 @@
|
||||
- `flash` accepts a `:links` vector of `{:label :callback}` actions, appended after the download link that `:report-link?` produces. `flash-persistence` passes a reload link, so a terminal save failure hands the user both the report and the only action that restarts persistence for the session. `labels.reload-page` plus `js/location.reload` is the established full-restart pattern; the link cannot be invoked under the test runner because `js/location` does not exist there, so it is covered by its label and by being callable.
|
||||
- `flash-persistence` only delegates to another handler for `:authentication`, `:not-found` and `:code :vern-conflict`. Everything else it reports itself, and the report type is derived from the cause: `#{:validation :server-error :internal}` are application defects and report as `unhandled-exception` so they reach the internal reports and the alerts; the rest stay `handled-exception`. `:cause-type` wins over `:type` when classifying, because `persistence-failed` wraps the original cause under `:type :persistence`. Environment failures are deliberately kept handled: they land here after the retry budget is spent and the user's connection is not our bug. `:assertion` is also kept handled, matching its own handler.
|
||||
- The report governor is global and stateful across tests: two reports with the same fingerprint inside `report-window-ms` collapse. Call `errors/reset-report-governor!` between assertions that expect an emission.
|
||||
- Error messages are bounded at the source: `app.common.exceptions/get-hint` clamps to `max-message-length` (1000 chars) and the CLJS `format-throwable` clamps the message line of the stack trace and honors `:trace-length`. A message can embed a whole payload (a serialized file, for example) and reach megabytes; the bound keeps the audit `:hint` prop and the `:report` string small. The `Context: Hint:` header in `generate-report` prints `get-hint` (not the raw `:hint` from `ex-data`) for the same reason.
|
||||
|
||||
## Store and websocket
|
||||
|
||||
For general store mechanics such as `emit!`, `last-events`, persistence, and undo, read `mem:frontend/workspace-state-persistence-subtleties`.
|
||||
|
||||
- Websocket initialization uses `cf/public-uri` joined with `ws/notifications`, converting `http/https` to `ws/wss`, and includes the current `session-id` as query param.
|
||||
- Reinitializing or finalizing websocket stops the previous receive stream. Incoming websocket payloads become Potok data events under `app.main.data.websocket/message`.
|
||||
- Reinitializing or finalizing websocket stops the previous receive stream. Incoming websocket payloads become Potok data events under `app.main.data.websocket/message`.
|
||||
|
||||
@ -29,6 +29,22 @@
|
||||
(def ^:dynamic *data-length* 8)
|
||||
(def ^:dynamic *data-level* 8)
|
||||
|
||||
(def max-message-length
|
||||
"Upper bound, in characters, for an exception message surfaced as a hint or
|
||||
embedded in a formatted stack trace. A message can carry a whole payload (a
|
||||
serialized file, for instance) and grow to megabytes; only the head helps to
|
||||
identify the failure, and the rest is noise that bloats reports and the audit
|
||||
events built from them."
|
||||
1000)
|
||||
|
||||
(defn- truncate
|
||||
"Truncates a string to at most `n` characters, appending an ellipsis when it
|
||||
had to cut. Values that are not strings pass through unchanged."
|
||||
[s n]
|
||||
(if (and (string? s) (> (count s) n))
|
||||
(str (subs s 0 n) "...")
|
||||
s))
|
||||
|
||||
(defmacro error
|
||||
[& {:keys [type hint] :as params}]
|
||||
`(ex-info ~(or hint (name type))
|
||||
@ -243,7 +259,7 @@
|
||||
|
||||
:cljs
|
||||
(defn format-throwable
|
||||
[cause & {:as opts}]
|
||||
[cause & {:keys [trace-length]}]
|
||||
(with-out-str
|
||||
(when-let [exdata (ex-data cause)]
|
||||
(when-let [hint (or (get exdata :hint)
|
||||
@ -251,7 +267,7 @@
|
||||
(when (str/index-of hint "\n")
|
||||
(println "Hint:")
|
||||
(println "--------------------")
|
||||
(println hint)
|
||||
(println (truncate hint max-message-length))
|
||||
(println)))
|
||||
|
||||
(when-let [explain (get exdata ::sm/explain)]
|
||||
@ -272,14 +288,20 @@
|
||||
(println))
|
||||
|
||||
(when-let [trace (.-stack cause)]
|
||||
(println "Trace:")
|
||||
(println "--------------------")
|
||||
(println (.-stack cause))))))
|
||||
(let [lines (str/lines trace)
|
||||
head (truncate (first lines) max-message-length)
|
||||
tail (rest lines)]
|
||||
(println "Trace:")
|
||||
(println "--------------------")
|
||||
(when head
|
||||
(println head))
|
||||
(doseq [line (if trace-length (take trace-length tail) tail)]
|
||||
(println line)))))))
|
||||
|
||||
(defn first-line
|
||||
[s]
|
||||
(let [break-index (str/index-of s "\n")]
|
||||
(if (pos? break-index)
|
||||
(if (and (some? break-index) (pos? break-index))
|
||||
(subs s 0 break-index)
|
||||
s)))
|
||||
|
||||
@ -308,5 +330,6 @@
|
||||
|
||||
(defn get-hint
|
||||
[cause]
|
||||
(or (some-> (ex-data cause) (get :hint) first-line)
|
||||
(some-> (ex-message cause) first-line)))
|
||||
(truncate (or (some-> (ex-data cause) (get :hint) first-line)
|
||||
(some-> (ex-message cause) first-line))
|
||||
max-message-length))
|
||||
|
||||
57
common/test/common_tests/exceptions_test.cljc
Normal file
57
common/test/common_tests/exceptions_test.cljc
Normal file
@ -0,0 +1,57 @@
|
||||
;; 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 common-tests.exceptions-test
|
||||
"Exception message bounding in `app.common.exceptions`.
|
||||
|
||||
A message can carry a whole payload (a serialized file, for instance) and
|
||||
grow to megabytes. `get-hint` and the CLJS `format-throwable` keep a bounded
|
||||
head so neither the audit `:hint` prop nor the report can explode."
|
||||
(:require
|
||||
#?(:cljs [cuerdas.core :as str])
|
||||
[app.common.exceptions :as ex]
|
||||
[clojure.test :as t]))
|
||||
|
||||
(defn- huge-string
|
||||
[n]
|
||||
(apply str (repeat n "x")))
|
||||
|
||||
(t/deftest first-line-keeps-a-single-line-message-intact-test
|
||||
(t/is (= "boom" (ex/first-line "boom")))
|
||||
(t/is (= "boom" (ex/first-line "boom\nrest of the message"))))
|
||||
|
||||
(t/deftest get-hint-returns-the-first-line-test
|
||||
(t/is (= "boom" (ex/get-hint (ex-info "boom\nsecond line" {})))))
|
||||
|
||||
(t/deftest get-hint-prefers-the-ex-data-hint-test
|
||||
(t/is (= "from-data" (ex/get-hint (ex-info "from-message" {:hint "from-data"})))))
|
||||
|
||||
(t/deftest get-hint-bounds-a-huge-message-test
|
||||
(let [huge (huge-string (* 4 ex/max-message-length))
|
||||
hint (ex/get-hint (ex-info huge {}))]
|
||||
(t/is (string? hint))
|
||||
(t/is (<= (count hint) (+ ex/max-message-length 3)))
|
||||
(t/is (= (subs hint 0 32) (subs huge 0 32)))))
|
||||
|
||||
(t/deftest get-hint-bounds-a-huge-ex-data-hint-test
|
||||
(let [huge (huge-string (* 4 ex/max-message-length))
|
||||
hint (ex/get-hint (ex-info "short" {:hint huge}))]
|
||||
(t/is (<= (count hint) (+ ex/max-message-length 3)))
|
||||
(t/is (= (subs hint 0 32) (subs huge 0 32)))))
|
||||
|
||||
#?(:cljs
|
||||
(t/deftest format-throwable-bounds-a-huge-trace-message-test
|
||||
(let [huge (huge-string (* 4 ex/max-message-length))
|
||||
out (ex/format-throwable (ex-info huge {}))
|
||||
msg (some #(when (str/includes? % (subs huge 0 16)) %) (str/lines out))]
|
||||
(t/is (some? msg))
|
||||
(t/is (<= (count msg) (+ ex/max-message-length 16))))))
|
||||
|
||||
#?(:cljs
|
||||
(t/deftest format-throwable-honors-trace-length-test
|
||||
(let [full (ex/format-throwable (ex-info "boom" {}))
|
||||
short (ex/format-throwable (ex-info "boom" {}) {:trace-length 1})]
|
||||
(t/is (< (count (str/lines short)) (count (str/lines full)))))))
|
||||
@ -16,6 +16,7 @@
|
||||
[common-tests.buffer-test]
|
||||
[common-tests.colors-test]
|
||||
[common-tests.data-test]
|
||||
[common-tests.exceptions-test]
|
||||
[common-tests.files-builder-test]
|
||||
[common-tests.files-changes-test]
|
||||
[common-tests.files-migrations-0025-test]
|
||||
@ -96,8 +97,9 @@
|
||||
'common-tests.buffer-test
|
||||
'common-tests.colors-test
|
||||
'common-tests.data-test
|
||||
'common-tests.files-changes-test
|
||||
'common-tests.exceptions-test
|
||||
'common-tests.files-builder-test
|
||||
'common-tests.files-changes-test
|
||||
'common-tests.files-migrations-0025-test
|
||||
'common-tests.files-migrations-0026-test
|
||||
'common-tests.files-migrations-test
|
||||
|
||||
@ -17,6 +17,19 @@
|
||||
|
||||
;; TODO: rename `:type` to `:name`
|
||||
|
||||
(defn- summarize-export-shapes
|
||||
"Builds a compact body for the `:export-shapes` modal event. We know the
|
||||
shape of these props here, so instead of dumping every export (each one
|
||||
carrying its full shape, which can be megabytes) we emit counters: total
|
||||
exports, distinct shapes and one count per export type."
|
||||
[props]
|
||||
(let [exports (:exports props)]
|
||||
(merge
|
||||
{:page (:name props)
|
||||
:num-shapes (count (distinct (map :object-id exports)))
|
||||
:num-exports (count exports)}
|
||||
(frequencies (map :type exports)))))
|
||||
|
||||
(defn show
|
||||
([props]
|
||||
(show (uuid/next) (:type props) props))
|
||||
@ -26,9 +39,17 @@
|
||||
(ptk/reify ::show-modal
|
||||
ev/Event
|
||||
(-data [_]
|
||||
(-> props
|
||||
(dissoc :type)
|
||||
(assoc :name type)))
|
||||
(let [origin (:origin props)
|
||||
data (if (= type :export-shapes)
|
||||
(assoc (summarize-export-shapes props) :name type)
|
||||
(-> props
|
||||
(dissoc :type)
|
||||
(assoc :name type)))]
|
||||
;; The origin is event metadata, not a prop: sending it as
|
||||
;; ::ev/origin makes `make-proto-event` put it on the event context
|
||||
;; (`:event-origin`); as a plain `:origin` it only stayed as a prop.
|
||||
(cond-> (dissoc data :origin)
|
||||
(some? origin) (assoc ::ev/origin origin))))
|
||||
|
||||
ptk/UpdateEvent
|
||||
(update [_ state]
|
||||
|
||||
@ -157,7 +157,7 @@
|
||||
(println "Context:")
|
||||
(println "--------------------")
|
||||
(println "Timestamp:" (ct/format-inst (ct/now) :rfc1123))
|
||||
(println "Hint: " (or (:hint data) (ex-message cause) "--"))
|
||||
(println "Hint: " (or (ex/get-hint cause) "--"))
|
||||
(println "Prof ID: " (str (or profile-id "--")))
|
||||
(println "Team ID: " (str (or team-id "--")))
|
||||
(when-let [file-id (or (:file-id data) file-id)]
|
||||
|
||||
55
frontend/test/frontend_tests/data/modal_test.cljs
Normal file
55
frontend/test/frontend_tests/data/modal_test.cljs
Normal file
@ -0,0 +1,55 @@
|
||||
;; 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 frontend-tests.data.modal-test
|
||||
(:require
|
||||
[app.common.uuid :as uuid]
|
||||
[app.main.data.event :as ev]
|
||||
[app.main.data.modal :as modal]
|
||||
[cljs.test :as t :include-macros true]))
|
||||
|
||||
(t/deftest export-shapes-modal-event-carries-a-summary
|
||||
(let [shape-a (uuid/next)
|
||||
shape-b (uuid/next)
|
||||
exports [{:object-id shape-a :type :png}
|
||||
{:object-id shape-a :type :svg}
|
||||
{:object-id shape-b :type :png}]
|
||||
event (modal/show :export-shapes
|
||||
{:exports exports
|
||||
:origin "workspace:sidebar"
|
||||
:name "Page 1"})
|
||||
data (ev/-data event)]
|
||||
|
||||
(t/testing "the raw exports never leak into the event"
|
||||
(t/is (not (contains? data :exports))))
|
||||
|
||||
(t/testing "the event carries counters instead"
|
||||
(t/is (= 3 (:num-exports data)))
|
||||
(t/is (= 2 (:num-shapes data)))
|
||||
(t/is (= 2 (:png data)))
|
||||
(t/is (= 1 (:svg data))))
|
||||
|
||||
(t/testing "the modal context is preserved"
|
||||
(t/is (= "Page 1" (:page data)))
|
||||
(t/is (= :export-shapes (:name data))))
|
||||
|
||||
(t/testing "origin is emitted as event metadata, not as a prop"
|
||||
(t/is (= "workspace:sidebar" (::ev/origin data)))
|
||||
(t/is (not (contains? data :origin))))))
|
||||
|
||||
(t/deftest other-modals-keep-their-props
|
||||
(let [event (modal/show :some-modal {:foo "bar" :items [1 2 3]})
|
||||
data (ev/-data event)]
|
||||
(t/is (= "bar" (:foo data)))
|
||||
(t/is (= [1 2 3] (:items data)))
|
||||
(t/is (= :some-modal (:name data)))))
|
||||
|
||||
(t/deftest modal-origin-becomes-event-metadata
|
||||
(let [event (modal/show :some-modal {:foo "bar" :origin "dashboard"})
|
||||
data (ev/-data event)]
|
||||
(t/is (= "dashboard" (::ev/origin data)))
|
||||
(t/is (not (contains? data :origin)))
|
||||
(t/is (= "bar" (:foo data)))))
|
||||
@ -11,6 +11,7 @@
|
||||
[frontend-tests.data.dashboard-test]
|
||||
[frontend-tests.data.exports-assets-test]
|
||||
[frontend-tests.data.fix-deleted-fonts-test]
|
||||
[frontend-tests.data.modal-test]
|
||||
[frontend-tests.data.nitrate-test]
|
||||
[frontend-tests.data.persistence-retry-test]
|
||||
[frontend-tests.data.persistence-test]
|
||||
@ -117,6 +118,7 @@
|
||||
'frontend-tests.copy-as-svg-test
|
||||
'frontend-tests.data.dashboard-test
|
||||
'frontend-tests.data.fix-deleted-fonts-test
|
||||
'frontend-tests.data.modal-test
|
||||
'frontend-tests.data.nitrate-test
|
||||
'frontend-tests.data.persistence-retry-test
|
||||
'frontend-tests.data.persistence-test
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user