From 52b64c91973b393dd2defa78b04a0860f9199831 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 19:51:14 +0000 Subject: [PATCH 01/12] Test four README settings pixa does not have yet (closes #61) Failing tests for access_control_allow_origin, upstream_fetch_timeout, upstream_max_response_size and downstream_timeout: their defaults, valid values from the file and the environment, invalid values aborting startup naming the key or variable and the value, the CORS middleware answering with the configured origin, and the server's write timeout coming from downstream_timeout. They do not compile until the settings exist. Model: opus-5-5 --- .../config/config_validation_internal_test.go | 215 ++++++++++++++++++ internal/config/env_internal_test.go | 33 +++ .../middleware/middleware_internal_test.go | 47 ++++ internal/server/http_internal_test.go | 10 +- 4 files changed, 302 insertions(+), 3 deletions(-) diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index c12d27d..cb6f9ca 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "git.eeqj.de/sneak/smartconfig" ) @@ -599,3 +600,217 @@ func TestEnsureStateDirFailsOnUncreatablePath(t *testing.T) { t.Errorf("error %q does not name the offending key state_dir", err.Error()) } } + +// TestOmittedOriginTimeoutsAndSizeUseDefaults checks that the CORS +// origin, the upstream fetch timeout, the upstream response size limit +// and the downstream timeout default to the values pixa used before they +// could be configured. +func TestOmittedOriginTimeoutsAndSizeUseDefaults(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.AccessControlAllowOrigin != "*" { + t.Errorf("AccessControlAllowOrigin = %q, want *", c.AccessControlAllowOrigin) + } + + if c.UpstreamFetchTimeout != 30*time.Second { + t.Errorf("UpstreamFetchTimeout = %v, want 30s", c.UpstreamFetchTimeout) + } + + if c.UpstreamMaxResponseSize != 50<<20 { + t.Errorf("UpstreamMaxResponseSize = %d, want %d (50 MiB)", + c.UpstreamMaxResponseSize, 50<<20) + } + + if c.DownstreamTimeout != 60*time.Second { + t.Errorf("DownstreamTimeout = %v, want 60s", c.DownstreamTimeout) + } +} + +// TestExplicitOriginTimeoutsAndSizeAreUsed checks that valid values for +// the CORS origin, the two timeouts and the response size limit are used +// as given. +func TestExplicitOriginTimeoutsAndSizeAreUsed(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+` +access_control_allow_origin: https://app.example.com +upstream_fetch_timeout: 10s +upstream_max_response_size: 1048576 +downstream_timeout: 2m +`) + if err != nil { + t.Fatalf("valid config should load, got error: %v", err) + } + + if c.AccessControlAllowOrigin != "https://app.example.com" { + t.Errorf("AccessControlAllowOrigin = %q, want https://app.example.com", + c.AccessControlAllowOrigin) + } + + if c.UpstreamFetchTimeout != 10*time.Second { + t.Errorf("UpstreamFetchTimeout = %v, want 10s", c.UpstreamFetchTimeout) + } + + if c.UpstreamMaxResponseSize != 1048576 { + t.Errorf("UpstreamMaxResponseSize = %d, want 1048576", + c.UpstreamMaxResponseSize) + } + + if c.DownstreamTimeout != 2*time.Minute { + t.Errorf("DownstreamTimeout = %v, want 2m", c.DownstreamTimeout) + } +} + +// TestOriginWithPortOrAnyOriginIsAccepted checks the other accepted forms +// of access_control_allow_origin: "*", and an origin with a port. +func TestOriginWithPortOrAnyOriginIsAccepted(t *testing.T) { + t.Parallel() + + for _, origin := range []string{"*", "http://localhost:3000"} { + c, err := configFromYAML(t, signingKeyLine+ + "access_control_allow_origin: \""+origin+"\"\n") + if err != nil { + t.Fatalf("origin %q should be accepted, got error: %v", origin, err) + } + + if c.AccessControlAllowOrigin != origin { + t.Errorf("AccessControlAllowOrigin = %q, want %q", + c.AccessControlAllowOrigin, origin) + } + } +} + +// invalidTimeoutCases are configs where upstream_fetch_timeout or +// downstream_timeout is not a positive Go duration string; each must +// abort startup naming the key and the value. +func invalidTimeoutCases() []abortCase { + return []abortCase{ + { + name: "upstream_fetch_timeout not a duration", + yaml: signingKeyLine + "upstream_fetch_timeout: soon\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "soon"}, + }, + { + name: "upstream_fetch_timeout number without a unit", + yaml: signingKeyLine + "upstream_fetch_timeout: 45\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "45"}, + }, + { + name: "upstream_fetch_timeout zero", + yaml: signingKeyLine + "upstream_fetch_timeout: 0s\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "0s"}, + }, + { + name: "upstream_fetch_timeout negative", + yaml: signingKeyLine + "upstream_fetch_timeout: -5s\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "-5s"}, + }, + { + name: "upstream_fetch_timeout null", + yaml: signingKeyLine + "upstream_fetch_timeout: null\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, nullValueText}, + }, + { + name: "downstream_timeout not a duration", + yaml: signingKeyLine + "downstream_timeout: 1 minute\n", + wantErrSubstrings: []string{keyDownstreamTimeout, "1 minute"}, + }, + { + name: "downstream_timeout zero", + yaml: signingKeyLine + "downstream_timeout: 0s\n", + wantErrSubstrings: []string{keyDownstreamTimeout, "0s"}, + }, + { + name: "downstream_timeout negative", + yaml: signingKeyLine + "downstream_timeout: -1m\n", + wantErrSubstrings: []string{keyDownstreamTimeout, "-1m"}, + }, + { + name: "downstream_timeout null", + yaml: signingKeyLine + "downstream_timeout:\n", + wantErrSubstrings: []string{keyDownstreamTimeout, nullValueText}, + }, + } +} + +// invalidSizeAndOriginCases are configs where upstream_max_response_size +// is not a positive whole number of bytes, or access_control_allow_origin +// is neither "*" nor an origin; each must abort startup naming the key +// and the value. +func invalidSizeAndOriginCases() []abortCase { + return []abortCase{ + { + name: "upstream_max_response_size with a unit", + yaml: signingKeyLine + "upstream_max_response_size: 50MB\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "50MB"}, + }, + { + name: "upstream_max_response_size fractional", + yaml: signingKeyLine + "upstream_max_response_size: 1.5\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "1.5"}, + }, + { + name: "upstream_max_response_size zero", + yaml: signingKeyLine + "upstream_max_response_size: 0\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "0"}, + }, + { + name: "upstream_max_response_size negative", + yaml: signingKeyLine + "upstream_max_response_size: -1\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "-1"}, + }, + { + name: "upstream_max_response_size null", + yaml: signingKeyLine + "upstream_max_response_size: null\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, nullValueText}, + }, + { + name: "access_control_allow_origin bare hostname", + yaml: signingKeyLine + "access_control_allow_origin: example.com\n", + wantErrSubstrings: []string{ + keyAccessControlAllowOrigin, "example.com", + }, + }, + { + name: "access_control_allow_origin with a path", + yaml: signingKeyLine + + "access_control_allow_origin: https://example.com/images\n", + wantErrSubstrings: []string{ + keyAccessControlAllowOrigin, "https://example.com/images", + }, + }, + { + name: "access_control_allow_origin trailing slash", + yaml: signingKeyLine + + "access_control_allow_origin: https://example.com/\n", + wantErrSubstrings: []string{ + keyAccessControlAllowOrigin, "https://example.com/", + }, + }, + { + name: "access_control_allow_origin empty", + yaml: signingKeyLine + "access_control_allow_origin: \"\"\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin}, + }, + { + name: "access_control_allow_origin null", + yaml: signingKeyLine + "access_control_allow_origin: null\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, nullValueText}, + }, + } +} + +// TestInvalidOriginTimeoutOrSizeAbortsStartup verifies the +// no-silent-fallback rule for the CORS origin, the two timeouts and the +// response size limit: a value that does not parse or is out of range +// aborts startup naming the key and the value. +func TestInvalidOriginTimeoutOrSizeAbortsStartup(t *testing.T) { + t.Parallel() + + runAbortCases(t, append(invalidTimeoutCases(), invalidSizeAndOriginCases()...)) +} diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go index 57f1f15..cdc8790 100644 --- a/internal/config/env_internal_test.go +++ b/internal/config/env_internal_test.go @@ -8,6 +8,7 @@ import ( "slices" "strings" "testing" + "time" "sneak.berlin/go/pixa/internal/globals" "sneak.berlin/go/pixa/internal/logger" @@ -69,6 +70,10 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { 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") + t.Setenv("PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", "https://app.example.com") + t.Setenv("PIXA_UPSTREAM_FETCH_TIMEOUT", "10s") + t.Setenv("PIXA_UPSTREAM_MAX_RESPONSE_SIZE", "1048576") + t.Setenv("PIXA_DOWNSTREAM_TIMEOUT", "2m") c, err := newFromSmartConfig(nil) if err != nil { @@ -92,6 +97,10 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { cacheMaxBytesExplicit: true, BlockedNetworks: []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, TrustedProxies: []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}, + AccessControlAllowOrigin: "https://app.example.com", + UpstreamFetchTimeout: 10 * time.Second, + UpstreamMaxResponseSize: 1048576, + DownstreamTimeout: 2 * time.Minute, } if !reflect.DeepEqual(*c, want) { @@ -280,6 +289,30 @@ func TestInvalidDebugFromEnvironmentAbortsStartup(t *testing.T) { wantStartupError(t, err, "PIXA_DEBUG", "maybe") } +// TestInvalidOriginTimeoutOrSizeFromEnvironmentAbortsStartup checks that +// an invalid CORS origin, timeout or response size limit in its variable +// aborts startup naming the variable and the value. +func TestInvalidOriginTimeoutOrSizeFromEnvironmentAbortsStartup(t *testing.T) { + cases := []struct { + variable string + value string + }{ + {"PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", "example.com"}, + {"PIXA_UPSTREAM_FETCH_TIMEOUT", "soon"}, + {"PIXA_UPSTREAM_MAX_RESPONSE_SIZE", "50MB"}, + {"PIXA_DOWNSTREAM_TIMEOUT", "0s"}, + } + + for _, tc := range cases { + t.Run(tc.variable, func(t *testing.T) { + t.Setenv(tc.variable, tc.value) + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, tc.variable, tc.value) + }) + } +} + // TestConfigFileAloneBehavesAsBefore checks that with no variables set // (TestMain unsets them) the config file's values are used and omitted // keys take their defaults. diff --git a/internal/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 9992a60..3421784 100644 --- a/internal/middleware/middleware_internal_test.go +++ b/internal/middleware/middleware_internal_test.go @@ -9,6 +9,53 @@ import ( "sneak.berlin/go/pixa/internal/config" ) +// TestCORSAnswersWithConfiguredOrigin checks that the CORS middleware +// uses access_control_allow_origin: "*" lets any origin read responses, +// and a single origin lets that origin read them and no other. +func TestCORSAnswersWithConfiguredOrigin(t *testing.T) { + t.Parallel() + + const appOrigin = "https://app.example.com" + + cases := []struct { + configured string + requestOrigin string + want string + }{ + {"*", "https://any.example.com", "*"}, + {appOrigin, appOrigin, appOrigin}, + {appOrigin, "https://other.example.com", ""}, + } + + testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + for _, tc := range cases { + mw := &Middleware{ + log: slog.Default(), + config: &config.Config{AccessControlAllowOrigin: tc.configured}, + } + + handler := mw.CORS()(testHandler) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/v1/image/example.com/a.jpg/1x1.png", nil) + req.Header.Set("Origin", tc.requestOrigin) + + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + got := rec.Header().Get("Access-Control-Allow-Origin") + if got != tc.want { + t.Errorf("configured %q, request from %q: "+ + "Access-Control-Allow-Origin = %q, want %q", + tc.configured, tc.requestOrigin, got, tc.want) + } + } +} + func TestSecurityHeaders(t *testing.T) { t.Parallel() diff --git a/internal/server/http_internal_test.go b/internal/server/http_internal_test.go index d94a380..25b60fb 100644 --- a/internal/server/http_internal_test.go +++ b/internal/server/http_internal_test.go @@ -11,11 +11,15 @@ import ( // carries every hardening timeout wired onto it, including the slowloris // defense (ReadHeaderTimeout) and the keep-alive bound (IdleTimeout). This // guards against a field being defined but never set on the server, so -// each assertion compares the server field to its constant. +// each assertion compares the server field to its constant, or, for +// WriteTimeout, to downstream_timeout from the config. func TestNewHTTPServerTimeouts(t *testing.T) { t.Parallel() - s := &Server{config: &config.Config{Port: 8080}} + s := &Server{config: &config.Config{ + Port: 8080, + DownstreamTimeout: 45 * time.Second, + }} srv := s.newHTTPServer() @@ -26,7 +30,7 @@ func TestNewHTTPServerTimeouts(t *testing.T) { }{ {"ReadTimeout", srv.ReadTimeout, HTTPReadTimeout}, {"ReadHeaderTimeout", srv.ReadHeaderTimeout, HTTPReadHeaderTimeout}, - {"WriteTimeout", srv.WriteTimeout, HTTPWriteTimeout}, + {"WriteTimeout", srv.WriteTimeout, 45 * time.Second}, {"IdleTimeout", srv.IdleTimeout, HTTPIdleTimeout}, } -- 2.54.0 From aaf1bbbd4e595ca6af8728f1974499e1bbb28865 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 19:51:14 +0000 Subject: [PATCH 02/12] Add the four settings the README documented but pixa lacked (closes #61) access_control_allow_origin, upstream_fetch_timeout, upstream_max_response_size and downstream_timeout were in the README but unknown to pixa, so a config that followed it aborted startup. Each is now a setting with its PIXA_ variable, defaulting to the value that was fixed in the code: *, 30s, 50 MiB and 60s. Durations are positive Go duration strings; the size is a positive whole number of bytes; the origin is * or one scheme and host. An invalid value aborts startup naming the key and value. downstream_timeout replaces HTTPWriteTimeout for the server's write timeout and the per-request timeout. Model: opus-5-5 --- README.md | 20 ++++- TODO.md | 10 ++- config.example.yml | 19 +++++ internal/config/config.go | 119 +++++++++++++++++++++++++++++- internal/handlers/handlers.go | 2 + internal/middleware/middleware.go | 2 +- internal/server/http.go | 3 +- internal/server/routes.go | 2 +- 8 files changed, 167 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 4102390..af1b0e1 100644 --- a/README.md +++ b/README.md @@ -235,6 +235,10 @@ variables set by the file's `env:` section are checked the same way. | `PIXA_TRUSTED_PROXIES` | `trusted_proxies` | CIDR ranges of proxies whose `X-Forwarded-For` is believed; default RFC 1918 | | `PIXA_ALLOW_HTTP` | `allow_http` | Allow plain-HTTP upstreams, for testing only; default `false` | | `PIXA_UPSTREAM_CONNECTIONS_PER_HOST` | `upstream_connections_per_host` | Concurrent connections per upstream host; default `20` | +| `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` | +| `PIXA_ACCESS_CONTROL_ALLOW_ORIGIN` | `access_control_allow_origin` | CORS origin allowed to read responses: `*` or one origin; default `*` | | `PIXA_METRICS_USERNAME` | `metrics.username` | Username for `/metrics`, which is served only when both are set | | `PIXA_METRICS_PASSWORD` | `metrics.password` | Password for `/metrics`; set together with the username | | `PIXA_SENTRY_DSN` | `sentry_dsn` | Sentry DSN for error reporting; empty disables it | @@ -243,7 +247,10 @@ variables set by the file's `env:` section are checked the same way. Key settings in more detail: -- `access_control_allow_origin` — CORS origin +- `access_control_allow_origin` — the origin a browser lets read pixa's + responses, sent as the CORS `Access-Control-Allow-Origin` header: `*`, the + default, is any site; otherwise one origin, scheme and host only, such as + `https://example.com`. Anything else aborts startup - `allowlist_hosts` — list of allowed upstream hosts - `blocked_networks` — list of CIDR ranges to refuse for SSRF protection, added to the always-enforced built-in ranges (loopback, private, @@ -269,9 +276,14 @@ Key settings in more detail: 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` — timeout for origin requests -- `upstream_max_response_size` — max origin response size -- `downstream_timeout` — client response timeout +- `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; default `52428800` (50 MiB). It also limits the image data pixa + decodes +- `downstream_timeout` — time allowed for answering one client request, as a + duration; default `60s`. The upstream fetch counts toward it, so keep it + longer than `upstream_fetch_timeout` - `signing_key` — HMAC secret for URL signatures - `cache_max_bytes` — disk cache size limit in bytes; `0` disables the disk cache entirely; omitted defaults to 75% of the free space on diff --git a/TODO.md b/TODO.md index 8f07fa8..6234c46 100644 --- a/TODO.md +++ b/TODO.md @@ -57,6 +57,15 @@ exhaustion that is sooner, never negative; an allowlisted host's URL that has an `exp` follows it too; `immutable` stays, as freshness now ends at the expiry; documented in `README.md`. +- 2026-09-28 add the four settings `README.md` documented but pixa did not + have, which aborted startup as unknown keys (closes #61): + `access_control_allow_origin` (default `*`, the CORS origin), + `upstream_fetch_timeout` (default `30s`), `upstream_max_response_size` + (default 50 MiB) and `downstream_timeout` (default `60s`, both the + server's write timeout and the per-request timeout); each has a + `PIXA_` variable; durations are positive Go duration strings, sizes a + whole number of bytes; an invalid value aborts startup naming the key + and the value; documented in `config.example.yml` and `README.md`. - 2026-09-28 cache stats report real numbers (closes #56): `Cache.Stats` counts the cached source images and processed variants (`source_content` plus `variant_content`) and takes their size from `Cache.UsageBytes`, @@ -306,7 +315,6 @@ exhaustion - X-Request-ID propagation - P2: auto format selection (format=auto based on Accept header) - P2: configuration - - add all configuration options from README - YAML config file support - P2: operational - optional Sentry error reporting diff --git a/config.example.yml b/config.example.yml index f92b752..686b45d 100644 --- a/config.example.yml +++ b/config.example.yml @@ -8,6 +8,10 @@ # this file's env: section is set while the file loads, so it overrides # both the environment the process was started with and this file's own # key. +# +# Durations are Go duration strings such as 30s or 2m and must be +# positive; a bare number has no unit and aborts startup. Sizes are a +# whole number of bytes. # Server settings port: 8080 @@ -67,6 +71,21 @@ allow_http: false # Maximum concurrent connections per upstream host (default: 20) upstream_connections_per_host: 20 +# Time allowed for one fetch from an upstream host (default: 30s) +upstream_fetch_timeout: 30s + +# Largest upstream response accepted, in bytes (default: 52428800, 50 MiB) +upstream_max_response_size: 52428800 + +# Time allowed for answering one client request, the upstream fetch +# included, so keep it longer than upstream_fetch_timeout (default: 60s) +downstream_timeout: 60s + +# The origin a browser lets read pixa's responses, sent as the CORS +# Access-Control-Allow-Origin header: "*" (the default) is any site; +# otherwise one origin, scheme and host only, such as https://example.com +access_control_allow_origin: "*" + # Maximum disk cache size in bytes. Explicit values are used exactly as # given; 0 disables the disk cache entirely (every request fetches and # processes uncached). When omitted, the default is 75% of the free diff --git a/internal/config/config.go b/internal/config/config.go index 3d948be..20bdf83 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -13,6 +13,7 @@ import ( "sort" "strconv" "strings" + "time" "git.eeqj.de/sneak/smartconfig" "go.uber.org/fx" @@ -24,6 +25,10 @@ const ( DefaultPort = 8080 DefaultStateDir = "/var/lib/pixa" DefaultUpstreamConnectionsPerHost = 20 + DefaultAccessControlAllowOrigin = "*" + DefaultUpstreamFetchTimeout = 30 * time.Second + DefaultUpstreamMaxResponseSize = 50 << 20 // 50 MiB + DefaultDownstreamTimeout = 60 * time.Second ) // Configuration key names. @@ -44,6 +49,10 @@ const ( keyCacheMaxBytes = "cache_max_bytes" keyBlockedNetworks = "blocked_networks" keyTrustedProxies = "trusted_proxies" + keyAccessControlAllowOrigin = "access_control_allow_origin" + keyUpstreamFetchTimeout = "upstream_fetch_timeout" + keyUpstreamMaxResponseSize = "upstream_max_response_size" + keyDownstreamTimeout = "downstream_timeout" ) // placeholderSigningKey is the dummy signing_key shipped in @@ -86,6 +95,10 @@ var ( errNotBareHostname = errors.New( "must be a bare hostname without scheme, path, or whitespace") errNoHostnameLabels = errors.New("contains no hostname labels") + errNotADuration = errors.New("not a duration such as 30s or 2m") + errMustBePositive = errors.New("must be positive") + errNotAnOrigin = errors.New( + `not "*" or an origin such as https://example.com`) ) // Params defines dependencies for Config. @@ -112,6 +125,23 @@ type Config struct { AllowHTTP bool // Allow non-TLS upstream (testing only) UpstreamConnectionsPerHost int // Max concurrent connections per upstream host + // UpstreamFetchTimeout is the time allowed for one fetch from an + // upstream host. UpstreamMaxResponseSize is the largest upstream + // response accepted, in bytes, and also the image processor's input + // limit. + UpstreamFetchTimeout time.Duration + UpstreamMaxResponseSize int64 + + // AccessControlAllowOrigin is the origin the CORS middleware allows + // to read responses: "*" for any, or one origin such as + // https://example.com. + AccessControlAllowOrigin string + + // DownstreamTimeout bounds how long answering one client request may + // take. It is both the HTTP server's write timeout and the deadline + // of the per-request timeout middleware. + DownstreamTimeout time.Duration + // 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. @@ -240,6 +270,14 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { AllowHTTP: loader.boolVal(keyAllowHTTP, false), UpstreamConnectionsPerHost: loader.intVal( keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), + UpstreamFetchTimeout: loader.durationVal( + keyUpstreamFetchTimeout, DefaultUpstreamFetchTimeout), + UpstreamMaxResponseSize: loader.int64Val( + keyUpstreamMaxResponseSize, DefaultUpstreamMaxResponseSize), + AccessControlAllowOrigin: loader.stringVal( + keyAccessControlAllowOrigin, DefaultAccessControlAllowOrigin), + DownstreamTimeout: loader.durationVal( + keyDownstreamTimeout, DefaultDownstreamTimeout), CacheMaxBytes: loader.int64Val(keyCacheMaxBytes, 0), BlockedNetworks: blockedNetworks, TrustedProxies: trustedProxies, @@ -354,7 +392,8 @@ func isKnownConfigKey(key string) bool { case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, - keyTrustedProxies, "env": + keyTrustedProxies, keyAccessControlAllowOrigin, keyUpstreamFetchTimeout, + keyUpstreamMaxResponseSize, keyDownstreamTimeout, "env": return true } @@ -382,6 +421,10 @@ func envVarNames() map[string]string { keyCacheMaxBytes: "PIXA_CACHE_MAX_BYTES", keyBlockedNetworks: "PIXA_BLOCKED_NETWORKS", keyTrustedProxies: "PIXA_TRUSTED_PROXIES", + keyAccessControlAllowOrigin: "PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", + keyUpstreamFetchTimeout: "PIXA_UPSTREAM_FETCH_TIMEOUT", + keyUpstreamMaxResponseSize: "PIXA_UPSTREAM_MAX_RESPONSE_SIZE", + keyDownstreamTimeout: "PIXA_DOWNSTREAM_TIMEOUT", } } @@ -535,6 +578,12 @@ func (c *Config) validate() error { settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) } + if c.UpstreamMaxResponseSize <= 0 { + return fmt.Errorf("%s: value %d %w", + settingName(keyUpstreamMaxResponseSize), c.UpstreamMaxResponseSize, + errMustBePositive) + } + for _, host := range c.AllowlistHosts { err := validateAllowlistHost(host) if err != nil { @@ -556,6 +605,25 @@ func (c *Config) validate() error { errMustBeSetTogether) } + return c.validateAccessControlAllowOrigin() +} + +// validateAccessControlAllowOrigin checks that access_control_allow_origin +// is "*" or one origin, a scheme and host with nothing after them, as +// browsers send it in the Origin header. Anything else, such as a bare +// hostname or a trailing slash, would match no request. +func (c *Config) validateAccessControlAllowOrigin() error { + origin := c.AccessControlAllowOrigin + if origin == "*" { + return nil + } + + parsed, err := url.Parse(origin) + if err != nil || parsed.Host == "" || parsed.Scheme+"://"+parsed.Host != origin { + return fmt.Errorf("%s: value %q is %w", + settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) + } + return nil } @@ -673,6 +741,19 @@ func (l *strictLoader) int64Val(key string, defaultVal int64) int64 { return val } +func (l *strictLoader) durationVal(key string, defaultVal time.Duration) time.Duration { + if l.err != nil { + return 0 + } + + val, err := getDuration(l.sc, key, defaultVal) + if err != nil { + l.err = err + } + + return val +} + func (l *strictLoader) boolVal(key string, defaultVal bool) bool { if l.err != nil { return false @@ -794,6 +875,42 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro } } +// getDuration returns the duration value for key, or defaultVal if the +// key is omitted. A present value must be a positive Go duration string +// such as "30s" or "2m", read with time.ParseDuration; a bare number has +// no unit and is an error, as is an explicit null. +func getDuration( + sc *smartconfig.Config, key string, defaultVal time.Duration, +) (time.Duration, error) { + raw, ok := lookupValue(sc, key) + if !ok { + return defaultVal, nil + } + + if raw == nil { + return 0, errNullConfigValue(key) + } + + str, ok := raw.(string) + if !ok { + return 0, fmt.Errorf("config key %q: value %v (%T) is %w", + key, raw, raw, errNotADuration) + } + + parsed, err := time.ParseDuration(strings.TrimSpace(str)) + if err != nil { + return 0, fmt.Errorf("%s: value %q is %w", + settingName(key), str, errNotADuration) + } + + if parsed <= 0 { + return 0, fmt.Errorf("%s: value %q %w", + settingName(key), str, errMustBePositive) + } + + return parsed, nil +} + // getBool returns the boolean value for key, or defaultVal if the key // is omitted. A present value that is not a boolean (or a ParseBool-able // string), or is explicitly null, is an error; numbers are not accepted diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 09f3ffc..3da580d 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -106,6 +106,8 @@ func (s *Handlers) initImageService() error { // Create the fetcher config fetcherCfg := httpfetcher.DefaultConfig() fetcherCfg.AllowHTTP = s.config.AllowHTTP + fetcherCfg.Timeout = s.config.UpstreamFetchTimeout + fetcherCfg.MaxResponseSize = s.config.UpstreamMaxResponseSize if s.config.UpstreamConnectionsPerHost > 0 { fetcherCfg.MaxConnectionsPerHost = s.config.UpstreamConnectionsPerHost diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 30feaa5..492ba87 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -172,7 +172,7 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler { // CORS returns a CORS middleware. func (s *Middleware) CORS() func(http.Handler) http.Handler { return cors.Handler(cors.Options{ - AllowedOrigins: []string{"*"}, + AllowedOrigins: []string{s.config.AccessControlAllowOrigin}, AllowedMethods: []string{"GET", "HEAD", "OPTIONS"}, AllowedHeaders: []string{"Accept", "Authorization", "Content-Type"}, ExposedHeaders: []string{"Link"}, diff --git a/internal/server/http.go b/internal/server/http.go index bdd1238..13162e0 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -14,7 +14,6 @@ const ( // short, so a slowloris client dribbling headers is dropped well // before it ties up a connection for the whole ReadTimeout window. HTTPReadHeaderTimeout = 10 * time.Second - HTTPWriteTimeout = 60 * time.Second // HTTPIdleTimeout bounds how long an idle keep-alive connection is // held open, so idle connections cannot accumulate without limit on a // service targeting high concurrency. @@ -30,7 +29,7 @@ func (s *Server) newHTTPServer() *http.Server { Addr: fmt.Sprintf(":%d", s.config.Port), ReadTimeout: HTTPReadTimeout, ReadHeaderTimeout: HTTPReadHeaderTimeout, - WriteTimeout: HTTPWriteTimeout, + WriteTimeout: s.config.DownstreamTimeout, IdleTimeout: HTTPIdleTimeout, MaxHeaderBytes: HTTPMaxHeaderBytes, Handler: s, diff --git a/internal/server/routes.go b/internal/server/routes.go index 0bb2748..4623b75 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -33,7 +33,7 @@ func (s *Server) SetupRoutes() { } s.router.Use(s.mw.CORS()) - s.router.Use(middleware.Timeout(HTTPWriteTimeout)) + s.router.Use(middleware.Timeout(s.config.DownstreamTimeout)) if s.sentryEnabled { sentryHandler := sentryhttp.New(sentryhttp.Options{ -- 2.54.0 From 807de97a9d450e808034a1cf461050832c0d555b Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 19:51:14 +0000 Subject: [PATCH 03/12] Test that an origin with a * inside aborts startup (closes #61) Failing cases for access_control_allow_origin set to https://*, https://*.example.com and https://*example.com. The CORS middleware reads a * inside an origin as a pattern, so these would let other sites read responses; each must abort startup naming the key and the value. Model: opus-5-5 --- .../config/config_validation_internal_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index cb6f9ca..cb8efb2 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -792,6 +792,23 @@ func invalidSizeAndOriginCases() []abortCase { keyAccessControlAllowOrigin, "https://example.com/", }, }, + { + name: "access_control_allow_origin host *", + yaml: signingKeyLine + "access_control_allow_origin: https://*\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, "https://*"}, + }, + { + name: "access_control_allow_origin host *.example.com", + yaml: signingKeyLine + + "access_control_allow_origin: https://*.example.com\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, "https://*.example.com"}, + }, + { + name: "access_control_allow_origin host *example.com", + yaml: signingKeyLine + + "access_control_allow_origin: https://*example.com\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, "https://*example.com"}, + }, { name: "access_control_allow_origin empty", yaml: signingKeyLine + "access_control_allow_origin: \"\"\n", -- 2.54.0 From 6e81852b49cb6c50f3abab6ac9314f21b23f8c65 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 19:51:14 +0000 Subject: [PATCH 04/12] Refuse an origin with a * inside at startup (closes #61) access_control_allow_origin accepted values such as https://* or https://*example.com, which the CORS middleware reads as a pattern that lets other sites read responses. Any value that contains * and is not exactly * now aborts startup naming the key, its variable and the value. Model: opus-5-5 --- internal/config/config.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 20bdf83..4f63900 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -611,7 +611,10 @@ func (c *Config) validate() error { // validateAccessControlAllowOrigin checks that access_control_allow_origin // is "*" or one origin, a scheme and host with nothing after them, as // browsers send it in the Origin header. Anything else, such as a bare -// hostname or a trailing slash, would match no request. +// hostname or a trailing slash, would match no request. An origin with a +// "*" inside, such as https://*example.com, is refused because the CORS +// middleware reads that "*" as a pattern and would let other sites read +// responses. func (c *Config) validateAccessControlAllowOrigin() error { origin := c.AccessControlAllowOrigin if origin == "*" { @@ -619,7 +622,8 @@ func (c *Config) validateAccessControlAllowOrigin() error { } parsed, err := url.Parse(origin) - if err != nil || parsed.Host == "" || parsed.Scheme+"://"+parsed.Host != origin { + if err != nil || parsed.Host == "" || parsed.Scheme+"://"+parsed.Host != origin || + strings.Contains(origin, "*") { return fmt.Errorf("%s: value %q is %w", settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) } -- 2.54.0 From 4370ccb0833841bbfeb442bd34e01ca31259ed88 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 19:51:14 +0000 Subject: [PATCH 05/12] Reword the comment on TestCORSAnswersWithConfiguredOrigin (closes #61) The comment was not a readable sentence; it is now one plain sentence saying what the test checks. Model: opus-5-5 --- internal/middleware/middleware_internal_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 3421784..5dac1be 100644 --- a/internal/middleware/middleware_internal_test.go +++ b/internal/middleware/middleware_internal_test.go @@ -10,8 +10,8 @@ import ( ) // TestCORSAnswersWithConfiguredOrigin checks that the CORS middleware -// uses access_control_allow_origin: "*" lets any origin read responses, -// and a single origin lets that origin read them and no other. +// answers with access_control_allow_origin, where "*" lets any origin read +// responses and a single origin lets only that origin read them. func TestCORSAnswersWithConfiguredOrigin(t *testing.T) { t.Parallel() -- 2.54.0 From ff50a59bae370be7d022d5e21524ba8e577daf95 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 19:57:20 +0000 Subject: [PATCH 06/12] Test origin host and port checks and a 1 GiB size maximum (closes #61) Failing cases: access_control_allow_origin with two hosts, an empty port, no host, port 0, port 99999 or a non-ASCII host name, and upstream_max_response_size above 1 GiB, up to the largest 64-bit integer, where the image processor's limit wraps negative. Each must abort startup naming the key and the value. The bad origins are now one list so the table fits the linter's function length limit; the size test uses 1 GiB to show the maximum itself is accepted, and IPv4 and IPv6 origins are shown to be accepted. Model: opus-5-5 --- .../config/config_validation_internal_test.go | 113 +++++++++--------- 1 file changed, 56 insertions(+), 57 deletions(-) diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index cb8efb2..fdcd17b 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -633,14 +633,14 @@ func TestOmittedOriginTimeoutsAndSizeUseDefaults(t *testing.T) { // TestExplicitOriginTimeoutsAndSizeAreUsed checks that valid values for // the CORS origin, the two timeouts and the response size limit are used -// as given. +// as given. The size is the largest accepted, 1 GiB. func TestExplicitOriginTimeoutsAndSizeAreUsed(t *testing.T) { t.Parallel() c, err := configFromYAML(t, signingKeyLine+` access_control_allow_origin: https://app.example.com upstream_fetch_timeout: 10s -upstream_max_response_size: 1048576 +upstream_max_response_size: 1073741824 downstream_timeout: 2m `) if err != nil { @@ -656,8 +656,8 @@ downstream_timeout: 2m t.Errorf("UpstreamFetchTimeout = %v, want 10s", c.UpstreamFetchTimeout) } - if c.UpstreamMaxResponseSize != 1048576 { - t.Errorf("UpstreamMaxResponseSize = %d, want 1048576", + if c.UpstreamMaxResponseSize != 1073741824 { + t.Errorf("UpstreamMaxResponseSize = %d, want 1073741824", c.UpstreamMaxResponseSize) } @@ -667,11 +667,14 @@ downstream_timeout: 2m } // TestOriginWithPortOrAnyOriginIsAccepted checks the other accepted forms -// of access_control_allow_origin: "*", and an origin with a port. +// of access_control_allow_origin: "*", an origin with a port, and origins +// whose host is an IPv4 or IPv6 address. func TestOriginWithPortOrAnyOriginIsAccepted(t *testing.T) { t.Parallel() - for _, origin := range []string{"*", "http://localhost:3000"} { + for _, origin := range []string{ + "*", "http://localhost:3000", "http://192.0.2.1", "http://[2001:db8::1]:8080", + } { c, err := configFromYAML(t, signingKeyLine+ "access_control_allow_origin: \""+origin+"\"\n") if err != nil { @@ -739,11 +742,44 @@ func invalidTimeoutCases() []abortCase { } // invalidSizeAndOriginCases are configs where upstream_max_response_size -// is not a positive whole number of bytes, or access_control_allow_origin -// is neither "*" nor an origin; each must abort startup naming the key -// and the value. +// is not a whole number of bytes from 1 to 1 GiB, or +// access_control_allow_origin is neither "*" nor an origin; each must +// abort startup naming the key and the value. func invalidSizeAndOriginCases() []abortCase { - return []abortCase{ + badOrigins := []string{ + "", // empty + "example.com", // no scheme + "https://example.com/images", // a path + "https://example.com/", // a trailing slash + // The CORS middleware reads a * inside an origin as a pattern + // that lets other sites read responses. + "https://*", + "https://*.example.com", + "https://*example.com", + "https://a.com,b.com", // two hosts + "https://example.com:", // an empty port + "https://:8443", // no host + "https://example.com:0", // a port below 1 + "https://example.com:99999", // a port above 65535 + "https://exämple.com", // a host name that is not ASCII + } + + cases := make([]abortCase, 0, len(badOrigins)) + for _, origin := range badOrigins { + cases = append(cases, abortCase{ + name: "access_control_allow_origin " + origin, + yaml: signingKeyLine + + "access_control_allow_origin: \"" + origin + "\"\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, origin}, + }) + } + + return append(cases, []abortCase{ + { + name: "access_control_allow_origin null", + yaml: signingKeyLine + "access_control_allow_origin: null\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, nullValueText}, + }, { name: "upstream_max_response_size with a unit", yaml: signingKeyLine + "upstream_max_response_size: 50MB\n", @@ -770,56 +806,19 @@ func invalidSizeAndOriginCases() []abortCase { wantErrSubstrings: []string{keyUpstreamMaxResponseSize, nullValueText}, }, { - name: "access_control_allow_origin bare hostname", - yaml: signingKeyLine + "access_control_allow_origin: example.com\n", + name: "upstream_max_response_size above 1 GiB", + yaml: signingKeyLine + "upstream_max_response_size: 1073741825\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "1073741825"}, + }, + { + name: "upstream_max_response_size largest 64-bit integer", + yaml: signingKeyLine + + "upstream_max_response_size: 9223372036854775807\n", wantErrSubstrings: []string{ - keyAccessControlAllowOrigin, "example.com", + keyUpstreamMaxResponseSize, "9223372036854775807", }, }, - { - name: "access_control_allow_origin with a path", - yaml: signingKeyLine + - "access_control_allow_origin: https://example.com/images\n", - wantErrSubstrings: []string{ - keyAccessControlAllowOrigin, "https://example.com/images", - }, - }, - { - name: "access_control_allow_origin trailing slash", - yaml: signingKeyLine + - "access_control_allow_origin: https://example.com/\n", - wantErrSubstrings: []string{ - keyAccessControlAllowOrigin, "https://example.com/", - }, - }, - { - name: "access_control_allow_origin host *", - yaml: signingKeyLine + "access_control_allow_origin: https://*\n", - wantErrSubstrings: []string{keyAccessControlAllowOrigin, "https://*"}, - }, - { - name: "access_control_allow_origin host *.example.com", - yaml: signingKeyLine + - "access_control_allow_origin: https://*.example.com\n", - wantErrSubstrings: []string{keyAccessControlAllowOrigin, "https://*.example.com"}, - }, - { - name: "access_control_allow_origin host *example.com", - yaml: signingKeyLine + - "access_control_allow_origin: https://*example.com\n", - wantErrSubstrings: []string{keyAccessControlAllowOrigin, "https://*example.com"}, - }, - { - name: "access_control_allow_origin empty", - yaml: signingKeyLine + "access_control_allow_origin: \"\"\n", - wantErrSubstrings: []string{keyAccessControlAllowOrigin}, - }, - { - name: "access_control_allow_origin null", - yaml: signingKeyLine + "access_control_allow_origin: null\n", - wantErrSubstrings: []string{keyAccessControlAllowOrigin, nullValueText}, - }, - } + }...) } // TestInvalidOriginTimeoutOrSizeAbortsStartup verifies the -- 2.54.0 From 841571ea083c465efa4038a40d007b9d6c3bb9e8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 20:03:35 +0000 Subject: [PATCH 07/12] Check the origin's host and port; cap the size at 1 GiB (closes #61) access_control_allow_origin now needs a host name (ASCII letters, digits, hyphens, dots) or an IP address, and a port, when given, from 1 to 65535, read with net/url, net/netip and strconv. Two hosts, an empty port, no host, a bad port or a non-ASCII host name abort startup, as a * inside the value already did. upstream_max_response_size above 1 GiB aborts startup: the image processor reads one byte past the limit, which wrapped negative at the largest 64-bit value, and a response is held whole in memory. config.example.yml states the maximum. Model: opus-5-5 --- TODO.md | 8 ++-- config.example.yml | 3 +- internal/config/config.go | 81 ++++++++++++++++++++++++++++++++------- 3 files changed, 74 insertions(+), 18 deletions(-) diff --git a/TODO.md b/TODO.md index 6234c46..848a58e 100644 --- a/TODO.md +++ b/TODO.md @@ -63,9 +63,11 @@ exhaustion `upstream_fetch_timeout` (default `30s`), `upstream_max_response_size` (default 50 MiB) and `downstream_timeout` (default `60s`, both the server's write timeout and the per-request timeout); each has a - `PIXA_` variable; durations are positive Go duration strings, sizes a - whole number of bytes; an invalid value aborts startup naming the key - and the value; documented in `config.example.yml` and `README.md`. + `PIXA_` variable; durations are positive Go duration strings, the size a + whole number of bytes up to 1 GiB, the origin `*` or one scheme, host + name or IP address and optional port; an invalid value aborts startup + naming the key and the value; documented in `config.example.yml` and + `README.md`. - 2026-09-28 cache stats report real numbers (closes #56): `Cache.Stats` counts the cached source images and processed variants (`source_content` plus `variant_content`) and takes their size from `Cache.UsageBytes`, diff --git a/config.example.yml b/config.example.yml index 686b45d..e08a8d1 100644 --- a/config.example.yml +++ b/config.example.yml @@ -74,7 +74,8 @@ upstream_connections_per_host: 20 # Time allowed for one fetch from an upstream host (default: 30s) upstream_fetch_timeout: 30s -# Largest upstream response accepted, in bytes (default: 52428800, 50 MiB) +# Largest upstream response accepted, in bytes, at most 1073741824 +# (1 GiB) (default: 52428800, 50 MiB) upstream_max_response_size: 52428800 # Time allowed for answering one client request, the upstream fetch diff --git a/internal/config/config.go b/internal/config/config.go index 4f63900..1c17369 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -78,6 +78,7 @@ var ( errEmptyEntry = errors.New("contains an empty entry") errNotAValidURL = errors.New("not a valid URL") errPortOutOfRange = errors.New("outside the valid port range") + errSizeOutOfRange = errors.New("outside the accepted range") errTooFewConnections = errors.New("must be at least 1") errValueTooShort = errors.New("value too short") errPlaceholderKey = errors.New( @@ -578,10 +579,9 @@ func (c *Config) validate() error { settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) } - if c.UpstreamMaxResponseSize <= 0 { - return fmt.Errorf("%s: value %d %w", - settingName(keyUpstreamMaxResponseSize), c.UpstreamMaxResponseSize, - errMustBePositive) + err = c.validateUpstreamMaxResponseSize() + if err != nil { + return err } for _, host := range c.AllowlistHosts { @@ -608,29 +608,82 @@ func (c *Config) validate() error { return c.validateAccessControlAllowOrigin() } +// 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 +// must not overflow. +func (c *Config) validateUpstreamMaxResponseSize() error { + const maxUpstreamMaxResponseSize = 1 << 30 // 1 GiB + if c.UpstreamMaxResponseSize < 1 || + c.UpstreamMaxResponseSize > maxUpstreamMaxResponseSize { + return fmt.Errorf("%s: value %d is %w 1-%d", + settingName(keyUpstreamMaxResponseSize), c.UpstreamMaxResponseSize, + errSizeOutOfRange, maxUpstreamMaxResponseSize) + } + + return nil +} + // validateAccessControlAllowOrigin checks that access_control_allow_origin -// is "*" or one origin, a scheme and host with nothing after them, as -// browsers send it in the Origin header. Anything else, such as a bare -// hostname or a trailing slash, would match no request. An origin with a -// "*" inside, such as https://*example.com, is refused because the CORS -// middleware reads that "*" as a pattern and would let other sites read -// responses. +// is "*" or one origin as browsers send it in the Origin header: a scheme, +// a host name or IP address, and optionally a port from 1 to 65535, with +// nothing after them. Anything else, such as a bare hostname or a trailing +// slash, would match no request. A "*" is not allowed in a host name, so +// https://*example.com is refused; the CORS middleware would read that +// "*" as a pattern and let other sites read responses. func (c *Config) validateAccessControlAllowOrigin() error { origin := c.AccessControlAllowOrigin if origin == "*" { return nil } + errOrigin := fmt.Errorf("%s: value %q is %w", + settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) + + // url.Parse accepts an empty port, as in https://example.com:, and + // then reports no port, so a trailing colon is refused here. parsed, err := url.Parse(origin) - if err != nil || parsed.Host == "" || parsed.Scheme+"://"+parsed.Host != origin || - strings.Contains(origin, "*") { - return fmt.Errorf("%s: value %q is %w", - settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) + if err != nil || parsed.Scheme+"://"+parsed.Host != origin || + strings.HasSuffix(origin, ":") { + return errOrigin + } + + host := parsed.Hostname() + + _, err = netip.ParseAddr(host) + if err != nil && !isHostName(host) { + return errOrigin + } + + // A port is a 16-bit number, and 0 is not a port a browser sends. + if parsed.Port() != "" { + port, err := strconv.ParseUint(parsed.Port(), 10, 16) + if err != nil || port == 0 { + return errOrigin + } } return nil } +// isHostName reports whether host is not empty and has only ASCII +// letters, digits, hyphens and dots. +func isHostName(host string) bool { + if host == "" { + return false + } + + for _, r := range host { + isLetterOrDigit := 'a' <= r && r <= 'z' || 'A' <= r && r <= 'Z' || + '0' <= r && r <= '9' + if !isLetterOrDigit && r != '-' && r != '.' { + return false + } + } + + return true +} + // validateAllowlistHost checks that an allowlist_hosts entry is a bare // hostname, optionally with a leading dot for suffix matching. URLs, // paths, and whitespace indicate a misconfigured entry. An entry with -- 2.54.0 From 2e06be40c915d3c59ac3f15dab46e19cf637ccfb Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 20:24:12 +0000 Subject: [PATCH 08/12] Test that origins a browser does not send abort startup (closes #61) Adds the review's examples to invalidSizeAndOriginCases: a scheme's default port, a port with a leading zero, hosts a browser reads as an IPv4 address or refuses, an IPv6 address not in its shortest form, plus a scheme other than http or https and a host name in upper case. Model: opus-5-5 --- internal/config/config_validation_internal_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index fdcd17b..c275e38 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -762,6 +762,16 @@ func invalidSizeAndOriginCases() []abortCase { "https://example.com:0", // a port below 1 "https://example.com:99999", // a port above 65535 "https://exämple.com", // a host name that is not ASCII + "https://example.com:443", // the default port for https + "http://example.com:80", // the default port for http + "https://example.com:08080", // a port with a leading zero + "https://01.2.3.4", // an IPv4 address with a leading zero + "https://10.0.0", // an IPv4 address with three parts + "https://192.168.1.256", // an IPv4 address part above 255 + "https://example.123", // a host name whose last part is a number + "https://[0:0:0:0:0:0:0:1]", // an IPv6 address not in its shortest form + "file://example.com", // a scheme other than http or https + "https://Example.com", // a host name in upper case } cases := make([]abortCase, 0, len(badOrigins)) -- 2.54.0 From e836e88b8fb343be5444483e6e89c859f0566417 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 20:25:42 +0000 Subject: [PATCH 09/12] Accept only an origin written exactly as a browser sends it (closes #61) access_control_allow_origin is now "*", or http or https, a host that is an IP address as net/netip writes it (IPv6 in brackets) or a lowercase host name whose last part contains a letter, and an optional port 1-65535 with no leading zero that is not the scheme's default. The value must equal the origin rebuilt from those parts; anything else aborts startup naming the key, its variable and the value. README.md and config.example.yml say an origin is scheme, host and optional port, exactly as the browser sends it. Model: opus-5-5 --- README.md | 5 +-- TODO.md | 6 ++-- config.example.yml | 3 +- internal/config/config.go | 70 +++++++++++++++++++-------------------- 4 files changed, 43 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index af1b0e1..3c0df38 100644 --- a/README.md +++ b/README.md @@ -249,8 +249,9 @@ Key settings in more detail: - `access_control_allow_origin` — the origin a browser lets read pixa's responses, sent as the CORS `Access-Control-Allow-Origin` header: `*`, the - default, is any site; otherwise one origin, scheme and host only, such as - `https://example.com`. Anything else aborts startup + default, is any site; otherwise one origin: scheme, host and optional port, + exactly as the browser sends it, such as `https://example.com`. Anything + else aborts startup - `allowlist_hosts` — list of allowed upstream hosts - `blocked_networks` — list of CIDR ranges to refuse for SSRF protection, added to the always-enforced built-in ranges (loopback, private, diff --git a/TODO.md b/TODO.md index 848a58e..b619ac9 100644 --- a/TODO.md +++ b/TODO.md @@ -65,9 +65,9 @@ exhaustion server's write timeout and the per-request timeout); each has a `PIXA_` variable; durations are positive Go duration strings, the size a whole number of bytes up to 1 GiB, the origin `*` or one scheme, host - name or IP address and optional port; an invalid value aborts startup - naming the key and the value; documented in `config.example.yml` and - `README.md`. + and optional port, exactly as the browser sends it; an invalid value + aborts startup naming the key and the value; documented in + `config.example.yml` and `README.md`. - 2026-09-28 cache stats report real numbers (closes #56): `Cache.Stats` counts the cached source images and processed variants (`source_content` plus `variant_content`) and takes their size from `Cache.UsageBytes`, diff --git a/config.example.yml b/config.example.yml index e08a8d1..562e3a5 100644 --- a/config.example.yml +++ b/config.example.yml @@ -84,7 +84,8 @@ downstream_timeout: 60s # The origin a browser lets read pixa's responses, sent as the CORS # Access-Control-Allow-Origin header: "*" (the default) is any site; -# otherwise one origin, scheme and host only, such as https://example.com +# otherwise one origin: scheme, host and optional port, exactly as the +# browser sends it, such as https://example.com access_control_allow_origin: "*" # Maximum disk cache size in bytes. Explicit values are used exactly as diff --git a/internal/config/config.go b/internal/config/config.go index 1c17369..5c9504f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -624,13 +624,9 @@ func (c *Config) validateUpstreamMaxResponseSize() error { return nil } -// validateAccessControlAllowOrigin checks that access_control_allow_origin -// is "*" or one origin as browsers send it in the Origin header: a scheme, -// a host name or IP address, and optionally a port from 1 to 65535, with -// nothing after them. Anything else, such as a bare hostname or a trailing -// slash, would match no request. A "*" is not allowed in a host name, so -// https://*example.com is refused; the CORS middleware would read that -// "*" as a pattern and let other sites read responses. +// validateAccessControlAllowOrigin accepts "*" or an origin exactly as a browser +// sends it: http or https, an IP address as netip writes it or a lowercase name +// with a letter in its last part, and an optional port 1-65535, not the default. func (c *Config) validateAccessControlAllowOrigin() error { origin := c.AccessControlAllowOrigin if origin == "*" { @@ -640,50 +636,54 @@ func (c *Config) validateAccessControlAllowOrigin() error { errOrigin := fmt.Errorf("%s: value %q is %w", settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) - // url.Parse accepts an empty port, as in https://example.com:, and - // then reports no port, so a trailing colon is refused here. parsed, err := url.Parse(origin) - if err != nil || parsed.Scheme+"://"+parsed.Host != origin || - strings.HasSuffix(origin, ":") { + if err != nil { return errOrigin } + defaultPort := map[string]string{"http": "80", "https": "443"}[parsed.Scheme] + if defaultPort == "" { + return errOrigin + } + + const letters = "abcdefghijklmnopqrstuvwxyz" + host := parsed.Hostname() + lastPart := host[strings.LastIndex(host, ".")+1:] - _, err = netip.ParseAddr(host) - if err != nil && !isHostName(host) { + addr, err := netip.ParseAddr(host) + + switch { + case err == nil && addr.Is6(): + host = "[" + addr.String() + "]" + case err == nil: + host = addr.String() + case strings.Trim(host, letters+"0123456789-.") != "": // a character other than these + return errOrigin + case !strings.ContainsAny(lastPart, letters): return errOrigin } - // A port is a 16-bit number, and 0 is not a port a browser sends. - if parsed.Port() != "" { - port, err := strconv.ParseUint(parsed.Port(), 10, 16) - if err != nil || port == 0 { + // The value must be exactly the origin rebuilt from its parts. + rebuilt := parsed.Scheme + "://" + host + + port := parsed.Port() + if port != "" { + _, err := strconv.ParseUint(port, 10, 16) + if err != nil || port[0] == '0' || port == defaultPort { return errOrigin } + + rebuilt += ":" + port + } + + if rebuilt != origin { + return errOrigin } return nil } -// isHostName reports whether host is not empty and has only ASCII -// letters, digits, hyphens and dots. -func isHostName(host string) bool { - if host == "" { - return false - } - - for _, r := range host { - isLetterOrDigit := 'a' <= r && r <= 'z' || 'A' <= r && r <= 'Z' || - '0' <= r && r <= '9' - if !isLetterOrDigit && r != '-' && r != '.' { - return false - } - } - - return true -} - // validateAllowlistHost checks that an allowlist_hosts entry is a bare // hostname, optionally with a leading dot for suffix matching. URLs, // paths, and whitespace indicate a misconfigured entry. An entry with -- 2.54.0 From 014de9c87fde6ce7f66071d3f14b0be60613ad71 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 20:43:49 +0000 Subject: [PATCH 10/12] Test that an origin with an upper case scheme aborts startup (closes #61) Model: opus-5-5 --- internal/config/config_validation_internal_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index c275e38..c269a38 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -771,6 +771,7 @@ func invalidSizeAndOriginCases() []abortCase { "https://example.123", // a host name whose last part is a number "https://[0:0:0:0:0:0:0:1]", // an IPv6 address not in its shortest form "file://example.com", // a scheme other than http or https + "HTTPS://example.com", // a scheme in upper case "https://Example.com", // a host name in upper case } -- 2.54.0 From d2dfd6e58a44a77c77fef3f89e897224056b9d47 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 20:43:49 +0000 Subject: [PATCH 11/12] State the origin rule in README.md and config.example.yml (closes #61) Both now say what access_control_allow_origin accepts, instead of "exactly as the browser sends it", and that another scheme, such as a browser extension's, aborts startup. Model: opus-5-5 --- README.md | 9 ++++++--- TODO.md | 4 ++-- config.example.yml | 8 ++++++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 3c0df38..7a3a013 100644 --- a/README.md +++ b/README.md @@ -249,9 +249,12 @@ Key settings in more detail: - `access_control_allow_origin` — the origin a browser lets read pixa's responses, sent as the CORS `Access-Control-Allow-Origin` header: `*`, the - default, is any site; otherwise one origin: scheme, host and optional port, - exactly as the browser sends it, such as `https://example.com`. Anything - else aborts startup + default, is any site; otherwise one `http` or `https` origin such as + `https://example.com`, whose host is a lowercase host name (letters, + digits, hyphens and dots, with a letter in its last part) or an IP address + (IPv6 in brackets, in its shortest form), with an optional port 1-65535 + that has no leading zero and is not the scheme's default. Any other value, + including another scheme such as a browser extension's, aborts startup - `allowlist_hosts` — list of allowed upstream hosts - `blocked_networks` — list of CIDR ranges to refuse for SSRF protection, added to the always-enforced built-in ranges (loopback, private, diff --git a/TODO.md b/TODO.md index b619ac9..e6ad439 100644 --- a/TODO.md +++ b/TODO.md @@ -64,8 +64,8 @@ exhaustion (default 50 MiB) and `downstream_timeout` (default `60s`, both the server's write timeout and the per-request timeout); each has a `PIXA_` variable; durations are positive Go duration strings, the size a - whole number of bytes up to 1 GiB, the origin `*` or one scheme, host - and optional port, exactly as the browser sends it; an invalid value + whole number of bytes up to 1 GiB, the origin `*` or one `http` or + `https` origin as `README.md` describes it; an invalid value aborts startup naming the key and the value; documented in `config.example.yml` and `README.md`. - 2026-09-28 cache stats report real numbers (closes #56): `Cache.Stats` diff --git a/config.example.yml b/config.example.yml index 562e3a5..27c2422 100644 --- a/config.example.yml +++ b/config.example.yml @@ -84,8 +84,12 @@ downstream_timeout: 60s # The origin a browser lets read pixa's responses, sent as the CORS # Access-Control-Allow-Origin header: "*" (the default) is any site; -# otherwise one origin: scheme, host and optional port, exactly as the -# browser sends it, such as https://example.com +# otherwise one http or https origin such as https://example.com, whose +# host is a lowercase host name (letters, digits, hyphens and dots, with a +# letter in its last part) or an IP address (IPv6 in brackets, in its +# shortest form), with an optional port 1-65535 that has no leading zero +# and is not the scheme's default. Any other value, including another +# scheme such as a browser extension's, aborts startup. access_control_allow_origin: "*" # Maximum disk cache size in bytes. Explicit values are used exactly as -- 2.54.0 From 2c4dfe929af3d6293807e27a9676f5e5800c61df Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 06:10:58 +0000 Subject: [PATCH 12/12] Give the login rate limit tests the default request timeout (closes #61) newTestServer left downstream_timeout at zero, so every request in those tests ran with an already expired per-request timeout. It now uses config.DefaultDownstreamTimeout, as a real config would. Approved by the owner on the issue. Model: opus-5-5 --- internal/server/login_rate_limit_internal_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/server/login_rate_limit_internal_test.go b/internal/server/login_rate_limit_internal_test.go index 13195c0..c37d86e 100644 --- a/internal/server/login_rate_limit_internal_test.go +++ b/internal/server/login_rate_limit_internal_test.go @@ -51,11 +51,12 @@ func newTestServer(t *testing.T) *Server { 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")}, + Debug: true, + SigningKey: testSigningKey, + StateDir: stateDir, + DBURL: "file:" + filepath.Join(stateDir, "state.sqlite3"), + TrustedProxies: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, + DownstreamTimeout: config.DefaultDownstreamTimeout, } lc := fxtest.NewLifecycle(t) -- 2.54.0