allVariantKeys and allSourceContentHashes were byte-for-byte the same
query-scan-collect loop over a single string column, differing only in
the SQL, the result type and the noun in their error messages. Both now
delegate to a generic queryStringColumn helper.
Extracted rather than suppressed with a nolint: the duplication was
real, and the two call sites are the only places this shape appears.
Every error message is preserved verbatim, which is why the helper takes
both a plural and a singular noun.
- goconst: extracted testVariantKeyTwo alongside testVariantKeyOne.
- paralleltest: t.Parallel() on
TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent. The test
asserts that a concurrent store stays blocked for 200ms while eviction
holds the content lock; parallel load can only make it more blocked,
never less, so the assertion does not become timing-fragile. Verified
over repeated full -race runs.
- gosec G115: a live, justified suppression on the int64 conversion in
ComputeDefaultCacheMaxBytes. The preceding clamp was an explicit
if-statement that gosec could follow; the modernize linter requires it
to be min(), which gosec's range analysis cannot see through. The
clamp is still there and still correct, so the conversion cannot
overflow. Unlike the directives removed earlier in this branch, this
one is live: nolintlint confirms it suppresses a finding that is
actually raised.
The size-accounting INSERT in StoreVariant used db.Exec, and
StoreVariant took no context at all, so the write was uncancellable and
untraceable. Rather than paper over that with context.Background() at
the call site, StoreVariant now takes a context.Context and uses
ExecContext.
The plumbing is small: the only production caller is
Service.processAndStore, which already has the request context in
scope, so the accounting insert now shares the lifetime of the request
that produced the variant. Test callers pass t.Context().
The insert remains best-effort: a failure is logged and the startup and
periodic reconciliation passes still adopt any variant file whose
accounting row is missing.
Hand-reviewed rather than mechanical, because two of these functions
carried cleanup that depended on the shape being replaced.
internal/imgcache/storage.go, writeIfAbsent: dropped the named result
and the deferred temp-file cleanup that read it (nonamedreturns), in
favour of an explicit os.Remove(tmpPath) on each failing path. The
deferred form removed tmpPath whenever the function returned a non-nil
error, which is reachable on exactly three paths once the temp file
exists: Write, Close and Rename. Each of those now unlinks explicitly,
in the same order relative to tmpFile.Close(). The paths that must NOT
unlink are unchanged and still cannot: the content-already-present
early return, a MkdirAll failure and a CreateTemp failure all happen
before tmpPath exists, and the success path renames the temp file away.
This is the same cleanup shape MetadataStorage.Store and
VariantStorage.Store already use in this file. StoreHashed likewise
loses its named results.
internal/imgcache/eviction.go: converted 26 inline assignments. In
evictSourceBlob the conversions reuse the function-scope err that the
transaction already used; the rollback defer does not read it, and the
ordering of the delete transaction, its commit, the sidecar deletes and
the blob unlink is untouched. In the rows.Next() loops the scan error
is declared inside the loop body and rows.Err() is checked after it, as
before.
internal/config: the remaining conversions are in straight-line code
with no defer or named result.
No behavior changes. make test (with -race, per script/test) is green,
including TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent,
which exercises the commit-to-unlink window this cleanup protects.
No behavior changes. Covers the purely mechanical findings the canonical
v2.12.2 config raises on the cache-size/eviction work:
- nolintlint: deleted 7 dead //nolint:gosec directives (cachesize.go x2,
config.go, eviction.go x2, storage.go x2). gosec never raises G115/G703
on those lines under the pinned toolchain, exactly the defect class
that failed round 2 of this PR. The 6 live gosec suppressions are
untouched.
- funcorder: moved writeIfAbsent after Exists (storage.go) and
touchVariant/touchSourceContent after IncrementStats (cache.go).
- lll: wrapped over-length signatures, calls and messages at 88 columns.
- paralleltest: t.Parallel() on the new eviction, contentlock and
cache_max_bytes tests and their subtests. configFromYAML uses only
t.TempDir, so the config cases are parallel-safe.
- noctx: test helper DB calls now use ExecContext/QueryContext/
QueryRowContext with t.Context().
- goconst: extracted testHeaderContentType into the shared test constant
block and testVariantKeyOne into the eviction tests; reused the
existing testContentTypeJPEG and keyCacheMaxBytes constants.
- modernize: interface{} to any, atomic.Int32 for the contentlock
counters, min/max in ComputeDefaultCacheMaxBytes.
- intrange: integer range loops in the contentlock tests.
- wsl_v5: whitespace before the contentlock rendezvous statements.
- sloglint: slog.DiscardHandler in the config test logger.
- cyclop/funlen: split TestZeroMaxBytesDisablesDiskCache into four
assertion helpers and extracted the concurrent store goroutine from
TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent. Every
assertion is preserved verbatim; only their grouping changed.
Absorbs the cache size management and LRU eviction work (#55). All four
textual conflicts resolved in favor of main's implementation, with this
branch's mechanical lint conformance re-applied on top:
- internal/config/config.go: took main's cache_max_bytes wiring
(CacheMaxBytes, cacheMaxBytesExplicit) verbatim and expressed the key
through this branch's constant convention as keyCacheMaxBytes.
- internal/imgcache/cache.go: took main's disabled-cache guards, LRU
touch on lookup, and variant_content size accounting verbatim;
re-applied this branch's signature wrapping for lll.
- internal/imgcache/storage.go: took main's new writeIfAbsent helper and
its temp-file cleanup defer verbatim. The auto-merge had silently
dropped that defer in favor of this branch's inline cleanup form;
restored so the cleanup semantics that arrived from main are intact.
- TODO.md: kept both sides' Completed Steps entries.
.golangci.yml resolves to this branch's canonical version
(sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb).
make build and make test are green; #55's code is not yet conformant
with the canonical lint config, which the following commits address.
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>
The 15 //nolint:gosec G703/G704 directives added in 13e9f2c suppressed
findings that only a stale local golangci-lint (v2.10.1) reports; the
repo's pinned golangci-lint:v2.12.2-alpine never raises gosec G703/G704
on these lines, so nolintlint correctly flagged every directive as an
unused suppression and failed the pinned lint build.
Verified with the authoritative docker build --target lint . using the
pinned image, and separately with a locally installed pinned v2.12.2
binary: both report 0 issues after removal.
The v2.12.2 gosec ruleset's new path-traversal (G703) and SSRF (G704)
taint checks flag os.Stat/os.Remove/os.Rename calls on paths that are
never attacker-controlled: our own temp files created immediately
before in the same function, content-hash- or cache-key-derived
storage paths, the operator-supplied config search path, and the
already SSRF-guarded upstream fetch (protected by ssrfSafeDialer at
the transport layer). Each suppression carries the rule ID and a
one-line justification, matching this repo's existing gosec nolint
convention in internal/imgcache/storage.go. No behavior change.
Insert the blank line wsl_v5 requires above four defer statements
whose deferred closure does not share a variable with the immediately
preceding line, across internal/handlers, internal/httpfetcher, and
internal/imgcache. No behavior change.
The stricter canonical .golangci.yml surfaced 81 findings in the
config validation code merged from main (#53). Fix them all with no
behavior change: static sentinel errors wrapped with %w preserving the
existing messages (err113), config key name constants (goconst),
t.Parallel() throughout except the Setenv/Chdir test (paralleltest),
white-box test renamed to config_validation_internal_test.go
(testpackage), case tables extracted into builder functions plus a
shared runAbortCases helper (funlen/dupl/gochecknoglobals), plain
error assignments (noinlineerr), any instead of interface{} and
strings.SplitSeq (modernize), slog.DiscardHandler (sloglint), 88-col
wrapping (lll), and removal of two stale nolint:gosec directives
(nolintlint).
Resolves conflicts with the startup config validation from #53:
internal/config/config.go takes main's validation implementation
wholesale, with getStringSlice mechanically adapted to this branch's
keyless signature; TODO.md keeps both Completed Steps entries.
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>
Replace .golangci.yml with the canonical v2-schema config
(default: all minus six disabled linters, lll 88, tests included)
and bump every golangci-lint pin to v2.12.2:
- Dockerfile: golangci/golangci-lint:v2.12.2-alpine (hash-pinned)
- script/bootstrap: GOLANGCI_LINT_VERSION 2.12.2 with new
linux-amd64/arm64 release-archive sha256 pins
Fix all 747 findings the stricter config surfaces, with no behavior
changes: t.Parallel() throughout the test suite, static sentinel
errors and errors.Is comparisons, checked error returns, context
propagation (contextcheck/noctx), 88-column wrapping, extracted
constants and helpers for goconst/dupl/funlen/cyclop, exhaustive
switch cases replicating existing defaults, and white-box test files
renamed to *_internal_test.go for testpackage. Three
nolint:tagliatelle directives preserve the existing snake_case JSON
wire and on-disk metadata formats.
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>
## Summary
Moves the `schema_migrations` table definition from inline Go code into `internal/database/schema/000.sql`, so the migration tracking table schema lives alongside all other schema files.
closes#29
## Changes
### New file: `internal/database/schema/000.sql`
- Contains the `CREATE TABLE IF NOT EXISTS schema_migrations` DDL
- This is applied as a bootstrap step before the normal migration loop
### Refactored: `internal/database/database.go`
- Removed the inline `CREATE TABLE IF NOT EXISTS schema_migrations` SQL from both `runMigrations` and `ApplyMigrations`
- Added `bootstrapMigrationsTable()` which:
- Checks `sqlite_master` to see if the table already exists
- If missing: reads and executes `000.sql` to create it, then records version `000`
- If present (backwards compat with existing DBs created by old inline code): back-fills version `000` so the normal loop skips the bootstrap file
- Deduplicated: both `Database.runMigrations()` and the exported `ApplyMigrations()` now delegate to a single `applyMigrations()` helper
- Added `logInfo`/`logDebug` helpers to handle the optional logger (nil when called from `ApplyMigrations` in tests)
### New file: `internal/database/database_test.go`
- `TestApplyMigrations_CreatesSchemaAndTables` — verifies all migrations apply and all expected tables exist
- `TestApplyMigrations_Idempotent` — verifies running migrations twice produces no errors or duplicates
- `TestBootstrapMigrationsTable_FreshDatabase` — verifies bootstrap creates the table and records version 000
- `TestBootstrapMigrationsTable_ExistingTableBackwardsCompat` — verifies existing DBs (from old inline-SQL code) get version 000 back-filled without data loss
## Conflict note
[PR #33](#33) (for [issue #28](#28)) is also modifying migration code. This PR is based on current `main` and the conflict will be resolved at merge time.
Co-authored-by: user <user@Mac.lan guest wan>
Co-authored-by: clawbot <clawbot@noreply.git.eeqj.de>
Co-authored-by: clawbot <clawbot@sneak.berlin>
Co-authored-by: clawbot <clawbot@eeqj.de>
Co-authored-by: Jeffrey Paul <sneak@noreply.example.org>
Reviewed-on: #36
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Closes #27
Signatures are per-URL only — this PR adds explicit tests and documentation enforcing that HMAC-SHA256 signatures verify against exact URLs only. No suffix matching, wildcard matching, or partial matching is supported.
## What this does NOT touch
**The host whitelist code (`whitelist.go`) is not modified.** This PR is exclusively about signature verification, per sneak's instructions on [issue #27](#27), [PR #32](#32), and [PR #35](#35).
## Changes
### `internal/imgcache/signature.go`
- Added documentation comments on `Verify()` and `buildSignatureData()` explicitly specifying that signatures are exact-match only — no suffix, wildcard, or partial matching
### `internal/imgcache/signature_test.go`
- **`TestSigner_Verify_ExactMatchOnly`**: 14 tamper cases verifying that modifying any signed component (host, path, query, dimensions, format) causes verification to fail. Host-specific cases include:
- Parent domain (`example.com`) does not match subdomain signature (`cdn.example.com`)
- Sibling subdomain (`images.example.com`) does not match
- Deeper subdomain (`images.cdn.example.com`) does not match
- Evil suffix domain (`cdn.example.com.evil.com`) does not match
- Prefixed host (`evilcdn.example.com`) does not match
- **`TestSigner_Sign_ExactHostInData`**: Verifies that suffix-related hosts (`cdn.example.com`, `example.com`, `images.example.com`, etc.) all produce distinct signatures
### `internal/imgcache/service_test.go`
- **`TestService_ValidateRequest_SignatureExactHostMatch`**: Integration test through `ValidateRequest` verifying that a valid signature for `cdn.example.com` is rejected when presented with a different host (parent domain, sibling subdomain, deeper subdomain, evil suffix, prefixed host)
### `README.md`
- Updated Signature Specification section to explicitly document exact-match-only semantics
Co-authored-by: user <user@Mac.lan guest wan>
Reviewed-on: #40
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Closes [issue #30](#30).
The appname `"pixad"` was redundantly defined in both `cmd/pixad/main.go` (as a package-level var) and `internal/globals/globals.go` (as a package-level var that got copied from main). Since the appname is always `"pixad"` and is not actually set via ldflags (only `Version` is), this PR:
- Defines `appname` once as an unexported constant in `internal/globals/globals.go`
- Removes the `Appname` var from `cmd/pixad/main.go`
- Removes the `globals.Appname = Appname` assignment from `main.run()`
- Keeps `Version` flow unchanged (still set via ldflags in main, passed to globals)
The `Globals.Appname` struct field remains available to all consumers — they just get it from the constant now instead of a package var that was always `"pixad"`.
All existing tests pass, `docker build .` succeeds.
Co-authored-by: user <user@Mac.lan guest wan>
Co-authored-by: Jeffrey Paul <sneak@noreply.example.org>
Reviewed-on: #34
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
closes#31
## Problem
`ImageProcessor.Process` used `io.ReadAll(input)` without any size limit, allowing arbitrarily large inputs to exhaust all available memory. This is a DoS vector — even though the upstream fetcher has a `MaxResponseSize` limit (50 MiB), the processor interface accepts any `io.Reader` and should defend itself independently.
Additionally, the service layer's `processFromSourceOrFetch` read cached source content with `io.ReadAll` without a bound, so an unexpectedly large cached file could also cause unbounded memory consumption.
## Changes
### Processor (`processor.go`)
- Added `maxInputBytes` field to `ImageProcessor` (configurable, defaults to 50 MiB via `DefaultMaxInputBytes`)
- `NewImageProcessor` now accepts a `maxInputBytes` parameter (0 or negative uses the default)
- `Process` now wraps the input reader with `io.LimitReader` and rejects inputs exceeding the limit with `ErrInputDataTooLarge`
- Added `DefaultMaxInputBytes` and `ErrInputDataTooLarge` exported constants/errors
### Service (`service.go`)
- `NewService` now wires the fetcher's `MaxResponseSize` through to the processor
- Extracted `loadCachedSource` helper method to flatten nesting in `processFromSourceOrFetch`
- Cached source reads are now bounded by `maxResponseSize` — oversized cached files are discarded and re-fetched
### Tests (`processor_test.go`)
- `TestImageProcessor_RejectsOversizedInputData` — verifies that inputs exceeding `maxInputBytes` are rejected with `ErrInputDataTooLarge`
- `TestImageProcessor_AcceptsInputWithinLimit` — verifies that inputs within the limit are processed normally
- `TestImageProcessor_DefaultMaxInputBytes` — verifies that 0 and negative values use the default
- All existing tests updated to use `NewImageProcessor(0)` (default limit)
Co-authored-by: user <user@Mac.lan guest wan>
Co-authored-by: clawbot <clawbot@eeqj.de>
Reviewed-on: #37
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Closes#28
Migration filenames now follow the pattern `<version>_<description>.sql` (e.g. `001_initial_schema.sql`). The version stored in `schema_migrations` is the numeric prefix only, not the full filename stem.
## Changes
- **`ParseMigrationVersion()`** — new exported function that extracts the numeric prefix from migration filenames. Validates that the prefix is purely numeric and rejects malformed filenames (empty prefix, non-numeric characters, leading underscore).
- **Renamed `001.sql` → `001_initial_schema.sql`** — migration files can now have descriptive names while the tracked version remains `001`. This is safe pre-1.0.0 (no installed base).
- **Deduplicated migration logic** — `runMigrations()` and `ApplyMigrations()` now share a single `applyMigrations()` implementation, plus extracted `collectMigrations()` and `ensureMigrationsTable()` helpers.
- **Unit tests** — `TestParseMigrationVersion` covers valid patterns (version-only, with description, multi-digit, multiple underscores) and error cases (empty, leading underscore, non-numeric, mixed alphanumeric). `TestApplyMigrations` and `TestApplyMigrationsIdempotent` verify end-to-end migration application against an in-memory SQLite database.
Co-authored-by: user <user@Mac.lan guest wan>
Reviewed-on: #33
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>
The golangci-lint binary was hardcoded as linux-amd64, causing Docker builds
to fail on arm64 hosts. The amd64 ELF binary cannot execute on aarch64,
producing a misleading shell syntax error during make check.
Use uname -m to detect the container architecture at build time and download
the matching binary. Both amd64 and arm64 SHA-256 hashes are pinned.
Closes#15
- 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 blank lines before return statements (nlreturn)
- Remove unused metaCacheMu field and sync import (unused)
- Rename unused groups parameter to _ (revive)
- Use StorageFilePerm constant instead of magic 0600 (mnd, gosec)
- Add nolint directive for vipsOnce global (gochecknoglobals)
