diff --git a/.gitea/workflows/check.yml b/.gitea/workflows/check.yml index ee73864..decade9 100644 --- a/.gitea/workflows/check.yml +++ b/.gitea/workflows/check.yml @@ -7,3 +7,4 @@ jobs: # actions/checkout v4.2.2, 2026-02-22 - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 - run: script/cibuild + - run: script/docker-smoke diff --git a/Dockerfile b/Dockerfile index 3eb5782..77510a0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,5 @@ # Lint stage +# Same image as Dockerfile.lint: change both pins together. # golangci/golangci-lint:v2.12.2-alpine, 2026-08-07 FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60 AS lint @@ -13,6 +14,9 @@ RUN go mod download # Copy source code COPY . . +# Tells script/lint it is inside a container, so it runs the linter. +ENV container=docker + # Run formatting check and linter RUN make fmt-check RUN make lint @@ -57,22 +61,30 @@ RUN apk add --no-cache \ vips \ libheif \ ca-certificates \ - tzdata + tzdata \ + su-exec # Copy binary from builder COPY --from=builder /pixad /usr/local/bin/pixad +COPY deploy/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh # Create non-root user, config directory, and data directory RUN adduser -D -H -s /sbin/nologin pixad && \ mkdir -p /var/lib/pixa /etc/pixa && \ chown pixad:pixad /var/lib/pixa -# Copy default config (edit signing_key before use) -COPY config.example.yml /etc/pixa/config.yml - -USER pixad +# No USER: the entrypoint must start as root to give a bind-mounted +# /var/lib/pixa to pixad; it then runs the server as pixad. WORKDIR /var/lib/pixa EXPOSE 8080 -ENTRYPOINT ["/usr/local/bin/pixad", "--config", "/etc/pixa/config.yml"] +# Shell form so the probe follows PORT; a port set only in a mounted +# config file is not seen here. +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ + CMD wget --spider -q "http://localhost:${PORT:-8080}/.well-known/healthcheck.json" || exit 1 + +# Settings come from PORT and the PIXA_ environment variables; only +# PIXA_SIGNING_KEY is required. A config file mounted at +# /etc/pixa/config.yml is optional and is read when present. +ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"] diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..a07d8ca --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,34 @@ +# Dockerfile.lint: the container script/lint builds to run golangci-lint, +# which is never installed on the host. Pinned to the same image as the +# Dockerfile lint stage: change both pins together, or the two run +# different linter versions. +# +# golangci/golangci-lint:v2.12.2-alpine, 2026-08-07 +FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60 + +# pixa is CGO/libvips: the type-aware linters compile every package, so +# this image needs the same C libraries the build does. +RUN apk add --no-cache build-base vips-dev libheif-dev pkgconfig + +WORKDIR /src + +# Modules first for layer caching; go.mod/go.sum settle this layer's +# result, so it may safely be reused between runs. +COPY go.mod go.sum ./ +RUN go mod download + +COPY . . + +# Tells script/lint it is inside a container, so it runs the linter. +ENV container=docker + +# script/lint passes a different CACHEBUST on every run, and BuildKit +# keys every RUN after this ARG on its value, so the lint step always +# runs instead of returning a cached success that linted nothing. +# +# Go's and golangci-lint's caches (/root/.cache, hundreds of MB) go on a +# tmpfs that is discarded after the step. Written into the layer, they +# would pile up as build cache on every run, since no later run, with +# its new CACHEBUST, can reuse that layer. +ARG CACHEBUST +RUN --mount=type=tmpfs,target=/root/.cache script/lint diff --git a/Makefile b/Makefile index 2fbafdc..708d5e5 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: bootstrap setup check lint test fmt fmt-check build clean docker docker-versioned docker-test devserver devserver-stop hooks +.PHONY: bootstrap setup check lint test fmt fmt-check build clean docker docker-smoke docker-versioned docker-test devserver devserver-stop hooks VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") LDFLAGS := -X main.Version=$(VERSION) @@ -10,7 +10,7 @@ ifdef HAS_PKGCONFIG NIX_RUN_PREFIX = NIX_RUN_SUFFIX = else - NIX_RUN_PREFIX = nix-shell -p pkg-config vips libheif golangci-lint git --run ' + NIX_RUN_PREFIX = nix-shell -p pkg-config vips libheif git --run ' NIX_RUN_SUFFIX = ' endif @@ -54,6 +54,11 @@ clean: docker: @script/docker +# Build the image, start it, and wait for its healthcheck (needs Docker; +# not part of check) +docker-smoke: + @script/docker-smoke + # Build Docker image tagged pixad:$(VERSION) and pixad:latest docker-versioned: docker build --build-arg VERSION=$(VERSION) -t pixad:$(VERSION) -t pixad:latest . diff --git a/README.md b/README.md index 8e36dc0..4765703 100644 --- a/README.md +++ b/README.md @@ -15,14 +15,52 @@ git clone https://git.eeqj.de/sneak/pixa.git cd pixa make build -# run with a config file -./bin/pixad --config config.example.yml +# run with a config file: copy the example and set a real signing key +# (the example placeholder is refused at startup), e.g. with +# openssl rand -base64 32 +cp config.example.yml config.yml +$EDITOR config.yml # replace the signing_key placeholder +./bin/pixad --config config.yml # or build and run via Docker make docker -docker run -p 8080:8080 pixad:latest +docker run -p 8080:8080 -e PIXA_SIGNING_KEY="$(openssl rand -base64 32)" pixa:latest ``` +A container takes its settings from environment variables (see +Configuration below for the list). Only `PIXA_SIGNING_KEY` is required; if +it is unset the container exits at startup naming the variable. Everything +else has a built-in default. A config file mounted at `/etc/pixa/config.yml` +is optional: it is read when present, and an environment variable wins over +the same setting in it. + +## Running under upaas + +What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs: + +- **Port:** pixa listens on container port `8080`. +- **Volume:** container path `/var/lib/pixa`, where pixa keeps its + database and cache. upaas bind-mounts the host path it is given and + does not create it, so the host directory must exist before the first + deploy. +- **Environment variables:** + - `PIXA_SIGNING_KEY` (required): secret for signed and encrypted URLs + and login, 32+ characters, for example from + `openssl rand -base64 32` + - `PIXA_ALLOWLIST_HOSTS`: upstream hosts served without a signature, + comma-separated + - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it; + default 75% of free space + - the rest are in the table under Configuration below +- **Health check:** the image's `HEALTHCHECK` requests + `/.well-known/healthcheck.json`. upaas reads the container's health 60 + seconds after a deploy and marks the deploy failed unless it is + `healthy`. The probe uses the port from `PORT` (default `8080`), so a + port changed only in a mounted config file is not seen by it: change + the port with `PORT`. +- **First run:** create the host directory. It may be owned by root: the + container gives it to its `pixad` user when it starts. + ## Rationale Image-heavy web applications need a fast, caching reverse proxy that @@ -56,9 +94,38 @@ In-process caching of request-to-output mappings targets 1-5k r/s. Images are only fetched from origins using TLS with valid certificates. +A request whose query string cannot be decoded, or gives any parameter more +than once, is refused with 400. + - ``: one of `orig`, `png`, `jpeg`, `webp` - ``: `orig` or `x` (e.g. `800x600`) +The login form (`POST /`) is limited to 5 attempts per minute per client +address, counting an IPv6 client by its /64; an attempt over the limit is +refused with 429 and a `Retry-After` header. Behind a reverse proxy the client +address comes from `X-Forwarded-For` only when the proxy's address is in +`trusted_proxies`; otherwise all users behind the proxy are counted as one +client. With the default `trusted_proxies` (the RFC 1918 ranges), a client +with a private address can choose the address it is counted by through its own +`X-Forwarded-For`, whether it connects directly or through the proxy, because +its own address is trusted too. Setting `trusted_proxies` to the proxy's own +address closes this. + +### Image Metadata + +pixa decodes and re-encodes every image it serves, and removes all metadata from +the output: EXIF (GPS position, camera make, model and serial number, capture +time, embedded thumbnail), XMP, IPTC and the ICC colour profile. This cannot be +turned off. + +- The `orig` format means the source's own format, not the source's bytes: an + `orig` image is re-encoded and stripped like any other. +- An image with an EXIF orientation is turned upright first, so it displays the + same without the tag; a requested size applies to the upright image. +- An image with an ICC profile is converted to sRGB first, since clients show an + image with no profile as sRGB. Colours outside sRGB, such as the most + saturated ones in a Display P3 photo, are clipped. + ### Source Hosts Source hosts may be allowlisted in the configuration. Non-allowlisted @@ -68,14 +135,14 @@ hosts require an HMAC-SHA256 signature. Signatures use HMAC-SHA256 and include an expiration timestamp to prevent replay attacks. Signatures are **exact match only**: every -component (host, path, query, dimensions, format, expiration) must -match exactly what was signed. No suffix matching, wildcard matching, -or partial matching is supported. +component (host, path, query, dimensions, format, expiration, quality, +fit) must match exactly what was signed. No suffix matching, wildcard +matching, or partial matching is supported. **Signed data format** (colon-separated): ``` -HMAC-SHA256(secret, "host:path:query:width:height:format:expiration") +HMAC-SHA256(secret, "host:path:query:width:height:format:expiration:quality:fit") ``` Where: @@ -86,19 +153,30 @@ Where: - `width` — requested width in pixels, `0` for original - `height` — requested height in pixels, `0` for original - `format` — output format (jpeg, png, webp, avif, gif, orig) -- `expiration` — Unix timestamp when signature expires +- `expiration` — the URL's `exp` query parameter, the Unix timestamp when + the signature expires; a request whose `exp` is not a whole number, an + empty `exp=` included, is refused with 400 +- `quality` — the URL's `q` query parameter, a whole number from 1 to 100, + or `85` when the URL has no `q`; a request whose `q` is anything else is + refused with 400 +- `fit` — the URL's `fit` query parameter (cover, contain, fill, inside, + outside), or `cover` when the URL has no `fit`; a request whose `fit` is + anything else, an empty `fit=` included, is refused with 400 -**Example:** resize -`https://cdn.example.com/photos/cat.jpg` to 800x600 WebP with -expiration 1704067200: +**Example:** resize `https://cdn.example.com/photos/cat.jpg` to 800x600 +WebP with expiration 1704067200, default quality and fit: 1. Build input: - `cdn.example.com:/photos/cat.jpg::800:600:webp:1704067200` + `cdn.example.com:/photos/cat.jpg::800:600:webp:1704067200:85:cover` 2. Compute HMAC-SHA256 with your secret key 3. Base64URL-encode the result 4. URL: `/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=&exp=1704067200` +For the same image at quality 40 with fit `contain`, the input ends in +`:40:contain` and the URL is +`/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=&exp=1704067200&q=40&fit=contain`. + **Allowlist patterns:** - **Exact match**: `cdn.example.com` — matches only that host @@ -107,10 +185,60 @@ expiration 1704067200: ### Configuration -Configured via YAML file (`--config`). Key settings: +Every setting can be given as an environment variable, in a YAML config +file (`--config`), or both. A variable present in the environment wins over +the file, even when it is empty, and the file wins over the built-in +default. The one exception is a variable named in the file's `env:` section: +it is set while the file loads, so it overrides both the environment the +process was started with and the file's own key. A variable's value is +parsed as the same text in the file would be. The three lists take +comma-separated entries, with the spaces around each trimmed; an empty +variable is an empty list. A value that does not parse or is invalid aborts +startup, naming the variable. A variable whose name starts with `PIXA_` but +is not in the table below, such as a misspelled one or `PIXA_PORT`, aborts +startup naming it, as an unknown config key does. The one other accepted +name is `PIXA_CONFIG_PATH`, the config file's path (like `--config`). The +variables set by the file's `env:` section are checked the same way. + +| Variable | Config key | Meaning | +| ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- | +| `PIXA_SIGNING_KEY` | `signing_key` | Required: secret for signed and encrypted URLs and login, 32+ characters | +| `PORT` | `port` | Port to listen on; default `8080` | +| `PIXA_STATE_DIR` | `state_dir` | Directory for the database and the disk cache; default `/var/lib/pixa` | +| `PIXA_DB_URL` | `db_url` | SQLite database URL; default `state.sqlite3` in the state directory | +| `PIXA_CACHE_MAX_BYTES` | `cache_max_bytes` | Disk cache limit in bytes; `0` disables it; default 75% of free space | +| `PIXA_ALLOWLIST_HOSTS` | `allowlist_hosts` | Upstream hosts served without a signature | +| `PIXA_BLOCKED_NETWORKS` | `blocked_networks` | CIDR ranges never fetched from, on top of the built-in ones | +| `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_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 | +| `PIXA_DEBUG` | `debug` | Debug logging and plain-HTTP local development; default `false` | +| `PIXA_MAINTENANCE_MODE` | `maintenance_mode` | Maintenance flag reported by the health check; default `false` | + +Key settings in more detail: - `access_control_allow_origin` — CORS origin - `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, + link-local, CGNAT, benchmark, NAT64, and the like); an invalid CIDR + aborts startup +- `trusted_proxies` — list of CIDR ranges of the reverse proxies in front + of pixa. `X-Forwarded-For` is believed only when the direct peer falls + inside one of these ranges; the logged and login-recorded client + address is then the rightmost forwarded entry that is not itself a + trusted proxy. Otherwise the direct peer address is used and the header + is ignored, so a client connecting directly from an address outside + these ranges cannot spoof its address. + An omitted key defaults to the RFC 1918 private ranges (`10.0.0.0/8`, + `172.16.0.0/12`, `192.168.0.0/16`), since pixa is deployed behind a + proxy on a private network; an explicitly empty list (`[]`) trusts no + one, and an explicit list replaces the default. An invalid CIDR aborts + startup. Set this to your proxy's address range if it is not already + covered by the defaults - `upstream_fetch_timeout` — timeout for origin requests - `upstream_max_response_size` — max origin response size - `downstream_timeout` — client response timeout @@ -144,11 +272,13 @@ them. We provide: (bootstrap, then install-precommit) - `script/projectname` — output the project name ("pixa") - `script/test` — run the test suite -- `script/lint` — run golangci-lint +- `script/lint` — run golangci-lint, always in a container (builds + `Dockerfile.lint` when run outside one) - `script/fmt` — format all code (writes) - `script/fmt-check` — check formatting (read-only) - `script/check` — run test, lint, and fmt-check - `script/docker` — build the Docker image tagged via `script/projectname` +- `script/docker-smoke` — build the image, start it, wait for it to be healthy - `script/cibuild` — CI entrypoint: `docker build .` (the Dockerfile runs the checks, so a green build implies a green repo) - `script/precommit` — pre-commit checks (`go mod tidy` guard, then diff --git a/TODO.md b/TODO.md index 9421a23..56d0745 100644 --- a/TODO.md +++ b/TODO.md @@ -1,28 +1,169 @@ # Workflow -* branch (from `main`) +* branch per issue from `next` * do the work in Next Step * move Next Step to the top of Completed Steps * move the top item of Future Steps into Next Step * commit (`TODO.md` changes in the same commit as the work) -* merge to `main` if the branch is not protected, otherwise open a PR +* open a PR based on `next` +* an independent reviewer who did not write the change gates it +* the manager squash-merges the PR into `next` once review passes +* `next` stays green and mergeable to `main` at any time; only the owner + merges `next` into `main`, via the single milestone PR * push # Status -pre-1.0. No git tags exist. Recent work extracted the internal/magic, +pre-1.0. No git tags exist. The `1.0.0` milestone is in progress; work +lands on `next`, and `main` receives only the milestone PR that the +owner merges. `next` is at the canonical `golangci-lint` v2.12.2 config +and is green. Recent work extracted the internal/magic, internal/allowlist, internal/httpfetcher, and internal/signature -packages. The gosec findings from the 2026-07-06 survey are resolved -and `make check` is green on main. The disk cache is now size-bounded -with LRU eviction (`cache_max_bytes`), closing the unbounded disk -growth DoS vector. +packages. The gosec findings from the 2026-07-06 survey are resolved. +The disk cache is now size-bounded with LRU eviction +(`cache_max_bytes`), closing the unbounded disk growth DoS vector. # Next Step -P1: implement blocked networks configuration to extend SSRF protection +P1: rate limit global concurrent upstream fetches to prevent resource +exhaustion # Completed Steps +- 2026-09-28 strip metadata from processed images (closes #82): every output is + exported with govips' `StripMetadata`, so it carries no EXIF, XMP, IPTC or ICC + profile; the image is first turned upright with `AutoRotate` (before sizes are + worked out) and, when it has an ICC profile, converted to sRGB; the `orig` + format is re-encoded and stripped like any other, as pixa never serves the + source bytes; there is no setting to keep metadata; documented in `README.md`. +- 2026-09-28 rate limit the login form (closes #66): `POST /` is limited to 5 + attempts per minute per client address, and an attempt over the limit is + refused with 429 and a `Retry-After` header; the address is the one + `internal/clientip` resolves through `trusted_proxies`, an IPv6 client is + counted by its /64, and an IPv4-mapped address as the IPv4 address it + carries; the limit is a `RateLimit` middleware in `internal/middleware` on + `github.com/go-chi/httprate`, which the image routes can reuse; the library + keeps counts for the current and the previous minute only; documented in + `README.md`. +- 2026-09-28 refuse an unparseable `exp` on `/v1/image/` and log swallowed + cache errors (closes #72): an `exp` in the URL that is not a whole + number, an empty `exp=` included, is a 400 naming `exp` and the value, + instead of being ignored and answered with 401 as if the URL had no + `exp`; only an `exp` missing from the URL is unchanged; `README.md` says + so where it documents `exp`. A failed variant `.meta` write, source + metadata JSON write, `Stats` count query, stats counter update, negative + cache write or expired negative cache delete is now logged at `warn` + with the path or key and the error, and stays non-fatal. +- 2026-09-28 refuse an empty `fit` on `/v1/image/` (closes #139): a + `fit` in the URL with an empty value (`fit=`) is a 400 naming `fit`, + instead of being served as `cover` and verified against a signature + made for `cover`; only a `fit` missing from the URL is still `cover`; + any other value still goes through the existing fit-mode check; + `README.md` says so where it documents `fit`. +- 2026-09-28 refuse an invalid `q` on `/v1/image/` (closes #134): a `q` + that is not a whole number from 1 to 100, an empty `q` included, is a + 400 naming `q` and the value, instead of being served at the default + 85; the route reads `q` with the generator's quality check + (`parseFormInt` with `minQuality` and `maxQuality`); only a `q` missing + from the URL is still 85; a query string that cannot be decoded, such + as `q=80%`, is a 400 showing it; any query parameter given more than + once (`q`, `fit`, `sig`, `exp` alike) is a 400 naming it, so none is + read from its first value only; `README.md` states the range and both + query-string rules. +- 2026-09-28 unknown `PIXA_` environment variables abort startup (closes + #133): a variable whose name starts with `PIXA_` but is neither a + setting's variable nor `PIXA_CONFIG_PATH` aborts startup naming it, as + an unknown config key does, and `PIXA_PORT` is named with a pointer to + `PORT`; the check runs after the config file loads, so the variables + the file's `env:` section sets are checked too; documented in + `README.md`. +- 2026-09-28 start on a fresh upaas volume (closes #129): the image + starts as root only to give `/var/lib/pixa` to `pixad` when `pixad` + does not own it (`deploy/docker-entrypoint.sh`), then runs the server + as `pixad` through `su-exec`, so a root-owned host directory + bind-mounted there no longer stops the container at startup; + `README.md` gains a "Running under upaas" section. +- 2026-09-28 run all linting in Docker via `Dockerfile.lint` + + `script/lint` (closes #104): `make lint` calls `script/lint`, the only + way the linter is run; inside a container (both Dockerfiles set + `container=docker`) it runs `golangci-lint`, anywhere else it builds the + hash-pinned `Dockerfile.lint`, whose last step runs `script/lint` again; + the `Dockerfile` lint stage runs `make lint`; no host or nix-shell + `golangci-lint` path remains (`script/bootstrap` installs no linter); + a per-run `CACHEBUST` build-arg keeps the lint step from being served + from cache, and a tmpfs mount on that step keeps Go's and + golangci-lint's caches out of its layer, so a run leaves no large build + cache behind; `golangci-lint config verify` stays out, as it fetches its + schema over an unpinned live HTTPS call +- 2026-09-28 every setting as an environment variable (closes #128, also + covers #99): each config key can be set by `PIXA_` plus the key in upper + case (`.` written as `_`), and the port by `PORT`; a variable present in + the environment, even empty, wins over the config file, which wins over + the default; the typed getters read the variable first, so every existing + check applies to it and a bad value aborts startup naming the variable; + lists are comma-separated, and an empty variable (or `""` in the file) is + an empty list; the Docker image no longer bakes in `config.docker.yml` or + passes `--config`, and its `HEALTHCHECK` probes `PORT` (default `8080`); + the config file is looked for under `/etc/pixa` and `~/.config/pixa` + instead of the daemon name `pixad`; documented in `README.md` and + `config.example.yml`. +- 2026-09-28 quality and fit in the URL signature (closes #60): the signed + data is now `host:path:query:width:height:format:expiration:quality:fit`, + using `85` and `cover` when the URL has no `q` or `fit`, so one signed + URL can no longer be replayed across other quality and fit values to + create unauthorized cache entries and transcodes; the known-answer + vectors in `internal/signature/golden_test.go` and the README signature + specification describe the new format. +- 2026-09-28 Docker image healthcheck (closes #111): a `HEALTHCHECK` in + the runtime stage probing `/.well-known/healthcheck.json` with busybox + `wget`; `script/docker-smoke` (`make docker-smoke`) builds the image, + starts it with a throwaway `PIXA_SIGNING_KEY`, and passes only once + Docker reports it healthy within 30 seconds, removing the container on + exit; the Gitea workflow runs it after `script/cibuild`. +- 2026-09-21 trusted-proxy client IP resolution (closes #94): a + `trusted_proxies` config key taking a list of CIDRs, parsed by the same + `net/netip` list parser as `blocked_networks` (an invalid entry aborts + startup naming the key and value; an omitted key defaults to the RFC 1918 + private ranges, an explicitly empty list trusts no one, and an explicit + list replaces the default); a new + `internal/clientip` package resolves the client address by honoring + `X-Forwarded-For` only when the direct peer is a trusted proxy, walking + the chain right-to-left to the rightmost non-proxy entry, so a client + connecting directly cannot spoof its address; the resolved address is + stored in the request context by a new middleware and used by the + request-logging middleware and the login-attempt logs in place of the + raw peer address; documented in `README.md` and `config.example.yml`. +- 2026-09-21 blocked networks configuration extending SSRF protection: a + `blocked_networks` config key taking a list of CIDRs (parsed with + `net/netip`, an invalid entry aborts startup naming the key and value), + added to the built-in blocklist rather than replacing it; the built-in + ranges extended to CGNAT `100.64.0.0/10`, IETF protocol assignments + `192.0.0.0/24`, benchmark `198.18.0.0/15`, and NAT64 `64:ff9b::/96` + (IPv4-mapped forms covered); enforcement stays in the dial-time + re-resolution so the DNS-rebinding window remains closed; documented in + `README.md` and `config.example.yml`. +- 2026-09-21 validate dimensions and fit mode on the encrypted-URL + route and the token generator (closes #62): `imgcache.ValidateDimension` + alone holds the `MaxDimension` bound and is used by the path parser, by + the new `ValidateImageRequest` (which also applies `ValidateFitMode`) + and by the generator; both the `/v1/image/` and `/v1/e/` routes call + `ValidateImageRequest`, so an over-limit size or an unknown fit mode is a + 400 rather than an out-of-memory or a 500 from the processor; the URL + generator answers 400 naming the field for a `width` or `height` that is + not a number or fails the shared check, a `quality` that is not a number + from 1 to 100, a `ttl` that is not a number from 0 to the largest number + of seconds the expiry calculation can hold, or an unknown `fit`; an empty + `quality` is 85 and + an empty `ttl` never expires; the form's width and height inputs stop at + 8192 +- 2026-09-21 http.Server hardening (closes #92): added + `HTTPReadHeaderTimeout` (10s, bounds the slowloris header dribble) and + `HTTPIdleTimeout` (120s, bounds keep-alive reuse) alongside the + existing timeouts and wired them onto the server; added a `LimitBody` + middleware capping the two form POST bodies (`POST /`, `POST /generate`) + at `MaxFormBytes` (1 MiB) and returning 413, applied ahead of the CSRF + middleware so an oversized body is refused as 413 rather than being read + as a missing CSRF token (403); left `WriteTimeout` at 60s unchanged - 2026-08-07 update golangci-lint to v2.12.2 with the canonical `.golangci.yml` (v2 schema, `default: all` minus six disabled linters, `lll` 88, tests included): bumped the pinned @@ -116,12 +257,9 @@ P1: implement blocked networks configuration to extend SSRF protection # Future Steps -- P1: rate limit global concurrent upstream fetches to prevent - resource exhaustion -- P1: strip EXIF and other metadata from processed images (privacy) - P2: security - referer blacklist - - per-IP rate limiting + - per-IP rate limiting on the image routes - per-origin rate limiting - P2: HTTP response handling - Last-Modified headers @@ -130,7 +268,6 @@ P1: implement blocked networks configuration to extend SSRF protection - P2: auto format selection (format=auto based on Accept header) - P2: configuration - add all configuration options from README - - environment variable overrides - YAML config file support - P2: operational - optional Sentry error reporting diff --git a/config.example.yml b/config.example.yml index 122189e..8df6a35 100644 --- a/config.example.yml +++ b/config.example.yml @@ -1,4 +1,13 @@ # Pixa Example Configuration +# +# Every key can also be set by an environment variable, which wins over +# this file: PIXA_ plus the key in upper case, with "." written as "_" +# (state_dir is PIXA_STATE_DIR, metrics.username is +# PIXA_METRICS_USERNAME). The one exception is port, which is set by +# PORT. In a variable, a list is comma-separated. A variable named in +# 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. # Server settings port: 8080 @@ -22,6 +31,30 @@ allowlist_hosts: - github.com - user-images.githubusercontent.com +# Additional CIDR ranges to refuse when fetching upstream, extending the +# SSRF protection. These are added to the always-enforced built-in ranges +# (loopback, RFC 1918 private, link-local, CGNAT, benchmark, NAT64, and +# similar), never replacing them. Each entry must be a valid CIDR in IPv4 +# or IPv6 form; an invalid entry aborts startup. +# blocked_networks: +# - 100.64.0.0/10 +# - 2001:db8::/32 + +# CIDR ranges of the reverse proxies in front of pixa. X-Forwarded-For +# is believed only when the direct peer is inside one of these ranges; +# the client address in the access log and login records is then the +# rightmost forwarded entry that is not itself a trusted proxy. A client +# connecting directly (peer outside these ranges) cannot spoof its +# address: the header is ignored and the peer address is used. When +# omitted, this defaults to the RFC 1918 private ranges (10.0.0.0/8, +# 172.16.0.0/12, 192.168.0.0/16), since pixa is deployed behind a proxy on +# a private network. An explicitly empty list ([]) trusts no one; an +# explicit list replaces the default. An invalid CIDR aborts startup. +# Uncomment to override the defaults with your proxy's address range. +# trusted_proxies: +# - 10.0.0.0/8 +# - 2001:db8::/32 + # Allow HTTP upstream (only for testing, always use HTTPS in production) allow_http: false diff --git a/deploy/docker-entrypoint.sh b/deploy/docker-entrypoint.sh new file mode 100755 index 0000000..691ff2f --- /dev/null +++ b/deploy/docker-entrypoint.sh @@ -0,0 +1,15 @@ +#!/bin/sh +# deploy/docker-entrypoint.sh: the Docker image's ENTRYPOINT. It runs as +# root only to give /var/lib/pixa to pixad: a host directory +# bind-mounted there keeps its host owner, often root, and pixad could +# not write to it. The server itself always runs as pixad. +set -eu + +main() { + if [ "$(stat -c %U /var/lib/pixa)" != pixad ]; then + chown pixad:pixad /var/lib/pixa + fi + exec su-exec pixad /usr/local/bin/pixad "$@" +} + +main "$@" diff --git a/go.mod b/go.mod index ce9b477..f5c4c98 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,8 @@ require ( github.com/getsentry/sentry-go v0.40.0 github.com/go-chi/chi/v5 v5.2.3 github.com/go-chi/cors v1.2.2 + github.com/go-chi/httprate v0.16.0 + github.com/gorilla/csrf v1.7.3 github.com/gorilla/securecookie v1.1.2 github.com/prometheus/client_golang v1.23.2 github.com/slok/go-http-metrics v0.13.0 @@ -90,6 +92,7 @@ require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect + github.com/klauspost/cpuid/v2 v2.2.10 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect github.com/mattn/go-colorable v0.1.13 // indirect @@ -112,6 +115,7 @@ require ( github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.0 // indirect github.com/x448/float16 v0.8.4 // indirect + github.com/zeebo/xxh3 v1.0.2 // indirect go.etcd.io/etcd/api/v3 v3.6.2 // indirect go.etcd.io/etcd/client/pkg/v3 v3.6.2 // indirect go.etcd.io/etcd/client/v3 v3.6.2 // indirect diff --git a/go.sum b/go.sum index 002a2b4..f9a4f3d 100644 --- a/go.sum +++ b/go.sum @@ -114,6 +114,8 @@ github.com/go-chi/chi/v5 v5.2.3 h1:WQIt9uxdsAbgIYgid+BpYc+liqQZGMHRaUwp0JUcvdE= github.com/go-chi/chi/v5 v5.2.3/go.mod h1:L2yAIGWB3H+phAw1NxKwWM+7eUH/lU8pOMm5hHcoops= github.com/go-chi/cors v1.2.2 h1:Jmey33TE+b+rB7fT8MUy1u0I4L+NARQlK6LhzKPSyQE= github.com/go-chi/cors v1.2.2/go.mod h1:sSbTewc+6wYHBBCW7ytsFSn836hqM7JxpglAy2Vzc58= +github.com/go-chi/httprate v0.16.0 h1:8V5DH9j6pSK6UQoBsTpvMyFxycqaKEIToyPKzHJjUa8= +github.com/go-chi/httprate v0.16.0/go.mod h1:A8lo+qRhk+s9LiuP5saS7XCGDXRXMcrueq0NfIuCa/I= github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA= github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og= github.com/go-jose/go-jose/v4 v4.0.5 h1:M6T8+mKZl/+fNNuFHvGIzDz7BTLQPIounk/b9dw3AaE= @@ -175,6 +177,8 @@ github.com/googleapis/enterprise-certificate-proxy v0.3.6 h1:GW/XbdyBFQ8Qe+YAmFU github.com/googleapis/enterprise-certificate-proxy v0.3.6/go.mod h1:MkHOF77EYAE7qfSuSS9PU6g4Nt4e11cnsDUowfwewLA= github.com/googleapis/gax-go/v2 v2.14.2 h1:eBLnkZ9635krYIPD+ag1USrOAI0Nr0QYF3+/3GqO0k0= github.com/googleapis/gax-go/v2 v2.14.2/go.mod h1:ON64QhlJkhVtSqp4v1uaK92VyZ2gmvDQsweuyLV+8+w= +github.com/gorilla/csrf v1.7.3 h1:BHWt6FTLZAb2HtWT5KDBf6qgpZzvtbp9QWDRKZMXJC0= +github.com/gorilla/csrf v1.7.3/go.mod h1:F1Fj3KG23WYHE6gozCmBAezKookxbIvUJT+121wTuLk= github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA= github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo= github.com/grpc-ecosystem/grpc-gateway/v2 v2.26.3 h1:5ZPtiqj0JL5oKWmcsq4VMaAW5ukBEgSGXEN89zeH1Jo= @@ -249,6 +253,8 @@ github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo= github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ= +github.com/klauspost/cpuid/v2 v2.2.10 h1:tBs3QSyvjDyFTq3uoc/9xFpCuOsJQFNPiAhYdw2skhE= +github.com/klauspost/cpuid/v2 v2.2.10/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0= github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= @@ -394,6 +400,10 @@ github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcY github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= +github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= +github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= +github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0= +github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA= go.etcd.io/etcd/api/v3 v3.6.2 h1:25aCkIMjUmiiOtnBIp6PhNj4KdcURuBak0hU2P1fgRc= go.etcd.io/etcd/api/v3 v3.6.2/go.mod h1:eFhhvfR8Px1P6SEuLT600v+vrhdDTdcfMzmnxVXXSbk= go.etcd.io/etcd/client/pkg/v3 v3.6.2 h1:zw+HRghi/G8fKpgKdOcEKpnBTE4OO39T6MegA0RopVU= diff --git a/internal/clientip/clientip.go b/internal/clientip/clientip.go new file mode 100644 index 0000000..49c2ea7 --- /dev/null +++ b/internal/clientip/clientip.go @@ -0,0 +1,119 @@ +// Package clientip resolves the real client IP address of an HTTP request +// when pixa runs behind a reverse proxy. Forwarding headers are believed +// only when the immediate peer is a configured trusted proxy, so an +// untrusted client cannot spoof its address by sending the header. +package clientip + +import ( + "context" + "net" + "net/netip" + "slices" + "strings" +) + +// ForwardedForHeader is the request header carrying the proxy chain. It is +// honored only when the immediate peer is a trusted proxy. +const ForwardedForHeader = "X-Forwarded-For" + +// Resolver determines the client IP of a request against a fixed set of +// trusted proxy networks. +type Resolver struct { + trusted []netip.Prefix +} + +// NewResolver returns a Resolver that trusts forwarding headers only from +// peers inside the given CIDR ranges. A nil or empty list trusts no one, +// so the peer address is always used. +func NewResolver(trusted []netip.Prefix) *Resolver { + return &Resolver{trusted: trusted} +} + +// Resolve returns the client IP for a request whose direct peer is +// remoteAddr (a "host:port" string as in http.Request.RemoteAddr) and +// whose X-Forwarded-For header lines are forwardedFor (as returned by +// http.Header.Values). When the peer is not a trusted proxy, the peer +// address is returned and the header is ignored entirely. When the peer is +// trusted, the header is walked right to left and the first address that is +// not itself a trusted proxy is returned; this is the client the outermost +// trusted proxy observed, and entries an untrusted client may have prepended +// sit to its left and are never reached. +func (r *Resolver) Resolve(remoteAddr string, forwardedFor []string) string { + peer := hostOnly(remoteAddr) + + peerAddr, err := netip.ParseAddr(peer) + if err != nil || !r.isTrusted(peerAddr) { + return peer + } + + for _, hop := range slices.Backward(forwardedForChain(forwardedFor)) { + hopAddr, err := netip.ParseAddr(hop) + if err != nil || r.isTrusted(hopAddr) { + continue + } + + return hopAddr.String() + } + + return peerAddr.String() +} + +// isTrusted reports whether addr falls inside one of the trusted proxy +// ranges. Addresses are unmapped first so an IPv4-mapped IPv6 form matches +// an IPv4 range, matching the fetcher's blocklist comparison. +func (r *Resolver) isTrusted(addr netip.Addr) bool { + if !addr.IsValid() { + return false + } + + unmapped := addr.Unmap() + + return slices.ContainsFunc(r.trusted, func(prefix netip.Prefix) bool { + return prefix.Contains(unmapped) + }) +} + +// hostOnly strips the port from a "host:port" address. A value without a +// port (already a bare host) is returned unchanged. +func hostOnly(remoteAddr string) string { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + return remoteAddr + } + + return host +} + +// forwardedForChain flattens the comma-separated entries of every +// X-Forwarded-For header line into a single ordered, trimmed list. +func forwardedForChain(values []string) []string { + var chain []string + + for _, value := range values { + for part := range strings.SplitSeq(value, ",") { + trimmed := strings.TrimSpace(part) + if trimmed != "" { + chain = append(chain, trimmed) + } + } + } + + return chain +} + +// contextKey is the private key type under which the resolved client IP is +// stored in a request context. +type contextKey struct{} + +// WithClientIP returns a copy of ctx carrying the resolved client IP. +func WithClientIP(ctx context.Context, ip string) context.Context { + return context.WithValue(ctx, contextKey{}, ip) +} + +// FromContext returns the resolved client IP stored in ctx, or an empty +// string if none was set. +func FromContext(ctx context.Context) string { + ip, _ := ctx.Value(contextKey{}).(string) + + return ip +} diff --git a/internal/clientip/clientip_test.go b/internal/clientip/clientip_test.go new file mode 100644 index 0000000..ef3a839 --- /dev/null +++ b/internal/clientip/clientip_test.go @@ -0,0 +1,189 @@ +package clientip_test + +import ( + "net/netip" + "testing" + + "sneak.berlin/go/pixa/internal/clientip" +) + +// Addresses reused across the resolver cases. +const ( + trustedRangeV4 = "10.0.0.0/8" + forwardedV4 = "203.0.113.7" + untrustedV4 = "198.51.100.9" + trustedPeer = "10.0.0.1:5000" +) + +// mustPrefixes parses CIDR strings into prefixes for building a resolver. +func mustPrefixes(t *testing.T, cidrs ...string) []netip.Prefix { + t.Helper() + + prefixes := make([]netip.Prefix, 0, len(cidrs)) + + for _, c := range cidrs { + p, err := netip.ParsePrefix(c) + if err != nil { + t.Fatalf("netip.ParsePrefix(%q) error = %v", c, err) + } + + prefixes = append(prefixes, p) + } + + return prefixes +} + +type resolveCase struct { + name string + trusted []string + remoteAddr string + forwardedFor []string + want string +} + +// runResolveCases runs each case against a resolver built from its trusted +// list and checks the resolved address. +func runResolveCases(t *testing.T, cases []resolveCase) { + t.Helper() + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := clientip.NewResolver(mustPrefixes(t, tt.trusted...)) + + got := r.Resolve(tt.remoteAddr, tt.forwardedFor) + if got != tt.want { + t.Errorf("Resolve(%q, %v) = %q, want %q", + tt.remoteAddr, tt.forwardedFor, got, tt.want) + } + }) + } +} + +// TestResolvePeerTrust covers the trust decision on the direct peer: a +// forwarded header is believed only from a trusted peer, and a client +// connecting directly cannot spoof its address. +func TestResolvePeerTrust(t *testing.T) { + t.Parallel() + + runResolveCases(t, []resolveCase{ + { + name: "trusted peer honors forwarded client", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4}, + want: forwardedV4, + }, + { + name: "untrusted peer ignores forwarded header", + trusted: []string{trustedRangeV4}, + remoteAddr: untrustedV4 + ":33333", + forwardedFor: []string{forwardedV4}, + want: untrustedV4, + }, + { + name: "spoofed chain from untrusted peer cannot influence result", + trusted: []string{trustedRangeV4}, + remoteAddr: untrustedV4 + ":33333", + forwardedFor: []string{"1.2.3.4, 10.9.9.9, 127.0.0.1"}, + want: untrustedV4, + }, + { + name: "empty trusted list always uses peer", + trusted: nil, + remoteAddr: forwardedV4 + ":80", + forwardedFor: []string{"10.0.0.5"}, + want: forwardedV4, + }, + { + name: "trusted peer with no forwarded header uses peer", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: nil, + want: "10.0.0.1", + }, + { + name: "unparseable peer is returned unchanged", + trusted: []string{trustedRangeV4}, + remoteAddr: "garbage", + forwardedFor: []string{forwardedV4}, + want: "garbage", + }, + }) +} + +// TestResolveChainWalk covers walking the X-Forwarded-For chain from a +// trusted peer to the rightmost entry that is not itself a trusted proxy. +func TestResolveChainWalk(t *testing.T) { + t.Parallel() + + runResolveCases(t, []resolveCase{ + { + name: "rightmost untrusted entry across a mixed chain", + trusted: []string{trustedRangeV4, "192.168.0.0/16"}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4 + ", 192.168.1.1, 10.0.0.2"}, + want: forwardedV4, + }, + { + name: "spoofed client behind a trusted proxy is not believed", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{"1.2.3.4, " + untrustedV4}, + want: untrustedV4, + }, + { + name: "chain split across multiple header lines", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4, "10.0.0.2"}, + want: forwardedV4, + }, + { + name: "garbage entries are skipped", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4 + ", not-an-ip"}, + want: forwardedV4, + }, + { + name: "all-trusted chain falls back to peer", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{"10.0.0.9, 10.0.0.2"}, + want: "10.0.0.1", + }, + { + name: "trusted IPv6 peer honors forwarded client", + trusted: []string{"2001:db8::/32"}, + remoteAddr: "[2001:db8::1]:9000", + forwardedFor: []string{forwardedV4}, + want: forwardedV4, + }, + { + name: "IPv4-mapped peer matches IPv4 trusted range", + trusted: []string{trustedRangeV4}, + remoteAddr: "[::ffff:10.0.0.1]:5000", + forwardedFor: []string{forwardedV4}, + want: forwardedV4, + }, + }) +} + +func TestContextRoundTrip(t *testing.T) { + t.Parallel() + + ctx := clientip.WithClientIP(t.Context(), forwardedV4) + if got := clientip.FromContext(ctx); got != forwardedV4 { + t.Errorf("FromContext = %q, want %q", got, forwardedV4) + } +} + +func TestFromContextAbsent(t *testing.T) { + t.Parallel() + + if got := clientip.FromContext(t.Context()); got != "" { + t.Errorf("FromContext with no value = %q, want empty", got) + } +} diff --git a/internal/config/blocked_networks_internal_test.go b/internal/config/blocked_networks_internal_test.go new file mode 100644 index 0000000..25373be --- /dev/null +++ b/internal/config/blocked_networks_internal_test.go @@ -0,0 +1,90 @@ +package config + +import ( + "testing" +) + +// TestBlockedNetworksParsed loads a valid blocked_networks list and checks +// each CIDR is parsed into the resolved prefixes in order. +func TestBlockedNetworksParsed(t *testing.T) { + t.Parallel() + + yamlContent := signingKeyLine + `blocked_networks: + - 203.0.113.0/24 + - 2001:db8::/32 +` + + c, err := configFromYAML(t, yamlContent) + if err != nil { + t.Fatalf("valid blocked_networks should load, got error: %v", err) + } + + want := []string{"203.0.113.0/24", "2001:db8::/32"} + if len(c.BlockedNetworks) != len(want) { + t.Fatalf("BlockedNetworks = %v, want %d entries", c.BlockedNetworks, len(want)) + } + + for i, w := range want { + if got := c.BlockedNetworks[i].String(); got != w { + t.Errorf("BlockedNetworks[%d] = %q, want %q", i, got, w) + } + } +} + +// TestBlockedNetworksOmittedIsEmpty confirms an omitted key leaves the +// operator list empty; the built-in defaults still apply in the fetcher. +func TestBlockedNetworksOmittedIsEmpty(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 len(c.BlockedNetworks) != 0 { + t.Errorf("BlockedNetworks = %v, want empty", c.BlockedNetworks) + } +} + +// TestBlockedNetworksInvalidAbortsStartup checks that malformed values abort +// startup with an error naming the key and the offending value. +func TestBlockedNetworksInvalidAbortsStartup(t *testing.T) { + t.Parallel() + + runAbortCases(t, []abortCase{ + { + name: "not-a-cidr", + yaml: signingKeyLine + `blocked_networks: + - not-a-cidr +`, + wantErrSubstrings: []string{keyBlockedNetworks, "not-a-cidr"}, + }, + { + name: "bare-address-without-prefix", + yaml: signingKeyLine + `blocked_networks: + - 10.0.0.1 +`, + wantErrSubstrings: []string{keyBlockedNetworks, "10.0.0.1"}, + }, + { + name: "empty-entry", + yaml: signingKeyLine + `blocked_networks: + - "" +`, + wantErrSubstrings: []string{keyBlockedNetworks}, + }, + { + name: "non-string-entry", + yaml: signingKeyLine + `blocked_networks: + - 42 +`, + wantErrSubstrings: []string{keyBlockedNetworks}, + }, + { + name: "null-value", + yaml: signingKeyLine + `blocked_networks: +`, + wantErrSubstrings: []string{keyBlockedNetworks, nullValueText}, + }, + }) +} diff --git a/internal/config/config.go b/internal/config/config.go index fb6e02d..3d948be 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "math" + "net/netip" "net/url" "os" "path/filepath" @@ -15,7 +16,6 @@ import ( "git.eeqj.de/sneak/smartconfig" "go.uber.org/fx" - "sneak.berlin/go/pixa/internal/globals" "sneak.berlin/go/pixa/internal/logger" ) @@ -42,18 +42,28 @@ const ( keyAllowHTTP = "allow_http" keyUpstreamConnectionsPerHost = "upstream_connections_per_host" keyCacheMaxBytes = "cache_max_bytes" + keyBlockedNetworks = "blocked_networks" + keyTrustedProxies = "trusted_proxies" ) +// placeholderSigningKey is the dummy signing_key shipped in +// config.example.yml. It is 45 characters, so it passes the length +// check, but it is public in this repository and must be rejected at +// startup so no deployment ever signs URLs with it. +const placeholderSigningKey = "CHANGE_ME_generate_with_openssl_rand_base64_32" + // Static validation errors. Each use site attaches the offending key // and value by wrapping these with fmt.Errorf and %w. var ( errValueRequired = errors.New("a value is required") errValueEmpty = errors.New("value must not be empty") errUnknownConfigKeys = errors.New("unknown config keys") + errUnknownEnvVars = errors.New("unknown environment variables") errNotAString = errors.New("not a string") errNotAnInteger = errors.New("not an integer") errNotABoolean = errors.New("not a boolean") errNotAStringList = errors.New("not a list of strings") + errNotAValidCIDR = errors.New("not a valid CIDR network") errNotAMetricsMap = errors.New("not a map of metrics settings") errEmptyListEntry = errors.New("list contains an empty entry") errEmptyEntry = errors.New("contains an empty entry") @@ -61,6 +71,9 @@ var ( errPortOutOfRange = errors.New("outside the valid port range") errTooFewConnections = errors.New("must be at least 1") errValueTooShort = errors.New("value too short") + errPlaceholderKey = errors.New( + "is the placeholder from config.example.yml; " + + "generate a real key with: openssl rand -base64 32") errMustBeSetTogether = errors.New("must be set together") errMustNotBeNegative = errors.New("must not be negative") errOverflowsInt64 = errors.New("overflows a 64-bit integer") @@ -79,8 +92,7 @@ var ( type Params struct { fx.In - Globals *globals.Globals - Logger *logger.Logger + Logger *logger.Logger } // Config holds application configuration values. @@ -100,6 +112,22 @@ type Config struct { AllowHTTP bool // Allow non-TLS upstream (testing only) UpstreamConnectionsPerHost int // Max concurrent connections per upstream host + // 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. + BlockedNetworks []netip.Prefix + + // TrustedProxies are the CIDR ranges of reverse proxies whose + // forwarding headers may be believed. Forwarded headers are honored + // only when the immediate peer falls inside one of these ranges; + // otherwise the peer address is used and the headers are ignored, so + // an untrusted client cannot spoof its address. An omitted key + // defaults to the RFC 1918 private ranges (see defaultTrustedProxies), + // since pixa is deployed behind a proxy on a private network; an + // explicitly empty list trusts nothing and always uses the peer + // address, and an explicit list replaces the default. + TrustedProxies []netip.Prefix + // CacheMaxBytes is the disk cache size limit in bytes. Zero // disables the disk cache entirely. When cache_max_bytes is // omitted from the configuration, this holds the computed default @@ -108,24 +136,34 @@ type Config struct { CacheMaxBytes int64 // cacheMaxBytesExplicit records whether cache_max_bytes was - // explicitly set in the configuration file. Explicit values are - // used exactly as given; only an omitted key gets the computed - // default (and its floor) in resolveCacheMaxBytes. + // explicitly set, in the environment or the configuration file. + // Explicit values are used exactly as given; only an omitted key + // gets the computed default (and its floor) in resolveCacheMaxBytes. cacheMaxBytesExplicit bool } -// New creates a new Config instance by loading configuration from file. +// New creates a new Config instance from the environment and the +// config file. func New(_ fx.Lifecycle, params Params) (*Config, error) { log := params.Logger.Get() - name := params.Globals.Appname - sc, err := loadConfigFile(log, name) + // Look for the config file under the project name (/etc/pixa/, + // ~/.config/pixa/), matching the /var/lib/pixa state directory, + // not under the daemon name pixad. + sc, err := loadConfigFile(log, "pixa") + if err != nil { + return nil, err + } + + // Loading the config file sets the variables in its env section, + // so this also checks their names. + err = validateKnownEnvVars() if err != nil { return nil, err } if sc == nil { - log.Info("no config file found, using defaults") + log.Info("no config file found, using environment variables and defaults") } c, err := newFromSmartConfig(sc) @@ -150,22 +188,41 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { return c, nil } -// newFromSmartConfig constructs a Config from a loaded smartconfig -// instance and validates it. A nil sc means no config file was found, -// in which case every option takes its default value. A key that is -// present but unparseable or invalid is an error: defaults apply only -// to omitted keys, never to invalid explicit values. +// newFromSmartConfig constructs a Config from the environment and a +// loaded smartconfig instance, and validates it. A nil sc means no +// config file was found, in which case every option the environment +// does not set takes its default value. A key that is present but +// unparseable or invalid is an error: defaults apply only to omitted +// keys, never to invalid explicit values. func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { if sc != nil { err := validateKnownKeys(sc) if err != nil { return nil, err } + } - err = validateAllowlistHostsValue(sc) - if err != nil { - return nil, err - } + err := validateAllowlistHostsValue(sc) + if err != nil { + return nil, err + } + + blockedNetworks, err := parseCIDRList(sc, keyBlockedNetworks) + if err != nil { + return nil, err + } + + trustedProxies, err := parseCIDRList(sc, keyTrustedProxies) + if err != nil { + return nil, err + } + + // parseCIDRList returns a nil slice only when the key is absent; an + // explicitly empty list ([]) comes back non-nil and empty. An omitted + // key takes the RFC 1918 default, while an explicit empty list is left + // as trust-nothing. + if trustedProxies == nil { + trustedProxies = defaultTrustedProxies() } loader := &strictLoader{sc: sc} @@ -183,16 +240,16 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { AllowHTTP: loader.boolVal(keyAllowHTTP, false), UpstreamConnectionsPerHost: loader.intVal( keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), - CacheMaxBytes: loader.int64Val(keyCacheMaxBytes, 0), + CacheMaxBytes: loader.int64Val(keyCacheMaxBytes, 0), + BlockedNetworks: blockedNetworks, + TrustedProxies: trustedProxies, } // The computed default for cache_max_bytes needs a validated // state_dir, so it is resolved later (resolveCacheMaxBytes); here // we only record whether the operator set the key explicitly. - if sc != nil { - if _, present := sc.Get(keyCacheMaxBytes); present { - c.cacheMaxBytesExplicit = true - } + if _, present := lookupValue(sc, keyCacheMaxBytes); present { + c.cacheMaxBytesExplicit = true } // Build DBURL from StateDir if not explicitly set. The derived URL @@ -200,12 +257,9 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { // explicitly empty value. c.DBURL = loader.stringVal(keyDBURL, "") if c.DBURL == "" && loader.err == nil { - if sc != nil { - if _, present := sc.Get(keyDBURL); present { - return nil, fmt.Errorf( - "config key %q: %w; omit the key to derive it from state_dir", - keyDBURL, errValueEmpty) - } + if _, present := lookupValue(sc, keyDBURL); present { + return nil, fmt.Errorf("%s: %w; omit it to derive it from state_dir", + settingName(keyDBURL), errValueEmpty) } c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir) @@ -215,7 +269,7 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { return nil, loader.err } - err := c.validate() + err = c.validate() if err != nil { return nil, err } @@ -299,13 +353,99 @@ func isKnownConfigKey(key string) bool { switch key { case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, - keyUpstreamConnectionsPerHost, keyCacheMaxBytes, "env": + keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, + keyTrustedProxies, "env": return true } return false } +// envVarNames returns, for each configuration key, the environment +// variable that also sets it: PIXA_ plus the key in upper case, with "." +// written as "_", except the port, which REPO_POLICIES.md requires to be +// PORT. metrics is set through its two subkeys; env has no variable. +func envVarNames() map[string]string { + return map[string]string{ //nolint:gosec // G101: variable names, not secrets + keyDebug: "PIXA_DEBUG", + keyMaintenanceMode: "PIXA_MAINTENANCE_MODE", + keyPort: "PORT", + keyStateDir: "PIXA_STATE_DIR", + keySentryDSN: "PIXA_SENTRY_DSN", + keyDBURL: "PIXA_DB_URL", + keyMetricsUsername: "PIXA_METRICS_USERNAME", + keyMetricsPassword: "PIXA_METRICS_PASSWORD", + keySigningKey: "PIXA_SIGNING_KEY", + keyAllowlistHosts: "PIXA_ALLOWLIST_HOSTS", + keyAllowHTTP: "PIXA_ALLOW_HTTP", + keyUpstreamConnectionsPerHost: "PIXA_UPSTREAM_CONNECTIONS_PER_HOST", + keyCacheMaxBytes: "PIXA_CACHE_MAX_BYTES", + keyBlockedNetworks: "PIXA_BLOCKED_NETWORKS", + keyTrustedProxies: "PIXA_TRUSTED_PROXIES", + } +} + +// validateKnownEnvVars rejects environment variables whose names start +// with PIXA_ but that are neither a setting's variable nor +// PIXA_CONFIG_PATH, so a misspelled variable fails at startup instead of +// being silently ignored, as validateKnownKeys does for config file keys. +// New calls it after loading the config file, so the variables the +// file's env section sets are checked too. +func validateKnownEnvVars() error { + known := map[string]bool{"PIXA_CONFIG_PATH": true} + + for _, name := range envVarNames() { + known[name] = true + } + + var unknown []string + + for _, entry := range os.Environ() { + name, _, _ := strings.Cut(entry, "=") + + switch { + case !strings.HasPrefix(name, "PIXA_") || known[name]: + continue + case name == "PIXA_PORT": + unknown = append(unknown, name+" (use PORT for the port)") + default: + unknown = append(unknown, name) + } + } + + if len(unknown) > 0 { + sort.Strings(unknown) + + return fmt.Errorf("%w: %s", errUnknownEnvVars, strings.Join(unknown, ", ")) + } + + return nil +} + +// lookupValue returns the value set for key and whether one is set. The +// key's environment variable wins when it is present, even when empty; +// its value is a string, read exactly as the same text quoted in the +// config file would be. Otherwise the config file's value is used. +func lookupValue(sc *smartconfig.Config, key string) (any, bool) { + value, present := os.LookupEnv(envVarNames()[key]) + if present { + return value, true + } + + if sc == nil { + return nil, false + } + + return sc.Get(key) +} + +// settingName names key in an error message together with its +// environment variable, since either one may have set the value. +func settingName(key string) string { + return fmt.Sprintf("config key %q (environment variable %s)", + key, envVarNames()[key]) +} + // ensureStateDirWritable verifies at startup that StateDir can be // created and written to, so a misconfigured path aborts startup // instead of failing later at first use. @@ -314,28 +454,51 @@ func (c *Config) ensureStateDirWritable() error { err := os.MkdirAll(c.StateDir, stateDirPerms) if err != nil { - return fmt.Errorf("config key %q: cannot create directory %q: %w", - keyStateDir, c.StateDir, err) + return fmt.Errorf("%s: cannot create directory %q: %w", + settingName(keyStateDir), c.StateDir, err) } probe, err := os.CreateTemp(c.StateDir, ".startup-write-probe-*") if err != nil { - return fmt.Errorf("config key %q: directory %q is not writable: %w", - keyStateDir, c.StateDir, err) + return fmt.Errorf("%s: directory %q is not writable: %w", + settingName(keyStateDir), c.StateDir, err) } probePath := probe.Name() err = probe.Close() if err != nil { - return fmt.Errorf("config key %q: cannot close probe file %q: %w", - keyStateDir, probePath, err) + return fmt.Errorf("%s: cannot close probe file %q: %w", + settingName(keyStateDir), probePath, err) } err = os.Remove(probePath) if err != nil { - return fmt.Errorf("config key %q: cannot remove probe file %q: %w", - keyStateDir, probePath, err) + return fmt.Errorf("%s: cannot remove probe file %q: %w", + settingName(keyStateDir), probePath, err) + } + + return nil +} + +// validateSigningKey checks that the signing key is present, long +// enough, and not the public placeholder from config.example.yml. The +// key value itself is never echoed in error messages. +func (c *Config) validateSigningKey() error { + if c.SigningKey == "" { + return fmt.Errorf("%s: %w", settingName(keySigningKey), errValueRequired) + } + + // Minimum key length for security (32 bytes = 256 bits) + const minKeyLength = 32 + if len(c.SigningKey) < minKeyLength { + return fmt.Errorf("%s: %w: must be at least %d characters, got %d", + settingName(keySigningKey), errValueTooShort, minKeyLength, + len(c.SigningKey)) + } + + if c.SigningKey == placeholderSigningKey { + return fmt.Errorf("%s: %w", settingName(keySigningKey), errPlaceholderKey) } return nil @@ -344,39 +507,32 @@ func (c *Config) ensureStateDirWritable() error { // validate checks that all required configuration values are set and // that every value is within its valid range. func (c *Config) validate() error { - // The signing key value is never echoed in error messages. - if c.SigningKey == "" { - return fmt.Errorf("config key %q: %w", keySigningKey, errValueRequired) - } - - // Minimum key length for security (32 bytes = 256 bits) - const minKeyLength = 32 - if len(c.SigningKey) < minKeyLength { - return fmt.Errorf("config key %q: %w: must be at least %d characters, got %d", - keySigningKey, errValueTooShort, minKeyLength, len(c.SigningKey)) + err := c.validateSigningKey() + if err != nil { + return err } const maxPort = 65535 if c.Port < 1 || c.Port > maxPort { - return fmt.Errorf("config key %q: value %d is %w 1-%d", - keyPort, c.Port, errPortOutOfRange, maxPort) + return fmt.Errorf("%s: value %d is %w 1-%d", + settingName(keyPort), c.Port, errPortOutOfRange, maxPort) } if c.UpstreamConnectionsPerHost < 1 { - return fmt.Errorf("config key %q: value %d %w", - keyUpstreamConnectionsPerHost, c.UpstreamConnectionsPerHost, - errTooFewConnections) + return fmt.Errorf("%s: value %d %w", + settingName(keyUpstreamConnectionsPerHost), + c.UpstreamConnectionsPerHost, errTooFewConnections) } if c.StateDir == "" { - return fmt.Errorf("config key %q: %w", keyStateDir, errValueEmpty) + return fmt.Errorf("%s: %w", settingName(keyStateDir), errValueEmpty) } // Zero is valid (it disables the disk cache); only negative // values are rejected. No floor applies to explicit values. if c.CacheMaxBytes < 0 { - return fmt.Errorf("config key %q: value %d %w", - keyCacheMaxBytes, c.CacheMaxBytes, errMustNotBeNegative) + return fmt.Errorf("%s: value %d %w", + settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) } for _, host := range c.AllowlistHosts { @@ -389,14 +545,15 @@ func (c *Config) validate() error { if c.SentryDSN != "" { parsed, err := url.Parse(c.SentryDSN) if err != nil || parsed.Scheme == "" || parsed.Host == "" { - return fmt.Errorf("config key %q: value %q is %w", - keySentryDSN, c.SentryDSN, errNotAValidURL) + return fmt.Errorf("%s: value %q is %w", + settingName(keySentryDSN), c.SentryDSN, errNotAValidURL) } } if (c.MetricsUsername == "") != (c.MetricsPassword == "") { - return fmt.Errorf("config keys %q and %q %w", - keyMetricsUsername, keyMetricsPassword, errMustBeSetTogether) + return fmt.Errorf("%s and %s %w", + settingName(keyMetricsUsername), settingName(keyMetricsPassword), + errMustBeSetTogether) } return nil @@ -411,13 +568,13 @@ func (c *Config) validate() error { // disable URL signing. func validateAllowlistHost(host string) error { if strings.Contains(host, "://") || strings.ContainsAny(host, "/ \t") { - return fmt.Errorf("config key %q: entry %q %w", - keyAllowlistHosts, host, errNotBareHostname) + return fmt.Errorf("%s: entry %q %w", + settingName(keyAllowlistHosts), host, errNotBareHostname) } if strings.Trim(host, ".") == "" { - return fmt.Errorf("config key %q: entry %q %w", - keyAllowlistHosts, host, errNoHostnameLabels) + return fmt.Errorf("%s: entry %q %w", + settingName(keyAllowlistHosts), host, errNoHostnameLabels) } return nil @@ -533,11 +690,7 @@ func (l *strictLoader) boolVal(key string, defaultVal bool) bool { // is omitted. A present value that is not a string, or is explicitly // null, is an error. func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -559,11 +712,7 @@ func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { // omitted. A present value that is not a whole number, or is explicitly // null, is an error; fractional values are never truncated. func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -587,8 +736,8 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { case string: parsed, err := strconv.Atoi(strings.TrimSpace(val)) if err != nil { - return 0, fmt.Errorf("config key %q: value %q is %w", - key, val, errNotAnInteger) + return 0, fmt.Errorf("%s: value %q is %w", + settingName(key), val, errNotAnInteger) } return parsed, nil @@ -603,11 +752,7 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { // is explicitly null, is an error; fractional values are never // truncated and out-of-range values are never clamped. func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -638,8 +783,8 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro case string: parsed, err := strconv.ParseInt(strings.TrimSpace(val), 10, 64) if err != nil { - return 0, fmt.Errorf("config key %q: value %q is %w", - key, val, errNotAnInteger) + return 0, fmt.Errorf("%s: value %q is %w", + settingName(key), val, errNotAnInteger) } return parsed, nil @@ -654,11 +799,7 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro // string), or is explicitly null, is an error; numbers are not accepted // as booleans. func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -673,8 +814,8 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) case string: parsed, err := strconv.ParseBool(strings.TrimSpace(val)) if err != nil { - return false, fmt.Errorf("config key %q: value %q is %w", - key, val, errNotABoolean) + return false, fmt.Errorf("%s: value %q is %w", + settingName(key), val, errNotABoolean) } return parsed, nil @@ -690,7 +831,7 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) // (or a comma-separated string), a non-string entry, or an empty entry // is an error, never silently skipped. func validateAllowlistHostsValue(sc *smartconfig.Config) error { - raw, ok := sc.Get(keyAllowlistHosts) + raw, ok := lookupValue(sc, keyAllowlistHosts) if !ok { return nil } @@ -720,8 +861,8 @@ func validateAllowlistHostsValue(sc *smartconfig.Config) error { for part := range strings.SplitSeq(val, ",") { if strings.TrimSpace(part) == "" { - return fmt.Errorf("config key %q: value %q %w", - keyAllowlistHosts, val, errEmptyEntry) + return fmt.Errorf("%s: value %q %w", + settingName(keyAllowlistHosts), val, errEmptyEntry) } } default: @@ -737,11 +878,7 @@ func validateAllowlistHostsValue(sc *smartconfig.Config) error { // comma-separated string (backwards compatibility). Malformed entries // are rejected beforehand by validateAllowlistHostsValue. func getStringSlice(sc *smartconfig.Config) []string { - if sc == nil { - return nil - } - - val, ok := sc.Get(keyAllowlistHosts) + val, ok := lookupValue(sc, keyAllowlistHosts) if !ok || val == nil { return nil } @@ -778,3 +915,103 @@ func getStringSlice(sc *smartconfig.Config) []string { return nil } + +// defaultTrustedProxies returns the trusted_proxies default: the three RFC +// 1918 private ranges. pixa is always deployed behind a TLS-terminating +// reverse proxy, which in practice sits on a private network, so its +// forwarding headers are believed unless the operator says otherwise. +// Loopback is deliberately excluded: it is not an RFC 1918 range, and no +// deployment reaches pixa over it. A fresh slice is returned on each call so +// callers may hold it without aliasing shared state. +func defaultTrustedProxies() []netip.Prefix { + return []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/8"), + netip.MustParsePrefix("172.16.0.0/12"), + netip.MustParsePrefix("192.168.0.0/16"), + } +} + +// parseCIDRList parses the value of the named config key into CIDR +// prefixes, or returns nil if the key is omitted. It accepts a YAML list +// of strings or a comma-separated string. An explicitly null value, a +// wrong type, an empty entry, a non-string entry, or an unparseable CIDR +// aborts startup naming the key and the offending value; the default +// (an empty list) applies only to an omitted key. +func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { + raw, ok := lookupValue(sc, key) + if !ok { + return nil, nil + } + + if raw == nil { + return nil, errNullConfigValue(key) + } + + entries, err := cidrListEntries(raw, key) + if err != nil { + return nil, err + } + + prefixes := make([]netip.Prefix, 0, len(entries)) + + for _, entry := range entries { + prefix, err := netip.ParsePrefix(entry) + if err != nil { + return nil, fmt.Errorf("%s: value %q is %w", + settingName(key), entry, errNotAValidCIDR) + } + + prefixes = append(prefixes, prefix) + } + + return prefixes, nil +} + +// cidrListEntries extracts the raw entries of the named CIDR-list key as +// trimmed, non-empty strings, from either a YAML list of strings or a +// comma-separated string; an empty string is an empty list, as for +// allowlist_hosts. Any other shape is a configuration error. +func cidrListEntries(raw any, key string) ([]string, error) { + switch val := raw.(type) { + case []any: + entries := make([]string, 0, len(val)) + + for _, item := range val { + str, ok := item.(string) + if !ok { + return nil, fmt.Errorf("config key %q: list entry %v (%T) is %w", + key, item, item, errNotAString) + } + + if strings.TrimSpace(str) == "" { + return nil, fmt.Errorf("config key %q: %w", + key, errEmptyListEntry) + } + + entries = append(entries, strings.TrimSpace(str)) + } + + return entries, nil + case string: + entries := make([]string, 0) + + if strings.TrimSpace(val) == "" { + return entries, nil + } + + for part := range strings.SplitSeq(val, ",") { + trimmed := strings.TrimSpace(part) + if trimmed == "" { + return nil, fmt.Errorf("%s: value %q %w", + settingName(key), val, errEmptyEntry) + } + + entries = append(entries, trimmed) + } + + return entries, nil + default: + return nil, fmt.Errorf("config key %q: value %v (%T) is %w", + key, raw, raw, errNotAStringList) + } +} diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index ca1573e..c12d27d 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -303,6 +303,11 @@ func invalidHostAndCredentialCases() []abortCase { yaml: "signing_key: short\n", wantErrSubstrings: []string{keySigningKey}, }, + { + name: "signing_key is the documented placeholder", + yaml: "signing_key: " + placeholderSigningKey + "\n", + wantErrSubstrings: []string{keySigningKey}, + }, { name: "signing_key missing", yaml: "port: 8080\n", diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go new file mode 100644 index 0000000..57f1f15 --- /dev/null +++ b/internal/config/env_internal_test.go @@ -0,0 +1,367 @@ +package config + +import ( + "net/netip" + "os" + "path/filepath" + "reflect" + "slices" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/globals" + "sneak.berlin/go/pixa/internal/logger" +) + +// TestMain unsets PORT and every PIXA_ environment variable before the +// tests run, so each test sees only the variables it sets itself, not +// whatever the shell running the tests exports. +func TestMain(m *testing.M) { + for _, entry := range os.Environ() { + name, _, _ := strings.Cut(entry, "=") + if name != "PORT" && !strings.HasPrefix(name, "PIXA_") { + continue + } + + err := os.Unsetenv(name) + if err != nil { + panic(err) + } + } + + m.Run() +} + +// wantStartupError fails the test unless err is a startup error that +// mentions every one of wants. +func wantStartupError(t *testing.T, err error, wants ...string) { + t.Helper() + + if err == nil { + t.Fatalf("want a startup error mentioning %q, got none", wants) + } + + t.Logf("got expected error: %v", err) + + for _, want := range wants { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err.Error(), want) + } + } +} + +// TestEnvironmentSetsEveryKey sets every key from its environment +// variable, with no config file at all: PORT for the port, and PIXA_ +// plus the key in upper case, "." written as "_", for every other key. +func TestEnvironmentSetsEveryKey(t *testing.T) { + t.Setenv("PIXA_DEBUG", "true") + t.Setenv("PIXA_MAINTENANCE_MODE", "1") + t.Setenv("PORT", "9090") + t.Setenv("PIXA_STATE_DIR", "/srv/pixa-env") + t.Setenv("PIXA_SENTRY_DSN", "https://abc123@sentry.example.com/42") + t.Setenv("PIXA_DB_URL", "file:/srv/pixa-env/other.sqlite3") + t.Setenv("PIXA_METRICS_USERNAME", "metricsuser") + t.Setenv("PIXA_METRICS_PASSWORD", "metricspass") + t.Setenv("PIXA_SIGNING_KEY", validTestSigningKey) + t.Setenv("PIXA_ALLOWLIST_HOSTS", "s3.sneak.cloud,.example.com") + t.Setenv("PIXA_ALLOW_HTTP", "true") + t.Setenv("PIXA_UPSTREAM_CONNECTIONS_PER_HOST", "5") + 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") + + c, err := newFromSmartConfig(nil) + if err != nil { + t.Fatalf("configuration from the environment alone should load: %v", err) + } + + want := Config{ + Debug: true, + MaintenanceMode: true, + Port: 9090, + StateDir: "/srv/pixa-env", + SentryDSN: "https://abc123@sentry.example.com/42", + DBURL: "file:/srv/pixa-env/other.sqlite3", + MetricsUsername: "metricsuser", + MetricsPassword: "metricspass", + SigningKey: validTestSigningKey, + AllowlistHosts: []string{testHostS3, ".example.com"}, + AllowHTTP: true, + UpstreamConnectionsPerHost: 5, + CacheMaxBytes: 1024, + cacheMaxBytesExplicit: true, + BlockedNetworks: []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, + TrustedProxies: []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}, + } + + if !reflect.DeepEqual(*c, want) { + t.Errorf("config from the environment =\n%+v\nwant\n%+v", *c, want) + } +} + +// TestUnknownPixaVariableAbortsStartup checks that a PIXA_ variable that +// is not a setting's variable, such as a misspelled one, aborts startup +// naming it, as an unknown config key does, instead of being ignored. +func TestUnknownPixaVariableAbortsStartup(t *testing.T) { + t.Setenv("PIXA_TRUSTED_PROXY", "192.0.2.0/24") + t.Setenv("PIXA_SIGNINGKEY", validTestSigningKey) + + err := validateKnownEnvVars() + wantStartupError(t, err, "PIXA_TRUSTED_PROXY", "PIXA_SIGNINGKEY") +} + +// TestPixaPortAbortsStartupPointingToPort checks that PIXA_PORT aborts +// startup with a message saying to use PORT, which sets the port. +func TestPixaPortAbortsStartupPointingToPort(t *testing.T) { + t.Setenv("PIXA_PORT", "9090") + + err := validateKnownEnvVars() + wantStartupError(t, err, "PIXA_PORT", "use PORT") +} + +// TestSettingVariablesAndConfigPathAreAccepted checks that every +// setting's variable and PIXA_CONFIG_PATH pass the check for unknown +// PIXA_ variables. TestEnvironmentSetsEveryKey pins the names in the list. +func TestSettingVariablesAndConfigPathAreAccepted(t *testing.T) { + // A config file's env section loaded by another test can leave a + // PIXA_ variable set for the whole process, so every one is unset + // here first; t.Setenv restores each when the test ends. + for _, entry := range os.Environ() { + name, _, _ := strings.Cut(entry, "=") + if !strings.HasPrefix(name, "PIXA_") { + continue + } + + t.Setenv(name, "") + + err := os.Unsetenv(name) + if err != nil { + t.Fatalf("failed to unset %s: %v", name, err) + } + } + + t.Setenv("PIXA_CONFIG_PATH", "/etc/pixa/config.yml") + + for _, name := range envVarNames() { + t.Setenv(name, "") + } + + err := validateKnownEnvVars() + if err != nil { + t.Fatalf("PIXA_CONFIG_PATH and every setting's variable "+ + "must be accepted: %v", err) + } +} + +// configFromNew writes yamlContent to a temporary config file, points +// PIXA_CONFIG_PATH at it, and runs New, as the server does at startup. +// The state directory is a temporary one and the disk cache is off, so +// New succeeds unless something in the test is wrong. +func configFromNew(t *testing.T, yamlContent string) (*Config, error) { + t.Helper() + + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yml") + + err := os.WriteFile(configPath, []byte(yamlContent), 0o600) + if err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + t.Setenv("PIXA_CONFIG_PATH", configPath) + t.Setenv("PIXA_STATE_DIR", filepath.Join(tmpDir, "state")) + t.Setenv("PIXA_CACHE_MAX_BYTES", "0") + + testLogger, err := logger.New(nil, logger.Params{Globals: &globals.Globals{}}) + if err != nil { + t.Fatalf("failed to create logger: %v", err) + } + + return New(nil, Params{Logger: testLogger}) +} + +// TestUnknownPixaVariableAbortsNew checks that New, which the server +// calls at startup, aborts on a misspelled PIXA_ variable. +func TestUnknownPixaVariableAbortsNew(t *testing.T) { + t.Setenv("PIXA_TRUSTED_PROXY", "192.0.2.0/24") + + _, err := configFromNew(t, signingKeyLine) + wantStartupError(t, err, "PIXA_TRUSTED_PROXY") +} + +// TestUnknownPixaVariableInEnvSectionAbortsNew checks that New aborts +// on a misspelled PIXA_ name in the config file's env section, which +// loading the file sets as an environment variable. +func TestUnknownPixaVariableInEnvSectionAbortsNew(t *testing.T) { + // The variable must be absent until the file loads. t.Setenv makes + // sure the one the file sets is removed when the test ends. + t.Setenv("PIXA_TRUSTED_PROXY", "") + + err := os.Unsetenv("PIXA_TRUSTED_PROXY") + if err != nil { + t.Fatalf("failed to unset PIXA_TRUSTED_PROXY: %v", err) + } + + _, err = configFromNew(t, signingKeyLine+ + "env:\n PIXA_TRUSTED_PROXY: 192.0.2.0/24\n") + wantStartupError(t, err, "PIXA_TRUSTED_PROXY") +} + +// TestPortFromEnvironmentOverridesConfigFile checks that PORT wins over +// the port in the config file. +func TestPortFromEnvironmentOverridesConfigFile(t *testing.T) { + t.Setenv("PORT", "9090") + + c, err := configFromYAML(t, signingKeyLine+"port: 8080\n") + if err != nil { + t.Fatalf("PORT=9090 with port 8080 in the file should load: %v", err) + } + + if c.Port != 9090 { + t.Errorf("Port = %d, want 9090 from PORT, not 8080 from the file", c.Port) + } +} + +// TestInvalidPortFromEnvironmentAbortsStartup checks that a PORT that is +// not a number, or is outside the port range, aborts startup naming PORT +// and the value, even though the file's port is valid. +func TestInvalidPortFromEnvironmentAbortsStartup(t *testing.T) { + t.Setenv("PORT", "banana") + + _, err := configFromYAML(t, signingKeyLine+"port: 8080\n") + wantStartupError(t, err, "PORT", "banana") + + t.Setenv("PORT", "70000") + + _, err = configFromYAML(t, signingKeyLine+"port: 8080\n") + wantStartupError(t, err, "PORT", "70000") +} + +// TestListFromEnvironmentReplacesConfigFileList checks that a list +// variable replaces the file's list, split on commas with the spaces +// around each entry trimmed. +func TestListFromEnvironmentReplacesConfigFileList(t *testing.T) { + t.Setenv("PIXA_ALLOWLIST_HOSTS", " cdn.example.com , .example.org ") + + c, err := configFromYAML(t, signingKeyLine+ + "allowlist_hosts:\n - s3.sneak.cloud\n - sneak.berlin\n") + if err != nil { + t.Fatalf("PIXA_ALLOWLIST_HOSTS should load: %v", err) + } + + want := []string{"cdn.example.com", ".example.org"} + if !slices.Equal(c.AllowlistHosts, want) { + t.Errorf("AllowlistHosts = %v, want %v from PIXA_ALLOWLIST_HOSTS", + c.AllowlistHosts, want) + } +} + +// TestInvalidBlockedNetworksFromEnvironmentAbortsStartup checks that an +// invalid CIDR, or an empty entry, in PIXA_BLOCKED_NETWORKS aborts +// startup naming the variable, as the same list in the file does. +func TestInvalidBlockedNetworksFromEnvironmentAbortsStartup(t *testing.T) { + t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24,not-a-cidr") + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, "PIXA_BLOCKED_NETWORKS", "not-a-cidr") + + t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24,,198.51.100.0/24") + + _, err = configFromYAML(t, signingKeyLine) + wantStartupError(t, err, "PIXA_BLOCKED_NETWORKS") +} + +// TestInvalidDebugFromEnvironmentAbortsStartup checks that a PIXA_DEBUG +// that strconv.ParseBool rejects aborts startup instead of defaulting. +func TestInvalidDebugFromEnvironmentAbortsStartup(t *testing.T) { + t.Setenv("PIXA_DEBUG", "maybe") + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, "PIXA_DEBUG", "maybe") +} + +// TestConfigFileAloneBehavesAsBefore checks that with no variables set +// (TestMain unsets them) the config file's values are used and omitted +// keys take their defaults. +func TestConfigFileAloneBehavesAsBefore(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+"port: 9191\n") + if err != nil { + t.Fatalf("config file should load: %v", err) + } + + if c.Port != 9191 { + t.Errorf("Port = %d, want 9191 from the file", c.Port) + } + + if c.StateDir != DefaultStateDir { + t.Errorf("StateDir = %q, want default %q", c.StateDir, DefaultStateDir) + } + + if !slices.Equal(c.TrustedProxies, defaultTrustedProxies()) { + t.Errorf("TrustedProxies = %v, want default %v", + c.TrustedProxies, defaultTrustedProxies()) + } +} + +// TestEmptyTrustedProxiesFromEnvironmentTrustsNoOne checks that an empty +// PIXA_TRUSTED_PROXIES is an empty list, like [] in the file: it trusts +// no proxy instead of taking the default ranges. +func TestEmptyTrustedProxiesFromEnvironmentTrustsNoOne(t *testing.T) { + t.Setenv("PIXA_TRUSTED_PROXIES", "") + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("empty PIXA_TRUSTED_PROXIES should load: %v", err) + } + + if len(c.TrustedProxies) != 0 { + t.Errorf("TrustedProxies = %v, want none", c.TrustedProxies) + } +} + +// TestEmptyVariableDoesNotFallBackToConfigFile checks that a variable +// that is present but empty is a set value: an empty PIXA_STATE_DIR +// aborts startup like state_dir: "" in the file, instead of falling +// through to the file's state_dir. +func TestEmptyVariableDoesNotFallBackToConfigFile(t *testing.T) { + t.Setenv("PIXA_STATE_DIR", "") + + _, err := configFromYAML(t, signingKeyLine+"state_dir: /srv/pixa-file\n") + wantStartupError(t, err, "PIXA_STATE_DIR") +} + +// TestMissingSigningKeyNamesItsVariable checks that with no config file +// and no PIXA_SIGNING_KEY, startup aborts naming the variable, which is +// how a container started without it reports the problem. +func TestMissingSigningKeyNamesItsVariable(t *testing.T) { + t.Parallel() + + _, err := newFromSmartConfig(nil) + wantStartupError(t, err, "PIXA_SIGNING_KEY") +} + +// TestSecretsFromEnvironmentAreNotPrinted checks that errors about the +// signing key and the metrics password name their variables but never +// print their values. +func TestSecretsFromEnvironmentAreNotPrinted(t *testing.T) { + t.Setenv("PIXA_SIGNING_KEY", "short-signing-secret") + + _, err := newFromSmartConfig(nil) + wantStartupError(t, err, "PIXA_SIGNING_KEY") + + if strings.Contains(err.Error(), "short-signing-secret") { + t.Errorf("error %q prints the signing key", err.Error()) + } + + t.Setenv("PIXA_SIGNING_KEY", validTestSigningKey) + t.Setenv("PIXA_METRICS_PASSWORD", "metrics-password-secret") + + _, err = newFromSmartConfig(nil) + wantStartupError(t, err, "PIXA_METRICS_PASSWORD") + + if strings.Contains(err.Error(), "metrics-password-secret") { + t.Errorf("error %q prints the metrics password", err.Error()) + } +} diff --git a/internal/config/trusted_proxies_internal_test.go b/internal/config/trusted_proxies_internal_test.go new file mode 100644 index 0000000..ae61bfd --- /dev/null +++ b/internal/config/trusted_proxies_internal_test.go @@ -0,0 +1,85 @@ +package config + +import ( + "strings" + "testing" +) + +// TestTrustedProxiesConfig checks the trusted_proxies key wiring: an +// explicit CIDR list lands in TrustedProxies in order and replaces the +// default, an omitted key falls back to the RFC 1918 private ranges, and an +// explicitly empty list trusts no one. The list parser itself is shared with +// blocked_networks and is exercised in depth by that key's tests. +func TestTrustedProxiesConfig(t *testing.T) { + t.Parallel() + + t.Run("explicit list replaces the default in order", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, + signingKeyLine+`trusted_proxies: ["10.0.0.0/8", "2001:db8::/32"]`+"\n") + if err != nil { + t.Fatalf("valid trusted_proxies should load: %v", err) + } + + got := make([]string, len(c.TrustedProxies)) + for i, p := range c.TrustedProxies { + got[i] = p.String() + } + + if joined := strings.Join(got, ","); joined != "10.0.0.0/8,2001:db8::/32" { + t.Errorf("TrustedProxies = %v, want the two ranges in order", got) + } + }) + + t.Run("omitted key defaults to the RFC 1918 ranges", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("minimal config should load: %v", err) + } + + got := make([]string, len(c.TrustedProxies)) + for i, p := range c.TrustedProxies { + got[i] = p.String() + } + + want := "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" + if joined := strings.Join(got, ","); joined != want { + t.Errorf("TrustedProxies = %v, want the RFC 1918 ranges %q", got, want) + } + }) + + t.Run("explicitly empty list trusts no one", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+"trusted_proxies: []\n") + if err != nil { + t.Fatalf("empty trusted_proxies should load: %v", err) + } + + if len(c.TrustedProxies) != 0 { + t.Errorf("TrustedProxies = %v, want empty", c.TrustedProxies) + } + }) +} + +// TestTrustedProxiesInvalidAbortsStartup checks that an invalid or null +// value aborts startup with an error naming the key and the offending value. +func TestTrustedProxiesInvalidAbortsStartup(t *testing.T) { + t.Parallel() + + runAbortCases(t, []abortCase{ + { + name: "invalid cidr", + yaml: signingKeyLine + `trusted_proxies: ["999.0.0.0/8"]` + "\n", + wantErrSubstrings: []string{keyTrustedProxies, "999.0.0.0/8"}, + }, + { + name: "null value", + yaml: signingKeyLine + "trusted_proxies:\n", + wantErrSubstrings: []string{keyTrustedProxies, nullValueText}, + }, + }) +} diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 977756a..a0ad493 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -1,17 +1,37 @@ package handlers import ( + "bytes" "crypto/subtle" + "errors" + "fmt" + "html/template" + "math" "net/http" "net/url" "strconv" "time" + "sneak.berlin/go/pixa/internal/clientip" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/templates" ) +// errInvalidFormField reports a generator form field, or the q or exp +// parameter of /v1/image/, whose value is non-numeric or out of range. The +// offending field name is wrapped in so the response can name it. +var errInvalidFormField = errors.New("invalid") + +// Bounds for the generator's quality and ttl fields; the quality bounds also +// apply to the q parameter of /v1/image/. maxTTL is in seconds: the expiry +// calculation time.Duration(ttl) * time.Second overflows above it. +const ( + minQuality = 1 + maxQuality = 100 + maxTTL = int(math.MaxInt64 / time.Second) +) + // HandleRoot serves the login page or generator page based on authentication state. func (s *Handlers) HandleRoot() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -23,13 +43,13 @@ func (s *Handlers) HandleRoot() http.HandlerFunc { // Check if authenticated if s.sessMgr.IsAuthenticated(r) { - s.renderGenerator(w, nil) + s.renderGenerator(w, r, nil) return } // Show login page - s.renderLogin(w, "") + s.renderLogin(w, r, "") } } @@ -37,7 +57,7 @@ func (s *Handlers) HandleRoot() http.HandlerFunc { func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) { err := r.ParseForm() if err != nil { - s.renderLogin(w, "Invalid form data") + s.renderLogin(w, r, "Invalid form data") return } @@ -46,8 +66,9 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) { // Constant-time comparison to prevent timing attacks if subtle.ConstantTimeCompare([]byte(submittedKey), []byte(s.config.SigningKey)) != 1 { - s.log.Warn("failed login attempt", "remote_addr", r.RemoteAddr) - s.renderLogin(w, "Invalid signing key") + s.log.Warn("failed login attempt", + "remote_addr", clientip.FromContext(r.Context())) + s.renderLogin(w, r, "Invalid signing key") return } @@ -56,12 +77,13 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) { err = s.sessMgr.CreateSession(w) if err != nil { s.log.Error("failed to create session", "error", err) - s.renderLogin(w, "Failed to create session") + s.renderLogin(w, r, "Failed to create session") return } - s.log.Info("successful login", "remote_addr", r.RemoteAddr) + s.log.Info("successful login", + "remote_addr", clientip.FromContext(r.Context())) // Redirect to generator page http.Redirect(w, r, "/", http.StatusSeeOther) @@ -87,7 +109,7 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { err := r.ParseForm() if err != nil { - s.renderGenerator(w, &generatorData{Error: "Invalid form data"}) + s.renderGenerator(w, r, &generatorData{Error: "Invalid form data"}) return } @@ -97,18 +119,26 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { // Validate source URL parsed, err := url.Parse(sourceURL) if err != nil || parsed.Host == "" { - s.renderGeneratorWithForm(w, "Invalid source URL", r.Form) + s.renderGeneratorWithForm(w, r, "Invalid source URL", r.Form, + http.StatusBadRequest) return } - payload, expiresAt, ttl := buildGeneratePayload(parsed, r.Form) + payload, expiresAt, ttl, err := buildGeneratePayload(parsed, r.Form) + if err != nil { + s.renderGeneratorWithForm(w, r, err.Error(), r.Form, + http.StatusBadRequest) + + return + } // Generate encrypted token token, err := s.encGen.Generate(payload) if err != nil { s.log.Error("failed to generate encrypted URL", "error", err) - s.renderGeneratorWithForm(w, "Failed to generate URL", r.Form) + s.renderGeneratorWithForm(w, r, "Failed to generate URL", r.Form, + http.StatusInternalServerError) return } @@ -121,7 +151,7 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { expiresAtStr = expiresAt.Format(time.RFC3339) } - s.renderGenerator(w, &generatorData{ + s.renderGenerator(w, r, &generatorData{ GeneratedURL: generatedURL, ExpiresAt: expiresAtStr, FormURL: sourceURL, @@ -136,17 +166,40 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { } // buildGeneratePayload parses the numeric form fields and assembles the -// encrypted URL payload. ttl=0 means never expires (ExpiresAt stays 0). +// encrypted URL payload. ttl=0 means never expires (ExpiresAt stays 0). A +// non-numeric or out-of-range width, height, quality or ttl, or an +// unrecognized fit mode, is a client error naming the offending field. The +// format field is passed through unchecked. func buildGeneratePayload( parsed *url.URL, form url.Values, -) (*encurl.Payload, time.Time, int) { - width, _ := strconv.Atoi(form.Get("width")) - height, _ := strconv.Atoi(form.Get("height")) - quality, _ := strconv.Atoi(form.Get("quality")) - ttl, _ := strconv.Atoi(form.Get("ttl")) +) (*encurl.Payload, time.Time, int, error) { + width, err := parseFormDimension(form, "width") + if err != nil { + return nil, time.Time{}, 0, err + } - if quality <= 0 { - quality = 85 + height, err := parseFormDimension(form, "height") + if err != nil { + return nil, time.Time{}, 0, err + } + + quality, err := parseFormInt(form, "quality", + encurl.DefaultQuality, minQuality, maxQuality) + if err != nil { + return nil, time.Time{}, 0, err + } + + ttl, err := parseFormInt(form, "ttl", 0, 0, maxTTL) + if err != nil { + return nil, time.Time{}, 0, err + } + + fitMode := imgcache.FitMode(form.Get("fit")) + + err = imgcache.ValidateFitMode(fitMode) + if err != nil { + return nil, time.Time{}, 0, + fmt.Errorf("%w: %s", imgcache.ErrInvalidFitMode, form.Get("fit")) } var ( @@ -167,11 +220,57 @@ func buildGeneratePayload( Height: height, Format: imgcache.ImageFormat(form.Get("format")), Quality: quality, - FitMode: imgcache.FitMode(form.Get("fit")), + FitMode: fitMode, ExpiresAt: expiresAtUnix, } - return payload, expiresAt, ttl + return payload, expiresAt, ttl, nil +} + +// parseFormDimension reads an optional width or height form field. An empty +// value means "original size" (0). A non-numeric value, or one +// imgcache.ValidateDimension rejects, is an error naming the field. +func parseFormDimension(form url.Values, field string) (int, error) { + raw := form.Get(field) + if raw == "" { + return 0, nil + } + + value, err := strconv.Atoi(raw) + if err != nil { + return 0, fmt.Errorf("%w %s: not a number", errInvalidFormField, field) + } + + err = imgcache.ValidateDimension(field, value) + if err != nil { + return 0, err + } + + return value, nil +} + +// parseFormInt reads an optional integer form field or URL query parameter, +// returning def when the field is empty and an error naming the field when the +// value is non-numeric or outside minValue to maxValue. +func parseFormInt( + form url.Values, field string, def, minValue, maxValue int, +) (int, error) { + raw := form.Get(field) + if raw == "" { + return def, nil + } + + value, err := strconv.Atoi(raw) + if err != nil { + return 0, fmt.Errorf("%w %s: not a number", errInvalidFormField, field) + } + + if value < minValue || value > maxValue { + return 0, fmt.Errorf("%w %s: must be from %d to %d", + errInvalidFormField, field, minValue, maxValue) + } + + return value, nil } // generatorData holds template data for the generator page. @@ -186,15 +285,20 @@ type generatorData struct { FormQuality string FormFit string FormTTL string + CSRFField template.HTML } -func (s *Handlers) renderLogin(w http.ResponseWriter, errorMsg string) { +func (s *Handlers) renderLogin( + w http.ResponseWriter, r *http.Request, errorMsg string, +) { w.Header().Set("Content-Type", "text/html; charset=utf-8") data := struct { - Error string + Error string + CSRFField template.HTML }{ - Error: errorMsg, + Error: errorMsg, + CSRFField: csrfField(r), } err := templates.Render(w, "login.html", data) @@ -204,24 +308,49 @@ func (s *Handlers) renderLogin(w http.ResponseWriter, errorMsg string) { } } -func (s *Handlers) renderGenerator(w http.ResponseWriter, data *generatorData) { +func (s *Handlers) renderGenerator( + w http.ResponseWriter, r *http.Request, data *generatorData, +) { + s.renderGeneratorStatus(w, r, data, http.StatusOK) +} + +// renderGeneratorStatus renders the generator page with an explicit HTTP +// status; a rejected form uses 400. The page is rendered into a buffer before +// the status is written, so a template failure can still answer 500. +func (s *Handlers) renderGeneratorStatus( + w http.ResponseWriter, r *http.Request, data *generatorData, status int, +) { w.Header().Set("Content-Type", "text/html; charset=utf-8") if data == nil { data = &generatorData{} } - err := templates.Render(w, "generator.html", data) + data.CSRFField = csrfField(r) + + var page bytes.Buffer + + err := templates.Render(&page, "generator.html", data) if err != nil { s.log.Error("failed to render generator template", "error", err) http.Error(w, "Internal server error", http.StatusInternalServerError) + + return + } + + w.WriteHeader(status) + + _, err = page.WriteTo(w) + if err != nil { + s.log.Error("failed to write generator page", "error", err) } } func (s *Handlers) renderGeneratorWithForm( - w http.ResponseWriter, errorMsg string, form url.Values, + w http.ResponseWriter, r *http.Request, errorMsg string, + form url.Values, status int, ) { - s.renderGenerator(w, &generatorData{ + s.renderGeneratorStatus(w, r, &generatorData{ Error: errorMsg, FormURL: form.Get("url"), FormWidth: form.Get("width"), @@ -230,7 +359,7 @@ func (s *Handlers) renderGeneratorWithForm( FormQuality: form.Get("quality"), FormFit: form.Get("fit"), FormTTL: form.Get("ttl"), - }) + }, status) } func (s *Handlers) buildGeneratedURL(r *http.Request, token, format string) string { diff --git a/internal/handlers/auth_clientip_internal_test.go b/internal/handlers/auth_clientip_internal_test.go new file mode 100644 index 0000000..91d3fd2 --- /dev/null +++ b/internal/handlers/auth_clientip_internal_test.go @@ -0,0 +1,41 @@ +package handlers + +import ( + "bytes" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/clientip" + "sneak.berlin/go/pixa/internal/config" +) + +// TestFailedLoginLogsResolvedClientIP verifies the failed-login record +// carries the resolved client IP from the request context, not the raw +// proxy peer address. +func TestFailedLoginLogsResolvedClientIP(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + h := &Handlers{ + log: slog.New(slog.NewJSONHandler(&buf, nil)), + config: &config.Config{SigningKey: testSigningKey}, + } + + form := url.Values{loginKeyField: {"wrong-key"}} + req := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/", + strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = req.WithContext(clientip.WithClientIP(req.Context(), "203.0.113.7")) + + h.handleLoginPost(httptest.NewRecorder(), req) + + if !strings.Contains(buf.String(), `"remote_addr":"203.0.113.7"`) { + t.Errorf("failed-login log missing resolved client IP; got %q", buf.String()) + } +} diff --git a/internal/handlers/auth_csrf_internal_test.go b/internal/handlers/auth_csrf_internal_test.go new file mode 100644 index 0000000..161253b --- /dev/null +++ b/internal/handlers/auth_csrf_internal_test.go @@ -0,0 +1,273 @@ +package handlers + +import ( + "context" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + + "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/session" +) + +// testSigningKey is a throwaway signing key for the CSRF flow tests. It +// seeds the session manager, the encrypted-URL generator, and the CSRF +// token key, exactly as the real signing key does in production. +const testSigningKey = "test-signing-key-0123456789abcdef" + +// Form field names used in the CSRF flow tests. +const ( + loginKeyField = "key" + // gorilla/csrf's default form field name, not a credential. + csrfTokenField = "gorilla.csrf.Token" //nolint:gosec // G101 false positive +) + +// csrfFieldPattern extracts the token rendered by csrf.TemplateField into +// the form. The field name is gorilla/csrf's default. +var csrfFieldPattern = regexp.MustCompile( + `name="gorilla\.csrf\.Token" value="([^"]+)"`) + +// newCSRFTestRouter builds a router that mirrors the production wiring for +// the CSRF-protected UI routes (see server.SetupRoutes): the login and +// generator forms and their POST targets sit behind the real CSRF +// middleware. Requests are marked plaintext (Debug: true) so the flow runs +// over httptest's http transport without an https Referer. +func newCSRFTestRouter(t *testing.T) (*Handlers, http.Handler) { + t.Helper() + + cfg := &config.Config{SigningKey: testSigningKey, Debug: true} + + sessMgr, err := session.NewManager(testSigningKey) + if err != nil { + t.Fatalf("session.NewManager() error = %v", err) + } + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + protect, err := newCSRFProtect(testSigningKey, cfg.Debug) + if err != nil { + t.Fatalf("newCSRFProtect() error = %v", err) + } + + h := &Handlers{ + log: slog.New(slog.DiscardHandler), + config: cfg, + sessMgr: sessMgr, + encGen: encGen, + csrfProtect: protect, + } + + r := chi.NewRouter() + r.Group(func(r chi.Router) { + r.Use(h.CSRF()) + r.Get("/", h.HandleRoot()) + r.Post("/", h.HandleRoot()) + r.Post("/generate", h.HandleGenerateURL()) + }) + + return h, r +} + +// csrfCredentials performs a GET that renders a form and returns the CSRF +// cookies the middleware set and the token embedded in the form. Passing +// the authenticated session cookie renders the generator form instead of +// the login form. +func csrfCredentials( + t *testing.T, srv http.Handler, reqCookies []*http.Cookie, +) ([]*http.Cookie, string) { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil) + for _, c := range reqCookies { + req.AddCookie(c) + } + + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("GET / status = %d, want %d", rec.Code, http.StatusOK) + } + + match := csrfFieldPattern.FindStringSubmatch(rec.Body.String()) + if match == nil { + t.Fatalf("no CSRF token field found in rendered form") + } + + return rec.Result().Cookies(), match[1] +} + +// postForm submits form values with the given cookies and returns the +// recorder. +func postForm( + srv http.Handler, path string, + cookies []*http.Cookie, form url.Values, +) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, path, + strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + for _, c := range cookies { + req.AddCookie(c) + } + + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + return rec +} + +// TestLoginPostRejectedWithoutToken verifies that POST / with no CSRF token +// is rejected. This is login CSRF: no session cookie exists yet, so the +// protection must rest on a token bound to a pre-session cookie. +func TestLoginPostRejectedWithoutToken(t *testing.T) { + t.Parallel() + + _, srv := newCSRFTestRouter(t) + + rec := postForm(srv, "/", nil, url.Values{loginKeyField: {testSigningKey}}) + + if rec.Code != http.StatusForbidden { + t.Errorf("POST / without token status = %d, want %d", + rec.Code, http.StatusForbidden) + } +} + +// TestLoginPostRejectedWithForeignToken verifies that a token that does not +// match the request's CSRF cookie is rejected: a token minted for one +// cookie cannot authorize a request carrying a different cookie. +func TestLoginPostRejectedWithForeignToken(t *testing.T) { + t.Parallel() + + _, srv := newCSRFTestRouter(t) + + cookiesA, _ := csrfCredentials(t, srv, nil) + _, tokenB := csrfCredentials(t, srv, nil) + + rec := postForm(srv, "/", cookiesA, url.Values{ + loginKeyField: {testSigningKey}, + csrfTokenField: {tokenB}, + }) + + if rec.Code != http.StatusForbidden { + t.Errorf("POST / with foreign token status = %d, want %d", + rec.Code, http.StatusForbidden) + } +} + +// TestLoginPostAcceptedWithValidToken verifies that POST / with a matching +// cookie and token succeeds: the login is processed and a session is +// established (303 redirect). +func TestLoginPostAcceptedWithValidToken(t *testing.T) { + t.Parallel() + + _, srv := newCSRFTestRouter(t) + + cookies, token := csrfCredentials(t, srv, nil) + + rec := postForm(srv, "/", cookies, url.Values{ + loginKeyField: {testSigningKey}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusSeeOther { + t.Fatalf("POST / with valid token status = %d, want %d", + rec.Code, http.StatusSeeOther) + } + + var authed bool + + for _, c := range rec.Result().Cookies() { + if c.Name == session.CookieName && c.Value != "" { + authed = true + } + } + + if !authed { + t.Error("valid login did not set a session cookie") + } +} + +// TestGeneratePostRejectedWithoutToken verifies that POST /generate is +// rejected without a CSRF token even when the request carries a valid +// authenticated session. The session cookie is not sufficient; the policy +// requires a CSRF token on this cookie-authenticated form. +func TestGeneratePostRejectedWithoutToken(t *testing.T) { + t.Parallel() + + h, srv := newCSRFTestRouter(t) + + sessionCookie := newSessionCookie(t, h) + + rec := postForm(srv, "/generate", + []*http.Cookie{sessionCookie}, + url.Values{"url": {"https://example.com/a.jpg"}}) + + if rec.Code != http.StatusForbidden { + t.Errorf("POST /generate without token status = %d, want %d", + rec.Code, http.StatusForbidden) + } +} + +// TestGeneratePostAcceptedWithValidToken verifies that POST /generate +// succeeds with a valid session and a matching CSRF cookie and token. +func TestGeneratePostAcceptedWithValidToken(t *testing.T) { + t.Parallel() + + h, srv := newCSRFTestRouter(t) + + sessionCookie := newSessionCookie(t, h) + + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + rec := postForm(srv, "/generate", cookies, url.Values{ + "url": {"https://example.com/a.jpg"}, + "format": {"jpeg"}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusOK { + t.Fatalf("POST /generate with valid token status = %d, want %d", + rec.Code, http.StatusOK) + } + + if !strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("generator response did not contain a generated URL") + } +} + +// newSessionCookie creates an authenticated session cookie via the +// handler's session manager. +func newSessionCookie(t *testing.T, h *Handlers) *http.Cookie { + t.Helper() + + rec := httptest.NewRecorder() + + err := h.sessMgr.CreateSession(rec) + if err != nil { + t.Fatalf("CreateSession() error = %v", err) + } + + for _, c := range rec.Result().Cookies() { + if c.Name == session.CookieName { + return c + } + } + + t.Fatalf("session manager did not set a %q cookie", session.CookieName) + + return nil +} diff --git a/internal/handlers/auth_generate_internal_test.go b/internal/handlers/auth_generate_internal_test.go new file mode 100644 index 0000000..bc26990 --- /dev/null +++ b/internal/handlers/auth_generate_internal_test.go @@ -0,0 +1,163 @@ +package handlers + +import ( + "maps" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + "time" + + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// Generator form field names, and a value that is not a number. +const ( + widthField = "width" + heightField = "height" + qualityField = "quality" + ttlField = "ttl" + fitField = "fit" + notANumber = "abc" +) + +// generatePost submits the /generate form with a valid session and CSRF token +// plus the caller's extra fields, returning the recorder. +func generatePost( + t *testing.T, extra url.Values, +) *httptest.ResponseRecorder { + t.Helper() + + h, srv := newCSRFTestRouter(t) + + sessionCookie := newSessionCookie(t, h) + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + form := url.Values{ + sourceURLField: {testSourceURL}, + csrfTokenField: {token}, + } + maps.Copy(form, extra) + + return postForm(srv, "/generate", cookies, form) +} + +// TestGeneratePostRejectsNonNumericWidth verifies that a non-numeric width is +// rejected with 400 naming the field rather than being coerced to 0 and +// minting a 0-width token. +func TestGeneratePostRejectsNonNumericWidth(t *testing.T) { + t.Parallel() + + rec := generatePost(t, url.Values{"width": {"abc"}}) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("a token was generated for non-numeric width") + } +} + +// TestGeneratePostRejectsOverLimitWidth verifies that a width beyond +// MaxDimension is rejected at generation time so an unusable token cannot be +// minted. +func TestGeneratePostRejectsOverLimitWidth(t *testing.T) { + t.Parallel() + + rec := generatePost(t, url.Values{"width": {"100000"}}) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("a token was generated for an over-limit width") + } +} + +// TestGeneratePostRejectsBadField verifies that each generator field whose +// value is not a number, out of range, or unrecognized is rejected with 400, +// mints no token, and is named in the error shown on the page. +func TestGeneratePostRejectsBadField(t *testing.T) { + t.Parallel() + + tests := []struct { + field, value, wantError string + }{ + {widthField, notANumber, "invalid width: not a number"}, + {widthField, "-1", "width is negative"}, + {widthField, "8193", "width is above 8192"}, + {heightField, notANumber, "invalid height: not a number"}, + {heightField, "8193", "height is above 8192"}, + {qualityField, notANumber, "invalid quality: not a number"}, + {qualityField, "0", "invalid quality: must be from 1 to 100"}, + {qualityField, "101", "invalid quality: must be from 1 to 100"}, + {ttlField, notANumber, "invalid ttl: not a number"}, + {ttlField, "-1", "invalid ttl: must be from 0 to"}, + {ttlField, "10000000000", "invalid ttl: must be from 0 to"}, + {fitField, "bogus", "invalid fit mode: bogus"}, + } + + for _, tt := range tests { + t.Run(tt.field+"="+tt.value, func(t *testing.T) { + t.Parallel() + + rec := generatePost(t, url.Values{tt.field: {tt.value}}) + body := rec.Body.String() + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(body, "/v1/e/") { + t.Error("a token was generated") + } + + if !strings.Contains(body, tt.wantError) { + t.Errorf("page does not show %q", tt.wantError) + } + }) + } +} + +// TestBuildGeneratePayloadDefaultAndLimits verifies that an empty quality +// takes the default, and that the largest accepted width, height, quality and +// ttl are accepted with an expiry still in the future. +func TestBuildGeneratePayloadDefaultAndLimits(t *testing.T) { + t.Parallel() + + parsed, err := url.Parse(testSourceURL) + if err != nil { + t.Fatalf("url.Parse() error = %v", err) + } + + payload, _, _, err := buildGeneratePayload(parsed, url.Values{}) + if err != nil { + t.Fatalf("empty form: error = %v", err) + } + + if payload.Quality != encurl.DefaultQuality { + t.Errorf("empty quality gave %d, want %d", + payload.Quality, encurl.DefaultQuality) + } + + _, expiresAt, _, err := buildGeneratePayload(parsed, url.Values{ + widthField: {strconv.Itoa(imgcache.MaxDimension)}, + heightField: {strconv.Itoa(imgcache.MaxDimension)}, + qualityField: {strconv.Itoa(maxQuality)}, + ttlField: {strconv.Itoa(maxTTL)}, + }) + if err != nil { + t.Fatalf("largest accepted values: error = %v", err) + } + + if !expiresAt.After(time.Now()) { + t.Errorf("ttl %d gave expiry %v, want a time in the future", + maxTTL, expiresAt) + } +} diff --git a/internal/handlers/bodylimit.go b/internal/handlers/bodylimit.go new file mode 100644 index 0000000..a2c9adc --- /dev/null +++ b/internal/handlers/bodylimit.go @@ -0,0 +1,45 @@ +package handlers + +import ( + "errors" + "net/http" +) + +// MaxFormBytes bounds the request body accepted on the HTML form POST +// routes (POST / and POST /generate). The forms carry a handful of short +// fields, so 1 MiB is generous while making the bound explicit rather than +// resting on ParseForm's incidental 10 MB cap. +const MaxFormBytes = 1 << 20 // 1 MiB + +// LimitBody returns middleware that caps the request body on POST requests +// at maxBytes and rejects an oversized body with 413 Request Entity Too +// Large. +// +// It parses the form here, before the CSRF middleware reads the token from +// it. The CSRF middleware reads the token with PostFormValue, which +// swallows a parse error, so if the body were only capped there an +// oversized body would read as a missing token and be refused as 403. By +// parsing under the cap first, an oversized body is refused as 413. A +// successful parse is cached on the request, so the CSRF check and the +// handler reuse it rather than reading the body again. +func (s *Handlers) LimitBody(maxBytes int64) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + r.Body = http.MaxBytesReader(w, r.Body, maxBytes) + + err := r.ParseForm() + + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + http.Error(w, "Request body too large", + http.StatusRequestEntityTooLarge) + + return + } + } + + next.ServeHTTP(w, r) + }) + } +} diff --git a/internal/handlers/bodylimit_internal_test.go b/internal/handlers/bodylimit_internal_test.go new file mode 100644 index 0000000..59e5fd7 --- /dev/null +++ b/internal/handlers/bodylimit_internal_test.go @@ -0,0 +1,177 @@ +package handlers + +import ( + "log/slog" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + + "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/session" +) + +// Form field names and a throwaway source image URL for the body-limit +// tests. +const ( + sourceURLField = "url" + testSourceURL = "https://example.com/a.jpg" +) + +// newBodyLimitTestRouter mirrors the production wiring for the form POST +// routes (see server.SetupRoutes): LimitBody sits in front of the CSRF +// middleware, which sits in front of the handlers. maxBytes is the body +// cap under test, so a test can trip the limit with a small body. +func newBodyLimitTestRouter( + t *testing.T, maxBytes int64, +) (*Handlers, http.Handler) { + t.Helper() + + cfg := &config.Config{SigningKey: testSigningKey, Debug: true} + + sessMgr, err := session.NewManager(testSigningKey) + if err != nil { + t.Fatalf("session.NewManager() error = %v", err) + } + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + protect, err := newCSRFProtect(testSigningKey, cfg.Debug) + if err != nil { + t.Fatalf("newCSRFProtect() error = %v", err) + } + + h := &Handlers{ + log: slog.New(slog.DiscardHandler), + config: cfg, + sessMgr: sessMgr, + encGen: encGen, + csrfProtect: protect, + } + + r := chi.NewRouter() + r.Group(func(r chi.Router) { + r.Use(h.LimitBody(maxBytes)) + r.Use(h.CSRF()) + r.Get("/", h.HandleRoot()) + r.Post("/", h.HandleRoot()) + r.Post("/generate", h.HandleGenerateURL()) + }) + + return h, r +} + +// TestOversizedLoginPostRejectedBeforeCSRF is the core regression: an +// oversized POST / carrying an otherwise valid CSRF cookie and token must +// be rejected with 413. If the body limit ran after CSRF, the truncated +// body would read as a missing token and return 403; if it ran after the +// handler, a valid token would return 303. Getting 413 proves the limit +// fires before CSRF parses the form. +func TestOversizedLoginPostRejectedBeforeCSRF(t *testing.T) { + t.Parallel() + + _, srv := newBodyLimitTestRouter(t, 16) + + cookies, token := csrfCredentials(t, srv, nil) + + rec := postForm(srv, "/", cookies, url.Values{ + loginKeyField: {testSigningKey}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Errorf("oversized POST / status = %d, want %d", + rec.Code, http.StatusRequestEntityTooLarge) + } +} + +// TestOversizedGeneratePostRejectedBeforeCSRF is the same regression for +// POST /generate, which also parses a form behind CSRF. +func TestOversizedGeneratePostRejectedBeforeCSRF(t *testing.T) { + t.Parallel() + + h, srv := newBodyLimitTestRouter(t, 16) + + sessionCookie := newSessionCookie(t, h) + + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + rec := postForm(srv, "/generate", cookies, url.Values{ + sourceURLField: {testSourceURL}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Errorf("oversized POST /generate status = %d, want %d", + rec.Code, http.StatusRequestEntityTooLarge) + } +} + +// TestWithinLimitLoginPostSucceeds verifies the limit does not disturb a +// normal request: under the production cap, a valid login still parses and +// establishes a session (303). This guards against the body limit +// consuming or corrupting the form the CSRF check and handler depend on. +func TestWithinLimitLoginPostSucceeds(t *testing.T) { + t.Parallel() + + _, srv := newBodyLimitTestRouter(t, MaxFormBytes) + + cookies, token := csrfCredentials(t, srv, nil) + + rec := postForm(srv, "/", cookies, url.Values{ + loginKeyField: {testSigningKey}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusSeeOther { + t.Fatalf("within-limit POST / status = %d, want %d", + rec.Code, http.StatusSeeOther) + } + + var authed bool + + for _, c := range rec.Result().Cookies() { + if c.Name == session.CookieName && c.Value != "" { + authed = true + } + } + + if !authed { + t.Error("within-limit valid login did not set a session cookie") + } +} + +// TestWithinLimitGeneratePostSucceeds is the same non-regression check for +// POST /generate. +func TestWithinLimitGeneratePostSucceeds(t *testing.T) { + t.Parallel() + + h, srv := newBodyLimitTestRouter(t, MaxFormBytes) + + sessionCookie := newSessionCookie(t, h) + + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + rec := postForm(srv, "/generate", cookies, url.Values{ + sourceURLField: {testSourceURL}, + "format": {"jpeg"}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusOK { + t.Fatalf("within-limit POST /generate status = %d, want %d", + rec.Code, http.StatusOK) + } + + if !strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("within-limit generate response did not contain a generated URL") + } +} diff --git a/internal/handlers/csrf.go b/internal/handlers/csrf.go new file mode 100644 index 0000000..166abdc --- /dev/null +++ b/internal/handlers/csrf.go @@ -0,0 +1,65 @@ +package handlers + +import ( + "html/template" + "net/http" + + "github.com/gorilla/csrf" + + "sneak.berlin/go/pixa/internal/seal" +) + +// csrfKeySalt provides domain separation for the CSRF authentication key, +// derived from the signing key so tokens survive restarts without extra +// configuration and never reuse the session or encrypted-URL key material. +const csrfKeySalt = "pixa-csrf-v1" + +// newCSRFProtect builds the CSRF-protection middleware for the +// state-mutating HTML form routes. The token lives in its own cookie, +// independent of the session cookie, so it also protects the login POST +// where no session exists yet (login CSRF). +// +// When plaintext is true (local HTTP development), requests are marked +// plaintext so the library neither demands an https Referer nor sets a +// Secure cookie the browser would withhold over http. In production the +// service runs behind a TLS-terminating proxy, so plaintext is false and +// the library enforces its https Referer origin check. +func newCSRFProtect( + signingKey string, plaintext bool, +) (func(http.Handler) http.Handler, error) { + key, err := seal.DeriveKey([]byte(signingKey), csrfKeySalt) + if err != nil { + return nil, err + } + + protect := csrf.Protect( + key[:], + csrf.Path("/"), + csrf.Secure(!plaintext), + csrf.SameSite(csrf.SameSiteStrictMode), + ) + + if !plaintext { + return protect, nil + } + + return func(next http.Handler) http.Handler { + protected := protect(next) + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + protected.ServeHTTP(w, csrf.PlaintextHTTPRequest(r)) + }) + }, nil +} + +// CSRF returns the CSRF-protection middleware for the login and generator +// form routes. +func (s *Handlers) CSRF() func(http.Handler) http.Handler { + return s.csrfProtect +} + +// csrfField returns the hidden form input carrying the CSRF token for the +// given request, to be embedded verbatim in a rendered form. +func csrfField(r *http.Request) template.HTML { + return csrf.TemplateField(r) +} diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 0398357..09f3ffc 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -31,23 +31,30 @@ type Params struct { // Handlers provides HTTP request handlers. type Handlers struct { - log *slog.Logger - hc *healthcheck.Healthcheck - db *database.Database - config *config.Config - imgSvc *imgcache.Service - imgCache *imgcache.Cache - sessMgr *session.Manager - encGen *encurl.Generator + log *slog.Logger + hc *healthcheck.Healthcheck + db *database.Database + config *config.Config + imgSvc *imgcache.Service + imgCache *imgcache.Cache + sessMgr *session.Manager + encGen *encurl.Generator + csrfProtect func(http.Handler) http.Handler } // New creates a new Handlers instance. func New(lc fx.Lifecycle, params Params) (*Handlers, error) { + csrfProtect, err := newCSRFProtect(params.Config.SigningKey, params.Config.Debug) + if err != nil { + return nil, err + } + s := &Handlers{ - log: params.Logger.Get(), - hc: params.Healthcheck, - db: params.Database, - config: params.Config, + log: params.Logger.Get(), + hc: params.Healthcheck, + db: params.Database, + config: params.Config, + csrfProtect: csrfProtect, } lc.Append(fx.Hook{ @@ -104,6 +111,8 @@ func (s *Handlers) initImageService() error { fetcherCfg.MaxConnectionsPerHost = s.config.UpstreamConnectionsPerHost } + fetcherCfg.BlockedNetworks = s.config.BlockedNetworks + // Create the service svc, err := imgcache.NewService(&imgcache.ServiceConfig{ Cache: cache, diff --git a/internal/handlers/handlers_internal_test.go b/internal/handlers/handlers_internal_test.go index 77bf959..35c1522 100644 --- a/internal/handlers/handlers_internal_test.go +++ b/internal/handlers/handlers_internal_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "database/sql" + "encoding/json" "image" "image/color" "image/jpeg" @@ -276,3 +277,120 @@ func TestHandleImage_ETagHeader(t *testing.T) { t.Errorf("ETag should be quoted, got %q", etag) } } + +// TestHandleImage_InvalidFitMode_Returns400 verifies that the plain image +// route rejects an unrecognized fit mode with 400. +func TestHandleImage_InvalidFitMode_Returns400(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + status := getImage(t, fix, + "/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg?fit=bogus") + + if status != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", status, http.StatusBadRequest) + } +} + +// TestHandleImage_InvalidQuery_Returns400 verifies that the plain image route +// answers a q that is not a whole number from 1 to 100, an empty one +// included, with 400 naming q and the value, a parameter given more than once +// with 400 naming it, and a query string that cannot be decoded with 400 +// showing it, instead of serving the image at the default quality 85 or at +// the first value given. +func TestHandleImage_InvalidQuery_Returns400(t *testing.T) { + t.Parallel() + + tests := []struct { + query, wantError string + }{ + {"q=banana", `invalid q: not a number, got "banana"`}, + {"q=0", `invalid q: must be from 1 to 100, got "0"`}, + {"q=101", `invalid q: must be from 1 to 100, got "101"`}, + {"q=", `invalid q: not a number, got ""`}, + {"q=80&q=500", `invalid q: given more than once`}, + {"q=80&q=", `invalid q: given more than once`}, + {"fit=cover&fit=contain", `invalid fit: given more than once`}, + {"q=80%", `invalid query string "q=80%": invalid URL escape "%"`}, + { + "q=50;fit=contain", + `invalid query string "q=50;fit=contain": ` + + `invalid semicolon separator in query`, + }, + } + + for _, tt := range tests { + t.Run(tt.query, func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + r := chi.NewRouter() + r.Get("/v1/image/*", fix.handler.HandleImage()) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg?"+tt.query, nil) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body) + + var body struct { + Error string `json:"error"` + } + + err := json.NewDecoder(rec.Body).Decode(&body) + if err != nil { + t.Fatalf("decoding response body: %v", err) + } + + if body.Error != tt.wantError { + t.Errorf("error = %q, want %q", body.Error, tt.wantError) + } + }) + } +} + +// TestHandleImage_EmptyFit_Returns400 verifies that the plain image route +// answers a fit that is in the URL but empty with 400 naming fit, instead of +// serving the image as cover. Only a fit missing from the URL means cover. +func TestHandleImage_EmptyFit_Returns400(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + r := chi.NewRouter() + r.Get("/v1/image/*", fix.handler.HandleImage()) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg?fit=", nil) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body) + + var body struct { + Error string `json:"error"` + } + + err := json.NewDecoder(rec.Body).Decode(&body) + if err != nil { + t.Fatalf("decoding response body: %v", err) + } + + wantError := `invalid fit: not a fit mode, got ""` + if body.Error != wantError { + t.Errorf("error = %q, want %q", body.Error, wantError) + } +} diff --git a/internal/handlers/image.go b/internal/handlers/image.go index c3de6b5..00b271e 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -2,12 +2,15 @@ package handlers import ( "errors" + "fmt" "io" "net/http" + "net/url" "strconv" "time" "github.com/go-chi/chi/v5" + "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imgcache" ) @@ -89,39 +92,66 @@ func (s *Handlers) parseImageRequest( // Convert to ImageRequest req := parsed.ToImageRequest() - // Parse signature params from query string - query := r.URL.Query() - req.Signature = query.Get("sig") + // Parse signature params from query string. r.URL.Query() would silently + // drop a pair it cannot decode, such as q=80%, so that q would be served + // at 85; a query string that cannot be decoded is refused instead. A + // parameter given more than once is refused too, as only its first value + // would be read. + query, err := url.ParseQuery(r.URL.RawQuery) + if err != nil { + s.respondError(w, fmt.Sprintf("invalid query string %q: %v", + r.URL.RawQuery, err), http.StatusBadRequest) - if expStr := query.Get("exp"); expStr != "" { - exp, parseErr := strconv.ParseInt(expStr, 10, 64) - if parseErr == nil { - req.Expires = time.Unix(exp, 0) - } + return nil, false } - // Parse optional quality and fit params - if qStr := query.Get("q"); qStr != "" { - q, parseErr := strconv.Atoi(qStr) - if parseErr == nil && q > 0 && q <= 100 { - req.Quality = q - } - } - - if fit := query.Get("fit"); fit != "" { - req.FitMode = imgcache.FitMode(fit) - - fitErr := imgcache.ValidateFitMode(req.FitMode) - if fitErr != nil { - s.respondError(w, "invalid fit mode: "+fit, http.StatusBadRequest) + for name, values := range query { + if len(values) > 1 { + s.respondError(w, fmt.Sprintf("invalid %s: given more than once", + name), http.StatusBadRequest) return nil, false } } - // Default quality if not set - if req.Quality == 0 { - req.Quality = 85 + req.Signature = query.Get("sig") + + req.Expires, err = parseExpires(query) + if err != nil { + s.respondError(w, err.Error(), http.StatusBadRequest) + + return nil, false + } + + // Parse optional quality and fit params. Only a q missing from the URL is + // 85. A q in the URL that is not a whole number from 1 to 100, an empty + // one included, is refused, checked as the generator checks its quality + // field; that check alone would take an empty q as missing. + qStr := query.Get("q") + if query.Has("q") && qStr == "" { + s.respondError(w, `invalid q: not a number, got ""`, + http.StatusBadRequest) + + return nil, false + } + + req.Quality, err = parseFormInt(query, "q", + encurl.DefaultQuality, minQuality, maxQuality) + if err != nil { + s.respondError(w, fmt.Sprintf("%v, got %q", err, qStr), + http.StatusBadRequest) + + return nil, false + } + + // Only a fit missing from the URL is cover. A fit in the URL that is not a + // fit mode is refused by the fit-mode check below; that check would take an + // empty fit as missing, so an empty one is refused here. + req.FitMode = imgcache.FitMode(query.Get("fit")) + if query.Has("fit") && req.FitMode == "" { + s.respondError(w, `invalid fit: not a fit mode, got ""`, http.StatusBadRequest) + + return nil, false } // Default fit mode if not set @@ -129,9 +159,41 @@ func (s *Handlers) parseImageRequest( req.FitMode = imgcache.FitCover } + // Enforce dimension and fit-mode bounds, shared with the encrypted-URL + // route. Dimensions are already bounded by the path parser above; this + // also rejects an unrecognized fit mode with 400 instead of letting it + // reach the processor as a 500. + err = imgcache.ValidateImageRequest(req) + if err != nil { + s.respondError(w, "invalid image request: "+err.Error(), + http.StatusBadRequest) + + return nil, false + } + return req, true } +// parseExpires reads the exp query parameter, a Unix time in seconds. An exp +// missing from the URL gives the zero time, which the signature check takes +// as no expiration. An exp in the URL that is not a whole number, an empty +// one included, is an error naming exp and the value. +func parseExpires(query url.Values) (time.Time, error) { + if !query.Has("exp") { + return time.Time{}, nil + } + + expStr := query.Get("exp") + + exp, err := strconv.ParseInt(expStr, 10, 64) + if err != nil { + return time.Time{}, fmt.Errorf("%w exp: not a number, got %q", + errInvalidFormField, expStr) + } + + return time.Unix(exp, 0), nil +} + // respondImageError maps image retrieval errors to HTTP responses. func (s *Handlers) respondImageError( w http.ResponseWriter, req *imgcache.ImageRequest, err error, diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go new file mode 100644 index 0000000..c435668 --- /dev/null +++ b/internal/handlers/image_signature_internal_test.go @@ -0,0 +1,171 @@ +package handlers + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-chi/chi/v5" + "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/signature" +) + +// signedHost is not on the allowlist setupTestHandler builds, so a request +// for it needs a valid signature. No image is served for it: a request that +// passes the signature check gets 502 from the failed fetch, and one that +// fails the check gets 401. +const signedHost = "signed.example.com" + +// getImage sends a GET for target to the image route of fix and returns the +// response status. +func getImage(t *testing.T, fix *testFixtures, target string) int { + t.Helper() + + r := chi.NewRouter() + r.Get("/v1/image/*", fix.handler.HandleImage()) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + t.Logf("GET %s: %d", target, rec.Code) + + return rec.Code +} + +// TestHandleImage_SignatureCoversQualityAndFit signs a URL for quality 85 +// and fit cover, the values the route uses when a URL has no q or fit, and +// sends that signature with each q and fit below. +func TestHandleImage_SignatureCoversQualityAndFit(t *testing.T) { + t.Parallel() + + expires := time.Now().Add(time.Hour) + signer := signature.New("test-signing-key-must-be-32-chars") + sig := signer.Sign(&signature.Request{ + SourceHost: signedHost, + SourcePath: "/images/photo.jpg", + Width: 50, + Height: 50, + Format: string(imgcache.FormatJPEG), + Quality: 85, + FitMode: string(imgcache.FitCover), + Expires: expires, + }) + signedURL := fmt.Sprintf("/v1/image/%s/images/photo.jpg/50x50.jpeg?sig=%s&exp=%d", + signedHost, sig, expires.Unix()) + + tests := []struct { + name string + query string + wantStatus int + }{ + {"no q or fit", "", http.StatusBadGateway}, + {"q=85 and fit=cover", "&q=85&fit=cover", http.StatusBadGateway}, + {"replayed with q=40", "&q=40", http.StatusUnauthorized}, + {"replayed with fit=contain", "&fit=contain", http.StatusUnauthorized}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + status := getImage(t, setupTestHandler(t), signedURL+tt.query) + if status != tt.wantStatus { + t.Errorf("status = %d, want %d", status, tt.wantStatus) + } + }) + } +} + +// TestHandleImage_GeneratedSignedURLVerifies sends URLs built by the +// service's signed-URL generator to the route. +func TestHandleImage_GeneratedSignedURLVerifies(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + quality int + fitMode imgcache.FitMode + }{ + {"quality 40 and fit contain", 40, imgcache.FitContain}, + {"quality and fit unset", 0, ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + signedURL, err := fix.service.GenerateSignedURL("", &imgcache.ImageRequest{ + SourceHost: signedHost, + SourcePath: "/images/photo.jpg", + Size: imgcache.Size{Width: 50, Height: 50}, + Format: imgcache.FormatJPEG, + Quality: tt.quality, + FitMode: tt.fitMode, + }, time.Hour) + if err != nil { + t.Fatalf("GenerateSignedURL() error = %v", err) + } + + status := getImage(t, fix, signedURL) + if status != http.StatusBadGateway { + t.Errorf("status = %d, want %d", status, http.StatusBadGateway) + } + }) + } +} + +// TestHandleImage_InvalidExp_Returns400 sends a signed-host URL whose exp is +// not a whole number, and one whose exp is empty. Each is refused with 400 +// naming exp and the value, not with the 401 a URL without exp still gets. +func TestHandleImage_InvalidExp_Returns400(t *testing.T) { + t.Parallel() + + tests := []struct { + query string + wantStatus int + wantError string + }{ + {"sig=x&exp=banana", http.StatusBadRequest, + `invalid exp: not a number, got "banana"`}, + {"sig=x&exp=", http.StatusBadRequest, `invalid exp: not a number, got ""`}, + {"sig=x", http.StatusUnauthorized, "unauthorized"}, + } + + for _, tt := range tests { + t.Run(tt.query, func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + r := chi.NewRouter() + r.Get("/v1/image/*", fix.handler.HandleImage()) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "/v1/image/"+signedHost+"/images/photo.jpg/50x50.jpeg?"+tt.query, nil) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body) + + var body struct { + Error string `json:"error"` + } + + err := json.NewDecoder(rec.Body).Decode(&body) + if err != nil { + t.Fatalf("decoding response body: %v", err) + } + + if rec.Code != tt.wantStatus || body.Error != tt.wantError { + t.Errorf("got %d %q, want %d %q", + rec.Code, body.Error, tt.wantStatus, tt.wantError) + } + }) + } +} diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index e01b2cd..8089f64 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -50,6 +50,19 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { // Convert payload to ImageRequest req := payload.ToImageRequest() + // Apply the same dimension and fit-mode bounds as the plain image + // route: a sealed payload is trusted for its origin, not for staying + // within limits, so an over-limit size or unknown fit mode is a 400 + // here rather than an out-of-memory or a 500 from the processor. + err = imgcache.ValidateImageRequest(req) + if err != nil { + s.log.Debug("encrypted URL failed validation", "error", err) + s.respondError(w, "invalid encrypted URL: "+err.Error(), + http.StatusBadRequest) + + return + } + // Log the request s.log.Debug("encrypted image request", "host", req.SourceHost, diff --git a/internal/handlers/imageenc_internal_test.go b/internal/handlers/imageenc_internal_test.go new file mode 100644 index 0000000..be2db22 --- /dev/null +++ b/internal/handlers/imageenc_internal_test.go @@ -0,0 +1,98 @@ +package handlers + +import ( + "context" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// newEncTestServer builds a router serving the encrypted-URL route with a +// generator seeded by the shared test signing key. The image service is left +// nil: these tests exercise validation that rejects a token before any image +// is fetched, so the handler must never reach the service. +func newEncTestServer(t *testing.T) (*encurl.Generator, http.Handler) { + t.Helper() + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + h := &Handlers{ + log: slog.New(slog.DiscardHandler), + encGen: encGen, + } + + r := chi.NewRouter() + r.Get("/v1/e/{token}/*", h.HandleImageEnc()) + + return encGen, r +} + +// getEncToken issues a GET for the given token and returns the recorder. +func getEncToken(srv http.Handler, token string) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/v1/e/"+token+"/img.jpg", nil) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + return rec +} + +// TestHandleImageEnc_OverLimitDimension_Returns400 verifies that a decrypted +// token requesting a dimension beyond MaxDimension is rejected with 400 +// instead of reaching the image processor and libvips. +func TestHandleImageEnc_OverLimitDimension_Returns400(t *testing.T) { + t.Parallel() + + encGen, srv := newEncTestServer(t) + + token, err := encGen.Generate(&encurl.Payload{ + SourceHost: "cdn.example.com", + SourcePath: "/photo.jpg", + Width: 100000, + Height: 100000, + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + rec := getEncToken(srv, token) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } +} + +// TestHandleImageEnc_InvalidFitMode_Returns400 verifies that a decrypted token +// carrying an unrecognized fit mode is rejected with 400 rather than surfacing +// as a 500 from the image processor's default branch. +func TestHandleImageEnc_InvalidFitMode_Returns400(t *testing.T) { + t.Parallel() + + encGen, srv := newEncTestServer(t) + + token, err := encGen.Generate(&encurl.Payload{ + SourceHost: "cdn.example.com", + SourcePath: "/photo.jpg", + Width: 800, + Height: 600, + FitMode: imgcache.FitMode("bogus"), + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + rec := getEncToken(srv, token) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } +} diff --git a/internal/httpfetcher/blocked_networks_internal_test.go b/internal/httpfetcher/blocked_networks_internal_test.go new file mode 100644 index 0000000..dfbf8f5 --- /dev/null +++ b/internal/httpfetcher/blocked_networks_internal_test.go @@ -0,0 +1,111 @@ +package httpfetcher + +import ( + "context" + "errors" + "net" + "net/http" + "net/netip" + "testing" +) + +// TestIsPrivateIPBlocksSpecialRanges covers the internal and special-use +// ranges added to the built-in blocklist, in IPv4, IPv6, and IPv4-mapped +// forms, alongside public controls that must stay reachable. +func TestIsPrivateIPBlocksSpecialRanges(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ip string + want bool + }{ + {"cgnat-low", "100.64.0.1", true}, + {"cgnat-high", "100.127.255.254", true}, + {"ietf-protocol", "192.0.0.1", true}, + {"benchmark-low", "198.18.0.1", true}, + {"benchmark-high", "198.19.255.254", true}, + {"nat64", "64:ff9b::1", true}, + {"nat64-embeds-private", "64:ff9b::a00:1", true}, // maps 10.0.0.1 + {"ipv4-mapped-private", "::ffff:10.0.0.1", true}, + {"cloud-metadata", "169.254.169.254", true}, + {"public-v4", "8.8.8.8", false}, + {"test-net-1-public", testPublicHost, false}, // TEST-NET-1, stays public + {"public-v6", "2001:4860:4860::8888", false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ip := net.ParseIP(tc.ip) + if ip == nil { + t.Fatalf("failed to parse IP %q", tc.ip) + } + + got := isPrivateIP(ip) + if got != tc.want { + t.Errorf("isPrivateIP(%q) = %v, want %v", tc.ip, got, tc.want) + } + }) + } +} + +// transportOf returns the *http.Transport backing a fetcher, so a test can +// exercise the SSRF-safe dialer New installed with the operator blocklist. +func transportOf(t *testing.T, f *HTTPFetcher) *http.Transport { + t.Helper() + + transport, ok := f.client.Transport.(*http.Transport) + if !ok { + t.Fatalf("transport is %T, want *http.Transport", f.client.Transport) + } + + return transport +} + +// TestDialerEnforcesBlockedNetworks proves an operator-supplied +// blocked_networks entry is enforced by the dialer, in addition to the +// built-in ranges, while an address outside both stays dialable. +func TestDialerEnforcesBlockedNetworks(t *testing.T) { + t.Parallel() + + cfg := DefaultConfig() + // TEST-NET-2 (198.51.100.0/24) is public to the built-in check, so + // blocking it can only come from the operator-supplied list. + cfg.BlockedNetworks = []netip.Prefix{netip.MustParsePrefix("198.51.100.0/24")} + + transport := transportOf(t, New(cfg)) + + blocked := []string{ + "198.51.100.5:80", // operator-supplied range + "10.0.0.5:80", // built-in RFC 1918, still enforced + "100.64.0.1:80", // built-in CGNAT range + } + + for _, addr := range blocked { + t.Run("blocked/"+addr, func(t *testing.T) { + t.Parallel() + + _, err := transport.DialContext(context.Background(), "tcp", addr) + if !errors.Is(err, ErrSSRFBlocked) { + t.Errorf("DialContext(%q) = %v, want ErrSSRFBlocked", addr, err) + } + }) + } + + t.Run("public-not-blocked", func(t *testing.T) { + t.Parallel() + + // A cancelled context makes the dial fail without touching the + // network; the point is only that a public literal outside every + // blocked range is not SSRF-blocked. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := transport.DialContext(ctx, "tcp", testPublicHost+":80") + if errors.Is(err, ErrSSRFBlocked) { + t.Errorf("public target SSRF-blocked with operator list set: %v", err) + } + }) +} diff --git a/internal/httpfetcher/fetch_internal_test.go b/internal/httpfetcher/fetch_internal_test.go new file mode 100644 index 0000000..c0d6e9d --- /dev/null +++ b/internal/httpfetcher/fetch_internal_test.go @@ -0,0 +1,421 @@ +package httpfetcher + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "slices" + "strings" + "sync" + "testing" + "time" +) + +// testPublicHost is a TEST-NET-1 (RFC 5737) literal. isPrivateIP treats it as +// public, so validateURL and the redirect check accept it with no DNS lookup, +// while the recording dialer routes it to the local httptest server. The +// address is reserved for documentation and is never routed on the network. +const testPublicHost = "192.0.2.10" + +// imagePayload is the body served by the fake upstream's image route. +const imagePayload = "fake-jpeg-bytes" + +// errUnexpectedDial reports a dial to any host other than testPublicHost, which +// would mean SSRF protection let a forbidden target reach the transport. +var errUnexpectedDial = errors.New("unexpected dial target") + +// upstreamURL builds a fetch URL on the fake public host for the given path. +func upstreamURL(path string) string { + return "http://" + testPublicHost + path +} + +// recordingDialer records every address the transport asks it to dial and +// routes connections for testPublicHost to a real local server, so the SSRF +// checks run against a public-looking host while bytes go to httptest. +type recordingDialer struct { + target string + + mu sync.Mutex + dialed []string +} + +func (d *recordingDialer) dialContext( + ctx context.Context, + network, addr string, +) (net.Conn, error) { + d.mu.Lock() + d.dialed = append(d.dialed, addr) + d.mu.Unlock() + + host, _, err := net.SplitHostPort(addr) + if err != nil { + return nil, err + } + + if host != testPublicHost { + return nil, fmt.Errorf("%w: %s", errUnexpectedDial, addr) + } + + var dialer net.Dialer + + return dialer.DialContext(ctx, network, d.target) +} + +// dialedAddrs returns a copy of the addresses the dialer was asked to reach. +func (d *recordingDialer) dialedAddrs() []string { + d.mu.Lock() + defer d.mu.Unlock() + + return slices.Clone(d.dialed) +} + +// startUpstream launches a fake upstream with the routes the fetch tests +// exercise and stops it when the test finishes. +func startUpstream(t *testing.T) *httptest.Server { + t.Helper() + + mux := http.NewServeMux() + mux.HandleFunc("/image", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", contentTypeJPEG) + _, _ = io.WriteString(w, imagePayload) + }) + mux.HandleFunc("/status/500", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + mux.HandleFunc("/html", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = io.WriteString(w, "") + }) + mux.HandleFunc("/redirect/private", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound) + }) + mux.HandleFunc("/redirect/public", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/image", http.StatusFound) + }) + mux.HandleFunc("/redirect/chain", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/redirect/hop", http.StatusFound) + }) + mux.HandleFunc("/redirect/hop", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/image", http.StatusFound) + }) + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + return srv +} + +// newServerFetcher builds a fetcher whose transport routes testPublicHost to +// srv, leaving the real SSRF validation and redirect checks in place. +func newServerFetcher( + t *testing.T, + srv *httptest.Server, + cfg *Config, +) (*HTTPFetcher, *recordingDialer) { + t.Helper() + + if cfg == nil { + cfg = DefaultConfig() + } + + cfg.AllowHTTP = true + + f := New(cfg) + + transport, ok := f.client.Transport.(*http.Transport) + if !ok { + t.Fatalf("transport is %T, want *http.Transport", f.client.Transport) + } + + dialer := &recordingDialer{target: srv.Listener.Addr().String()} + transport.DialContext = dialer.dialContext + + return f, dialer +} + +// testContext returns a context cancelled when the test ends, bounding any +// fetch that would otherwise block on a leaked semaphore slot. +func testContext(t *testing.T) context.Context { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + + return ctx +} + +// fetchImage fetches path from the fake upstream and fails on error. +func fetchImage(t *testing.T, f *HTTPFetcher, path string) *FetchResult { + t.Helper() + + res, err := f.Fetch(testContext(t), upstreamURL(path)) + if err != nil { + t.Fatalf("Fetch(%s) error = %v", path, err) + } + + return res +} + +// fetchExpectError fetches path and fails unless Fetch returns an error. +func fetchExpectError(t *testing.T, f *HTTPFetcher, path string) error { + t.Helper() + + res, err := f.Fetch(testContext(t), upstreamURL(path)) + if err == nil { + _ = res.Content.Close() + + t.Fatalf("Fetch(%s) = nil error, want an error", path) + } + + return err +} + +// fetchBody fetches path and returns the fully read, closed response body. +func fetchBody(t *testing.T, f *HTTPFetcher, path string) string { + t.Helper() + + res := fetchImage(t, f, path) + defer func() { _ = res.Content.Close() }() + + data, err := io.ReadAll(res.Content) + if err != nil { + t.Fatalf("read body: %v", err) + } + + return string(data) +} + +// semLen reports how many per-host semaphore slots are currently held. +func semLen(f *HTTPFetcher, host string) int { + return len(f.getHostSemaphore(host)) +} + +func TestFetchRedirectToPrivateIPBlocked(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + f, dialer := newServerFetcher(t, srv, nil) + + _, err := f.Fetch(testContext(t), upstreamURL("/redirect/private")) + if !errors.Is(err, ErrSSRFBlocked) { + t.Fatalf("Fetch() error = %v, want ErrSSRFBlocked", err) + } + + for _, addr := range dialer.dialedAddrs() { + if strings.Contains(addr, "169.254.169.254") { + t.Errorf("dialer connected to the private redirect target: %s", addr) + } + } +} + +func TestFetchRedirectToPublicSucceeds(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + f, _ := newServerFetcher(t, srv, nil) + + if body := fetchBody(t, f, "/redirect/public"); body != imagePayload { + t.Errorf("body = %q, want %q", body, imagePayload) + } +} + +func TestFetchRedirectChainSucceeds(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + f, _ := newServerFetcher(t, srv, nil) + + if body := fetchBody(t, f, "/redirect/chain"); body != imagePayload { + t.Errorf("body = %q, want %q", body, imagePayload) + } +} + +func TestFetchRejectsNon2xx(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + f, _ := newServerFetcher(t, srv, nil) + + err := fetchExpectError(t, f, "/status/500") + if !errors.Is(err, ErrUpstreamError) { + t.Fatalf("Fetch() error = %v, want ErrUpstreamError", err) + } +} + +func TestFetchRejectsDisallowedContentType(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + f, _ := newServerFetcher(t, srv, nil) + + err := fetchExpectError(t, f, "/html") + if !errors.Is(err, ErrInvalidContentType) { + t.Fatalf("Fetch() error = %v, want ErrInvalidContentType", err) + } +} + +func TestFetchMaxResponseSizeEnforced(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + + cfg := DefaultConfig() + cfg.MaxResponseSize = 8 + + f, _ := newServerFetcher(t, srv, cfg) + + res := fetchImage(t, f, "/image") + defer func() { _ = res.Content.Close() }() + + data, err := io.ReadAll(res.Content) + if !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("read error = %v, want ErrResponseTooLarge", err) + } + + if int64(len(data)) > cfg.MaxResponseSize { + t.Errorf("read %d bytes, exceeds limit %d", len(data), cfg.MaxResponseSize) + } +} + +func TestFetchSemaphoreReleasedOnError(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + + cfg := DefaultConfig() + cfg.MaxConnectionsPerHost = 1 + + f, _ := newServerFetcher(t, srv, cfg) + + err := fetchExpectError(t, f, "/status/500") + if !errors.Is(err, ErrUpstreamError) { + t.Fatalf("Fetch() error = %v, want ErrUpstreamError", err) + } + + if held := semLen(f, testPublicHost); held != 0 { + t.Fatalf("semaphore slot leaked after error: %d held", held) + } + + // One slot per host: this fetch proceeds only if the slot was released. + res := fetchImage(t, f, "/image") + _ = res.Content.Close() +} + +// assertSlotReleasedByClose fetches an image over a one-slot host, hands the +// open result to consume, and asserts the slot is held before and freed after, +// then that a follow-up fetch can still acquire it. +func assertSlotReleasedByClose( + t *testing.T, + consume func(*testing.T, *FetchResult), +) { + t.Helper() + + srv := startUpstream(t) + + cfg := DefaultConfig() + cfg.MaxConnectionsPerHost = 1 + + f, _ := newServerFetcher(t, srv, cfg) + + res := fetchImage(t, f, "/image") + if held := semLen(f, testPublicHost); held != 1 { + t.Fatalf("slot not held while body is open: %d held", held) + } + + consume(t, res) + + if held := semLen(f, testPublicHost); held != 0 { + t.Fatalf("slot not released after close: %d held", held) + } + + next := fetchImage(t, f, "/image") + _ = next.Content.Close() +} + +func TestFetchSemaphoreReleasedOnBodyClose(t *testing.T) { + t.Parallel() + + assertSlotReleasedByClose(t, func(t *testing.T, res *FetchResult) { + t.Helper() + + _, err := io.ReadAll(res.Content) + if err != nil { + t.Fatalf("read body: %v", err) + } + + err = res.Content.Close() + if err != nil { + t.Fatalf("close body: %v", err) + } + }) +} + +func TestFetchSemaphoreReleasedOnPartialReadClose(t *testing.T) { + t.Parallel() + + assertSlotReleasedByClose(t, func(t *testing.T, res *FetchResult) { + t.Helper() + + buf := make([]byte, 1) + + _, err := res.Content.Read(buf) + if err != nil { + t.Fatalf("partial read: %v", err) + } + + err = res.Content.Close() + if err != nil { + t.Fatalf("close body: %v", err) + } + }) +} + +// The dial-time re-resolution in ssrfSafeDialer is what closes the DNS +// rebinding window: even if validateURL saw a public answer earlier, the +// dialer independently re-checks the address it is about to connect to. A full +// rebinding simulation (a resolver returning public, then private) would mean +// replacing the global net.DefaultResolver with a fake DNS server, which is +// heavyweight and unsafe to mutate under parallel -race tests. The property is +// proven directly here instead: the dialer rejects a private target outright, +// which is exactly the check that fires when a validated host later resolves +// to a private address. +func TestSSRFSafeDialerBlocksPrivateTarget(t *testing.T) { + t.Parallel() + + for _, addr := range []string{ + "169.254.169.254:80", // link-local (cloud metadata) + "127.0.0.1:80", // loopback + "10.0.0.5:80", // RFC 1918 private + } { + t.Run(addr, func(t *testing.T) { + t.Parallel() + + _, err := ssrfSafeDialer(context.Background(), "tcp", addr) + if !errors.Is(err, ErrSSRFBlocked) { + t.Errorf("ssrfSafeDialer(%q) = %v, want ErrSSRFBlocked", addr, err) + } + }) + } +} + +func TestSSRFSafeDialerAllowsPublicTarget(t *testing.T) { + t.Parallel() + + // A cancelled context makes the dial fail immediately without touching the + // network; the point is only that a public literal is not SSRF-blocked. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := ssrfSafeDialer(ctx, "tcp", testPublicHost+":80") + if err == nil { + t.Fatal("expected a dial error for an unreachable public target") + } + + if errors.Is(err, ErrSSRFBlocked) { + t.Errorf("public target was SSRF-blocked: %v", err) + } +} diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 0abf1fa..64e27eb 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -11,6 +11,7 @@ import ( "net" "net/http" "net/http/httptrace" + "net/netip" neturl "net/url" "slices" "strings" @@ -46,6 +47,20 @@ const ( localhostIPv6 = "::1" ) +// builtinBlockedPrefixes are internal or special-use ranges that Go's +// net.IP predicates (IsPrivate, IsLinkLocalUnicast, and the like) do not +// already cover. They are always blocked, in addition to any +// operator-supplied networks. IPv4-mapped IPv6 addresses are unmapped +// before matching, so these IPv4 ranges are caught in both forms. +// +//nolint:gochecknoglobals // immutable built-in blocklist +var builtinBlockedPrefixes = []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/10"), // RFC 6598 CGNAT / carrier-grade NAT + netip.MustParsePrefix("192.0.0.0/24"), // RFC 6890 IETF protocol assignments + netip.MustParsePrefix("198.18.0.0/15"), // RFC 2544 benchmarking range + netip.MustParsePrefix("64:ff9b::/96"), // RFC 6052 NAT64 (maps onto IPv4) +} + // Fetcher errors. var ( ErrSSRFBlocked = errors.New("request blocked: private or internal IP") @@ -107,6 +122,9 @@ type Config struct { AllowHTTP bool // MaxConnectionsPerHost limits concurrent connections to each upstream host. MaxConnectionsPerHost int + // BlockedNetworks are operator-supplied CIDR ranges refused by the + // dialer, in addition to the always-enforced built-in ranges. + BlockedNetworks []netip.Prefix } // DefaultConfig returns a Config with sensible defaults. @@ -142,9 +160,13 @@ func New(config *Config) *HTTPFetcher { config = DefaultConfig() } - // Create transport with SSRF-safe dialer + // Create transport with SSRF-safe dialer. The dialer re-resolves and + // re-checks at connect time (closing the DNS-rebinding window) against + // both the built-in ranges and the operator-supplied blocklist. transport := &http.Transport{ - DialContext: ssrfSafeDialer, + DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + return dialSSRFSafe(ctx, network, addr, config.BlockedNetworks) + }, TLSHandshakeTimeout: DefaultTLSTimeout, MaxIdleConns: DefaultMaxIdleConns, IdleConnTimeout: DefaultIdleConnTimeout, @@ -451,11 +473,53 @@ func isPrivateIP(ip net.IP) bool { } } - return false + // Special-use ranges the net.IP predicates above do not cover. + addr, ok := netip.AddrFromSlice(ip) + if !ok { + return true + } + + addr = addr.Unmap() + + return slices.ContainsFunc(builtinBlockedPrefixes, func(prefix netip.Prefix) bool { + return prefix.Contains(addr) + }) } -// ssrfSafeDialer is a custom dialer that validates IP addresses before connecting. +// isBlockedIP reports whether ip is refused, either by the built-in +// internal-range check or by one of the operator-supplied prefixes. +func isBlockedIP(ip net.IP, blocked []netip.Prefix) bool { + if isPrivateIP(ip) { + return true + } + + addr, ok := netip.AddrFromSlice(ip) + if !ok { + return true + } + + addr = addr.Unmap() + + return slices.ContainsFunc(blocked, func(prefix netip.Prefix) bool { + return prefix.Contains(addr) + }) +} + +// ssrfSafeDialer validates IP addresses against the built-in blocked ranges +// before connecting. New wraps dialSSRFSafe with the operator-supplied +// blocklist; this entry point enforces the built-in ranges alone. func ssrfSafeDialer(ctx context.Context, network, addr string) (net.Conn, error) { + return dialSSRFSafe(ctx, network, addr, nil) +} + +// dialSSRFSafe re-resolves addr and refuses to connect to any built-in +// internal range or operator-supplied blocked prefix, closing the +// DNS-rebinding window at connect time. +func dialSSRFSafe( + ctx context.Context, + network, addr string, + blocked []netip.Prefix, +) (net.Conn, error) { host, port, err := net.SplitHostPort(addr) if err != nil { return nil, err @@ -468,8 +532,10 @@ func ssrfSafeDialer(ctx context.Context, network, addr string) (net.Conn, error) } // Check all resolved IPs - if slices.ContainsFunc(ips, isPrivateIP) { - return nil, ErrSSRFBlocked + for _, ip := range ips { + if isBlockedIP(ip, blocked) { + return nil, ErrSSRFBlocked + } } // Connect using the first valid IP diff --git a/internal/imageprocessor/imageprocessor.go b/internal/imageprocessor/imageprocessor.go index 32e7b7a..dd77ac1 100644 --- a/internal/imageprocessor/imageprocessor.go +++ b/internal/imageprocessor/imageprocessor.go @@ -161,6 +161,13 @@ func (p *ImageProcessor) Process( } defer img.Close() + // Turn the image upright now: encode strips the EXIF orientation tag, + // and sizes below must be worked out on the upright image. + err = img.AutoRotate() + if err != nil { + return nil, fmt.Errorf("failed to auto-rotate: %w", err) + } + // Get original dimensions origWidth := img.Width() origHeight := img.Height() @@ -404,6 +411,21 @@ func (p *ImageProcessor) encode( return nil, fmt.Errorf("%w: %s", ErrUnsupportedOutputFormat, format) } + // Stripping drops the ICC profile as well, and clients show an image + // with no profile as sRGB, so convert to sRGB first. "srgb" names + // libvips' built-in profile; govips' own sRGB path variable is set on + // first use but read without a lock, so concurrent requests race on it. + if img.HasICCProfile() { + err := img.TransformICCProfileWithFallback("srgb", "srgb") + if err != nil { + return nil, fmt.Errorf("failed to convert to sRGB: %w", err) + } + } + + // Drop EXIF, XMP, IPTC and the ICC profile. govips ignores this for + // GIF, which carries none of them. + params.StripMetadata = true + output, _, err := img.Export(¶ms) if err != nil { return nil, err diff --git a/internal/imageprocessor/imageprocessor_internal_test.go b/internal/imageprocessor/imageprocessor_internal_test.go index 21de6c5..73decd0 100644 --- a/internal/imageprocessor/imageprocessor_internal_test.go +++ b/internal/imageprocessor/imageprocessor_internal_test.go @@ -9,7 +9,9 @@ import ( "image/jpeg" "image/png" "io" + "math" "os" + "slices" "testing" "github.com/davidbyttow/govips/v2/vips" @@ -561,3 +563,152 @@ func TestImageProcessor_EncodeAVIF(t *testing.T) { encodeAndCheck(t, FormatAVIF, 85, mimeAVIF) } + +// processAndDecode runs input through Process and decodes the output with +// vips, so a test can inspect the image a client would receive. +func processAndDecode(t *testing.T, input []byte, req *Request) *vips.ImageRef { + t.Helper() + + result, err := New(Params{}).Process( + context.Background(), bytes.NewReader(input), req, + ) + if err != nil { + t.Fatalf("Process() error = %v", err) + } + + defer func() { _ = result.Content.Close() }() + + data, err := io.ReadAll(result.Content) + if err != nil { + t.Fatalf("failed to read result: %v", err) + } + + output, err := vips.NewImageFromBuffer(data) + if err != nil { + t.Fatalf("failed to decode output: %v", err) + } + + t.Cleanup(output.Close) + + return output +} + +func TestImageProcessor_StripsEXIF(t *testing.T) { + t.Parallel() + + // gps-exif.jpg carries GPS coordinates, a camera make, model and serial + // number, and a capture time. + input, err := os.ReadFile("testdata/gps-exif.jpg") + if err != nil { + t.Fatalf("failed to read test JPEG: %v", err) + } + + fixture, err := vips.NewImageFromBuffer(input) + if err != nil { + t.Fatalf("failed to decode test JPEG: %v", err) + } + + t.Cleanup(fixture.Close) + + if !slices.Contains(fixture.GetFields(), "exif-ifd3-GPSLatitude") { + t.Fatal("testdata/gps-exif.jpg has no GPS latitude") + } + + formats := []Format{ + FormatJPEG, FormatPNG, FormatWebP, FormatAVIF, FormatGIF, FormatOriginal, + } + + for _, format := range formats { + t.Run(string(format), func(t *testing.T) { + t.Parallel() + + output := processAndDecode(t, input, &Request{Format: format}) + + if output.HasExif() { + t.Errorf("output has EXIF: %v", output.GetExif()) + } + }) + } +} + +func TestImageProcessor_AppliesEXIFOrientation(t *testing.T) { + t.Parallel() + + // orientation-6.jpg is stored 16x8, red on the left and blue on the + // right, with EXIF orientation 6 (turn 90 degrees clockwise to view). + // Upright it is 8x16, red on top and blue below. + input, err := os.ReadFile("testdata/orientation-6.jpg") + if err != nil { + t.Fatalf("failed to read test JPEG: %v", err) + } + + tests := []struct { + name string + size Size + wantW int + wantH int + }{ + {name: "original size", size: Size{}, wantW: 8, wantH: 16}, + {name: "width only", size: Size{Width: 4}, wantW: 4, wantH: 8}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + output := processAndDecode(t, input, &Request{ + Size: tt.size, + Format: FormatPNG, + }) + + if output.Width() != tt.wantW || output.Height() != tt.wantH { + t.Fatalf("output is %dx%d, want %dx%d", + output.Width(), output.Height(), tt.wantW, tt.wantH) + } + + top, err := output.GetPoint(tt.wantW/2, 0) + if err != nil { + t.Fatalf("GetPoint() error = %v", err) + } + + bottom, err := output.GetPoint(tt.wantW/2, tt.wantH-1) + if err != nil { + t.Fatalf("GetPoint() error = %v", err) + } + + if top[0] <= top[2] || bottom[2] <= bottom[0] { + t.Errorf("top pixel = %v, bottom pixel = %v, want red above blue", + top, bottom) + } + }) + } +} + +func TestImageProcessor_ConvertsWideGamutToSRGB(t *testing.T) { + t.Parallel() + + // display-p3.jpg is a flat 8x8 image with the Display P3 profile + // embedded, filled with Display P3 (234, 51, 35), which is sRGB red. + input, err := os.ReadFile("testdata/display-p3.jpg") + if err != nil { + t.Fatalf("failed to read test JPEG: %v", err) + } + + output := processAndDecode(t, input, &Request{Format: FormatPNG}) + + if output.HasICCProfile() { + t.Error("output has an ICC profile") + } + + pixel, err := output.GetPoint(4, 4) + if err != nil { + t.Fatalf("GetPoint() error = %v", err) + } + + want := []float64{255, 0, 0} + for i := range want { + if math.Abs(pixel[i]-want[i]) > 5 { + t.Fatalf("pixel = %v, want within 5 of %v", pixel, want) + } + } +} diff --git a/internal/imageprocessor/testdata/display-p3.jpg b/internal/imageprocessor/testdata/display-p3.jpg new file mode 100644 index 0000000..c2c49a6 Binary files /dev/null and b/internal/imageprocessor/testdata/display-p3.jpg differ diff --git a/internal/imageprocessor/testdata/gps-exif.jpg b/internal/imageprocessor/testdata/gps-exif.jpg new file mode 100644 index 0000000..8deb6af Binary files /dev/null and b/internal/imageprocessor/testdata/gps-exif.jpg differ diff --git a/internal/imageprocessor/testdata/orientation-6.jpg b/internal/imageprocessor/testdata/orientation-6.jpg new file mode 100644 index 0000000..da8dd48 Binary files /dev/null and b/internal/imageprocessor/testdata/orientation-6.jpg differ diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index f31880b..172b40e 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -125,7 +125,7 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) { } variants, err := NewVariantStorage( - filepath.Join(config.StateDir, "cache", "variants"), + filepath.Join(config.StateDir, "cache", "variants"), log, ) if err != nil { return nil, fmt.Errorf("failed to create variant storage: %w", err) @@ -263,23 +263,7 @@ func (c *Cache) StoreSource( return "", fmt.Errorf("failed to insert source metadata: %w", err) } - // Store metadata JSON file - meta := &SourceMetadata{ - Host: req.SourceHost, - Path: req.SourcePath, - Query: req.SourceQuery, - ContentHash: string(contentHash), - StatusCode: result.StatusCode, - ContentType: result.ContentType, - ContentLength: result.ContentLength, - ResponseHeaders: result.Headers, - FetchedAt: time.Now().UTC().Unix(), - FetchDurationMs: result.FetchDurationMs, - RemoteAddr: result.RemoteAddr, - } - - // A failure here is non-fatal; the metadata is in the database. - _ = c.srcMetadata.Store(req.SourceHost, pathHash, meta) + c.writeMetadataSidecar(req, pathHash, contentHash, result) c.notifyWritePressure() @@ -436,12 +420,19 @@ func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) { } // Get actual item count and total size from content tables - _ = c.db.QueryRowContext(ctx, + err = c.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM request_cache`, ).Scan(&stats.TotalItems) - _ = c.db.QueryRowContext(ctx, + if err != nil { + c.log.Warn("failed to count cache items for stats", "error", err) + } + + err = c.db.QueryRowContext(ctx, `SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`, ).Scan(&stats.TotalSizeBytes) + if err != nil { + c.log.Warn("failed to sum cache size for stats", "error", err) + } // Compute hit rate as a ratio if stats.HitCount+stats.MissCount > 0 { @@ -453,15 +444,17 @@ func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) { // IncrementStats increments cache statistics. func (c *Cache) IncrementStats(ctx context.Context, hit bool, fetchBytes int64) { + var err error + if hit { - _, _ = c.db.ExecContext(ctx, ` + _, err = c.db.ExecContext(ctx, ` UPDATE cache_stats SET hit_count = hit_count + 1, last_updated_at = CURRENT_TIMESTAMP WHERE id = 1 `) } else { - _, _ = c.db.ExecContext(ctx, ` + _, err = c.db.ExecContext(ctx, ` UPDATE cache_stats SET miss_count = miss_count + 1, last_updated_at = CURRENT_TIMESTAMP @@ -469,14 +462,52 @@ func (c *Cache) IncrementStats(ctx context.Context, hit bool, fetchBytes int64) `) } + if err != nil { + c.log.Warn("failed to count cache hit or miss", "hit", hit, "error", err) + } + if fetchBytes > 0 { - _, _ = c.db.ExecContext(ctx, ` + _, err = c.db.ExecContext(ctx, ` UPDATE cache_stats SET upstream_fetch_count = upstream_fetch_count + 1, upstream_fetch_bytes = upstream_fetch_bytes + ?, last_updated_at = CURRENT_TIMESTAMP WHERE id = 1 `, fetchBytes) + if err != nil { + c.log.Warn("failed to count upstream fetch", + "fetch_bytes", fetchBytes, "error", err) + } + } +} + +// writeMetadataSidecar writes the JSON metadata sidecar of a stored source. +// A failure is logged and is otherwise non-fatal; the metadata is in the +// database. +func (c *Cache) writeMetadataSidecar( + req *ImageRequest, + pathHash PathHash, + contentHash ContentHash, + result *httpfetcher.FetchResult, +) { + meta := &SourceMetadata{ + Host: req.SourceHost, + Path: req.SourcePath, + Query: req.SourceQuery, + ContentHash: string(contentHash), + StatusCode: result.StatusCode, + ContentType: result.ContentType, + ContentLength: result.ContentLength, + ResponseHeaders: result.Headers, + FetchedAt: time.Now().UTC().Unix(), + FetchDurationMs: result.FetchDurationMs, + RemoteAddr: result.RemoteAddr, + } + + err := c.srcMetadata.Store(req.SourceHost, pathHash, meta) + if err != nil { + c.log.Warn("failed to write metadata sidecar", + "host", req.SourceHost, "path_hash", pathHash, "error", err) } } @@ -528,10 +559,14 @@ func (c *Cache) checkNegativeCache( // Check if expired if time.Now().After(expiresAt) { // Clean up expired entry - _, _ = c.db.ExecContext(ctx, ` + _, err = c.db.ExecContext(ctx, ` DELETE FROM negative_cache WHERE source_host = ? AND source_path = ? AND source_query = ? `, req.SourceHost, req.SourcePath, req.SourceQuery) + if err != nil { + c.log.Warn("failed to delete expired negative cache entry", + "host", req.SourceHost, "path", req.SourcePath, "error", err) + } return false, nil } diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go index e3dd61e..7fb069b 100644 --- a/internal/imgcache/imgcache.go +++ b/internal/imgcache/imgcache.go @@ -59,6 +59,24 @@ func ValidateFitMode(fit FitMode) error { } } +// ValidateImageRequest checks a request's width and height with +// ValidateDimension and its fit mode with ValidateFitMode. Both the plain +// /v1/image/ route and the encrypted /v1/e/ route validate through this +// function so a request from either source enforces identical bounds. +func ValidateImageRequest(req *ImageRequest) error { + err := ValidateDimension("width", req.Size.Width) + if err != nil { + return err + } + + err = ValidateDimension("height", req.Size.Height) + if err != nil { + return err + } + + return ValidateFitMode(req.FitMode) +} + // ImageRequest represents a request for a processed image type ImageRequest struct { // SourceHost is the origin host (e.g., "cdn.example.com") diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index ecb745d..3b295c7 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -205,12 +205,23 @@ func (s *Service) ValidateRequest(req *ImageRequest) error { return s.signer.Verify(signatureRequest(req)) } -// GenerateSignedURL generates a signed URL for the given request. +// GenerateSignedURL generates a signed URL for the given request. The URL +// carries q and fit next to sig and exp, so the image route verifies it for +// the quality and fit it was signed with. An unset quality or fit is first +// set to 85 or cover, the values the route uses when a URL has no q or fit. func (s *Service) GenerateSignedURL( baseURL string, req *ImageRequest, ttl time.Duration, ) (string, error) { + if req.Quality == 0 { + req.Quality = 85 + } + + if req.FitMode == "" { + req.FitMode = FitCover + } + sigReq := signatureRequest(req) path, sig, exp := s.signer.GenerateSignedURL(sigReq, ttl) @@ -218,7 +229,8 @@ func (s *Service) GenerateSignedURL( req.Expires = sigReq.Expires req.Signature = sigReq.Signature - return fmt.Sprintf("%s%s?sig=%s&exp=%d", baseURL, path, sig, exp), nil + return fmt.Sprintf("%s%s?sig=%s&exp=%d&q=%d&fit=%s", + baseURL, path, sig, exp, req.Quality, req.FitMode), nil } // loadCachedSource attempts to load source content from cache, returning nil @@ -310,7 +322,12 @@ func (s *Service) fetchAndProcess( // Store negative cache for certain errors if isNegativeCacheable(err) { statusCode := extractStatusCode(err) - _ = s.cache.StoreNegative(ctx, req, statusCode, err.Error()) + + storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error()) + if storeErr != nil { + s.log.Warn("failed to store negative cache entry", + "host", req.SourceHost, "path", req.SourcePath, "error", storeErr) + } } return nil, fmt.Errorf("upstream fetch failed: %w", err) @@ -452,6 +469,8 @@ func signatureRequest(req *ImageRequest) *signature.Request { Width: req.Size.Width, Height: req.Size.Height, Format: string(req.Format), + Quality: req.Quality, + FitMode: string(req.FitMode), Signature: req.Signature, Expires: req.Expires, } diff --git a/internal/imgcache/stats_internal_test.go b/internal/imgcache/stats_internal_test.go index 4cecd4c..5076179 100644 --- a/internal/imgcache/stats_internal_test.go +++ b/internal/imgcache/stats_internal_test.go @@ -1,9 +1,12 @@ package imgcache import ( + "bytes" "context" "database/sql" + "log/slog" "math" + "strings" "testing" "time" @@ -101,3 +104,82 @@ func TestStats_ZeroCounts(t *testing.T) { t.Errorf("HitRate = %f, want 0.0 for zero counts", stats.HitRate) } } + +// TestStats_LogsFailedCountQueries verifies that a failed item count query +// and a failed size query are each logged at warn and Stats still succeeds. +func TestStats_LogsFailedCountQueries(t *testing.T) { + t.Parallel() + + db := setupStatsTestDB(t) + + var logBuf bytes.Buffer + + cache, err := NewCache(db, CacheConfig{ + StateDir: t.TempDir(), + CacheTTL: time.Hour, + NegativeTTL: 5 * time.Minute, + Logger: slog.New(slog.NewJSONHandler(&logBuf, nil)), + }) + if err != nil { + t.Fatal(err) + } + + _, err = db.ExecContext(t.Context(), + `DROP TABLE request_cache; DROP TABLE output_content`) + if err != nil { + t.Fatal(err) + } + + _, err = cache.Stats(t.Context()) + if err != nil { + t.Fatalf("Stats() error = %v, want nil", err) + } + + for _, msg := range []string{ + "failed to count cache items for stats", + "failed to sum cache size for stats", + } { + want := `"level":"WARN","msg":"` + msg + `"` + if !strings.Contains(logBuf.String(), want) { + t.Errorf("log missing %s; got %q", want, logBuf.String()) + } + } +} + +// TestIncrementStats_LogsFailedUpdates verifies that a failed hit or miss +// count update and a failed upstream fetch count update are each logged at +// warn. +func TestIncrementStats_LogsFailedUpdates(t *testing.T) { + t.Parallel() + + db := setupStatsTestDB(t) + + var logBuf bytes.Buffer + + cache, err := NewCache(db, CacheConfig{ + StateDir: t.TempDir(), + CacheTTL: time.Hour, + NegativeTTL: 5 * time.Minute, + Logger: slog.New(slog.NewJSONHandler(&logBuf, nil)), + }) + if err != nil { + t.Fatal(err) + } + + _, err = db.ExecContext(t.Context(), `DROP TABLE cache_stats`) + if err != nil { + t.Fatal(err) + } + + cache.IncrementStats(t.Context(), false, 1024) + + for _, msg := range []string{ + "failed to count cache hit or miss", + "failed to count upstream fetch", + } { + want := `"level":"WARN","msg":"` + msg + `"` + if !strings.Contains(logBuf.String(), want) { + t.Errorf("log missing %s; got %q", want, logBuf.String()) + } + } +} diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index 893b176..b9487ce 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "log/slog" "os" "path/filepath" "time" @@ -392,6 +393,7 @@ func CacheKey(req *ImageRequest) VariantKey { // Unlike ContentStorage, the key is provided by the caller (not computed from content). type VariantStorage struct { baseDir string + log *slog.Logger } // VariantMeta contains metadata about a cached variant. @@ -404,13 +406,14 @@ type VariantMeta struct { } // NewVariantStorage creates a new variant storage at the given base directory. -func NewVariantStorage(baseDir string) (*VariantStorage, error) { +// A failed .meta write is logged to log. +func NewVariantStorage(baseDir string, log *slog.Logger) (*VariantStorage, error) { err := os.MkdirAll(baseDir, StorageDirPerm) if err != nil { return nil, fmt.Errorf("failed to create variant storage directory: %w", err) } - return &VariantStorage{baseDir: baseDir}, nil + return &VariantStorage{baseDir: baseDir, log: log}, nil } // Store writes content and metadata to storage at the given key. @@ -478,7 +481,11 @@ func (s *VariantStorage) Store( } // Metadata write failure is non-fatal; content is already stored. - _ = os.WriteFile(metaPath, metaData, StorageFilePerm) + err = os.WriteFile(metaPath, metaData, StorageFilePerm) + if err != nil { + s.log.Warn("failed to write variant metadata sidecar", + "path", metaPath, "error", err) + } return size, nil } diff --git a/internal/imgcache/storage_internal_test.go b/internal/imgcache/storage_internal_test.go index a72ec94..0465bb1 100644 --- a/internal/imgcache/storage_internal_test.go +++ b/internal/imgcache/storage_internal_test.go @@ -4,8 +4,10 @@ import ( "bytes" "errors" "io" + "log/slog" "os" "path/filepath" + "strings" "testing" ) @@ -404,3 +406,35 @@ func TestCacheKey(t *testing.T) { t.Error("CacheKey() produced same key for different quality") } } + +// TestVariantStorage_StoreLogsFailedMetaWrite verifies that a .meta write +// that fails is logged at warn and the store still succeeds. +func TestVariantStorage_StoreLogsFailedMetaWrite(t *testing.T) { + t.Parallel() + + var logBuf bytes.Buffer + + storage, err := NewVariantStorage( + t.TempDir(), slog.New(slog.NewJSONHandler(&logBuf, nil))) + if err != nil { + t.Fatalf("NewVariantStorage() error = %v", err) + } + + key := CacheKey(&ImageRequest{SourceHost: testHostCDN, SourcePath: testPathCat}) + + // A directory where the .meta file goes makes the .meta write fail. + err = os.MkdirAll(storage.keyToPath(key)+".meta", StorageDirPerm) + if err != nil { + t.Fatalf("failed to create directory: %v", err) + } + + _, err = storage.Store(key, bytes.NewReader([]byte("variant data")), "image/webp") + if err != nil { + t.Fatalf("Store() error = %v, want nil", err) + } + + want := `"level":"WARN","msg":"failed to write variant metadata sidecar"` + if !strings.Contains(logBuf.String(), want) { + t.Errorf("log missing %s; got %q", want, logBuf.String()) + } +} diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 6a391ee..a1a424c 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -23,6 +23,21 @@ var ( // MaxDimension is the maximum allowed width or height. const MaxDimension = 8192 +// ValidateDimension checks one requested width or height; name ("width" or +// "height") appears in the error. 0 means "original size" and is valid. +func ValidateDimension(name string, value int) error { + if value < 0 { + return fmt.Errorf("%w: %s is negative", ErrInvalidSize, name) + } + + if value > MaxDimension { + return fmt.Errorf("%w: %s is above %d", + ErrDimensionTooLarge, name, MaxDimension) + } + + return nil +} + // sizeFormatRegex matches patterns like "800x600.webp", "0x0.jpeg", "orig.png" var sizeFormatRegex = regexp.MustCompile(`^(\d+)x(\d+)\.(\w+)$|^(orig)\.(\w+)$`) @@ -225,14 +240,20 @@ func parseSizeFormat(s string) (Size, ImageFormat, error) { return Size{}, "", ErrInvalidSize } - if width > MaxDimension || height > MaxDimension { - return Size{}, "", ErrDimensionTooLarge - } - size = Size{Width: width, Height: height} formatStr = matches[3] } + err := ValidateDimension("width", size.Width) + if err != nil { + return Size{}, "", err + } + + err = ValidateDimension("height", size.Height) + if err != nil { + return Size{}, "", err + } + format, err := parseFormat(formatStr) if err != nil { return Size{}, "", err diff --git a/internal/imgcache/validate_internal_test.go b/internal/imgcache/validate_internal_test.go new file mode 100644 index 0000000..99ecf36 --- /dev/null +++ b/internal/imgcache/validate_internal_test.go @@ -0,0 +1,64 @@ +package imgcache + +import ( + "errors" + "testing" +) + +func TestValidateImageRequest(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + req ImageRequest + wantErr error + }{ + { + name: "within bounds", + req: ImageRequest{Size: Size{Width: 800, Height: 600}, FitMode: FitCover}, + }, + { + name: "original size and empty fit", + req: ImageRequest{Size: Size{Width: 0, Height: 0}}, + }, + { + name: "width over limit", + req: ImageRequest{Size: Size{Width: MaxDimension + 1, Height: 600}}, + wantErr: ErrDimensionTooLarge, + }, + { + name: "height over limit", + req: ImageRequest{Size: Size{Width: 800, Height: MaxDimension + 1}}, + wantErr: ErrDimensionTooLarge, + }, + { + name: "negative width", + req: ImageRequest{Size: Size{Width: -1, Height: 600}}, + wantErr: ErrInvalidSize, + }, + { + name: "invalid fit mode", + req: ImageRequest{Size: Size{Width: 800, Height: 600}, FitMode: "bogus"}, + wantErr: ErrInvalidFitMode, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateImageRequest(&tt.req) + if tt.wantErr == nil { + if err != nil { + t.Fatalf("ValidateImageRequest() error = %v, want nil", err) + } + + return + } + + if !errors.Is(err, tt.wantErr) { + t.Fatalf("ValidateImageRequest() error = %v, want %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/middleware/client_ip_internal_test.go b/internal/middleware/client_ip_internal_test.go new file mode 100644 index 0000000..7200f46 --- /dev/null +++ b/internal/middleware/client_ip_internal_test.go @@ -0,0 +1,118 @@ +package middleware + +import ( + "bytes" + "log/slog" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/clientip" + "sneak.berlin/go/pixa/internal/config" +) + +// testForwardedClient is the client address the proxy forwards. +const testForwardedClient = "203.0.113.7" + +// newTestMiddleware builds a Middleware whose resolver trusts the given +// CIDRs and whose logger writes JSON to buf. +func newTestMiddleware(t *testing.T, buf *bytes.Buffer, trusted ...string) *Middleware { + t.Helper() + + prefixes := make([]netip.Prefix, 0, len(trusted)) + + for _, c := range trusted { + p, err := netip.ParsePrefix(c) + if err != nil { + t.Fatalf("netip.ParsePrefix(%q) error = %v", c, err) + } + + prefixes = append(prefixes, p) + } + + return &Middleware{ + log: slog.New(slog.NewJSONHandler(buf, nil)), + config: &config.Config{TrustedProxies: prefixes}, + clientIP: clientip.NewResolver(prefixes), + } +} + +// TestClientIPMiddlewareStoresResolvedIP verifies the ClientIP middleware +// puts the resolved address into the request context for a trusted and an +// untrusted peer. +func TestClientIPMiddlewareStoresResolvedIP(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + remoteAddr string + forwarded string + want string + }{ + { + name: "trusted peer honors forwarded client", + remoteAddr: "10.0.0.1:5000", + forwarded: testForwardedClient, + want: testForwardedClient, + }, + { + name: "untrusted peer ignores forwarded header", + remoteAddr: "198.51.100.9:5000", + forwarded: testForwardedClient, + want: "198.51.100.9", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mw := newTestMiddleware(t, &bytes.Buffer{}, "10.0.0.0/8") + + var got string + + handler := mw.ClientIP()(http.HandlerFunc( + func(_ http.ResponseWriter, r *http.Request) { + got = clientip.FromContext(r.Context()) + })) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil) + req.RemoteAddr = tt.remoteAddr + req.Header.Set("X-Forwarded-For", tt.forwarded) + + handler.ServeHTTP(httptest.NewRecorder(), req) + + if got != tt.want { + t.Errorf("client IP in context = %q, want %q", got, tt.want) + } + }) + } +} + +// TestLoggingUsesResolvedClientIP verifies the logging middleware records +// the resolved forwarded client IP rather than the proxy peer address. +func TestLoggingUsesResolvedClientIP(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + mw := newTestMiddleware(t, &buf, "10.0.0.0/8") + + handler := mw.ClientIP()(mw.Logging()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }))) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.1:5000" + req.Header.Set("X-Forwarded-For", testForwardedClient) + + handler.ServeHTTP(httptest.NewRecorder(), req) + + if !strings.Contains(buf.String(), `"remoteIP":"`+testForwardedClient+`"`) { + t.Errorf("log output missing resolved client IP; got %q", buf.String()) + } +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 754ebd1..30feaa5 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -3,17 +3,19 @@ package middleware import ( "log/slog" - "net" "net/http" + "net/netip" "time" basicauth "github.com/99designs/basicauth-go" "github.com/go-chi/chi/v5/middleware" "github.com/go-chi/cors" + "github.com/go-chi/httprate" metrics "github.com/slok/go-http-metrics/metrics/prometheus" ghmm "github.com/slok/go-http-metrics/middleware" "github.com/slok/go-http-metrics/middleware/std" "go.uber.org/fx" + "sneak.berlin/go/pixa/internal/clientip" "sneak.berlin/go/pixa/internal/config" "sneak.berlin/go/pixa/internal/logger" ) @@ -21,6 +23,33 @@ import ( // CORSMaxAgeSeconds is the max age for CORS preflight cache (24 hours). const CORSMaxAgeSeconds = 86400 +// HSTSValue is the Strict-Transport-Security header value: one year with +// includeSubDomains. Emitted unconditionally even though pixa listens plain +// HTTP behind a TLS-terminating proxy; browsers ignore an HSTS header received +// over plaintext (RFC 6797 section 8.1), so it never lies about the connection, +// and emitting it here avoids trusting a forwarded-proto header. +const HSTSValue = "max-age=31536000; includeSubDomains" + +// ContentSecurityPolicyValue is the Content-Security-Policy header value. +// default-src 'self' is the baseline and frame-ancestors 'none' is the primary +// clickjacking control. 'unsafe-inline' is required in script-src and style-src +// because the served templates carry inline onclick handlers (generator page) +// and the bundled Tailwind asset injects a runtime