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. It uses
the login limit's RateLimit middleware, so clients are told apart the same
way, and sits in the image routes' group behind the CORS middleware, so a
preflight is not counted. 0 turns it off; a negative or non-integer value
stops startup.

TestEnvironmentSetsEveryKey gains the new variable, under the owner's
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

README.md

@@ -319,9 +319,8 @@ One client may send `image_requests_per_minute` requests per minute
 `HEAD`, cache hits included; a request over the limit is refused with 429 and a
 `Retry-After` header. Clients are counted as for the login limit. A browser's
 CORS preflight is not counted. Behind a CDN or proxy whose addresses are not in
-`trusted_proxies`, all requests arriving from one proxy or CDN address count as
-one client, so such a deployment needs a higher limit, or `0`, which turns the
-limit off.
+`trusted_proxies`, every request counts as the same client, so such a deployment
+needs a higher limit, or `0`, which turns the limit off.
 
 ### Encrypted URLs
 
@@ -584,9 +583,8 @@ Key settings in more detail:
 - `image_requests_per_minute` — how many requests one client may send to the
   image routes per minute (see the limit on them under Routes); default `600`.
   `0` turns the limit off, and a negative or non-integer value aborts startup.
-  Behind a CDN or proxy whose addresses are not in `trusted_proxies`, all
-  requests arriving from one proxy or CDN address count as one client, so raise
-  the limit there, or set `0`
+  Behind a CDN or proxy whose addresses are not in `trusted_proxies`, every
+  request counts as the same client, so raise the limit there, or set `0`
 - `maintenance_mode` — while `true`, the image routes (`/v1/image/` and
   `/v1/e/`) answer every request for an image with 503, a `Retry-After` header
   and a JSON error body. The health check (`/.well-known/healthcheck.json`)

configs/config.example.yml

@@ -119,8 +119,8 @@ upstream_connections: 64
 # may send per minute; a request over the limit is answered 429 with a
 # Retry-After header. 0 turns the limit off. Clients are told apart as for
 # the login limit, so behind a CDN or proxy whose addresses are not in
-# trusted_proxies all requests arriving from one proxy or CDN address count
-# as one client: raise the limit there, or set 0. (default: 600)
+# trusted_proxies every request counts as the same client: raise the limit
+# there, or set 0. (default: 600)
 image_requests_per_minute: 600
 
 # Time allowed for one fetch from an upstream host (default: 30s)

internal/handlers/image.go

@@ -206,16 +206,6 @@ 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,10 +98,9 @@ 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. limitHandler writes the 429 after Retry-After is set; nil
-// keeps the rate limit library's plain-text "Too Many Requests".
+// stays bounded.
 func (s *Middleware) RateLimit(
-	requestLimit int, window time.Duration, limitHandler http.HandlerFunc,
+	requestLimit int, window time.Duration,
 ) func(http.Handler) http.Handler {
 	return httprate.LimitBy(requestLimit, window,
 		func(r *http.Request) (string, error) {
@@ -113,8 +112,7 @@ 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

@@ -1,7 +1,6 @@
 package server
 
 import (
-	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"strconv"
@@ -72,57 +71,6 @@ func TestImageRequestLimitRefusesRequestOverLimit(t *testing.T) {
 	}
 }
 
-// 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) {

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, nil)).
+		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute)).
 			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
 	})
@@ -88,13 +88,11 @@ 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
+			// Limited per client, as the login POST is; 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()))
+				r.Use(s.mw.RateLimit(s.config.ImageRequestsPerMinute, time.Minute))
 			}
 
 			// Main image proxy route

script/loadtest

@@ -94,8 +94,7 @@ main() {
 }
 
 # start_containers starts a new origin and a new pixad, and waits up to 30
-# seconds for pixad's health check to pass. pixad sees all of vegeta's
-# requests come from one address, so its image request limit is off.
+# seconds for pixad's health check to pass.
 start_containers() {
     docker run -d --name "$run-origin" \
         --network "$run" --network-alias origin \
@@ -105,7 +104,6 @@ start_containers() {
         --network "$run" -p 127.0.0.1::8080 --health-interval=1s \
         -e PIXA_SIGNING_KEY="$(head -c 32 /dev/urandom | base64)" \
         -e PIXA_ALLOWLIST_HOSTS=origin -e PIXA_ALLOW_HTTP=true \
-        -e PIXA_IMAGE_REQUESTS_PER_MINUTE=0 \
         "$image" >/dev/null
 
     waited=0