sneak/pixa

Files

check / check (push) Successful in 4s

Details

feat: validate configuration on startup, fail fast on bad config (closes #52 ) (#53 )

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>

2026-08-07 22:39:40 +02:00

4.7 KiB

Raw Blame History

Workflow

branch (from main)
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
push

Status

pre-1.0. No git tags exist. 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: the last two open findings (G124, session cookie attributes in internal/session) are fixed as of this change, so make check is green on main.

Next Step

P0: implement cache size management and eviction so the disk cannot fill up

Completed Steps

2026-08-07 validate configuration on startup, fail fast on bad config (closes #52): a config value that is set but unparseable or invalid aborts startup naming the key and value (defaults apply only to omitted keys), unknown config keys abort startup, a malformed config file aborts instead of being skipped, and state_dir is verified creatable and writable before the listener binds
2026-08-07 manual test pass of the auth and encrypted URL flows against a locally built and running pixad (built from main at 6573b9d, port 18099, local throwaway config); all six checks passed, plus all nine tests in scripts/manual-test.sh (closes #49):
- visit / and see the login form: HTTP 200, Pixa - Login page with name="key" password form
- wrong key shows an error: POST / with key=wrong-key returned HTTP 200 login page containing "Invalid signing key"
- correct signing key shows the generator form: POST / returned HTTP 303 to / with Set-Cookie: pixa_session=...; HttpOnly; Secure; SameSite=Strict; GET / with that cookie rendered Pixa - URL Generator with the /generate form and logout link
- a generated encrypted URL serves the image: POST /generate (ttl=3600) produced a /v1/e/<token>/img.jpeg URL that returned HTTP 200, Content-Type: image/jpeg, an 800x600 baseline JPEG of 61706 bytes
- an expired URL (short TTL) returns 410: a ttl=1 URL fetched after 3 s returned HTTP 410 Gone with {"error":"URL has expired","status":410,...}
- logout redirects back to login: GET /logout returned HTTP 303 to / with Set-Cookie: pixa_session=; Max-Age=0; subsequent GET / rendered the login form again
2026-08-07 fix the two remaining gosec findings (G124 in internal/session): session cookies now always carry Secure/HttpOnly/SameSite=Strict on both the set and clear paths; make check green (closes #47)
2026-07-07 Adopted scripts-to-rule-them-all: script/ entrypoints, Makefile shims, README Entrypoints section
2026-04-07 extract magic byte detection into internal/magic (#42)
2026-03-25 extract allowlist package from internal/imgcache (#41)
2026-03-25 move schema_migrations table creation into 000.sql (#36)
2026-03-20 enforce and document exact-match-only signature verification (#40)
2026-03-20 bound imageprocessor.Process input read to prevent unbounded memory use (#37); consolidate appname into an internal/globals constant (#34)
2026-03-18 parse version prefix from migration filenames (#33)
2026-03-15 QA audit fixes for 1.0/MVP readiness (#25)
2026-03-02 split Dockerfile with pre-built golangci-lint stage for faster CI (#23)
2026-02-25 repo policy compliance: CI workflow, hash-pinned images, golangci-lint and gosec fixes of that date (#14); arm64 Docker build fix (#16)
2026-01-08 WebP and AVIF encoding support via govips (both former P0 image processing items, now done)

Future Steps

P1: implement blocked networks configuration to extend SSRF protection
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-origin rate limiting
P2: HTTP response handling
- Last-Modified headers
- Vary header for content negotiation
- X-Request-ID propagation
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
- comprehensive request logging
- Prometheus performance metrics
- integration tests for the image proxy flow
- load tests to verify the 1k to 5k req/s target
P2: documentation
- configuration options
- API endpoints
- deployment guide
- example nginx or caddy reverse proxy config