access_control_allow_origin is now "*", or http or https, a host that is
an IP address as net/netip writes it (IPv6 in brackets) or a lowercase
host name whose last part contains a letter, and an optional port 1-65535
with no leading zero that is not the scheme's default. The value must
equal the origin rebuilt from those parts; anything else aborts startup
naming the key, its variable and the value. README.md and
config.example.yml say an origin is scheme, host and optional port,
exactly as the browser sends it.
Model: opus-5-5
Adds the review's examples to invalidSizeAndOriginCases: a scheme's
default port, a port with a leading zero, hosts a browser reads as an
IPv4 address or refuses, an IPv6 address not in its shortest form,
plus a scheme other than http or https and a host name in upper case.
Model: opus-5-5
access_control_allow_origin now needs a host name (ASCII letters,
digits, hyphens, dots) or an IP address, and a port, when given, from 1
to 65535, read with net/url, net/netip and strconv. Two hosts, an empty
port, no host, a bad port or a non-ASCII host name abort startup, as a
* inside the value already did.
upstream_max_response_size above 1 GiB aborts startup: the image
processor reads one byte past the limit, which wrapped negative at the
largest 64-bit value, and a response is held whole in memory.
config.example.yml states the maximum.
Model: opus-5-5
Failing cases: access_control_allow_origin with two hosts, an empty
port, no host, port 0, port 99999 or a non-ASCII host name, and
upstream_max_response_size above 1 GiB, up to the largest 64-bit
integer, where the image processor's limit wraps negative. Each must
abort startup naming the key and the value. The bad origins are now one
list so the table fits the linter's function length limit; the size
test uses 1 GiB to show the maximum itself is accepted, and IPv4 and
IPv6 origins are shown to be accepted.
Model: opus-5-5
access_control_allow_origin accepted values such as https://* or
https://*example.com, which the CORS middleware reads as a pattern that
lets other sites read responses. Any value that contains * and is not
exactly * now aborts startup naming the key, its variable and the value.
Model: opus-5-5
Failing cases for access_control_allow_origin set to https://*,
https://*.example.com and https://*example.com. The CORS middleware
reads a * inside an origin as a pattern, so these would let other sites
read responses; each must abort startup naming the key and the value.
Model: opus-5-5
access_control_allow_origin, upstream_fetch_timeout,
upstream_max_response_size and downstream_timeout were in the README
but unknown to pixa, so a config that followed it aborted startup. Each
is now a setting with its PIXA_ variable, defaulting to the value that
was fixed in the code: *, 30s, 50 MiB and 60s. Durations are positive Go
duration strings; the size is a positive whole number of bytes; the
origin is * or one scheme and host. An invalid value aborts startup
naming the key and value. downstream_timeout replaces HTTPWriteTimeout
for the server's write timeout and the per-request timeout.
Model: opus-5-5
Failing tests for access_control_allow_origin, upstream_fetch_timeout,
upstream_max_response_size and downstream_timeout: their defaults,
valid values from the file and the environment, invalid values aborting
startup naming the key or variable and the value, the CORS middleware
answering with the configured origin, and the server's write timeout
coming from downstream_timeout. They do not compile until the settings
exist.
Model: opus-5-5
A variable whose name starts with PIXA_ but is neither a setting's
variable, from the list pairing each config key with its variable, nor
PIXA_CONFIG_PATH now aborts startup naming it, as an unknown config key
does. 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. README.md says so under Configuration.
Model: opus-5-5
Each config key can now be set by PIXA_ plus the key in upper case
("." written as "_"), and the port by PORT. A present variable, even
an empty one, is read before the config file through the existing
typed getters, so every existing check covers it; errors name the key
and the variable, never the signing key or metrics password. A
variable named in the file's env: section overrides both. An empty
string for blocked_networks or trusted_proxies is now an empty list.
The image no longer bakes in config.docker.yml or passes --config; its
HEALTHCHECK probes ${PORT:-8080}. The config file is looked for under
/etc/pixa rather than /etc/pixad. Also covers #99.
Model: opus-5-5
RFC1918 ranges are the default trusted proxy set on an omitted key; an explicit list replaces the default; an explicit empty list trusts no one; unparseable values abort startup; forwarded headers honored only from trusted peers. Independent review passed: #127 (comment)
model: claude-opus-4-8 (implementation and review); merged by claude-fable-5
Adds the blocked_networks config key: a list of CIDRs, parsed with net/netip, that is added to the built-in list of address ranges the fetcher refuses to contact and can never remove an entry from it. An invalid CIDR aborts startup naming the key and the value.
The built-in list gains 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. Resolved addresses are unmapped before matching, so IPv4-mapped IPv6 forms are caught too. Enforcement stays in the dial-time re-resolution, which is what closes the DNS rebinding window.
What a reader would trip over: 192.0.0.0/24 is now blocked but TEST-NET-1 (192.0.2.0/24), which the Fetch tests use as a public upstream, is a different range and stays dialable. The package-level dialer enforces the built-in ranges only; operator entries are applied by the fetcher.
Disclosure: one nolint:gochecknoglobals on the immutable built-in prefix list.
Model: opus-4-8 (implementation, review); fable-5-1 (landing message)
The Docker image now ships config.docker.yml, which sets only signing_key (read from the PIXA_SIGNING_KEY environment variable), state_dir and port. The placeholder key and the five-host allowlist from config.example.yml are no longer in the image; anything else is configured by mounting a file over /etc/pixa/config.yml. A container started without PIXA_SIGNING_KEY exits naming it.
Startup now refuses the exact placeholder signing_key from config.example.yml. It is 45 characters long and used to pass the length check, so a deployment could sign URLs with a key that is public in this repository. README Getting Started is corrected to match.
What a reader would trip over: the unset-variable error comes from config interpolation, not from validate(); the signing key checks moved into validateSigningKey to stay under the complexity limit.
Disclosure: TODO.md is not updated by this change.
Model: opus-4-8 (implementation, review); fable-5-1 (landing message)
Accumulating milestone branch. One squashed commit per closed issue; `next` is kept green and mergeable to `main` at any time without notice.
Landed so far:
- `chore: update golangci-lint to v2.12.2 with canonical config` (#54) — canonical v2-schema `.golangci.yml`, pins bumped in `Dockerfile` and `script/bootstrap`, tree at `0 issues.`. Three behaviour deltas are recorded in that PR's body: `Cache.StoreVariant` takes a context, `MetadataStorage.Store` no longer leaks temp files on failure, and the `signing_key` too-short error text gained a `value too short:` prefix.
Sequencing for the milestone is tracked in #103.
Reviewed-on: #105
Co-authored-by: clawbot <clawbot@noreply.example.org>
Implements #51 per the issue DoD and the owner direction comment (issuecomment-44068).
## Behavior
**Config: `cache_max_bytes`** (integrates with the #52/#53 validation framework)
- Strict int64 parsing via a new `getInt64`/`int64Val` getter in the existing strict-loader pattern; the key is registered in the known-keys list. A SET but invalid value — negative, float, null, non-numeric string, boolean, list — aborts startup with exit 1 naming the key and the offending value.
- Explicit values are used exactly as given, any non-negative amount, no floor. `cache_max_bytes: 0` is a valid value that disables the disk cache entirely.
- Omitted: after `state_dir` validation, the default resolves to `max(75% of free bytes on the filesystem containing <state_dir>/cache/, 500 MiB)`. The cache directory is created first and statfs runs on that actual path, so the measurement hits the right filesystem. The probe is injectable (`FreeSpaceProbeFunc`) so tests do not depend on the host disk. The effective limit (and disabled state) is logged at startup.
**Size accounting** (no directory scans on the hot path)
- Migration `002` adds a `variant_content` table — processed variants were previously untracked anywhere — and a `last_accessed_at` column on `source_content`, both indexed. Total usage is two SUM queries.
- Stores record accounting rows; cache hits touch the LRU timestamps (same cost class as the existing per-request stats UPDATEs). The variant accounting insert is best-effort with a warning: the reconciliation pass (below) adopts any file that missed its row, and this keeps the pre-migration inline test schema working.
**Eviction policy: global LRU across both content classes**
- Candidates are the least-recently-used entries from `variant_content` and `source_content` (batched, 100 per class per pass, merged oldest-first by `COALESCE(last_accessed_at, fetched_at)`), evicted until usage is at or below the limit.
- Why global LRU: recency of actual use is the best cheap predictor of future use for a CDN-style cache, and treating both classes in one ordering avoids pathologies of class-priority schemes (e.g. evicting every variant before any cold source blob, which would tank hit rate, or the reverse, which would hoard stale sources). Byte-for-byte, the coldest data goes first regardless of what kind it is. LFU-style schemes need more bookkeeping for marginal gain at this scale.
- Reference safety (the multi-reference DoD case): evicting a source blob deletes ALL `source_metadata` rows referencing it plus its `source_content` row in a single transaction BEFORE the file is unlinked. A blob referenced by multiple source paths is only ever removed together with all of its references, and DB rows never point at deleted files (the crash window leaves at worst an orphaned file, which reconciliation sweeps). The JSON metadata sidecars for removed rows are deleted as well.
**Triggers, off the request path**
- A background goroutine (started in the handlers OnStart hook, stopped in OnStop) runs an eviction pass on a periodic ticker (5 min) and on write pressure: every store sends a non-blocking notification on a capacity-1 channel. Requests never wait on eviction.
- On startup the goroutine first reconciles accounting with the disk (off the hot path): adopts untracked variant files (size/mtime from disk, content type from the `.meta` sidecar), drops accounting rows whose files are missing, removes source blob files the DB does not know (unreachable, since lookups go through `source_metadata`), removes rows whose files are gone, and sweeps `.tmp-*` files older than an hour.
**`cache_max_bytes: 0` disables the disk cache**
- No cache directories are created, lookups always miss, `StoreSource`/`StoreVariant` are no-ops, no evictor runs; every request fetches and processes uncached. Verified end-to-end (below).
## Notes for review
- At the `imgcache.CacheConfig` layer, disabling is an explicit `DisableDiskCache` flag rather than `MaxBytes == 0`, because existing test fixtures construct `CacheConfig` without `MaxBytes` and rely on the legacy "no limit" behavior; per repo rules those tests were not touched. The config layer maps `cache_max_bytes: 0` to the flag in `handlers`. `MaxBytes == 0` at that layer means "no limit enforced" and is unreachable from production config (the computed default is always at least 500 MiB).
- The negative cache stays active in disabled mode: it is DB-backed (TTL-expired rows in SQLite), not part of the disk cache this issue bounds, and it protects against hammering failing upstreams. Flagging explicitly since the direction said "no cache reads, no cache writes" — I read that as the disk cache; happy to disable it too if intended.
- One deviation from pure red/green: after the red commit I extended the new-test fixture helper (`newEvictionTestCache`) to pass `DisableDiskCache: maxBytes == 0`, mirroring the production mapping, when the flag design emerged. Assertions were not touched; no pre-existing tests were modified.
- Sidecar files (`.meta`, metadata JSON) are not counted in usage; they are bounded by entry counts and small (tens of bytes to ~1 KiB per entry) while content bytes dominate. Documented here for transparency.
- Discovered while working: `Cache.Stats` reads the never-populated `output_content`/`request_cache` tables, so `TotalItems`/`TotalSizeBytes` are always 0. Out of scope here; filing as a separate issue.
## Verification
- TDD: commit `3963ec3` adds the failing tests first (18 new tests covering strict parsing, default computation with injected probe including floor and 75% branches, explicit-no-floor, zero-disables, size accounting, dedup accounting, LRU order, multi-reference blob eviction with the no-dangling-references invariant, under-limit no-op, write-pressure trigger, periodic trigger, reconciliation); implementation follows in `8cb09b6`/`bdd86a4` until green.
- `make check` green (all tests, lint 0 issues, fmt-check) at HEAD.
- Pinned CI lint gate: `docker build --target lint .` green (golangci-lint v2.10.1).
- End-to-end with the built binary:
- omitted key: startup logs `computed default cache size limit from free space` and `effective cache size limit` (75% of the test host's free space);
- `cache_max_bytes: banana`: exit 1 with `config key "cache_max_bytes": value "banana" is not an integer`;
- `cache_max_bytes: 0`: `cache_disabled=true` logged, two identical requests both fetch upstream (2 upstream fetches logged), 200 `image/jpeg` responses, no `cache/` directory created, only `state.sqlite3` in the state dir;
- enabled: second request served from cache (1 upstream fetch), `variant_content` and `source_content` rows match the on-disk file sizes.
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #55
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
closes#52
Implements startup configuration validation per the plan on #52. Two commits, TDD: the first commit adds the enforcement tests (red — six test functions fail against the lenient behavior) plus a mechanical extraction of `newFromSmartConfig` from `config.New` so construction is testable without fx; the second commit makes them green and carries the `TODO.md` bookkeeping.
## Behavior
- **No silent fallbacks**: a config value that is SET but unparseable or invalid aborts startup with an error naming the key and value. Defaults apply only to OMITTED keys. The old `getString`/`getInt`/`getBool` helpers swallowed every conversion error and returned the default; they are now strict. Fractional ports are rejected, not truncated (smartconfig's `GetInt` would have turned `8080.5` into `8080`).
- **Unknown keys abort**: unknown top-level keys and unknown `metrics` subkeys are fatal, each named in the error (`unknown config keys: whitelist_hosts`). The `env` section stays permitted because smartconfig consumes it for environment injection.
- **Malformed config file aborts**: a config file that exists at a standard location but fails to parse was previously logged as a warning and skipped (the server would start on defaults); it is now fatal.
- **Range/sanity checks**: `port` in 1-65535; `upstream_connections_per_host` at least 1; `signing_key` required, at least 32 characters (keyless mode was never implemented; the stale "leave empty" comment in `config.example.yml` is corrected); `allowlist_hosts` entries must be bare hostnames (leading-dot suffix patterns still allowed; schemes, paths, whitespace, non-string and empty entries rejected); `state_dir` non-empty and verified creatable+writable with a probe file before the listener binds; `sentry_dsn` must be a URL with scheme and host when set; `metrics.username`/`metrics.password` must be set together.
## Verification
- `make check` green on the branch head (all tests, golangci-lint 0 issues, fmt-check clean).
- End-to-end: `./bin/pixad` with `port: banana` exits 1 printing `config key "port": value "banana" is not an integer`; with `whitelist_hosts:` it exits 1 printing `unknown config keys: whitelist_hosts`.
## Notes for review
- `getStringSlice` keeps its lenient signature because the existing tests in `config_test.go` exercise it and modifying existing tests requires explicit approval. Strictness for `allowlist_hosts` is instead enforced up front on the raw value by `validateAllowlistHostsValue`, so nothing is silently skipped; extraction then reuses the existing parser. If you prefer the helper folded into a single strict function, that requires retargeting those three tests — happy to do that as a follow-up with approval.
- `TODO.md` here is edited against current `main`; PR #50 (merge-ready) edits adjacent lines, so whichever merges second will need a trivial rebase of `TODO.md` only.
- The README Configuration section lists keys that have never existed in the code (`access_control_allow_origin`, `upstream_fetch_timeout`, `upstream_max_response_size`, `downstream_timeout`). Under this change a config using them now fails fast instead of silently doing nothing — that is the intended behavior. Implementing them is already tracked as the P2 "add all configuration options from README" item in `TODO.md`.
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #53
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Extracts HMAC-SHA256 request signing out of `internal/imgcache/` into its own `internal/signature/` package, per the plan in [issue #39](#39).
This is one of the remaining "easily separable" extractions (`imageprocessor`, `allowlist`, `magic`, and `httpfetcher` already landed). Only the signer is moved here so the diff stays reviewable.
## What moved
From `internal/imgcache/signature.go` and its tests into `internal/signature/`:
- `Signer` type, its `New` constructor, `Sign`, `Verify`, `GenerateSignedURL`
- `ParseParams` (query-string signature/expiration parsing)
- Signature error sentinels
## One-way import edge
To keep the import edge one-way (`imgcache` depends on `signature`, never the reverse), the package defines a standalone `Request` type carrying just the fields the signature covers, instead of importing `imgcache.ImageRequest`. `imgcache` projects its `ImageRequest` onto `signature.Request` via a small unexported `signatureRequest` helper. This mirrors how the `magic` extraction defined its own `ImageFormat` type.
## Renames (no stuttering)
- `NewSigner` -> `signature.New`
- `ParseSignatureParams` -> `signature.ParseParams`
- `ErrSignatureRequired`/`Invalid`/`Expired` -> `signature.ErrRequired`/`Invalid`/`Expired`
The `ErrRequired` message is updated from "non-whitelisted host" to "non-allowlisted host" for inclusive terminology, consistent with the `allowlist` rename.
## Rework (post-review)
Three commits added after review feedback:
- `d69019b` — golden known-answer test pinning the exact HMAC signatures and signed URL paths for three fixed vectors (resized, resized+query, orig size), cross-validated against an independent HMAC implementation. Any change to the signed byte format now fails loudly.
- `43b9f1c` — whitelist→allowlist rename completed across `internal/imgcache` and `internal/handlers` (`ServiceConfig.Allowlist`, `Allowlist` interface, `IsAllowlisted`, test helpers and test names).
- `3dc1999` — one-pass config surface rename, no back-compat alias: YAML key `whitelist_hosts` → `allowlist_hosts`, `Config.WhitelistHosts` → `Config.AllowlistHosts`, `config.example.yml`, `scripts/manual-test.sh`, and `README.md` (which also documented a nonexistent `source_host_whitelist` key — now fixed to the real one).
## Behavior
Pure refactor apart from the config key rename above. The bytes fed to the HMAC are unchanged (`host:path:query:width:height:format:expiration`), so previously issued signatures remain valid — now enforced by the golden test. All existing tests move with the package. `script/cibuild` passes at head `3dc1999` (fmt-check, lint, test, build).
refs #39
Co-authored-by: sneak <sneak@sneak.berlin>
Co-authored-by: Jeffrey Paul <sneak@noreply.example.org>
Reviewed-on: #46
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
- Update Dockerfile base image from golang:1.24-alpine to golang:1.25.4-alpine
(pinned by sha256 digest) to match go.mod requirement of go >= 1.25.4
- Fix gosec G703 (path traversal) false positives by adding filepath.Clean()
at call sites with nolint annotations for internally-constructed paths
- Fix gosec G704 (SSRF) false positive with nolint annotation; URL is already
validated by validateURL() which checks scheme, resolves DNS, and blocks
private IPs
- All make check passes clean (lint + tests)
- Add config validation: signing_key required, minimum 32 characters
- Server now fails to start without valid signing_key (no more runtime errors)
- Add config.example.yml with default whitelist hosts
- Copy config to /etc/pixa/config.yml in Docker image
- Update entrypoint to use --config /etc/pixa/config.yml
- Add config.dev.yml for local Docker development
- Mount dev config in make devserver
- Change default StateDir from ./data to /var/lib/pixa (proper Unix convention)
- Create directory owned by pixad user in Dockerfile
- Set WORKDIR to /var/lib/pixa
clawbot
clawbot