Limit each client's requests to the image routes (closes #236)

The new setting image_requests_per_minute (PIXA_IMAGE_REQUESTS_PER_MINUTE),
default 600, caps the GET and HEAD requests one client sends to /v1/image/
and /v1/e/ per minute; the next is answered 429 with Retry-After and the
image routes' JSON error body. It uses the login limit's RateLimit
middleware, so clients are told apart the same way, behind the CORS
middleware, so a preflight is not counted. 0 turns it off, as
script/loadtest does; a negative or non-integer value stops startup.

TestEnvironmentSetsEveryKey gains the new variable, under the standing
approval in #64. To stay within the
linters' limits, the sentry_dsn check moves unchanged into
validateSentryDSN, and the cache_max_bytes explicit flag is set in one
line.

Model: opus-5-5

internal/config/config.go

@@ -34,6 +34,7 @@ const (
 	DefaultUpstreamFetchTimeout       = 30 * time.Second
 	DefaultUpstreamMaxResponseSize    = 50 << 20 // 50 MiB
 	DefaultDownstreamTimeout          = 60 * time.Second
+	DefaultImageRequestsPerMinute     = 600
 )
 
 // Configuration key names.
@@ -61,6 +62,7 @@ const (
 	keyUpstreamFetchTimeout       = "upstream_fetch_timeout"
 	keyUpstreamMaxResponseSize    = "upstream_max_response_size"
 	keyDownstreamTimeout          = "downstream_timeout"
+	keyImageRequestsPerMinute     = "image_requests_per_minute"
 )
 
 // placeholderSigningKey is the dummy signing_key shipped in
@@ -158,6 +160,11 @@ type Config struct {
 	// of the per-request timeout middleware.
 	DownstreamTimeout time.Duration
 
+	// ImageRequestsPerMinute is how many requests one client may make to
+	// the image routes per minute; the next is refused with 429 Too Many
+	// Requests. 0 turns the limit off.
+	ImageRequestsPerMinute int
+
 	// BlockedNetworks are operator-supplied CIDR ranges to refuse in
 	// addition to the built-in SSRF blocklist. Enforced by the upstream
 	// fetcher's dialer; the built-in ranges always apply.
@@ -302,6 +309,8 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
 			keyAccessControlAllowOrigin, DefaultAccessControlAllowOrigin),
 		DownstreamTimeout: loader.durationVal(
 			keyDownstreamTimeout, DefaultDownstreamTimeout),
+		ImageRequestsPerMinute: loader.intVal(
+			keyImageRequestsPerMinute, DefaultImageRequestsPerMinute),
 		CacheMaxBytes:    loader.int64Val(keyCacheMaxBytes, 0),
 		BlockedNetworks:  blockedNetworks,
 		TrustedProxies:   trustedProxies,
@@ -311,9 +320,7 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
 	// The default for an omitted cache_max_bytes is worked out when
 	// the cache opens; here we only record whether the operator set
 	// the key explicitly.
-	if _, present := lookupValue(sc, keyCacheMaxBytes); present {
-		c.CacheMaxBytesExplicit = true
-	}
+	_, c.CacheMaxBytesExplicit = lookupValue(sc, keyCacheMaxBytes)
 
 	// Build DBURL from StateDir if not explicitly set. The derived URL
 	// is a default: it applies only when db_url is omitted, never to an
@@ -426,7 +433,7 @@ func isKnownConfigKey(key string) bool {
 		keyMaxConcurrentProcessing, keyCacheMaxBytes, keyBlockedNetworks,
 		keyTrustedProxies, keyAccessControlAllowOrigin, keyUpstreamFetchTimeout,
 		keyUpstreamMaxResponseSize, keyDownstreamTimeout, keyRefererBlocklist,
-		"env":
+		keyImageRequestsPerMinute, "env":
 		return true
 	}
 
@@ -461,6 +468,7 @@ func envVarNames() map[string]string {
 		keyUpstreamFetchTimeout:       "PIXA_UPSTREAM_FETCH_TIMEOUT",
 		keyUpstreamMaxResponseSize:    "PIXA_UPSTREAM_MAX_RESPONSE_SIZE",
 		keyDownstreamTimeout:          "PIXA_DOWNSTREAM_TIMEOUT",
+		keyImageRequestsPerMinute:     "PIXA_IMAGE_REQUESTS_PER_MINUTE",
 	}
 }
 
@@ -613,6 +621,14 @@ func (c *Config) validate() error {
 			settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative)
 	}
 
+	// Zero is valid (it turns the limit off); only negative values are
+	// rejected.
+	if c.ImageRequestsPerMinute < 0 {
+		return fmt.Errorf("%s: value %d %w",
+			settingName(keyImageRequestsPerMinute), c.ImageRequestsPerMinute,
+			errMustNotBeNegative)
+	}
+
 	err = c.validateUpstreamMaxResponseSize()
 	if err != nil {
 		return err
@@ -625,12 +641,9 @@ func (c *Config) validate() error {
 		}
 	}
 
-	if c.SentryDSN != "" {
-		parsed, err := url.Parse(c.SentryDSN)
-		if err != nil || parsed.Scheme == "" || parsed.Host == "" {
-			return fmt.Errorf("%s: value %q is %w",
-				settingName(keySentryDSN), c.SentryDSN, errNotAValidURL)
-		}
+	err = c.validateSentryDSN()
+	if err != nil {
+		return err
 	}
 
 	if (c.MetricsUsername == "") != (c.MetricsPassword == "") {
@@ -642,6 +655,22 @@ func (c *Config) validate() error {
 	return c.validateAccessControlAllowOrigin()
 }
 
+// validateSentryDSN checks that sentry_dsn, when set, is a URL with a scheme
+// and a host.
+func (c *Config) validateSentryDSN() error {
+	if c.SentryDSN == "" {
+		return nil
+	}
+
+	parsed, err := url.Parse(c.SentryDSN)
+	if err != nil || parsed.Scheme == "" || parsed.Host == "" {
+		return fmt.Errorf("%s: value %q is %w",
+			settingName(keySentryDSN), c.SentryDSN, errNotAValidURL)
+	}
+
+	return nil
+}
+
 // validateUpstreamMaxResponseSize checks that upstream_max_response_size
 // is from 1 byte to 1 GiB. An upstream response is read whole into
 // memory, and the image processor reads one byte past this limit, which

internal/config/env_internal_test.go

@@ -70,6 +70,7 @@ func TestEnvironmentSetsEveryKey(t *testing.T) {
 	t.Setenv("PIXA_UPSTREAM_CONNECTIONS_PER_HOST", "5")
 	t.Setenv("PIXA_UPSTREAM_CONNECTIONS", "10")
 	t.Setenv("PIXA_MAX_CONCURRENT_PROCESSING", "3")
+	t.Setenv("PIXA_IMAGE_REQUESTS_PER_MINUTE", "30")
 	t.Setenv("PIXA_CACHE_MAX_BYTES", "1024")
 	t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24")
 	t.Setenv("PIXA_TRUSTED_PROXIES", "192.0.2.0/24")
@@ -99,6 +100,7 @@ func TestEnvironmentSetsEveryKey(t *testing.T) {
 		UpstreamConnectionsPerHost: 5,
 		UpstreamConnections:        10,
 		MaxConcurrentProcessing:    3,
+		ImageRequestsPerMinute:     30,
 		CacheMaxBytes:              1024,
 		CacheMaxBytesExplicit:      true,
 		BlockedNetworks:            []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")},

internal/config/image_requests_per_minute_internal_test.go

@@ -0,0 +1,107 @@
+package config
+
+import "testing"
+
+// testImageRequestsPerMinuteVar is the variable that sets
+// image_requests_per_minute.
+const testImageRequestsPerMinuteVar = "PIXA_IMAGE_REQUESTS_PER_MINUTE"
+
+// TestImageRequestsPerMinuteValues checks that an omitted
+// image_requests_per_minute is 600, and that 0, which turns the limit off,
+// and any larger whole number are used as given.
+func TestImageRequestsPerMinuteValues(t *testing.T) {
+	t.Parallel()
+
+	c, err := configFromYAML(t, signingKeyLine)
+	if err != nil {
+		t.Fatalf("minimal config should be valid, got error: %v", err)
+	}
+
+	if c.ImageRequestsPerMinute != 600 {
+		t.Errorf("ImageRequestsPerMinute = %d, want 600", c.ImageRequestsPerMinute)
+	}
+
+	explicit := []struct {
+		value string
+		want  int
+	}{
+		{"0", 0},
+		{"30", 30},
+	}
+
+	for _, tc := range explicit {
+		c, err := configFromYAML(t,
+			signingKeyLine+"image_requests_per_minute: "+tc.value+"\n")
+		if err != nil {
+			t.Fatalf("image_requests_per_minute: %s should load, got error: %v",
+				tc.value, err)
+		}
+
+		if c.ImageRequestsPerMinute != tc.want {
+			t.Errorf("ImageRequestsPerMinute = %d, want %d",
+				c.ImageRequestsPerMinute, tc.want)
+		}
+	}
+}
+
+// TestInvalidImageRequestsPerMinuteAbortsStartup checks that a value that is
+// negative, not a whole number, or null aborts startup naming the key and
+// the value, and the variable too where the value could have come from it.
+func TestInvalidImageRequestsPerMinuteAbortsStartup(t *testing.T) {
+	t.Parallel()
+
+	key := keyImageRequestsPerMinute
+
+	runAbortCases(t, []abortCase{
+		{
+			name: "negative",
+			yaml: signingKeyLine + key + ": -1\n",
+			wantErrSubstrings: []string{
+				key, testImageRequestsPerMinuteVar, "value -1",
+			},
+		},
+		{
+			name: "not a number",
+			yaml: signingKeyLine + key + ": plenty\n",
+			wantErrSubstrings: []string{
+				key, testImageRequestsPerMinuteVar, "plenty",
+			},
+		},
+		{
+			name:              "fractional",
+			yaml:              signingKeyLine + key + ": 2.5\n",
+			wantErrSubstrings: []string{key, "2.5"},
+		},
+		{
+			name:              "explicit null",
+			yaml:              signingKeyLine + key + ": null\n",
+			wantErrSubstrings: []string{key, nullValueText},
+		},
+	})
+}
+
+// TestImageRequestsPerMinuteFromEnvironment checks that the variable sets the
+// limit over the config file, and that an invalid value in it aborts startup
+// naming the variable and the value.
+func TestImageRequestsPerMinuteFromEnvironment(t *testing.T) {
+	t.Setenv(testImageRequestsPerMinuteVar, "0")
+
+	c, err := configFromYAML(t, signingKeyLine+"image_requests_per_minute: 30\n")
+	if err != nil {
+		t.Fatalf("limit from the environment should load: %v", err)
+	}
+
+	if c.ImageRequestsPerMinute != 0 {
+		t.Errorf("ImageRequestsPerMinute = %d, want 0 from the environment",
+			c.ImageRequestsPerMinute)
+	}
+
+	for _, value := range []string{"-5", "ten"} {
+		t.Run(value, func(t *testing.T) {
+			t.Setenv(testImageRequestsPerMinuteVar, value)
+
+			_, err := configFromYAML(t, signingKeyLine)
+			wantStartupError(t, err, testImageRequestsPerMinuteVar, value)
+		})
+	}
+}

internal/handlers/image.go

@@ -206,6 +206,16 @@ func parseExpires(query url.Values) (time.Time, error) {
 	return time.Unix(exp, 0), nil
 }
 
+// HandleTooManyRequests answers 429 Too Many Requests with the JSON error
+// body the image routes send for their other errors. The image routes' rate
+// limit calls it for a request over the limit.
+func (s *Handlers) HandleTooManyRequests() http.HandlerFunc {
+	return func(w http.ResponseWriter, _ *http.Request) {
+		s.respondError(w, "too many requests, try again later",
+			http.StatusTooManyRequests)
+	}
+}
+
 // respondImageError maps image retrieval errors to HTTP responses.
 func (s *Handlers) respondImageError(
 	w http.ResponseWriter, req *imgcache.ImageRequest, err error,

internal/middleware/middleware.go

@@ -98,9 +98,10 @@ func (s *Middleware) ClientIP() func(http.Handler) http.Handler {
 // 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.
+// stays bounded. limitHandler writes the 429 after Retry-After is set; nil
+// keeps the rate limit library's plain-text "Too Many Requests".
 func (s *Middleware) RateLimit(
-	requestLimit int, window time.Duration,
+	requestLimit int, window time.Duration, limitHandler http.HandlerFunc,
 ) func(http.Handler) http.Handler {
 	return httprate.LimitBy(requestLimit, window,
 		func(r *http.Request) (string, error) {
@@ -112,7 +113,8 @@ func (s *Middleware) RateLimit(
 			}
 
 			return httprate.CanonicalizeIP(ip), nil
-		})
+		},
+		httprate.WithLimitHandler(limitHandler))
 }
 
 // requestIDPattern is what a request's own X-Request-Id must look like to be

internal/server/image_rate_limit_internal_test.go

@@ -0,0 +1,204 @@
+package server
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"testing"
+
+	"sneak.berlin/go/pixa/internal/config"
+)
+
+// requestImageFrom sends method path to s from remoteAddr and returns the
+// response. The image handlers refuse an unsigned image URL with 401 and a
+// token they cannot decrypt with 400, so either status shows the limit let
+// the request through.
+func requestImageFrom(
+	t *testing.T, s *Server, method, path, remoteAddr string,
+) *httptest.ResponseRecorder {
+	t.Helper()
+
+	req := httptest.NewRequestWithContext(t.Context(), method, path, nil)
+	req.RemoteAddr = remoteAddr
+
+	rec := httptest.NewRecorder()
+	s.ServeHTTP(rec, req)
+
+	return rec
+}
+
+// TestImageRequestLimitRefusesRequestOverLimit verifies that GET and HEAD on
+// both image routes count toward one limit per client, and that the request
+// after image_requests_per_minute of them is refused with 429 and a
+// Retry-After header.
+func TestImageRequestLimitRefusesRequestOverLimit(t *testing.T) {
+	t.Parallel()
+
+	requests := []struct {
+		method string
+		path   string
+		want   int
+	}{
+		{http.MethodGet, unsignedImagePath, http.StatusUnauthorized},
+		{http.MethodHead, unsignedImagePath, http.StatusUnauthorized},
+		{http.MethodGet, encryptedImagePath, http.StatusBadRequest},
+	}
+
+	s := newTestServer(t)
+	s.config.ImageRequestsPerMinute = len(requests)
+	s.SetupRoutes()
+
+	for _, tc := range requests {
+		rec := requestImageFrom(t, s, tc.method, tc.path, firstClient)
+		if rec.Code != tc.want {
+			t.Fatalf("%s %s within the limit status = %d, want %d",
+				tc.method, tc.path, rec.Code, tc.want)
+		}
+	}
+
+	rec := requestImageFrom(t, s, http.MethodHead, encryptedImagePath, firstClient)
+	if rec.Code != http.StatusTooManyRequests {
+		t.Fatalf("request over the limit status = %d, want %d",
+			rec.Code, http.StatusTooManyRequests)
+	}
+
+	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)
+	}
+}
+
+// TestImageRequestLimitAnswersWithJSONError verifies that the 429 for a
+// request over the limit carries Retry-After and the JSON error body the
+// image routes send for their other errors.
+func TestImageRequestLimitAnswersWithJSONError(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+	s.config.ImageRequestsPerMinute = 1
+	s.SetupRoutes()
+
+	requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
+
+	rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
+	t.Logf("status %d, body %s", rec.Code, rec.Body.String())
+
+	if rec.Code != http.StatusTooManyRequests {
+		t.Fatalf("request over the limit status = %d, want %d",
+			rec.Code, http.StatusTooManyRequests)
+	}
+
+	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)
+	}
+
+	contentType := rec.Header().Get("Content-Type")
+	if contentType != "application/json" {
+		t.Errorf("Content-Type = %q, want %q", contentType, "application/json")
+	}
+
+	var body struct {
+		Error     string `json:"error"`
+		Status    int    `json:"status"`
+		Timestamp string `json:"timestamp"`
+	}
+
+	err = json.NewDecoder(rec.Body).Decode(&body)
+	if err != nil {
+		t.Fatalf("body is not JSON: %v", err)
+	}
+
+	if body.Error == "" || body.Status != http.StatusTooManyRequests ||
+		body.Timestamp == "" {
+		t.Errorf("body = %+v, want an error, status %d and a timestamp",
+			body, http.StatusTooManyRequests)
+	}
+}
+
+// TestImageRequestLimitCountsClientsSeparately verifies that one client going
+// over the limit does not limit another.
+func TestImageRequestLimitCountsClientsSeparately(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+	s.config.ImageRequestsPerMinute = 1
+	s.SetupRoutes()
+
+	rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
+	if rec.Code != http.StatusUnauthorized {
+		t.Fatalf("first client's first request status = %d, want %d",
+			rec.Code, http.StatusUnauthorized)
+	}
+
+	rec = requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
+	if rec.Code != http.StatusTooManyRequests {
+		t.Fatalf("first client's second request status = %d, want %d",
+			rec.Code, http.StatusTooManyRequests)
+	}
+
+	rec = requestImageFrom(t, s, http.MethodGet, unsignedImagePath, secondClient)
+	if rec.Code != http.StatusUnauthorized {
+		t.Errorf("second client's first request status = %d, want %d",
+			rec.Code, http.StatusUnauthorized)
+	}
+}
+
+// TestImageRequestLimitZeroLetsEveryRequestThrough verifies that with
+// image_requests_per_minute set to 0 no image request is refused with 429,
+// not even past the default limit.
+func TestImageRequestLimitZeroLetsEveryRequestThrough(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+	s.config.ImageRequestsPerMinute = 0
+	s.SetupRoutes()
+
+	for request := range config.DefaultImageRequestsPerMinute + 1 {
+		rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
+		if rec.Code != http.StatusUnauthorized {
+			t.Fatalf("request %d status = %d, want %d",
+				request+1, rec.Code, http.StatusUnauthorized)
+		}
+	}
+}
+
+// TestImageRequestLimitDoesNotCountPreflight verifies that a browser's CORS
+// preflight request to an image route is answered without being counted, so
+// a client that has sent several still gets its image request through.
+func TestImageRequestLimitDoesNotCountPreflight(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+	s.config.AccessControlAllowOrigin = config.DefaultAccessControlAllowOrigin
+	s.config.ImageRequestsPerMinute = 1
+	s.SetupRoutes()
+
+	for range 3 {
+		req := httptest.NewRequestWithContext(
+			t.Context(), http.MethodOptions, unsignedImagePath, nil)
+		req.RemoteAddr = firstClient
+		req.Header.Set("Origin", "https://app.example.com")
+		req.Header.Set("Access-Control-Request-Method", http.MethodGet)
+
+		rec := httptest.NewRecorder()
+		s.ServeHTTP(rec, req)
+
+		if rec.Code != http.StatusOK {
+			t.Fatalf("preflight status = %d, want %d", rec.Code, http.StatusOK)
+		}
+	}
+
+	rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
+	if rec.Code != http.StatusUnauthorized {
+		t.Errorf("image request after preflights status = %d, want %d",
+			rec.Code, http.StatusUnauthorized)
+	}
+}

internal/server/routes.go

@@ -66,7 +66,7 @@ func (s *Server) SetupRoutes() {
 		r.Use(s.h.LimitBody(handlers.MaxFormBytes))
 		r.Use(s.h.CSRF())
 		r.Get("/", s.h.HandleRoot())
-		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute)).
+		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute, nil)).
 			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
 	})
@@ -88,6 +88,15 @@ func (s *Server) SetupRoutes() {
 		r.Group(func(r chi.Router) {
 			r.Use(s.refuseDuringMaintenance)
 
+			// Limited per client, as the login POST is, but the 429 has
+			// the image routes' JSON error body; 0 turns the limit off.
+			// The CORS middleware answers a preflight before it gets
+			// here, so a preflight is not counted.
+			if s.config.ImageRequestsPerMinute > 0 {
+				r.Use(s.mw.RateLimit(s.config.ImageRequestsPerMinute,
+					time.Minute, s.h.HandleTooManyRequests()))
+			}
+
 			// Main image proxy route
 			// /v1/image/<host>/<path>/<width>x<height>.<format>, or with no
 			// format /v1/image/<host>/<path>/<width>x<height>