🐛 Restore rate-limit headers and add Retry-After on 429 (#11895)

* 🐛 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 * 🐛 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
2026-09-25 21:36:15 +00:00 · 2026-09-24 12:19:29 +02:00 · 2026-09-24 12:19:29 +02:00 · acd146f6f4
commit acd146f6f4
parent fe8a305811
8 changed files with 172 additions and 18 deletions
--- 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`.
--- 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 _ _]
--- 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
--- 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]
--- a/backend/test/backend_tests/http_errors_test.clj
+++ 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")))))
--- 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")))))
--- 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"))))))
--- 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 <code class="language-bash">enable-account-lockout</code> 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 <code class="language-bash">PENPOT_PUBLIC_URI</code> environment variable in case you go to serve Penpot to the users;