From b6f8055c561b4aec5d5688a7b45e4bd59218e1d4 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 22:53:29 +0200 Subject: [PATCH] fix: /login throttles failed logins per client address behind trusted proxies (surf.ClientIP), like /register Behind a reverse proxy every visitor had the proxy's RemoteAddr, so one client's failed logins locked the account for all. The throttle key is now surf.ClientIP with http.trusted_proxies, the expression /register uses; X-Forwarded-For from an untrusted peer is still ignored. RemoteIP is unchanged. --- README.md | 6 +- controllers/api_controller.go | 13 +++-- login_client_ip_test.go | 106 ++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 7 deletions(-) create mode 100644 login_client_ip_test.go diff --git a/README.md b/README.md index 46e29fa..f1b9a28 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ It owns the `users` table extensions, `user_throttle`, `jwt_blacklist`, `user_ap - **JWT sessions.** `POST /login`, `GET /fetch`, `POST /refresh`, `POST /logout`. Boot registers the `jwt` guard with the framework's `bouncer.Registry` and exposes it as the `jwt.auth` middleware for other plugins. The guard reads the session cookies `classes.SessionCookieNames()` lists and resolves the published `bouncer.BlacklistStore` on every request. Boot installs the plugin's GORM hooks (`classes.RegisterHooks`), publishes the Postgres `jwt_blacklist` store (`bouncer.NewPostgresBlacklist`) and starts its background sweep, which prunes the blacklist, through `lagoon.OnDatabase`. Under `summer serve`, which publishes the database after Boot, they take effect when `lagoon.Publish` runs, before the router is built; when the database is already published they take effect at once. While no store is published, no token counts as blacklisted. `POST /logout` ends the session however the token travels: the bearer header, or only the `token` or `auth_token` cookie the guard also reads. Every session token the request carries goes to the Postgres `jwt_blacklist` until its refresh window closes (the later of `exp` and `iat` plus `refresh_ttl`, plus a minute), so the guard refuses it on the next request and `POST /refresh` cannot revive it after the access token expired; a token whose access lifetime passed is revoked too while it could still be refreshed. The answer, a 401 included, expires the `auth.cookie.*` token cookie (the one `golem15.sociallogin`'s `token_cookie` sets), and any other session cookie the request carries, with that cookie's path, domain, SameSite and Secure. `POST /refresh` loads the token's user and refuses (401, `Could not refresh token`, `User not found`) a deactivated or banned user and a token issued before the user's `tokens_valid_after`; a suspension alone does not stop a refresh. `GET /fetch`, `POST /refresh`, `POST /update` and `POST /change-password`, like the plugin's other signed-in routes, read the session token as the `jwt` guard does: the bearer header first, then the `token` and `auth_token` cookies, so a cookie-only session works on them and a bearer header wins when both are sent. A cookie-carried `POST /refresh` answers the rotated token in the body and writes no cookie; the old token is blacklisted after `golem15.user.jwt.blacklist_grace`, so the caller that wrote the cookie stores the new token in it. A cookie-carried session relies on the cookie's SameSite attribute against cross-site requests (the `auth.cookie` default is `lax`), as on the `jwt.auth` routes. - **Password reset and change.** `POST /forgot-password` answers the same body for known and unknown addresses; `POST /reset-password` consumes a code from `IssueResetCode`. `POST /change-password` requires the current password. - **Must-change-password lock.** The `user:require-password-change` console command flags an account. The flag rides on the authenticated principal (`bouncer.Principal.MustChangePassword`) for guards and other plugins to act on, and a successful `POST /change-password` clears it. -- **Login throttling.** Failed logins are counted per user and IP in `user_throttle` (`CheckAndRecordLogin`, `RejectIfThrottled`) and suspend the account for a configured time. +- **Login throttling.** Failed logins are counted per user and client address in `user_throttle` (`CheckAndRecordLogin`, `RejectIfThrottled`) and suspend the account for a configured time. The client address is `surf.ClientIP` with the framework's `http.trusted_proxies`, as for the `/register` IP limit and the `user-api` rate limit: behind a listed reverse proxy it is the `X-Forwarded-For` client, otherwise the connection address, and an `X-Forwarded-For` header from any other peer is ignored. A deployment behind a proxy lists the proxy's address in `http.trusted_proxies`; otherwise every visitor shares the proxy's counter. - **Rate limiting.** Every route runs behind the `user-api` bucket: 120 requests a minute per authenticated user, or per client IP for anonymous calls. - **Profile.** `POST /update`, `POST /marketing-consent`, `POST /avatar` and `POST /avatar/remove` (the avatar is stored through the framework's attachment storage). - **Deactivation and bans.** Deactivation is self-service and reversible: a password login or a `golem15.sociallogin` sign-in restores a deactivated account and sends the reactivate mail, and an administrator can restore it too. It ends every earlier session JWT and personal API token of the user (`RevokeUserAccess` stamps `tokens_valid_after` and revokes the `user_api_tokens` rows), so the user signs in again and gets a fresh token, while the old API tokens stay revoked. A ban is the lock: password login, social sign-in and `POST /refresh` refuse a banned user, and banning revokes the user's sessions and API tokens. Neither unban nor restore revives a token. Activation gates login, deactivation does not: while `require_activation` is on, an account that is both deactivated and not activated is refused and stays deactivated, with no reactivate mail. Upgrade note: accounts deactivated or banned before this version keep their API tokens until they are deactivated or banned again (both actions are idempotent) or `RevokeUserAccess` runs for them. @@ -180,7 +180,7 @@ Packages: - `APIArray(ctx, app, user)` builds the user payload, extended by `GetApiArrayEvent` listeners. - `FinishLogin(ctx, app, db, user, ip)` runs the steps `POST /login` takes after authentication, for a caller that authenticated the user another way (a social sign-in): it restores a soft-deleted account and sends the reactivate mail, records `ip` as `last_ip_address` and touches `last_seen` (a failure there is only logged). It checks no password, throttle, activation or ban and fires no event; the caller does those checks. - `RegistrationSettings(app)` returns `allow_registration`, `use_register_throttle`, `activate_mode` and `require_activation` with their defaults (`true`, `true`, `auto`, `true`), and `RegistrationsFromIP(ctx, db, ip)` counts the accounts created from an IP in the last 60 minutes (`/register` refuses at 3 with the throttle on). - - `RefreshTTL(app)` is the refresh window, `golem15.user.jwt.refresh_ttl` minutes (default 20160), and `RemoteIP(r)` the host part of `r.RemoteAddr` that `/login` uses for its throttle. + - `RefreshTTL(app)` is the refresh window, `golem15.user.jwt.refresh_ttl` minutes (default 20160), and `RemoteIP(r)` the host part of `r.RemoteAddr`, with no proxy handling (`/login` and `/register` use `surf.ClientIP` with `http.trusted_proxies`). - `classes`: `JWTSecret`, `GormUsers` (the `bouncer.UserProvider`), `LookupByEmail`, `UserPermissionGrants`, `UserGroupCodes`, `HasGroupCode`, `APITokenPrefix`, `MintAPIToken`, `HashAPIToken` and `RevokeAPIToken`, plus the activation/reset, throttle, mail locale and event helpers. `RegisterEvent` carries `User` and `Payload map[string]any`, a read-only copy of the register input without the password keys. For the admin and for other plugins it also has: - `MergedPermissions(ctx, db, userID)`, `HasPermission(merged, code)` and `UserHasPermission(ctx, db, userID, code)`, the frontend permission resolver, and `FrontendPermissionOptions(ctx, db)`, the catalogue by tab and code. - `PermissionManagePrivilegedGroups`, `PrivilegedGroupCodes(cfg)`, `IsPrivilegedCode(codes, code)`, `PrivilegedGroupIDs(ctx, db, codes)` and `IsPrivilegedMember(ctx, db, codes, userID)`, the privileged-group rule. @@ -267,6 +267,8 @@ go test ./... # full run, starts Postgres with testcontainers (needs D `TestLoginRequiresActivation` pins the activation gate of `POST /login`: under the default and an explicit `require_activation: true` a not-activated account's correct password gets the byte-identical 401 (status, body, `Content-Type` and `Cache-Control`) of a wrong password, stamps no `last_seen`, adds one throttle attempt and suspends the account after `attempt_limit` refusals; `require_activation: false` and activated accounts sign in; a deactivated activated account is restored with the reactivate mail, and a deactivated not-activated account is refused and stays deactivated unless the key is off. +`TestLoginThrottleClientIP` sends failed logins through one trusted proxy (`http.trusted_proxies`). It checks that the throttle row carries the `X-Forwarded-For` client, and that this client is suspended while another client behind the same proxy still signs in. It also checks that without trusted proxies a changing `X-Forwarded-For` neither moves the counter nor escapes the suspension. + `TestAdminOrganisationMembershipClearsRole` checks that the members manager's unlink and link and a Users form save that changes the organisation clear `organisation_role`, that a save keeping the organisation keeps it, and that deleting an organisation clears both columns. The admin tests (`TestAdmin*`) boot the plugin with the admin API mounted on a database of their own and sign in backend administrators with a chosen permission set; the helpers are in `admin_harness_test.go`. diff --git a/controllers/api_controller.go b/controllers/api_controller.go index 9fb5d23..4423acd 100644 --- a/controllers/api_controller.go +++ b/controllers/api_controller.go @@ -54,7 +54,9 @@ const invalidLoginBody = "Invalid email or password" // as a wrong password and counts as a failed attempt for the throttle; an // account that is also deactivated is refused before the restore, so it // stays deactivated and gets no mail. With the key off such an account signs -// in as before. Activation gates login, deactivation does not. +// in as before. Activation gates login, deactivation does not. The throttle +// counts failed attempts per user and client address, which is +// surf.ClientIP with http.trusted_proxies, as for /register. func Login(app *backpack.App) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { gdb, secret, ok := sessionDeps(w, app) @@ -74,7 +76,7 @@ func Login(app *backpack.App) http.HandlerFunc { writeOpaque500(w) return } - ip := remoteIP(r) + ip := surf.ClientIP(r, surf.TrustedProxies(appConfig(app))) if user != nil { if err := classes.RejectIfThrottled(ctx, gdb, user, ip); err != nil { if classes.IsLoginRejected(err) { @@ -1159,9 +1161,10 @@ func requestURL(r *http.Request) string { return scheme + "://" + r.Host + r.URL.Path } -// RemoteIP returns the host part of r.RemoteAddr, the address /login uses -// for its throttle (r.RemoteAddr itself when it has no port, "" when it is -// empty). /register records surf.ClientIP with the trusted proxies instead. +// RemoteIP returns the host part of r.RemoteAddr (r.RemoteAddr itself when +// it has no port, "" when it is empty), with no proxy handling. /login and +// /register key their throttles on surf.ClientIP with the trusted proxies; +// RemoteIP stays for callers that want the connection address. func RemoteIP(r *http.Request) string { return remoteIP(r) } diff --git a/login_client_ip_test.go b/login_client_ip_test.go new file mode 100644 index 0000000..bf1f2b4 --- /dev/null +++ b/login_client_ip_test.go @@ -0,0 +1,106 @@ +package user + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "git.golem15.com/golem15/sm-user-plugin/models" + "git.golem15.com/golem15/summercms/modules/compass" +) + +// TestLoginThrottleClientIP pins the /login throttle key: the client address +// surf.ClientIP derives with http.trusted_proxies, as /register and the +// user-api rate limit use, and never an X-Forwarded-For header from an +// untrusted peer. +func TestLoginThrottleClientIP(t *testing.T) { + t.Run("behind a trusted proxy", func(t *testing.T) { + e := newAdminEnv(t, func(cfg *compass.Config) { + if err := cfg.Set("http.trusted_proxies", []string{"10.0.0.0/8"}); err != nil { + t.Fatal(err) + } + }) + user := e.seedUser(t, "throttle-proxy", nil) + const proxy = "10.0.0.2:5000" + for i := range 5 { + rec := e.loginFrom(t, proxy, "203.0.113.10", user.Email, "wrong-password") + if rec.Code != http.StatusUnauthorized || rec.Body.String() != invalidLoginBody { + t.Fatalf("failed login %d = %d %s", i+1, rec.Code, rec.Body.String()) + } + } + assertThrottleRow(t, e, user, "203.0.113.10") + if rec := e.loginFrom(t, proxy, "203.0.113.10", user.Email, "correct-horse"); rec.Code != http.StatusUnauthorized || rec.Body.String() != invalidLoginBody { + t.Fatalf("suspended client login = %d %s", rec.Code, rec.Body.String()) + } + rec := e.loginFrom(t, proxy, "203.0.113.20", user.Email, "correct-horse") + var body struct { + Token string `json:"token"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); rec.Code != http.StatusOK || err != nil || body.Token == "" { + t.Fatalf("other client behind the proxy = %d %s", rec.Code, rec.Body.String()) + } + }) + + t.Run("untrusted X-Forwarded-For is ignored", func(t *testing.T) { + e := newAdminEnv(t) + user := e.seedUser(t, "throttle-direct", nil) + const peer = "192.0.2.50:4000" + for _, forged := range []string{"203.0.113.1", "203.0.113.2", "203.0.113.3", "203.0.113.4", "203.0.113.5"} { + if rec := e.loginFrom(t, peer, forged, user.Email, "wrong-password"); rec.Code != http.StatusUnauthorized { + t.Fatalf("failed login with X-Forwarded-For %s = %d %s", forged, rec.Code, rec.Body.String()) + } + } + assertThrottleRow(t, e, user, "192.0.2.50") + if rec := e.loginFrom(t, peer, "203.0.113.99", user.Email, "correct-horse"); rec.Code != http.StatusUnauthorized || rec.Body.String() != invalidLoginBody { + t.Fatalf("forged X-Forwarded-For escaped the suspension: %d %s", rec.Code, rec.Body.String()) + } + if rec := e.loginFrom(t, "192.0.2.51:4000", "", user.Email, "correct-horse"); rec.Code != http.StatusOK { + t.Fatalf("login from another address = %d %s", rec.Code, rec.Body.String()) + } + }) +} + +// assertThrottleRow: the account has exactly one user_throttle row, keyed on +// ip, with five attempts and suspended. +func assertThrottleRow(t *testing.T, e *adminEnv, user *models.User, ip string) { + t.Helper() + var rows []models.Throttle + if err := e.gdb.Where("user_id = ?", user.ID).Find(&rows).Error; err != nil { + t.Fatal(err) + } + if len(rows) != 1 { + t.Fatalf("throttle rows = %d, want 1: %+v", len(rows), rows) + } + row := rows[0] + got := "" + if row.IPAddress != nil { + got = *row.IPAddress + } + if got != ip || row.Attempts != 5 || !row.IsSuspended { + t.Fatalf("throttle row ip=%s attempts=%d suspended=%v, want ip=%s attempts=5 suspended", got, row.Attempts, row.IsSuspended, ip) + } +} + +// loginFrom posts a login from a fixed connection address, with an +// X-Forwarded-For header when forwardedFor is set. send cannot model a +// reverse proxy: it gives every request its own address. +func (e *adminEnv) loginFrom(t *testing.T, remoteAddr, forwardedFor, email, password string) *httptest.ResponseRecorder { + t.Helper() + raw, err := json.Marshal(map[string]string{"email": email, "password": password}) + if err != nil { + t.Fatal(err) + } + req := httptest.NewRequest(http.MethodPost, "/_user/api/v1/login", bytes.NewReader(raw)) + req.Host = "example.com" + req.RemoteAddr = remoteAddr + req.Header.Set("Accept-Language", "en") + req.Header.Set("Content-Type", "application/json") + if forwardedFor != "" { + req.Header.Set("X-Forwarded-For", forwardedFor) + } + rec := httptest.NewRecorder() + e.h.ServeHTTP(rec, req) + return rec +}