POST / had no limit, so the signing key could be guessed at no cost. It
is now limited to LoginAttemptsPerMinute (5) attempts per minute per
client by a new RateLimit middleware on github.com/go-chi/httprate. It
counts by the address the ClientIP middleware resolved through
trusted_proxies, an IPv6 client by its /64, and answers an attempt over
the limit with 429 and Retry-After. It runs after the body-size and CSRF
checks, so every attempt that reaches the key comparison is counted. The
image routes can reuse it. README states the limit; TODO narrows the
per-IP item to the image routes.
Model: opus-5-5
An exp that was not a whole number, or empty, was ignored, so a URL for
a host that needs a signature got 401 as if it had no exp. It is now a
400 naming exp and the value, on every host; only an exp missing from
the URL is unchanged.
A failed variant .meta write, source metadata JSON write, Stats count
query, stats counter update, negative cache write or expired negative
cache delete was discarded without a trace. Each is now logged at warn
with the path or key and the error, and stays non-fatal, with tests for
those that can be made to fail. VariantStorage takes the cache's logger.
Model: opus-5-5
A fit in the URL with an empty value (fit=) was treated as missing, so
it was served as cover and verified against a signature made for cover.
It is now a 400 naming fit, the same rule the route applies to an empty
q. It is checked before the existing fit-mode check, which takes an
empty fit as missing; any other value still goes through that check
unchanged. Only a fit missing from the URL is cover.
Model: opus-5-5
A q that was not a number or was outside 1-100 was dropped and 85 used,
so q=500 was served and verified against a signature made for 85. It is
now a 400 naming q and the value, read with the generator's quality
check; only a q missing from the URL is 85.
The route also refuses with 400 a query string that cannot be decoded
(r.URL.Query() drops such a pair, so q=80% arrived as no q) and any
parameter given more than once, which was read from its first value only
(q=80&q=500 was served at 80).
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
The encrypted /v1/e/ route used the decrypted payload unchecked, so a
token could request an over-limit size or an unknown fit mode; the
generator turned unparseable numbers into 0.
imgcache.ValidateDimension alone holds the MaxDimension bound and is
used by the path parser, by the new ValidateImageRequest (which adds
ValidateFitMode) and by the generator. Both image routes call
ValidateImageRequest, so each answers 400. The generator answers 400
naming the field for a width or height that is not a number or fails
that check, a quality that is not a number from 1 to 100, a ttl that is
not a number from 0 to the largest the expiry calculation can hold, or
an unknown fit. Empty quality is 85; empty ttl never expires. The
form's size inputs stop at 8192.
Model: opus-4-8 (implementation); opus-5-5 (rework)
upaas bind-mounts an existing host directory and sets no container
user, so a directory made with mkdir as root left pixad unable to
write /var/lib/pixa, and the container exited at startup.
The image now starts as root: deploy/docker-entrypoint.sh gives
/var/lib/pixa to pixad when pixad does not own it, then runs the
server as pixad through su-exec (alpine's package), so the server
never runs as root. README.md gains a "Running under upaas" section:
port, volume, environment variables, health check, first-run step.
Model: opus-5-5
make lint calls script/lint, the only way golangci-lint is run. Inside a
container it runs the linter; anywhere else it builds Dockerfile.lint,
whose last step runs script/lint again. Both Dockerfiles set
container=docker to mark the container, since /.dockerenv is missing in
build steps and present on hosts that are themselves containers. The
Dockerfile lint stage runs make lint.
A new CACHEBUST build-arg on every run keeps the lint step from being
served from cache; a tmpfs mount keeps Go's and golangci-lint's caches
out of that step's layer, so runs do not pile up build cache.
script/bootstrap and the nix-shell package lists no longer carry
golangci-lint. golangci-lint config verify is not run: it fetches its
schema over an unpinned live HTTPS call.
Model: opus-4-8 (implementation); opus-5-5 (rework)
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
The signed data is now
host:path:query:width:height:format:expiration:quality:fit. The route
turns a missing q into 85 and a missing fit into cover before checking
the signature, so those are the values signed for a URL without them;
imgcache fills both from the parsed request.
imgcache.Service.GenerateSignedURL now writes q and fit into the URL
next to sig and exp, first setting an unset quality or fit to 85 or
cover, so a generated URL verifies for the values it signed.
The known-answer vectors in golden_test.go, including one for quality
40 and fit contain, and the README signature section describe the new
format.
Model: opus-4-8 (implementation); opus-5-5 (rework)
The runtime stage declares a HEALTHCHECK that probes
/.well-known/healthcheck.json with busybox wget. script/docker-smoke
(make docker-smoke) builds the image with script/docker, starts it with
a random PIXA_SIGNING_KEY, and passes only once Docker reports the
container healthy within 30 seconds; the container is removed on exit
and its log printed on failure. The Gitea workflow runs it after
script/cibuild; it is not part of make check.
It waits on Docker's health status instead of polling a published host
port because the Gitea job runs in its own container on its own
network, where such a port is not reachable at localhost.
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 http.Server now sets ReadHeaderTimeout (10s), which bounds the slow header dribble that ReadTimeout alone does not, and IdleTimeout (120s), which bounds keep-alive reuse. Server construction moved into a small helper so a test can assert the timeouts without binding a listener.
POST / and POST /generate bodies are capped at 1 MiB and an oversized body returns 413.
What a reader would trip over: the CSRF library reads its token from the form and swallows a parse error, so a cap applied only inside it would surface as 403. The body limit therefore parses the form under the cap before the CSRF check; the parsed form is reused afterwards. A test covers an oversized body that carries a valid token.
Judgement call: WriteTimeout stays at 60s; it also bounds how long a large image may take to send over a slow link.
Model: opus-4-8 (implementation, review); fable-5-1 (landing message)
The Workflow section of TODO.md still told contributors to branch from main and merge there. It now describes the current model: one branch per issue cut from next, a PR based on next, an independent reviewer, a squash-merge into next by the manager, and only the owner merging next into main through the milestone PR. The Status paragraph no longer claims work is green on main.
Disclosure: only the wrong lines are touched; reflowing the whole file is left to #100.
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>
closes#49
Records the P0 manual test pass in `TODO.md` per its Workflow section
(checked-off results into Completed Steps; cache size management and
eviction promoted to Next Step). `TODO.md` is the only changed file —
no production code changes, as the issue requires.
## Test setup
`pixad` built from `main` at `6573b9d` via `make build`, run on port
18099 with a throwaway local config (temp state dir, known
`signing_key`, `allowlist_hosts` including `s3.sneak.cloud`), driven
with curl using explicit cookie replay (session cookies are
`Secure`/`HttpOnly`/`SameSite=Strict`).
## Results — all six checks PASS
1. **Login form**: GET `/` → HTTP 200, `Pixa - Login` page with
`name="key"` password form.
2. **Wrong key error**: POST `/` with `key=wrong-key` → HTTP 200 login
page containing "Invalid signing key".
3. **Generator form**: POST `/` with the correct signing key → HTTP 303
to `/` with `Set-Cookie: pixa_session=...; HttpOnly; Secure;
SameSite=Strict`; GET `/` with that cookie → `Pixa - URL Generator`
with the `/generate` form and logout link.
4. **Encrypted URL serves image**: POST `/generate` (ttl=3600) produced
a `/v1/e/<token>/img.jpeg` URL → HTTP 200, `Content-Type:
image/jpeg`, 800x600 baseline JPEG, 61706 bytes.
5. **Expired URL → 410**: a ttl=1 URL fetched after 3 s → HTTP 410 Gone
with `{"error":"URL has expired","status":410,...}`.
6. **Logout**: GET `/logout` → HTTP 303 to `/` with `Set-Cookie:
pixa_session=; Max-Age=0`; subsequent GET `/` → login form again.
Additionally, all nine checks in `scripts/manual-test.sh` passed
against the same server instance.
## Verification
`make check` green on the branch head (all tests, golangci-lint 0
issues, fmt-check clean) — the first fully green `make check` on a
`main`-derived branch under the current linter, confirming the #47/#48
fix on merged `main`.
Note for review: the test execution was performed this session; the
adversarial re-review (independently re-running the six flows) is still
pending and should happen before merge.
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #50
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
closes#47
Fixes the two remaining `gosec` findings on `main`, both `G124`
(http.Cookie missing or has insecure `Secure`, `HttpOnly`, or
`SameSite` attribute):
- `internal/session/session.go:84` (`CreateSession`, the login
set-cookie path)
- `internal/session/session.go:128` (`ClearSession`, the logout
delete-cookie path)
## What changed
- Both cookie-writing paths now unconditionally set `Secure: true`,
`HttpOnly: true`, and `SameSite: http.SameSiteStrictMode`.
- The `secure` field (previously wired to `!config.Debug`) and the
`sameSite` field are removed from `session.Manager`, and the dead
secure-toggle parameter is removed from `session.NewManager`, which
now takes only the signing key (reviewer-directed; the mechanical
call-shape updates in `session_test.go` leave every assertion
untouched).
- TDD per repo rules: the first commit adds
`TestSessionCookieAttributesAlwaysSecure` (failing), asserting that
every cookie emitted by the session manager carries `HttpOnly`,
`Secure`, and `SameSite` of Lax or stricter, for both write paths.
The second commit makes it pass.
- `TODO.md` updated per its Workflow section (Next Step completed,
next Future Step promoted, stale "10 open findings" Status text
corrected).
## Attribute choices and reasoning
- `Secure: true` always: the `G124` analyzer only accepts a constant
`true` store, and there is no legitimate configuration in which the
authentication cookie should be sent over plaintext HTTP. The old
behavior disabled `Secure` whenever `debug` was on. Local development
over `http://localhost` keeps working: browsers treat `localhost` as
a trustworthy origin and accept `Secure` cookies there. Any
plain-HTTP flow on a non-localhost host will no longer keep a
session, which is the point of the fix.
- `SameSite: Strict` (unchanged from current production behavior, and
stricter than the Lax minimum): the login form is a same-origin POST
to `/` followed by a same-site redirect, so `Strict` breaks nothing.
- `HttpOnly: true` (unchanged).
## Verification
`make check` (tests, golangci-lint, fmt-check) is fully green on the
branch head `cb9e14e`: all tests pass and the linter reports 0 issues,
independently confirmed by the reviewer in a fresh worktree. Commit
history: `ca15f52` (failing test) → `02ca16a` (fix + TODO.md, closes
#47) → `cb9e14e` (drop the dead `NewManager` parameter).
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #48
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
closes#24
## QA Audit Fixes
This PR addresses issues found during the 1.0/MVP QA audit.
### Changes
1. **TODO.md: Mark AVIF encoding as done** — AVIF encoding is fully implemented via govips in `processor.go` but was still listed as a TODO item.
2. **scripts/manual-test.sh: Fix form field names** — The manual test script was using wrong field names:
- Login form: was sending `password=...`, should be `key=...` (matching the HTML form's `name="key"`)
- Generator form: was sending `source_url`, `fit_mode` — should be `url`, `fit` (matching the handler's `r.FormValue()` calls)
- This means **the manual test script never actually worked** — login always failed silently because the `key` field was empty.
### Full QA Audit Results
The comprehensive QA audit report has been posted as a comment on [issue #24](#24).
Co-authored-by: user <user@Mac.lan guest wan>
Reviewed-on: #25
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Uses github.com/gen2brain/webp - a CGO-free library that uses WASM via
wazero runtime for encoding. WebP decoding was already supported.
- Add gen2brain/webp dependency for encoding
- Implement WebP encoding in processor.go
- Add FormatWebP to SupportedOutputFormats
- Re-enable WebP option in generator form dropdown
- Mark WebP encoding as complete in TODO.md
- check: default target, runs fmt-check, lint, and test
- fmt-check: verifies code is properly formatted
- fmt: formats code with gofmt
- lint: runs golangci-lint
- test: runs go test
- build: builds pixad binary with version info
- clean: removes build artifacts
clawbot
