6526b717dd7090f9b684b8608242a0722306ebed
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  
|
b6e9ac2a93 |
refactor: extract httpfetcher package from imgcache (#43)
check / check (push) Successful in 4s
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> |
||
|
clawbotanduser
|
6b4a1d7607 |
refactor: extract magic byte detection into internal/magic package (#42)
check / check (push) Successful in 1m39s
## 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> |
||
|
|
e34743f070 |
refactor: extract whitelist package from internal/imgcache (#41)
check / check (push) Successful in 4s
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> |
||
|
|
7010d55d72 |
Move schema_migrations table creation into 000.sql (#36)
check / check (push) Successful in 1m43s
## 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> |
||
|
clawbotanduser
|
a50364bfca |
Enforce and document exact-match-only for signature verification (#40)
check / check (push) Successful in 58s
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> |
||
|
|
e85b5ff033 |
Consolidate appname to internal/globals as a constant (#34)
check / check (push) Successful in 1m48s
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> |
||
|
|
55a609dd77 |
Bound imageprocessor.Process input read to prevent unbounded memory use (#37)
check / check (push) Successful in 4s
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> |
||
|
clawbotanduser
|
9c29cb57df |
feat: parse version prefix from migration filenames (#33)
check / check (push) Successful in 1m49s
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> |
||
|
clawbotanduser
|
2e934c8894 |
fix: QA audit fixes for 1.0/MVP readiness (#25)
check / check (push) Successful in 5s
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> |
||
|
clawbotandclawbot
|
2f15340f26 |
Split Dockerfile: pre-built golangci-lint stage for faster CI (#23)
check / check (push) Successful in 5s
## Summary Splits the Dockerfile into a dedicated lint stage using the pre-built `golangci/golangci-lint:v2.10.1-alpine` Docker image, replacing the manual binary download with curl/sha256 verification. ## Changes - **Lint stage** (`AS lint`): Uses `golangci/golangci-lint:v2.10.1-alpine` pinned by sha256. Runs `make fmt-check` + `make lint`. Includes CGO deps (`build-base`, `vips-dev`, `libheif-dev`, `pkgconfig`) needed for type-checking govips imports. - **Build stage** (`AS builder`): Depends on lint stage via `COPY --from=lint /src/go.sum /dev/null`. Runs `make test` + builds the binary. Removes `curl` (no longer needed) and the manual golangci-lint download block. - **Runtime stage**: Unchanged. ## Benefits - Eliminates slow multi-arch binary download + sha256 verification step - Lint and build stages can potentially run in parallel with BuildKit - Better Docker layer caching — lint deps cached separately from build deps - All images remain pinned by sha256 with version+date comments ## Verification - `docker build .` passes: fmt-check ✅, lint (0 issues) ✅, all tests pass ✅, binary builds ✅ Closes [#18](#18) <!-- session: agent:sdlc-manager:subagent:7aac9c54-81c8-4494-94ab-0843f97a1e62 --> Co-authored-by: clawbot <clawbot@noreply.git.eeqj.de> Reviewed-on: #23 Co-authored-by: clawbot <clawbot@noreply.example.org> Co-committed-by: clawbot <clawbot@noreply.example.org> |