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

README.md

@@ -319,8 +319,9 @@ 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`, every request counts as the same client, so such a deployment
-needs a higher limit, or `0`, which turns the limit off.
+`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.
 
 ### Encrypted URLs
 
@@ -583,8 +584,9 @@ 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`, every
-  request counts as the same client, so raise the limit there, or set `0`
+  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`
 - `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 every request counts as the same client: raise the limit
-# there, or set 0. (default: 600)
+# trusted_proxies all requests arriving from one proxy or CDN address count
+# as one 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,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

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"strconv"
@@ -71,6 +72,57 @@ 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)).
+		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute, nil)).
 			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
 	})
@@ -88,11 +88,13 @@ func (s *Server) SetupRoutes() {
 		r.Group(func(r chi.Router) {
 			r.Use(s.refuseDuringMaintenance)
 
-			// Limited per client, as the login POST is; 0 turns the limit
-			// off. The CORS middleware answers a preflight before it gets
+			// 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))
+				r.Use(s.mw.RateLimit(s.config.ImageRequestsPerMinute,
+					time.Minute, s.h.HandleTooManyRequests()))
 			}
 
 			// Main image proxy route

script/loadtest

@@ -94,7 +94,8 @@ main() {
 }
 
 # start_containers starts a new origin and a new pixad, and waits up to 30
-# seconds for pixad's health check to pass.
+# 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.
 start_containers() {
     docker run -d --name "$run-origin" \
         --network "$run" --network-alias origin \
@@ -104,6 +105,7 @@ 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