diff --git a/README.md b/README.md index 4102390..7a3a013 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,14 @@ 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 `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, @@ -269,9 +280,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..e6ad439 100644 --- a/TODO.md +++ b/TODO.md @@ -57,6 +57,17 @@ 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, the size a + 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` counts the cached source images and processed variants (`source_content` plus `variant_content`) and takes their size from `Cache.UsageBytes`, @@ -306,7 +317,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..27c2422 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,27 @@ 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, at most 1073741824 +# (1 GiB) (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 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 # 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..5c9504f 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 @@ -69,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( @@ -86,6 +96,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 +126,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 +271,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 +393,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 +422,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 +579,11 @@ func (c *Config) validate() error { settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) } + err = c.validateUpstreamMaxResponseSize() + if err != nil { + return err + } + for _, host := range c.AllowlistHosts { err := validateAllowlistHost(host) if err != nil { @@ -556,6 +605,82 @@ func (c *Config) validate() error { errMustBeSetTogether) } + 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 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 == "*" { + return nil + } + + errOrigin := fmt.Errorf("%s: value %q is %w", + settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) + + parsed, err := url.Parse(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:] + + 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 + } + + // 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 } @@ -673,6 +798,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 +932,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/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index c12d27d..c269a38 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,244 @@ 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. 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: 1073741824 +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 != 1073741824 { + t.Errorf("UpstreamMaxResponseSize = %d, want 1073741824", + 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: "*", 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", "http://192.0.2.1", "http://[2001:db8::1]:8080", + } { + 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 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 { + 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 + "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 scheme in upper case + "https://Example.com", // a host name in upper case + } + + 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", + 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: "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{ + keyUpstreamMaxResponseSize, "9223372036854775807", + }, + }, + }...) +} + +// 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/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/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 9992a60..5dac1be 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 +// 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() + + 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.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/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}, } 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) 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{