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
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)
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>
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>
- Return ErrUnsupportedOutputFormat for WebP/AVIF encoding
- Return ErrInvalidFitMode for unknown fit mode values
- Add ValidateFitMode() for input validation
- Validate fit mode at handler level before processing
Silent fallbacks violate the principle of least surprise and mask bugs.
When a user explicitly specifies a value, we should either honor it or
return an error - never silently substitute a different value.
- StoreOutput now returns output hash for immediate retrieval
- Cache misses now serve from disk file after storing (same as hits)
- Log served_bytes from actual io.Copy result (avoids stat calls)
- Remove ContentLength field usage for cache hits (stream from file)
- Fix tests to properly check all return values
clawbot
