Both now say what access_control_allow_origin accepts, instead of
"exactly as the browser sends it", and that another scheme, such as a
browser extension's, aborts startup.
Model: opus-5-5
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
REPO_POLICIES.md puts migrations in internal/db/migrations/ as
000_migration.sql and 001_schema.sql. The two files move there with
their contents unchanged. go:embed cannot reach outside its own
package, so internal/db/migrations has a small package that embeds
them, and internal/database reads them through its FS(). The database
package stays where CONVENTIONS.md puts it; moving it would change
existing test files in other packages.
The version still comes from the filename prefix, so a database that
has recorded versions 0 and 1 runs neither again. A new test applies
the migrations twice to one database file and checks that the second
run applies nothing.
Model: opus-5-5
The README told operators to set trusted_proxies to the proxy's own
address. A proxy on the Docker host that connects over 127.0.0.1 reaches
pixa from the Docker network's gateway, so that advice made pixa count
every user as one client for the login limit. The login-limit paragraph,
the trusted_proxies entry and config.example.yml now say to use the
address pixa sees for requests through the proxy, that a proxy connecting
through another host address is seen with that address, and how to read
it from the request log.
The signature section now says sig is base64url with the = padding
kept, since pixa compares it exactly, and shows the example's sig for a
stated key, computed with pixa's signer.
Model: opus-5-5
Cache.Stats read request_cache and output_content, which nothing
writes, so TotalItems and TotalSizeBytes were always 0. They now count
source_content plus variant_content and use UsageBytes; a disabled disk
cache reports 0 for both. The upstream fetch count and bytes and the
transform count never moved: Get now passes the bytes it fetched
(including those read before a failed body read) and counts each
successful transcode. Hits, misses and these counters are written with
context.WithoutCancel, so a client disconnect or the request timeout no
longer loses them. The unused tables stay; metaCache is #70.
Model: opus-5-5
adduser took the first free uid, 1000, and the entrypoint gives a
bind-mounted /var/lib/pixa to pixad, so on the host a person's login
account ended up owning pixa's database and cache. The image now creates
the pixad group with gid 65532 and the pixad user with uid 65532, which
host login and system accounts do not use. The first-run step of
"Running under upaas" in README.md names the uid and gid.
Model: opus-5-5
Both image routes sent Cache-Control: public, max-age=31536000,
immutable unconditionally, so a browser or proxy could keep serving an
image for a year after its signed or encrypted URL had expired. max-age
is now the whole seconds left until the URL expires, never negative and
at most one year; a URL with no expiry keeps one year. The 304 answer
uses the same value. An encrypted URL's expiry now reaches
ImageRequest.Expires through ToImageRequest. immutable stays: freshness
now ends no later than the URL's expiry. README.md documents the header.
Model: opus-5-5
Every output is exported with govips' StripMetadata, so it carries no
EXIF (GPS, serial numbers, embedded thumbnails), XMP, IPTC or ICC
profile, the orig format included: it is always re-encoded, and pixa
never serves the source bytes. The image is turned upright with
AutoRotate right after decoding, so dropping the orientation tag does
not leave it rotated, and a requested size applies to the upright
image. An image with an ICC profile is converted to sRGB before export.
No setting turns this off. README.md documents it.
Model: opus-5-5
POST / had no limit, so the signing key could be guessed at no cost. It
is now limited to 5 attempts per minute per client by a new RateLimit
middleware on github.com/go-chi/httprate; an attempt over the limit gets
429 with Retry-After. It counts by the address the ClientIP middleware
resolved through trusted_proxies (an IPv4-mapped address as its IPv4
address, IPv6 by its /64) and runs after the body-size and CSRF checks,
so every attempt that reaches the key comparison is counted. README says
that with the default trusted_proxies a client with a private address
can choose its counted address, and how to close that.
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 old comment said the flag stops the image's 30-second interval from
delaying the first probe past the wait. From Docker 25 on, the first
probe runs 5 seconds after start either way; the flag makes probes
after the 10-second start period come every second instead of every
30. Only the comment changes; TODO.md is left alone because the issue
limits the change to this script.
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 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)
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)
SecurityHeaders() now also sets Strict-Transport-Security (one year, includeSubDomains), a Content-Security-Policy (default-src self, frame-ancestors none) and a Permissions-Policy denying the browser features pixa does not use. X-Frame-Options stays as the legacy fallback.
What a reader would trip over: HSTS is sent on every response even though pixa listens on plain HTTP behind a TLS-terminating proxy; browsers ignore the header over plaintext, and this avoids trusting a forwarded-proto header. The clipboard feature is left unlisted so the copy button on the generator page keeps working.
Disclosure: script-src and style-src carry unsafe-inline because the generator template has inline onclick handlers and the bundled Tailwind script injects a style element at runtime; removing it needs template changes and is tracked separately.
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)
internal/httpfetcher had only helper-level tests. This adds tests of the full Fetch path, with no non-test code changed: a redirect to a private address is refused and never dialed while public redirects and a two-hop chain still work; the per-host semaphore is released on error, after a full read and after a partial read; an oversized body yields ErrResponseTooLarge; non-2xx and disallowed content types are rejected; the dialer blocks private, link-local and loopback targets.
What a reader would trip over: the upstream host in the tests is the TEST-NET-1 literal 192.0.2.10, which the private-IP check treats as public; a recording dialer routes it to the local test server and records every dial.
Disclosure: DNS rebinding is not simulated end to end (it would mean changing the global resolver under parallel race tests); the dial-time re-resolution is tested directly instead.
Model: opus-4-8 (implementation, review); fable-5-1 (landing message)
script/test now runs the suite quietly first (with -race and -cover, 30s timeout) and re-runs it with -v only when that run fails, then exits non-zero. This is the pattern REPO_POLICIES.md mandates; before, every green run printed full per-test output.
What a reader would trip over: the whole compound command is passed as one string to run_with_cgo_deps, so it behaves the same on the host path and under the nix-shell fallback. -cover is on the first run only; the verbose rerun exists for diagnostics.
Disclosure: no test was written for the wrapper script itself; the failure path was exercised by hand by author and reviewer.
Disclosure: the nix-shell fallback is kept; moving tests into Docker belongs to #101 and #104.
Model: opus-4-8 (implementation, review); fable-5-1 (landing message)
Adds CSRF protection to the two cookie-authenticated form posts, POST / (login) and POST /generate, using github.com/gorilla/csrf, the recorded default for this job.
The token key is derived from signing_key with its own HKDF salt, so it needs no new config and survives restarts. The token cookie is separate from the session cookie, which also covers login CSRF, where no session exists yet. Both templates carry the hidden token field.
What a reader would trip over: outside debug mode the library enforces its https Referer origin check, so the TLS-terminating proxy must preserve the Host and Referer headers from the browser or form posts are rejected.
Disclosure: one nolint:gosec on a test constant holding the library field name (G101 false positive).
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>
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>
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>
Extracts the HTTP fetcher concern out of `internal/imgcache/` into its own `internal/httpfetcher/` package, per the plan in [issue #39](#39).
This is one of four planned extractions; only the fetcher is moved here so the diff stays reviewable. The remaining three (signature, magic, urlparser) will land in separate PRs.
## What moved
From `internal/imgcache/fetcher.go` and `internal/imgcache/mock_fetcher.go` into `internal/httpfetcher/`:
- `HTTPFetcher` type, its `New` constructor, and `Config` (formerly `FetcherConfig`) with `DefaultConfig`
- SSRF-safe dialer, `validateURL`, `isPrivateIP`, `isLocalhost`, `extractHost`
- Per-host connection semaphore (rate limiting) and `limitedReader`
- Content-type validation (`isAllowedContentType`, `detectContentTypeFromPath`)
- All related error values (`ErrSSRFBlocked`, `ErrUpstreamError`, `ErrUpstreamTimeout`, `ErrPayloadTooLarge`, `ErrDisallowedContentType`)
- All related constants (`DefaultFetchTimeout`, `DefaultMaxPayloadBytes`, `DefaultMaxConnectionsPerHost`, etc.)
- The `Fetcher` interface and `FetchResult` type (moved here to keep the import edge one-way: `imgcache` depends on `httpfetcher`, never the reverse)
- `MockFetcher` test helper
## Renames (no stuttering)
- `NewHTTPFetcher` → `httpfetcher.New`
- `FetcherConfig` → `httpfetcher.Config`
- `DefaultFetcherConfig` → `httpfetcher.DefaultConfig`
- `NewMockFetcher` → `httpfetcher.NewMock`
The `ServiceConfig.FetcherConfig` field name is retained — it describes what kind of config the field holds (not a stutter).
## Behavior
Pure refactor. No behavior changes. All existing tests pass; unit tests for the new package are included.
`docker build .` passes (fmt-check, lint, test, build).
refs #39
Co-authored-by: clawbot <clawbot@eeqj.de>
Co-authored-by: Jeffrey Paul <sneak@noreply.example.org>
Reviewed-on: #43
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
## Summary
Extract magic byte detection and MIME type handling from `internal/imgcache/` into a new focused `internal/magic/` package.
Part of [issue #39](#39)
## Changes
### New package: `internal/magic/`
Moved the following from `internal/imgcache/magic.go`:
- `MIMEType` type and constants (`MIMETypeJPEG`, `MIMETypePNG`, etc.)
- `DetectFormat()` — detects image format from magic bytes
- `ValidateMagicBytes()` — validates content matches declared MIME type
- `PeekAndValidate()` — reads minimum bytes, validates, returns combined reader
- `IsSupportedMIMEType()` — checks if a MIME type is supported
- `MIMEToImageFormat()` — converts MIME type to ImageFormat
- `ImageFormatToMIME()` — converts ImageFormat to MIME string
- All error sentinels (`ErrUnknownFormat`, `ErrMagicByteMismatch`, `ErrNotEnoughData`)
- All helper functions (`detectSVG`, `skipBOM`, `normalizeMIMEType`)
The magic package defines its own `ImageFormat` type and constants to avoid circular imports (`imgcache` → `magic` for validation; `magic` cannot import `imgcache`).
### Updated imports
- `internal/imgcache/service.go`: uses `magic.ValidateMagicBytes()`
- `internal/imgcache/service_test.go`: uses `magic.DetectFormat()` and `magic.MIMEToImageFormat()`
### Naming
- Clean package-qualified names: `magic.DetectFormat()`, `magic.ValidateMagicBytes()`, etc.
- No stuttering names
### Tests
- Full test suite moved to `internal/magic/magic_test.go` (all 15 test functions preserved)
- All existing tests pass unchanged
- `docker build .` passes (includes `make check`: fmt, lint, tests)
Co-authored-by: user <user@Mac.lan guest wan>
Reviewed-on: #42
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Extract `HostWhitelist`, `NewHostWhitelist`, `IsWhitelisted`, `IsEmpty`, and `Count` from `internal/imgcache/` into the new `internal/whitelist/` package.
The whitelist package is completely self-contained, depending only on `net/url` and `strings` from the standard library. No circular imports introduced.
**Changes:**
- Moved `whitelist.go` → `internal/whitelist/whitelist.go` (added package comment)
- Moved `whitelist_test.go` → `internal/whitelist/whitelist_test.go` (adapted to external test style)
- Updated `internal/imgcache/service.go` to import from `sneak.berlin/go/pixa/internal/whitelist`
`docker build .` passes (lint, tests, build).
Part of [issue #39](#39)
Co-authored-by: clawbot <clawbot@noreply.git.eeqj.de>
Co-authored-by: user <user@Mac.lan guest wan>
Reviewed-on: #41
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
clawbot
sneak
