Refactor: split internal/imgcache into focused packages #39
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/pixa#39
Reference in New Issue
Block a user
Per PR #37 discussion,
internal/imgcache/currently bundles many distinct concerns into a single package. This issue tracks splitting it into focused, independently testable packages.Already Done
internal/imageprocessor/— image format conversion and resizing (extracted in PR #37)internal/allowlist/— host allow list (extracted in PR #41, renamed from whitelist per sneak's feedback)internal/magic/— magic byte detection (extracted in PR #42)internal/httpfetcher/— SSRF-safe fetcher (extracted in PR #43)internal/signature/— HMAC signing and verification (extracted in PR #46)Remaining
urlparser.go) →internal/urlparser/ParsedURL,ParseImagePath,ParseImageURL, path traversal validation, size/format parsingImageRequest,Size,ImageFormat,FitMode— would need to import shared types, or define standalone result types the caller maps onto the imgcache vocabularyurlparser_test.gowith it; keep coverage of the path-traversal rejection cases intactThis is the last extraction in scope for this issue. Once it lands, this issue closes.
Tightly Coupled (keep together, out of scope)
These share significant state and data flow with the service orchestrator. Splitting them further would require significant interface surgery and may not be worth it:
cache.go) — cache lookup, variant storage, negative cache, stats. Tightly coupled withservice.goandstorage.go.storage.go) — content-addressedContentStorageandMetadataStorage. Created and managed byCache.service.go) — the core orchestrator that wires cache, fetcher, and processor together.imgcache.go) —ImageRequest,ImageResponse,ImageFormat,FitMode,Size, etc. These are the shared vocabulary used across all subpackages.Definition of Done
internal/urlparser/exists and contains the URL/path parsing logic and its tests;internal/imgcache/urlparser.goandurlparser_test.goare gone.urlparserdoes not importimgcache.New(), notNewURLParser()).make checkgreen anddocker build .passes.Approach
Each extraction should:
internal/docker build .passes (fmt, lint, test, build)Progress update on the
internal/imgcachesplit.Done and merged into
main:imageprocessor->internal/imageprocessor/(PR #37)allowlist->internal/allowlist/(PR #41)magic->internal/magic/(PR #42)httpfetcher->internal/httpfetcher/(PR #43, merged 2026-07-25)This pass:
refactor/extract-signature) extractingsignature->internal/signature/, based on currentmain. It is independent of thehttpfetchermove, so no dependency on other in-flight branches.Signer(New,Sign,Verify,GenerateSignedURL),ParseParams, and the error sentinels. To keep the import edge one-way, the package defines a standalonesignature.Request;imgcacheprojects itsImageRequestonto it via a small helper (same approachmagicused forImageFormat).NewSigner->New,ParseSignatureParams->ParseParams,ErrSignature{Required,Invalid,Expired}->Err{Required,Invalid,Expired}. HMAC input bytes are unchanged, so existing signatures still verify.docker build .passes (fmt-check, lint, test, build). Labelledneeds-review, awaiting merge into protectedmain.Remaining "easily separable" extraction:
urlparser(urlparser.go) ->internal/urlparser/. Note it is consumed outsideimgcache(byinternal/handlers/image.goviaParseImagePath/ParsedURL) and couples toImageRequest,Size,ImageFormat,FitMode, so it needs standalone shared types on the extracted side. This is the natural next PR once #46 lands.The "tightly coupled" group (
cache,storage,service, and the shared types inimgcache.go) stays together as planned.Direction for reworking PR #46 (currently labelled
needs-rework): the two suggestions the previous review marked "non-blocking" are blocking, per sneak's comment on the PR. Both must land onrefactor/extract-signaturebefore re-review:Golden known-answer test in
internal/signature: a test with a fixed signing key and a fully-specifiedRequest, asserting the exact base64url signature string, plus a companion assertion for the exact generated signed URL. This pins the on-the-wire byte format (host:path:query:width:height:format:expiration) so any future change to the signed bytes fails loudly instead of silently invalidating issued URLs.Finish the allowlist terminology migration inside
internal/imgcache: renameWithNoWhitelist()and every other whitelist-named identifier, test function name, and comment in the package to allowlist equivalents, consistent withinternal/allowlist. User-facing YAML config keys stay unchanged in this PR; if any whitelist-named config surface remains, note it on the PR for a follow-up.Process: work on the existing
refactor/extract-signaturebranch, one logical change per commit,make checkgreen before each commit, push when done, then summarize the rework in a comment on PR #46. All further discussion of the rework itself belongs on the PR.Additional rework direction for PR #46, per sneak's comment there (2026-08-07): the whitelist→allowlist rename must cover the user-facing config surface in the same pass — pre-1.0, there is no installed base and no deprecation window. Concretely, on
refactor/extract-signature:whitelist_hosts→allowlist_hosts(no back-compat alias for the old key).Config.WhitelistHosts→Config.AllowlistHostsininternal/config/config.go, plus every reference (including any existing test call sites — this mechanical rename is explicitly authorized by the one-pass mandate; test assertions themselves must not be weakened).config.example.ymlandREADME.mdupdated to the new key. NoteREADME.mdcurrently documents the key assource_host_whitelist, which does not match the actualwhitelist_hostskey — fix the README to the real, renamed key while there.whitelist/Whitelistafterward; the only acceptable survivors are historical entries inTODO.md/commit history.make checkgreen (script/cibuildfor the authoritative lint), push, then summarize on PR #46.Status checkpoint (session handoff — a fresh session will pick up from the tracker):
signatureextraction + full whitelist→allowlist rename including theallowlist_hostsconfig key) is merged intomainat6573b9d. Done extractions:imageprocessor,allowlist,magic,httpfetcher,signature.urlparser(urlparser.go) →internal/urlparser/. Note from the earlier analysis: it is consumed outsideimgcache(byinternal/handlers/image.goviaParseImagePath/ParsedURL) and couples toImageRequest,Size,ImageFormat,FitMode, so it needs standalone types on the extracted side, mirroring howsignature.Requestandmagic'sImageFormatwere handled. That is the natural next PR here; the tightly coupled remainder (cache,storage,service, shared types) stays together, after which this issue can close.whiteliststring in the repo is the illustrative naming example inCLAUDE.mdline 51 (SourceHostWhitelistas a good-naming example). Say the word and it becomesSourceHostAllowlistin a trivial follow-up.