From 1a50dd20d9159a78b1959eba04387940a3b48366 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 21:11:42 +0000 Subject: [PATCH 1/4] Test that login attempts are rate limited per client (closes #66) The tests build the server's real routes and log in as a browser does. The attempt after LoginAttemptsPerMinute failed logins from one client must get 429 with Retry-After; another client must still get the login form and log in with the signing key; two clients behind a trusted proxy must be counted separately; X-Forwarded-For from an untrusted peer must not get around the limit; an IPv6 client must be counted by its /64. They do not compile yet: LoginAttemptsPerMinute comes with the change. Model: opus-5-5 --- .../server/login_rate_limit_internal_test.go | 262 ++++++++++++++++++ 1 file changed, 262 insertions(+) create mode 100644 internal/server/login_rate_limit_internal_test.go diff --git a/internal/server/login_rate_limit_internal_test.go b/internal/server/login_rate_limit_internal_test.go new file mode 100644 index 0000000..721f148 --- /dev/null +++ b/internal/server/login_rate_limit_internal_test.go @@ -0,0 +1,262 @@ +package server + +import ( + "io" + "net/http" + "net/http/httptest" + "net/netip" + "net/url" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" + + "go.uber.org/fx/fxtest" + + "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/database" + "sneak.berlin/go/pixa/internal/globals" + "sneak.berlin/go/pixa/internal/handlers" + "sneak.berlin/go/pixa/internal/logger" + "sneak.berlin/go/pixa/internal/middleware" +) + +// testSigningKey is a throwaway signing key; submitting it logs in. +const testSigningKey = "test-signing-key-0123456789abcdef" + +// wrongKey is submitted for a failed login. +const wrongKey = "not-the-signing-key" + +// Addresses for the login rate limit tests. The test server trusts +// 10.0.0.0/8 as its proxies, so the X-Forwarded-For sent by proxyPeer is +// believed and the one sent by firstClient or secondClient is ignored. +const ( + firstClient = "198.51.100.1:40000" + secondClient = "198.51.100.2:40000" + proxyPeer = "10.0.0.1:40000" + firstForwarded = "203.0.113.1" + secondForwarded = "203.0.113.2" +) + +// csrfFieldPattern extracts the CSRF token rendered into the login form. +var csrfFieldPattern = regexp.MustCompile( + `name="gorilla\.csrf\.Token" value="([^"]+)"`) + +// newTestServer builds the server's real routes from the constructors +// cmd/pixad uses, with a throwaway state directory. Debug marks requests +// as plain HTTP, so the CSRF check runs without an https Referer. +func newTestServer(t *testing.T) *Server { + t.Helper() + + stateDir := t.TempDir() + cfg := &config.Config{ + Debug: true, + SigningKey: testSigningKey, + StateDir: stateDir, + DBURL: "file:" + filepath.Join(stateDir, "state.sqlite3"), + TrustedProxies: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, + } + + lc := fxtest.NewLifecycle(t) + + log, err := logger.New(lc, logger.Params{Globals: &globals.Globals{}}) + if err != nil { + t.Fatalf("logger.New() error = %v", err) + } + + db, err := database.New(lc, database.Params{Logger: log, Config: cfg}) + if err != nil { + t.Fatalf("database.New() error = %v", err) + } + + h, err := handlers.New(lc, handlers.Params{ + Logger: log, Database: db, Config: cfg, + }) + if err != nil { + t.Fatalf("handlers.New() error = %v", err) + } + + mw, err := middleware.New(lc, middleware.Params{Logger: log, Config: cfg}) + if err != nil { + t.Fatalf("middleware.New() error = %v", err) + } + + lc.RequireStart() + t.Cleanup(lc.RequireStop) + + s := &Server{config: cfg, mw: mw, h: h} + s.SetupRoutes() + + return s +} + +// clientRequest builds a request for / arriving from remoteAddr, carrying +// forwardedFor as its X-Forwarded-For header when that is not empty. +func clientRequest( + t *testing.T, method string, body io.Reader, remoteAddr, forwardedFor string, +) *http.Request { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), method, "/", body) + req.RemoteAddr = remoteAddr + + if forwardedFor != "" { + req.Header.Set("X-Forwarded-For", forwardedFor) + } + + return req +} + +// postLogin loads the login form with GET / and submits key in it with +// POST /, as a browser does, both from the same client. GET / is not rate +// limited, so the form must load even for a client over the limit. +func postLogin( + t *testing.T, s *Server, remoteAddr, forwardedFor, key string, +) *httptest.ResponseRecorder { + t.Helper() + + page := httptest.NewRecorder() + s.ServeHTTP(page, + clientRequest(t, http.MethodGet, nil, remoteAddr, forwardedFor)) + + if page.Code != http.StatusOK { + t.Fatalf("GET / status = %d, want %d", page.Code, http.StatusOK) + } + + match := csrfFieldPattern.FindStringSubmatch(page.Body.String()) + if match == nil { + t.Fatalf("no CSRF token field found in the login form") + } + + form := url.Values{"key": {key}, "gorilla.csrf.Token": {match[1]}} + req := clientRequest(t, http.MethodPost, + strings.NewReader(form.Encode()), remoteAddr, forwardedFor) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + for _, c := range page.Result().Cookies() { + req.AddCookie(c) + } + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + return rec +} + +// tripLoginRateLimit makes LoginAttemptsPerMinute failed logins from one +// client, each answered with the login form again, then one more, which +// must be refused with 429. It returns the response to that last attempt. +func tripLoginRateLimit( + t *testing.T, s *Server, remoteAddr, forwardedFor string, +) *httptest.ResponseRecorder { + t.Helper() + + for attempt := range LoginAttemptsPerMinute { + rec := postLogin(t, s, remoteAddr, forwardedFor, wrongKey) + if rec.Code != http.StatusOK { + t.Fatalf("failed login %d status = %d, want %d", + attempt+1, rec.Code, http.StatusOK) + } + } + + rec := postLogin(t, s, remoteAddr, forwardedFor, wrongKey) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("login over the limit status = %d, want %d", + rec.Code, http.StatusTooManyRequests) + } + + return rec +} + +// TestLoginRateLimitRefusesAttemptOverLimit verifies the login attempt +// after LoginAttemptsPerMinute failed ones from one client is refused with +// 429 and a Retry-After header, and that the client cannot get around the +// limit by sending X-Forwarded-For: from a peer that is not a trusted +// proxy, the header is ignored. +func TestLoginRateLimitRefusesAttemptOverLimit(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + rec := tripLoginRateLimit(t, s, firstClient, "") + + retryAfter := rec.Header().Get("Retry-After") + + seconds, err := strconv.Atoi(retryAfter) + if err != nil || seconds <= 0 { + t.Errorf("Retry-After = %q, want a positive number of seconds", + retryAfter) + } + + rec = postLogin(t, s, firstClient, secondForwarded, wrongKey) + if rec.Code != http.StatusTooManyRequests { + t.Errorf("login with X-Forwarded-For from an untrusted peer "+ + "status = %d, want %d", rec.Code, http.StatusTooManyRequests) + } +} + +// TestLoginRateLimitLeavesOtherClientsAlone verifies one client going over +// the limit does not limit another: a failed login from a different +// address is answered with the login form, and the signing key still logs +// it in. +func TestLoginRateLimitLeavesOtherClientsAlone(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + tripLoginRateLimit(t, s, firstClient, "") + + rec := postLogin(t, s, secondClient, "", wrongKey) + if rec.Code != http.StatusOK { + t.Errorf("failed login from another client status = %d, want %d", + rec.Code, http.StatusOK) + } + + rec = postLogin(t, s, secondClient, "", testSigningKey) + if rec.Code != http.StatusSeeOther { + t.Errorf("login with the signing key from another client "+ + "status = %d, want %d", rec.Code, http.StatusSeeOther) + } +} + +// TestLoginRateLimitCountsClientsBehindProxySeparately verifies the limit +// counts the client address resolved from X-Forwarded-For, not the address +// of the trusted proxy the requests arrive from, so two clients behind the +// same proxy are counted separately. +func TestLoginRateLimitCountsClientsBehindProxySeparately(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + tripLoginRateLimit(t, s, proxyPeer, firstForwarded) + + rec := postLogin(t, s, proxyPeer, secondForwarded, wrongKey) + if rec.Code != http.StatusOK { + t.Errorf("failed login from a second client behind the proxy "+ + "status = %d, want %d", rec.Code, http.StatusOK) + } +} + +// TestLoginRateLimitCountsIPv6ClientsByPrefix verifies an IPv6 client is +// counted by its /64: another address in the same /64 is refused too, +// while an address in a different /64 is not. +func TestLoginRateLimitCountsIPv6ClientsByPrefix(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + tripLoginRateLimit(t, s, proxyPeer, "2001:db8::1") + + rec := postLogin(t, s, proxyPeer, "2001:db8::2", wrongKey) + if rec.Code != http.StatusTooManyRequests { + t.Errorf("login from the same /64 status = %d, want %d", + rec.Code, http.StatusTooManyRequests) + } + + rec = postLogin(t, s, proxyPeer, "2001:db8:0:1::1", wrongKey) + if rec.Code != http.StatusOK { + t.Errorf("login from another /64 status = %d, want %d", + rec.Code, http.StatusOK) + } +} -- 2.54.0 From e6c326fc96880f6ccb6497611d7109d92dcb4352 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 21:22:25 +0000 Subject: [PATCH 2/4] Rate limit login attempts per client address (closes #66) POST / had no limit, so the signing key could be guessed at no cost. It is now limited to LoginAttemptsPerMinute (5) attempts per minute per client by a new RateLimit middleware on github.com/go-chi/httprate. It counts by the address the ClientIP middleware resolved through trusted_proxies, an IPv6 client by its /64, and answers an attempt over the limit with 429 and Retry-After. It runs after the body-size and CSRF checks, so every attempt that reaches the key comparison is counted. The image routes can reuse it. README states the limit; TODO narrows the per-IP item to the image routes. Model: opus-5-5 --- README.md | 7 +++++++ TODO.md | 10 +++++++++- go.mod | 3 +++ go.sum | 8 ++++++++ internal/middleware/middleware.go | 17 +++++++++++++++++ internal/server/routes.go | 10 +++++++++- 6 files changed, 53 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index ce8883d..04eb12e 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,13 @@ than once, is refused with 400. - ``: one of `orig`, `png`, `jpeg`, `webp` - ``: `orig` or `x` (e.g. `800x600`) +The login form (`POST /`) is limited to 5 attempts per minute per client +address, counting an IPv6 client by its /64; an attempt over the limit is +refused with 429 and a `Retry-After` header. Behind a reverse proxy the client +address comes from `X-Forwarded-For` only when the proxy's address is in +`trusted_proxies`; otherwise all users behind the proxy are counted as one +client. + ### Source Hosts Source hosts may be allowlisted in the configuration. Non-allowlisted diff --git a/TODO.md b/TODO.md index 672d6d1..d4ed1ed 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,14 @@ exhaustion # Completed Steps +- 2026-09-28 rate limit the login form (closes #66): `POST /` is limited to 5 + attempts per minute per client address, and an attempt over the limit is + refused with 429 and a `Retry-After` header; the address is the one + `internal/clientip` resolves through `trusted_proxies`, and an IPv6 client is + counted by its /64; the limit is a `RateLimit` middleware in + `internal/middleware` on `github.com/go-chi/httprate`, which the image routes + can reuse; the library keeps counts for the current and the previous minute + only; documented in `README.md`. - 2026-09-28 refuse an unparseable `exp` on `/v1/image/` and log swallowed cache errors (closes #72): an `exp` in the URL that is not a whole number, an empty `exp=` included, is a 400 naming `exp` and the value, @@ -245,7 +253,7 @@ exhaustion - P1: strip EXIF and other metadata from processed images (privacy) - P2: security - referer blacklist - - per-IP rate limiting + - per-IP rate limiting on the image routes - per-origin rate limiting - P2: HTTP response handling - Last-Modified headers diff --git a/go.mod b/go.mod index f3648e0..f5c4c98 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/getsentry/sentry-go v0.40.0 github.com/go-chi/chi/v5 v5.2.3 github.com/go-chi/cors v1.2.2 + github.com/go-chi/httprate v0.16.0 github.com/gorilla/csrf v1.7.3 github.com/gorilla/securecookie v1.1.2 github.com/prometheus/client_golang v1.23.2 @@ -91,6 +92,7 @@ require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect + github.com/klauspost/cpuid/v2 v2.2.10 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect github.com/mattn/go-colorable v0.1.13 // indirect @@ -113,6 +115,7 @@ require ( github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.0 // indirect github.com/x448/float16 v0.8.4 // indirect + github.com/zeebo/xxh3 v1.0.2 // indirect go.etcd.io/etcd/api/v3 v3.6.2 // indirect go.etcd.io/etcd/client/pkg/v3 v3.6.2 // indirect go.etcd.io/etcd/client/v3 v3.6.2 // indirect diff --git a/go.sum b/go.sum index af3e229..f9a4f3d 100644 --- a/go.sum +++ b/go.sum @@ -114,6 +114,8 @@ github.com/go-chi/chi/v5 v5.2.3 h1:WQIt9uxdsAbgIYgid+BpYc+liqQZGMHRaUwp0JUcvdE= github.com/go-chi/chi/v5 v5.2.3/go.mod h1:L2yAIGWB3H+phAw1NxKwWM+7eUH/lU8pOMm5hHcoops= github.com/go-chi/cors v1.2.2 h1:Jmey33TE+b+rB7fT8MUy1u0I4L+NARQlK6LhzKPSyQE= github.com/go-chi/cors v1.2.2/go.mod h1:sSbTewc+6wYHBBCW7ytsFSn836hqM7JxpglAy2Vzc58= +github.com/go-chi/httprate v0.16.0 h1:8V5DH9j6pSK6UQoBsTpvMyFxycqaKEIToyPKzHJjUa8= +github.com/go-chi/httprate v0.16.0/go.mod h1:A8lo+qRhk+s9LiuP5saS7XCGDXRXMcrueq0NfIuCa/I= github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA= github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og= github.com/go-jose/go-jose/v4 v4.0.5 h1:M6T8+mKZl/+fNNuFHvGIzDz7BTLQPIounk/b9dw3AaE= @@ -251,6 +253,8 @@ github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo= github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ= +github.com/klauspost/cpuid/v2 v2.2.10 h1:tBs3QSyvjDyFTq3uoc/9xFpCuOsJQFNPiAhYdw2skhE= +github.com/klauspost/cpuid/v2 v2.2.10/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0= github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= @@ -396,6 +400,10 @@ github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcY github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= +github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= +github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= +github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0= +github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA= go.etcd.io/etcd/api/v3 v3.6.2 h1:25aCkIMjUmiiOtnBIp6PhNj4KdcURuBak0hU2P1fgRc= go.etcd.io/etcd/api/v3 v3.6.2/go.mod h1:eFhhvfR8Px1P6SEuLT600v+vrhdDTdcfMzmnxVXXSbk= go.etcd.io/etcd/client/pkg/v3 v3.6.2 h1:zw+HRghi/G8fKpgKdOcEKpnBTE4OO39T6MegA0RopVU= diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 916e3c8..95a83e8 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -9,6 +9,7 @@ import ( basicauth "github.com/99designs/basicauth-go" "github.com/go-chi/chi/v5/middleware" "github.com/go-chi/cors" + "github.com/go-chi/httprate" metrics "github.com/slok/go-http-metrics/metrics/prometheus" ghmm "github.com/slok/go-http-metrics/middleware" "github.com/slok/go-http-metrics/middleware/std" @@ -88,6 +89,22 @@ func (s *Middleware) ClientIP() func(http.Handler) http.Handler { } } +// RateLimit returns a middleware that limits each client to requestLimit +// requests per window and refuses a request over the limit with 429 Too Many +// Requests and a Retry-After header. Clients are told apart by the address +// the ClientIP middleware stored in the request context, so ClientIP must +// run first. An IPv6 client is counted by its /64, which one client usually +// holds whole. Counts are kept only for the current and the previous +// window, so memory stays bounded. +func (s *Middleware) RateLimit( + requestLimit int, window time.Duration, +) func(http.Handler) http.Handler { + return httprate.LimitBy(requestLimit, window, + func(r *http.Request) (string, error) { + return httprate.CanonicalizeIP(clientip.FromContext(r.Context())), nil + }) +} + type loggingResponseWriter struct { http.ResponseWriter diff --git a/internal/server/routes.go b/internal/server/routes.go index 8e41fd7..0bb2748 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -2,6 +2,7 @@ package server import ( "net/http" + "time" sentryhttp "github.com/getsentry/sentry-go/http" "github.com/go-chi/chi/v5" @@ -12,6 +13,10 @@ import ( "sneak.berlin/go/pixa/internal/static" ) +// LoginAttemptsPerMinute is how many login attempts (POST /) one client may +// make per minute; the next is refused with 429 Too Many Requests. +const LoginAttemptsPerMinute = 5 + // SetupRoutes configures all HTTP routes. func (s *Server) SetupRoutes() { s.router = chi.NewRouter() @@ -50,11 +55,14 @@ func (s *Server) SetupRoutes() { // token cookie is independent of the session cookie, so it also // covers the login POST, where no session exists yet. LimitBody caps // the POST body ahead of CSRF, which reads its token from that body. + // The login POST is rate limited per client after both, so every + // attempt that reaches the signing key comparison is counted. s.router.Group(func(r chi.Router) { r.Use(s.h.LimitBody(handlers.MaxFormBytes)) r.Use(s.h.CSRF()) r.Get("/", s.h.HandleRoot()) - r.Post("/", s.h.HandleRoot()) + r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute)). + Post("/", s.h.HandleRoot()) r.Post("/generate", s.h.HandleGenerateURL()) }) -- 2.54.0 From 39051ee4a358a4696d2d49b096aa8f848cc76d2f Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 22:48:12 +0000 Subject: [PATCH 3/4] Test that IPv4-mapped login clients are counted apart (closes #66) Two IPv4 clients that the trusted proxy forwards in IPv4-mapped form must not share one login count. Fails: both fall in the same /64. Model: opus-5-5 --- .../server/login_rate_limit_internal_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/server/login_rate_limit_internal_test.go b/internal/server/login_rate_limit_internal_test.go index 721f148..13195c0 100644 --- a/internal/server/login_rate_limit_internal_test.go +++ b/internal/server/login_rate_limit_internal_test.go @@ -260,3 +260,21 @@ func TestLoginRateLimitCountsIPv6ClientsByPrefix(t *testing.T) { rec.Code, http.StatusOK) } } + +// TestLoginRateLimitCountsIPv4MappedClientsSeparately verifies an IPv4 +// client that the proxy forwards in IPv4-mapped IPv6 form (::ffff:a.b.c.d) +// is counted by its IPv4 address, not by the /64 that every such address +// shares, so two of them behind the proxy are counted separately. +func TestLoginRateLimitCountsIPv4MappedClientsSeparately(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + tripLoginRateLimit(t, s, proxyPeer, "::ffff:"+firstForwarded) + + rec := postLogin(t, s, proxyPeer, "::ffff:"+secondForwarded, wrongKey) + if rec.Code != http.StatusOK { + t.Errorf("failed login from a second IPv4-mapped client "+ + "status = %d, want %d", rec.Code, http.StatusOK) + } +} -- 2.54.0 From 194c0ded63e99e1e5b9e97e25e5357a6fd773d27 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 22:49:07 +0000 Subject: [PATCH 4/4] Count an IPv4-mapped login client by its IPv4 address (closes #66) A proxy on a dual-stack listener forwards an IPv4 client as ::ffff:a.b.c.d, whose /64 is the same for every IPv4 client, so one client's failed logins refused everyone's. The rate limit key now unmaps the address first. README.md now says that with the default trusted_proxies a client with a private address can choose its counted address through X-Forwarded-For, and that setting trusted_proxies to the proxy's own address closes this. Model: opus-5-5 --- README.md | 9 +++++++-- TODO.md | 11 ++++++----- internal/middleware/middleware.go | 16 +++++++++++++--- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 04eb12e..85af3f2 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,11 @@ address, counting an IPv6 client by its /64; an attempt over the limit is refused with 429 and a `Retry-After` header. Behind a reverse proxy the client address comes from `X-Forwarded-For` only when the proxy's address is in `trusted_proxies`; otherwise all users behind the proxy are counted as one -client. +client. With the default `trusted_proxies` (the RFC 1918 ranges), a client +with a private address can choose the address it is counted by through its own +`X-Forwarded-For`, whether it connects directly or through the proxy, because +its own address is trusted too. Setting `trusted_proxies` to the proxy's own +address closes this. ### Source Hosts @@ -212,7 +216,8 @@ Key settings in more detail: inside one of these ranges; the logged and login-recorded client address is then the rightmost forwarded entry that is not itself a trusted proxy. Otherwise the direct peer address is used and the header - is ignored, so a client connecting directly cannot spoof its address. + is ignored, so a client connecting directly from an address outside + these ranges cannot spoof its address. An omitted key defaults to the RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), since pixa is deployed behind a proxy on a private network; an explicitly empty list (`[]`) trusts no diff --git a/TODO.md b/TODO.md index d4ed1ed..486c8eb 100644 --- a/TODO.md +++ b/TODO.md @@ -33,11 +33,12 @@ exhaustion - 2026-09-28 rate limit the login form (closes #66): `POST /` is limited to 5 attempts per minute per client address, and an attempt over the limit is refused with 429 and a `Retry-After` header; the address is the one - `internal/clientip` resolves through `trusted_proxies`, and an IPv6 client is - counted by its /64; the limit is a `RateLimit` middleware in - `internal/middleware` on `github.com/go-chi/httprate`, which the image routes - can reuse; the library keeps counts for the current and the previous minute - only; documented in `README.md`. + `internal/clientip` resolves through `trusted_proxies`, an IPv6 client is + counted by its /64, and an IPv4-mapped address as the IPv4 address it + carries; the limit is a `RateLimit` middleware in `internal/middleware` on + `github.com/go-chi/httprate`, which the image routes can reuse; the library + keeps counts for the current and the previous minute only; documented in + `README.md`. - 2026-09-28 refuse an unparseable `exp` on `/v1/image/` and log swallowed cache errors (closes #72): an `exp` in the URL that is not a whole number, an empty `exp=` included, is a 400 naming `exp` and the value, diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 95a83e8..30feaa5 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -4,6 +4,7 @@ package middleware import ( "log/slog" "net/http" + "net/netip" "time" basicauth "github.com/99designs/basicauth-go" @@ -94,14 +95,23 @@ func (s *Middleware) ClientIP() func(http.Handler) http.Handler { // Requests and a Retry-After header. Clients are told apart by the address // the ClientIP middleware stored in the request context, so ClientIP must // run first. An IPv6 client is counted by its /64, which one client usually -// holds whole. Counts are kept only for the current and the previous -// window, so memory stays bounded. +// holds whole; an IPv4-mapped address (::ffff:a.b.c.d) is counted as the +// IPv4 address it carries, since every such address falls in the same /64. +// Counts are kept only for the current and the previous window, so memory +// stays bounded. func (s *Middleware) RateLimit( requestLimit int, window time.Duration, ) func(http.Handler) http.Handler { return httprate.LimitBy(requestLimit, window, func(r *http.Request) (string, error) { - return httprate.CanonicalizeIP(clientip.FromContext(r.Context())), nil + ip := clientip.FromContext(r.Context()) + + addr, err := netip.ParseAddr(ip) + if err == nil { + ip = addr.Unmap().String() + } + + return httprate.CanonicalizeIP(ip), nil }) } -- 2.54.0