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 5 attempts per minute per client by a new RateLimit middleware on github.com/go-chi/httprate; an attempt over the limit gets 429 with Retry-After. It counts by the address the ClientIP middleware resolved through trusted_proxies (an IPv4-mapped address as its IPv4 address, IPv6 by its /64) and runs after the body-size and CSRF checks, so every attempt that reaches the key comparison is counted. README says that with the default trusted_proxies a client with a private address can choose its counted address, and how to close that. Model: opus-5-5
2026-09-29 01:03:37 +02:00
parent 6010f5beb0
commit e410146fb6
7 changed files with 350 additions and 3 deletions
@@ -0,0 +1,280 @@
+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)
+	}
+}
+
+// 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,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())
 	})