From acd146f6f47dd4791bac7dcae0de3a19dba7a661 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 24 Sep 2026 12:19:29 +0200 Subject: [PATCH] :bug: Restore rate-limit headers and add Retry-After on 429 (#11895) * :bug: Restore rate-limit headers and add Retry-After The account-lockout change replaced the header-forwarding 429 handler with a body-only one, so existing RPC rate-limit responses lost their x-rate-limit-remaining and x-rate-limit-reset headers. Account lockout never sent Retry-After either. Make handle-error :rate-limit preserve ::http/headers and add a retry-after header when the exception carries a non-nil :ttl in seconds, keeping the current JSON body. Add focused tests for both the lockout and the RPC limiter paths. Document activation, defaults, password/LDAP scope, Redis fail-open behavior, and the lockout risk, and record the final HTTP contract in the backend auth memory. Refs #11397 AI-assisted-by: deepseek-v4.1-flash * :bug: Add Retry-After to RPC 429 and expose headers in CORS Address review follow-ups on the account-lockout 429 contract: - The RPC limiter now sets retry-after in its 429 headers (seconds until the longest rejecting limit resets), so it matches the account-lockout response and the HTTP standard. - CORS exposes retry-after, x-rate-limit-remaining, and x-rate-limit-reset so browser clients can read them. - Use backticks for Retry-After and account-locked in the docs for consistency with nearby sections. Refs #11397 AI-assisted-by: deepseek-v4.1-flash --- .../auth-permissions-product-domains.md | 1 + backend/src/app/http/errors.clj | 16 +++-- backend/src/app/http/middleware.clj | 2 +- backend/src/app/rpc/rlimit.clj | 29 +++++--- .../test/backend_tests/http_errors_test.clj | 70 +++++++++++++++++++ .../backend_tests/http_middleware_test.clj | 3 +- .../test/backend_tests/rpc_rlimit_test.clj | 38 ++++++++++ docs/technical-guide/configuration.md | 31 ++++++++ 8 files changed, 172 insertions(+), 18 deletions(-) create mode 100644 backend/test/backend_tests/http_errors_test.clj diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index 86da108a52..dd0b09edde 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -9,6 +9,7 @@ - LDAP login validates credentials against the external directory, fetches identity data, then logs in or registers a matching Penpot profile. LDAP registration is not a separate Penpot signup flow. - LDAP session identity MUST come from the directory-returned email (`info.email`): the profile matching the typed email can differ (aliases, UPNs, multi-valued `mail` attributes) and is used only for lockout checks, never to bind the session. - Account lockout (flag `:account-lockout`, `app.auth.login-lockout`) is Redis-backed and keyed per profile id. Password and LDAP flows check/increment on the profile derived from the typed email and clear on the profile that actually logs in. +- Lockout and RPC rate limits share the `app.http.errors/handle-error :rate-limit` HTTP path: status 429, body `{:type :rate-limit :code ... :hint ... :ttl ...}`, and any supplied `::http/headers` preserved. Account lockout raises `:code :account-locked` and, when it carries a non-nil `:ttl` (seconds), the handler adds `retry-after`. The RPC limiter (`app.rpc.rlimit`) raises `:code :request-blocked` and sets `retry-after` itself in `::http/headers` (seconds until the longest rejecting limit resets), alongside `x-rate-limit-remaining`/`x-rate-limit-reset`. CORS (`app.http.middleware/with-cors-headers`) exposes `content-type`, `retry-after`, and both `x-rate-limit-*` headers. External flags: `enable-account-lockout`, `enable-rpc-rlimit`. - Logout may return an OIDC provider redirect URI when the session claims include provider/session data and the provider has a logout URI. - Invitation tokens are verified through token issuers and only accepted when the token member id/email matches the authenticated profile; otherwise login proceeds without consuming the invitation. - HTTP/session parsing details such as cookie/header precedence, JWT session token versions, and SameSite behavior are in `mem:backend/subtleties`. diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index 9d370bffa5..e8b103c417 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -77,12 +77,16 @@ (defmethod handle-error :rate-limit [err _ _] - (let [data (ex-data err)] - {::yres/status 429 - ::yres/body {:type :rate-limit - :code (:code data) - :hint (:hint data) - :ttl (:ttl data)}})) + (let [data (ex-data err) + headers (cond-> (::http/headers data) + (some? (:ttl data)) + (assoc "retry-after" (str (:ttl data))))] + {::yres/status 429 + ::yres/headers headers + ::yres/body {:type :rate-limit + :code (:code data) + :hint (:hint data) + :ttl (:ttl data)}})) (defmethod handle-error :concurrency-limit [err _ _] diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 3914cdc3de..6aa8e9d3a2 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -237,7 +237,7 @@ (-> (assoc "access-control-allow-origin" origin) (assoc "access-control-allow-credentials" "true") (assoc "access-control-allow-methods" "GET,POST,DELETE,OPTIONS,PUT,HEAD,PATCH") - (assoc "access-control-expose-headers" "content-type") + (assoc "access-control-expose-headers" "content-type, retry-after, x-rate-limit-remaining, x-rate-limit-reset") (assoc "access-control-allow-headers" "x-frontend-version, x-client, content-type, accept")))) (defn wrap-cors diff --git a/backend/src/app/rpc/rlimit.clj b/backend/src/app/rpc/rlimit.clj index d78897e50b..f11a4913bb 100644 --- a/backend/src/app/rpc/rlimit.clj +++ b/backend/src/app/rpc/rlimit.clj @@ -222,15 +222,22 @@ (defn- process-limits [{:keys [::rds/conn] :as cfg} uid limits now] - (let [results (into [] (map (partial process-limit conn uid now)) limits) - remaining (->> results - (d/index-by ::name ::lresult/remaining) - (uri/map->query-string)) - reset (->> results - (d/index-by ::name (comp ->seconds ::lresult/reset)) - (uri/map->query-string)) + (let [results (into [] (map (partial process-limit conn uid now)) limits) + remaining (->> results + (d/index-by ::name ::lresult/remaining) + (uri/map->query-string)) + reset (->> results + (d/index-by ::name (comp ->seconds ::lresult/reset)) + (uri/map->query-string)) - rejected (d/seek (complement ::lresult/allowed) results)] + rejected (d/seek (complement ::lresult/allowed) results) + + ;; Seconds until the client can retry: the longest reset among the + ;; limits that currently reject the request. Only emitted on 429. + retry-after (->> results + (remove ::lresult/allowed) + (map #(->seconds (ct/diff now (::lresult/reset %)))) + (reduce max 0))] (when rejected (let [event {::id (uuid/next) @@ -254,8 +261,10 @@ ::allowed (not (some? rejected)) ::remaingin remaining ::reset reset - ::headers {"x-rate-limit-remaining" remaining - "x-rate-limit-reset" reset}})) + ::headers (cond-> {"x-rate-limit-remaining" remaining + "x-rate-limit-reset" reset} + (some? rejected) + (assoc "retry-after" (str retry-after)))})) (defn- get-limits [state skey sname] diff --git a/backend/test/backend_tests/http_errors_test.clj b/backend/test/backend_tests/http_errors_test.clj new file mode 100644 index 0000000000..9fbf6e0345 --- /dev/null +++ b/backend/test/backend_tests/http_errors_test.clj @@ -0,0 +1,70 @@ +;; 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 backend-tests.http-errors-test + "Focused tests for the HTTP error mapping in `app.http.errors`." + (:require + [app.http :as-alias http] + [app.http.errors :as http-errors] + [clojure.test :as t] + [yetti.response :as yres])) + +(t/deftest account-lockout-returns-429-with-retry-after + (let [cause (ex-info "account locked" + {:type :rate-limit + :code :account-locked + :hint "account locked due to too many failed login attempts" + :ttl 900}) + response (http-errors/handle cause {}) + headers (::yres/headers response) + body (::yres/body response)] + (t/is (= 429 (::yres/status response))) + (t/is (= "900" (get headers "retry-after"))) + (t/is (= :rate-limit (:type body))) + (t/is (= :account-locked (:code body))) + (t/is (= 900 (:ttl body))))) + +(t/deftest rpc-rate-limit-preserves-headers + (let [cause (ex-info "rate limit reached" + {:type :rate-limit + :code :request-blocked + :hint "rate limit reached" + ::http/headers {"x-rate-limit-remaining" "3" + "x-rate-limit-reset" "60"}}) + response (http-errors/handle cause {}) + headers (::yres/headers response) + body (::yres/body response)] + (t/is (= 429 (::yres/status response))) + (t/is (= "3" (get headers "x-rate-limit-remaining"))) + (t/is (= "60" (get headers "x-rate-limit-reset"))) + (t/is (nil? (get headers "retry-after"))) + (t/is (= :rate-limit (:type body))) + (t/is (= :request-blocked (:code body))))) + +(t/deftest rate-limit-with-ttl-keeps-headers-and-adds-retry-after + (let [cause (ex-info "rate limit reached" + {:type :rate-limit + :code :request-blocked + :hint "rate limit reached" + :ttl 120 + ::http/headers {"x-rate-limit-remaining" "0" + "x-rate-limit-reset" "120"}}) + response (http-errors/handle cause {}) + headers (::yres/headers response)] + (t/is (= 429 (::yres/status response))) + (t/is (= "0" (get headers "x-rate-limit-remaining"))) + (t/is (= "120" (get headers "x-rate-limit-reset"))) + (t/is (= "120" (get headers "retry-after"))))) + +(t/deftest rate-limit-without-ttl-has-no-retry-after + (let [cause (ex-info "rate limit reached" + {:type :rate-limit + :code :request-blocked + :hint "rate limit reached"}) + response (http-errors/handle cause {}) + headers (::yres/headers response)] + (t/is (= 429 (::yres/status response))) + (t/is (nil? (get headers "retry-after"))))) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 37745673a2..e058e2fb53 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -139,7 +139,8 @@ (t/is (= "https://trusted.example" (get headers "access-control-allow-origin"))) (t/is (= "true" (get headers "access-control-allow-credentials"))) (t/is (= "Origin" (get headers "vary"))) - (t/is (= "content-type" (get headers "access-control-expose-headers"))) + (t/is (= "content-type, retry-after, x-rate-limit-remaining, x-rate-limit-reset" + (get headers "access-control-expose-headers"))) (t/is (not (str/includes? (get headers "access-control-allow-headers" "") "cookie"))))) diff --git a/backend/test/backend_tests/rpc_rlimit_test.clj b/backend/test/backend_tests/rpc_rlimit_test.clj index a10c6363af..cd2a4db7cf 100644 --- a/backend/test/backend_tests/rpc_rlimit_test.clj +++ b/backend/test/backend_tests/rpc_rlimit_test.clj @@ -7,6 +7,8 @@ (ns backend-tests.rpc-rlimit-test (:require [app.common.time :as ct] + [app.loggers.database] + [app.loggers.mattermost] [app.redis :as rds] [app.rpc.rlimit :as rlimit] [clojure.test :as t])) @@ -26,3 +28,39 @@ (let [result (rlimit/process-limit nil "profile" now limit)] (t/is (= (ct/inst 334) (:app.rpc.rlimit.result/reset result))))))) + +(defn- test-limit + [] + {::rlimit/name :test + ::rlimit/strategy :bucket + ::rlimit/key "test" + ::rlimit/method "main.test" + ::rlimit/capacity 5 + ::rlimit/rate 3 + ::rlimit/interval (ct/duration 1000) + ::rlimit/params [1 3 5] + ::rlimit/opts "5/3/1s"}) + +(t/deftest rejected-rate-limit-emits-retry-after-header + ;; When a limit rejects the request the 429 headers must carry a + ;; Retry-After with the seconds until the client can retry. + (with-redefs [rds/eval (fn [_ _] [false 2]) + app.loggers.mattermost/emit (fn [_ _] nil) + app.loggers.database/emit (fn [_ _] nil)] + (let [result (#'rlimit/process-limits {::rds/conn nil} "profile" [(test-limit)] (ct/inst 0)) + headers (::rlimit/headers result)] + (t/is (false? (::rlimit/allowed result))) + (t/is (= "1" (get headers "retry-after"))) + (t/is (some? (get headers "x-rate-limit-remaining"))) + (t/is (some? (get headers "x-rate-limit-reset")))))) + +(t/deftest allowed-rate-limit-omits-retry-after-header + ;; Retry-After is only meaningful on a rejected 429; allowed responses + ;; must not carry it. + (with-redefs [rds/eval (fn [_ _] [true 4]) + app.loggers.mattermost/emit (fn [_ _] nil) + app.loggers.database/emit (fn [_ _] nil)] + (let [result (#'rlimit/process-limits {::rds/conn nil} "profile" [(test-limit)] (ct/inst 0)) + headers (::rlimit/headers result)] + (t/is (true? (::rlimit/allowed result))) + (t/is (nil? (get headers "retry-after")))))) diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index ef6f4f1d65..cb5a4465c6 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -310,6 +310,37 @@ PENPOT_LDAP_ATTRS_FULLNAME: cn PENPOT_LDAP_ATTRS_PHOTO: jpegPhoto ``` +### Account lockout + +__Since version 2.19.0__ + +Account lockout is disabled by default. Backend administrators can enable it by +adding the enable-account-lockout flag: + +```bash +PENPOT_FLAGS: [...] enable-account-lockout +``` + +When enabled, Penpot locks an existing account after 5 failed password or LDAP +login attempts within 15 minutes. It only applies to the password and LDAP +logins; OIDC and the other authentication providers are not affected. The +defaults can be changed with: + +```bash +# Backend +PENPOT_LOGIN_LOCKOUT_MAX_ATTEMPTS: 5 +PENPOT_LOGIN_LOCKOUT_WINDOW: 15m +``` + +While the account is locked, login returns HTTP 429 with a `Retry-After` +header and a JSON error with the code `account-locked` and the remaining +seconds in `ttl`. + +Redis must be available. If Redis fails, login continues without lockout +checks. This feature prevents repeated password guessing, but anyone who knows +an email address can lock that account by failing the configured number of +attempts. + ## Penpot URI You will need to set the PENPOT_PUBLIC_URI environment variable in case you go to serve Penpot to the users;