diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 51b52afda9..bcec978f07 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -498,6 +498,15 @@ :type :handled :hint (tr "errors.connection-error"))) +(defn- handle-transport-error + "Reports a failure whose answer did not come from the backend. These affect + one request and pass on their own, so the user keeps the page and gets a + toast." + [error prefix] + (when-let [cause (::instance error)] + (ex/print-throwable cause :prefix prefix)) + (flash :cause (::instance error) :type :handled)) + (defmethod ptk/handle-error :network [error] ;; Transient network errors (e.g. lost connectivity, DNS failure) @@ -511,6 +520,18 @@ ;; that would report it as an unhandled application error. (handle-connectivity-error error "Offline Error")) +(defmethod ptk/handle-error :gateway-error + [error] + (handle-connectivity-error error "Gateway Error")) + +(defmethod ptk/handle-error :rate-limit + [error] + (handle-transport-error error "Rate Limit Error")) + +(defmethod ptk/handle-error :unexpected-response + [error] + (handle-transport-error error "Unexpected Response")) + (def ^:private delegated-persistence-types "Save failure causes routed to their own error handler: retaining the changes cannot resolve them." diff --git a/frontend/src/app/main/repo.cljs b/frontend/src/app/main/repo.cljs index 01ce9e4c01..8a9e832429 100644 --- a/frontend/src/app/main/repo.cljs +++ b/frontend/src/app/main/repo.cljs @@ -32,6 +32,7 @@ #{:network ; js/fetch network-level failure :bad-gateway ; 502 :service-unavailable ; 503 + :gateway-error ; 504 and the Cloudflare 52x family :offline}) ; status 0 (browser offline) (defn retryable-error? @@ -79,6 +80,28 @@ ;; -- Response handling ------------------------------------------------------- +(def ^:private gateway-statuses + "Statuses raised by an edge proxy when the request never reached the + backend or its answer never came back." + #{504 ; gateway timeout + 520 ; cloudflare: unknown origin error + 521 ; cloudflare: origin is down + 522 ; cloudflare: connection timed out + 523 ; cloudflare: origin unreachable + 524}) ; cloudflare: origin timed out + +(def ^:private max-body-excerpt + "Number of characters kept from a body that we could not interpret." + 500) + +(defn- body-excerpt + "Keeps the head of an uninterpretable body. Error pages from proxies run + into tens of kilobytes and the excerpt is enough to recognize them." + [body] + (if (and (string? body) (> (count body) max-body-excerpt)) + (str (subs body 0 max-body-excerpt) "...") + body)) + (defn handle-response [{:keys [status body headers uri] :as response}] (cond @@ -121,6 +144,35 @@ (and (>= status 400) (map? body)) (rx/throw (ex-info "http error" (assoc body :uri uri :status status))) + ;; The branches below handle responses the backend never wrote: the + ;; body carries no error code, so the status is all we can read. + + (contains? gateway-statuses status) + (rx/throw + (ex/error :type :gateway-error + :code :gateway-error + :hint "the request did not reach the backend" + :uri uri + :status status)) + + (= 429 status) + (rx/throw + (ex/error :type :rate-limit + :code :rate-limited + :hint "too many requests" + :uri uri + :status status)) + + (>= status 400) + (rx/throw + (ex/error :type :unexpected-response + :code :unexpected-error-response + :hint "unexpected error response" + :uri uri + :status status + :headers headers + :data (body-excerpt body))) + :else (rx/throw (ex/error :type :internal @@ -129,7 +181,7 @@ :uri uri :status status :headers headers - :data body)))) + :data (body-excerpt body))))) (def default-options {:update-file {:query-params [:id]} diff --git a/frontend/test/frontend_tests/data/repo_test.cljs b/frontend/test/frontend_tests/data/repo_test.cljs index 13066fbe9e..5334f1b719 100644 --- a/frontend/test/frontend_tests/data/repo_test.cljs +++ b/frontend/test/frontend_tests/data/repo_test.cljs @@ -8,7 +8,8 @@ (:require [app.main.repo :as repo] [beicon.v2.core :as rx] - [cljs.test :as t :include-macros true])) + [cljs.test :as t :include-macros true] + [cuerdas.core :as str])) ;; --------------------------------------------------------------------------- ;; retryable-error? tests (synchronous) @@ -29,6 +30,11 @@ (let [err (ex-info "service unavailable" {:type :service-unavailable})] (t/is (true? (repo/retryable-error? err)))))) +(t/deftest retryable-error-gateway-error + (t/testing "504 and the Cloudflare 52x family are retryable" + (let [err (ex-info "http error" {:type :gateway-error})] + (t/is (true? (repo/retryable-error? err)))))) + (t/deftest retryable-error-offline (t/testing "offline (status 0) is retryable" (let [err (ex-info "offline" {:type :offline})] @@ -215,3 +221,123 @@ (t/is (= 2 @call-count)) (t/is (= :authentication (:type (ex-data err)))) (done)))))))) + +;; --------------------------------------------------------------------------- +;; handle-response classification tests +;; --------------------------------------------------------------------------- + +(defn- capture-error + "Subscribes to `handle-response` for `response` and calls `k` with the + `ex-data` of the raised error." + [response k] + (->> (repo/handle-response response) + (rx/subs! + (fn [_] (t/is false "expected an error response") (k nil)) + (fn [cause] (k (ex-data cause)))))) + +(def ^:private html-error-page + "Web Page Blockedblocked") + +(t/deftest handle-response-gateway-statuses + (t/testing "every edge status that never reached the backend is a gateway error" + (t/async done + (let [statuses [504 520 521 522 523 524] + pending (atom (count statuses))] + (doseq [status statuses] + (capture-error {:status status + :body html-error-page + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :gateway-error (:type data)) + (str "status " status " should be a gateway error")) + (t/is (= status (:status data)) + (str "status " status " should be carried through")) + (when (zero? (swap! pending dec)) + (done))))))))) + +(t/deftest handle-response-rate-limit + (t/testing "a 429 with no interpretable body is a rate-limit error" + (t/async done + (capture-error {:status 429 + :body "" + :headers {} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :rate-limit (:type data))) + (t/is (= 429 (:status data))) + (done)))))) + +(t/deftest handle-response-intercepted-by-proxy + (t/testing "an error page from a filtering proxy is not an internal error" + (t/async done + (capture-error {:status 403 + :body html-error-page + :headers {"content-type" "text/html" + "server" "Zscaler/6.2"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= :unexpected-response (:type data))) + (t/is (= 403 (:status data))) + (done)))))) + +(t/deftest handle-response-keeps-body-excerpt + (t/testing "a body too long to keep is cut short and marked as cut" + (t/async done + (capture-error {:status 403 + :body (apply str (repeat 5000 "x")) + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= 503 (count (:data data))) "500 characters and an ellipsis") + (t/is (str/ends-with? (:data data) "...")) + (done)))))) + +(t/deftest handle-response-keeps-a-short-body-whole + (t/testing "a body short enough to read is passed through untouched" + (t/async done + (capture-error {:status 403 + :body html-error-page + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= html-error-page (:data data))) + (done)))))) + +(t/deftest handle-response-challenge-still-authorization + (t/testing "a Cloudflare challenge keeps its own type" + (t/async done + (capture-error {:status 403 + :body html-error-page + :headers {"server" "cloudflare" + "cf-mitigated" "challenge"} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :authorization (:type data))) + (t/is (= :challenge-required (:code data))) + (done)))))) + +(t/deftest handle-response-keeps-backend-error + (t/testing "an error the backend itself reported is passed through untouched" + (t/async done + (capture-error {:status 400 + :body {:type :validation :code :missing-param} + :headers {"content-type" "application/transit+json"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= :validation (:type data))) + (t/is (= :missing-param (:code data))) + (t/is (= 400 (:status data))) + (done)))))) + +(t/deftest handle-response-undecodable-non-error-is-internal + (t/testing "a non-error response we cannot interpret is still an internal error" + (t/async done + (capture-error {:status 301 + :body html-error-page + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :internal (:type data))) + (t/is (= :unable-to-process-repository-response (:code data))) + (done)))))) diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index a964620c15..44cc76eb1d 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -141,7 +141,7 @@ (t/deftest environment-error-classification (t/testing "environment failures are recognised" (doseq [type [:network :offline :bad-gateway :service-unavailable - :nitrate-unavailable :nitrate-not-configured]] + :gateway-error :nitrate-unavailable :nitrate-not-configured]] (t/is (true? (errors/environment-error? (error-cause :type type))) (str "expected environment error: " type)))) (t/testing "application defects are not environment failures" @@ -192,7 +192,7 @@ ;; handler. Each is reported as a governed compact audit event plus its ;; connection toast. Proves: per type, report first and toast right after, ;; with no stack in the report and the dedicated toast message. - (doseq [type [:network :offline]] + (doseq [type [:network :offline :gateway-error]] (errors/reset-report-governor!) (let [events (atom []) cause (ex-info "http error" {:type type :hint "http error"})] @@ -786,3 +786,35 @@ (let [state (ptk/update (last @events) {})] (t/is (nil? (get-in state [:notification :timeout]))) (t/is (some? (get-in state [:notification :links 0]))))))))) +;; --------------------------------------------------------------------------- +;; transport errors +;; --------------------------------------------------------------------------- + +(t/deftest ^:async transport-errors-are-handled-without-replacing-the-page + ;; Scenario: a rate limit and an error page the backend did not write, + ;; through the global handler. Proves: neither replaces the page, each is + ;; a handled report followed by one toast. + (doseq [type [:rate-limit :unexpected-response]] + (errors/reset-report-governor!) + (let [events (atom []) + assigned (atom []) + cause (ex-info "http error" + {:type type + :hint "http error" + :status 429 + :uri "https://design.penpot.app/api/main/methods/get-teams"})] + (await + (mock/with-mocks* + {st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace") + rt/assign-exception (fn [error] (swap! assigned conj error) error) + tm/schedule (mock/stub (fn [f] (f))) + st/format-last-events (mock/stub (fn [& _] "(stub last events)"))} + (errors/on-error cause) + (await (async/settle)) + (t/is (empty? @assigned) + (str type " must not replace the page with an error screen")) + (t/is (= ["handled-exception"] + (map (comp ::ev/name deref) (report-events @events))) + (str type " is reported as handled, not unhandled")) + (t/is (= 2 (count @events)) (str type " must show one notification")))))))