From ad0714801c6ef606b3d1e70392c017911d2849b2 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 12 Oct 2026 09:14:44 +0000 Subject: [PATCH] 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 https://git.eeqj.de/sneak/pixa/issues/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 | 49 ++++-- TODO.md | 12 +- configs/config.example.yml | 8 + internal/config/config.go | 49 ++++-- internal/config/env_internal_test.go | 2 + ...image_requests_per_minute_internal_test.go | 107 ++++++++++++ .../server/image_rate_limit_internal_test.go | 152 ++++++++++++++++++ internal/server/routes.go | 7 + 8 files changed, 356 insertions(+), 30 deletions(-) create mode 100644 internal/config/image_requests_per_minute_internal_test.go create mode 100644 internal/server/image_rate_limit_internal_test.go diff --git a/README.md b/README.md index 8b6b032..1aab093 100644 --- a/README.md +++ b/README.md @@ -46,9 +46,9 @@ everything in this list without further settings. The reverse proxy must: builds encrypted URLs from `Host`, and checks `Referer` against `referer_blocklist`; - set `X-Forwarded-For` to the client's address, with `trusted_proxies` set to - the address pixa sees the proxy's requests come from, so the login limit - counts each client by its own address (see `trusted_proxies` under - Configuration); + the address pixa sees the proxy's requests come from, so the login limit and + `image_requests_per_minute` count each client by its own address (see + `trusted_proxies` under Configuration); - wait for pixa's answer for at least `downstream_timeout` (default `60s`), the longest pixa takes to fetch, convert and send an image. @@ -58,8 +58,9 @@ password there. pixa does the rest itself: it checks signatures and encrypted URLs, applies the allowlist, refuses upstream hosts with private or local addresses, limits login -attempts, upstream response size and image dimensions, and sends the security -headers, `Strict-Transport-Security` included, with every response. +attempts, each client's image requests, upstream response size and image +dimensions, and sends the security headers, `Strict-Transport-Security` +included, with every response. The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the database and the disk cache: @@ -186,10 +187,10 @@ path under `/v1/` answers 200, in maintenance mode too. `Referer` names a host in `referer_blocklist`, checked before the signature, the cache and the upstream fetch; 403 when the upstream host, or a host it redirects to, is `localhost`, ends in `.localhost` or `.local`, or has an - address in a blocked network (see `blocked_networks`); 502 when the upstream - answered with an error status, and for 5 minutes after that for the same - source URL; 503 when pixa is busy or in maintenance mode; 500 for any other - failure. + address in a blocked network (see `blocked_networks`); 429 over + `image_requests_per_minute` (below); 502 when the upstream answered with an + error status, and for 5 minutes after that for the same source URL; 503 when + pixa is busy or in maintenance mode; 500 for any other failure. - `GET` or `HEAD` `/v1/e//` — an image through an encrypted URL (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when `If-None-Match` matches the image's `ETag`; 400 for a token that does not @@ -197,7 +198,7 @@ path under `/v1/` answers 200, in maintenance mode too. expired; 504 when the upstream has not sent its response headers within `upstream_fetch_timeout`, but 500 when that time runs out while the image itself is still arriving; 400 for an `Accept` header that is not valid, and - 406, 403, 502, 503 and 500, as for `/v1/image/`. + 406, 403, 429, 502, 503 and 500, as for `/v1/image/`. - `GET /robots.txt` — asks every crawler to stay away (`Disallow: /`). Needs: nothing. Answers: 200. - `GET /.well-known/healthcheck.json` — JSON with `status` (`ok`), `now`, @@ -313,6 +314,14 @@ its own `X-Forwarded-For`, whether it connects directly or through the proxy, because its own address is trusted too. Setting `trusted_proxies` to only the address pixa sees for requests that come through the proxy closes this. +One client may send `image_requests_per_minute` requests per minute +(default 600) to the image routes, `/v1/image/` and `/v1/e/` together, `GET` and +`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. + ### Encrypted URLs An encrypted URL is an image URL made on pixa's own web page by someone who @@ -482,6 +491,7 @@ tell. With no file, pixa uses the environment and the defaults. | `PIXA_UPSTREAM_CONNECTIONS_PER_HOST` | `upstream_connections_per_host` | Concurrent connections per upstream host; default `20` | | `PIXA_UPSTREAM_CONNECTIONS` | `upstream_connections` | Concurrent connections to all upstream hosts together; default `64` | | `PIXA_MAX_CONCURRENT_PROCESSING` | `max_concurrent_processing` | Images processed at once; default the number of CPUs | +| `PIXA_IMAGE_REQUESTS_PER_MINUTE` | `image_requests_per_minute` | Image requests per client per minute; `0` turns it off; default `600` | | `PIXA_UPSTREAM_FETCH_TIMEOUT` | `upstream_fetch_timeout` | Time allowed for one fetch from an upstream host; default `30s` | | `PIXA_UPSTREAM_MAX_RESPONSE_SIZE` | `upstream_max_response_size` | Largest upstream response accepted, in bytes; default 50 MiB | | `PIXA_DOWNSTREAM_TIMEOUT` | `downstream_timeout` | Time allowed for answering one client request; default `60s` | @@ -531,13 +541,13 @@ Key settings in more detail: explicit list replaces the default. An invalid CIDR aborts startup. Set this to the address pixa sees for requests that come through your proxy, such as `172.17.0.1/32`, when the defaults do not cover it, or to trust nothing else - (see the login limit under Routes). For a proxy on the Docker host that - connects to pixa over `127.0.0.1`, that address is the gateway of the - container's Docker network (`172.17.0.1` on the default bridge), not the - proxy's own address; a proxy that connects through another of the host's - addresses is seen with that address. To be sure which address it is, set this - to `[]` (or `PIXA_TRUSTED_PROXIES` to empty), send a request through the - proxy, and read `remoteIP` in pixa's request log line for it + (see the login limit and `image_requests_per_minute` under Routes). For a + proxy on the Docker host that connects to pixa over `127.0.0.1`, that address + is the gateway of the container's Docker network (`172.17.0.1` on the default + bridge), not the proxy's own address; a proxy that connects through another of + the host's addresses is seen with that address. To be sure which address it + is, set this to `[]` (or `PIXA_TRUSTED_PROXIES` to empty), send a request + through the proxy, and read `remoteIP` in pixa's request log line for it - `upstream_fetch_timeout` — time allowed for one fetch from an upstream host, as a duration such as `30s` (the default) or `2m` - `upstream_max_response_size` — largest upstream response accepted, in bytes; @@ -570,6 +580,11 @@ Key settings in more detail: container's CPU limit. A request that finds all of them in use waits up to 10 seconds for one to free up; if none does, and `downstream_timeout` has not ended first, it is answered 503 the same way +- `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` - `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`) diff --git a/TODO.md b/TODO.md index 7bee655..561b24e 100644 --- a/TODO.md +++ b/TODO.md @@ -26,10 +26,18 @@ resolved. The disk cache is now size-bounded with LRU eviction # Next Step -P2: security: per-IP rate limiting on the image routes +P2: security: per-origin rate limiting # Completed Steps +- 2026-10-12 per-client limit on the image routes (closes #236): the new setting + `image_requests_per_minute` (`PIXA_IMAGE_REQUESTS_PER_MINUTE`), default 600, + is how many `GET` and `HEAD` requests one client may send to `/v1/image/` and + `/v1/e/` together per minute, cache hits included; 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 runs after the CORS middleware, so a + preflight is not counted. `0` turns the limit off; a negative or non-integer + value aborts startup. - 2026-10-08 every output format is saved with settings pixa sets on purpose (closes #232): each format has its own govips export, as JPEG XL does, in place of govips' generic `Export`, which sent libvips a zero for some settings @@ -736,8 +744,6 @@ P2: security: per-IP rate limiting on the image routes # Future Steps -- P2: security - - per-origin rate limiting - P2: HTTP response handling - Last-Modified headers - P2: configuration diff --git a/configs/config.example.yml b/configs/config.example.yml index cede448..1e2351f 100644 --- a/configs/config.example.yml +++ b/configs/config.example.yml @@ -115,6 +115,14 @@ upstream_connections: 64 # frees up it is answered 503, unless downstream_timeout has ended first. # max_concurrent_processing: 4 +# How many requests to the image routes (/v1/image/ and /v1/e/) one client +# 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) +image_requests_per_minute: 600 + # Time allowed for one fetch from an upstream host (default: 30s) upstream_fetch_timeout: 30s diff --git a/internal/config/config.go b/internal/config/config.go index f4e0fc4..d4b78a2 100644 --- a/internal/config/config.go +++ b/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 diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go index 6eb79d9..4b23302 100644 --- a/internal/config/env_internal_test.go +++ b/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")}, diff --git a/internal/config/image_requests_per_minute_internal_test.go b/internal/config/image_requests_per_minute_internal_test.go new file mode 100644 index 0000000..12f63ab --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/server/image_rate_limit_internal_test.go b/internal/server/image_rate_limit_internal_test.go new file mode 100644 index 0000000..fc87de7 --- /dev/null +++ b/internal/server/image_rate_limit_internal_test.go @@ -0,0 +1,152 @@ +package server + +import ( + "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) + } +} + +// 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) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index ae2680b..f2099f4 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -88,6 +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 + // here, so a preflight is not counted. + if s.config.ImageRequestsPerMinute > 0 { + r.Use(s.mw.RateLimit(s.config.ImageRequestsPerMinute, time.Minute)) + } + // Main image proxy route // /v1/image///x., or with no // format /v1/image///x -- 2.54.0