diff --git a/backlog/docs/doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md b/backlog/docs/doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md new file mode 100644 index 00000000..36ee476c --- /dev/null +++ b/backlog/docs/doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md @@ -0,0 +1,117 @@ +--- +id: doc-21 +title: ML-181 Research - Public Asset Transform Hardening Routes +type: other +created_date: "2026-05-13 18:36" +--- + +# ML-181 Research: Public Asset Transform Hardening Routes + +## Problem Summary + +The unauthenticated `GET /public/assets/:transform_payload` endpoint (router.ex:61) decodes arbitrary Base64 JSON via `Transform.decode/1` without validating the `width` field. An attacker with a valid asset hash — obtainable from public email image URLs generated by `RecordsOnThisDayEmail.cover_image_url/2` — can craft variant payloads that: + +1. **Force repeated native libvips resize attempts** — Each unique payload that misses the ETS cache triggers `Vix.Vips.Operation.thumbnail_buffer/3` (`Image.resize/3`). +2. **Amplify ETS cache entries** — Cache key is `{payload, format}` where `payload` is the raw path segment string. Variant JSON representations (whitespace, key ordering, extra fields) create distinct cache entries for the same logical transform. + +**Current legitimate width values:** + +- `nil` — Original size (authenticated `/assets/` routes, API) +- `96` — Email thumbnails (`cover_image_url/2`) +- `150` — API mini covers (`collection_json.ex`) +- `480` — API thumbnails (`collection_json.ex`) + +The public route only uses `width: 96` (email). The authenticated routes and API routes are behind session/auth. + +## Route 1: Width Validation in Transform.decode/1 (Minimal) + +**Change:** Add validation in `Transform.decode/1` that `width` must be nil or a positive integer within a bounded range (e.g., ≤ 2048 or explicit allowlist `[96, 150, 480]`). Reject with `{:error, :invalid_payload}`. + +**Files touched:** + +- `lib/music_library/assets/transform.ex` — Add `validate_width/1` guard in decode + +**Pros:** + +- Single file change +- Directly blocks unbounded resource consumption +- Existing controller error handling already returns 400 for `:invalid_payload` + +**Cons:** + +- Does not address cache amplification from variant payloads (whitespace, key ordering, extra fields) for the same valid width +- Allowlist approach requires updating when new legitimate widths are added + +**Cache amplification remains:** `{"hash":"abc","width":96}` and `{"width":96,"hash":"abc"}` produce different cache entries for the same logical transform. + +--- + +## Route 2: Canonical Cache Key (Structural) + +**Change:** Derive ETS cache key from a canonical representation of the validated Transform struct instead of the raw payload string. E.g., `"hash:width"` or a binary term. + +**Files touched:** + +- `lib/music_library/assets/cache.ex` — Update key format and get/set functions +- `lib/music_library_web/controllers/asset_controller.ex` — Pass canonical key instead of raw payload to Cache + +**Pros:** + +- Eliminates cache amplification entirely — unique cache entries = unique (hash, width) pairs +- ETag can remain the raw payload for HTTP caching correctness, or switch to canonical key + +**Cons:** + +- Does not validate width — still needs Route 1 +- Requires coordinated change in Cache and Controller +- ETag change: if ETag switches to canonical key, existing cached browser responses are invalidated (acceptable — they'd re-fetch) + +--- + +## Route 3: Combined — Width Validation + Canonical Cache Key (Recommended) + +**Change:** Validate width in `Transform.decode/1` AND derive cache key from the canonical struct. + +**Files touched:** + +- `lib/music_library/assets/transform.ex` — Add `validate_width/1` in decode; add `canonical_key/1` +- `lib/music_library/assets/cache.ex` — No changes needed (key format stays `{payload, format}`, but payload is canonical) +- `lib/music_library_web/controllers/asset_controller.ex` — Use canonical key for Cache calls +- `test/music_library/assets/transform_test.exs` — Add validation tests +- `test/music_library_web/controllers/asset_controller_test.exs` — Add attacks as tests + +**Pros:** + +- Addresses both resource consumption and cache amplification +- Each change is small and independently verifiable +- No architectural changes — same modules, same data flow + +**Cons:** + +- Changes the cache key derivation for existing cached entries (acceptable — entries are ephemeral and TTL-based) + +--- + +## Route 4: Payload Signing (Comprehensive, Deferred) + +**Change:** HMAC-sign transform payloads using the app's secret key. Only server-generated URLs with valid signatures are accepted. + +**Pros:** + +- Strongest defense: attackers cannot construct any valid payload +- No need for width allowlists (signature proves server origin) + +**Cons:** + +- Adds key management complexity (signing key must be consistent across deploys) +- Requires changes at every URL generation site (email, API JSON, LiveView tests) +- Overkill for single-operator threat model — asset hash obscurity is sufficient +- Signature in URL makes email URLs longer and less cacheable + +**Verdict:** Deferred. Revisit if the app becomes multi-user or asset hashes become guessable. + +--- + +## Recommendation + +**Route 3 (Combined)** is the right trade-off. It addresses the finding completely with minimal complexity. Route 4 is documented for future consideration. diff --git a/backlog/tasks/ml-181 - Harden-public-asset-transforms-against-unbounded-resource-consumption-and-cache-amplification.md b/backlog/tasks/ml-181 - Harden-public-asset-transforms-against-unbounded-resource-consumption-and-cache-amplification.md new file mode 100644 index 00000000..3eb116ff --- /dev/null +++ b/backlog/tasks/ml-181 - Harden-public-asset-transforms-against-unbounded-resource-consumption-and-cache-amplification.md @@ -0,0 +1,245 @@ +--- +id: ML-181 +title: >- + Harden public asset transforms against unbounded resource consumption and + cache amplification +status: To Do +assignee: [] +created_date: "2026-05-13 18:33" +updated_date: "2026-05-13 19:27" +labels: + - security + - public-asset-endpoint + - validation +dependencies: [] +references: + - doc-19 - GPT-5.5-Security-Review.md + - >- + backlog/completed/ml-35 - + Harden-the-public-asset-endpoint-against-invalid-payloads.md + - doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md +priority: medium +ordinal: 9000 +--- + +## Description + + + +The unauthenticated `GET /public/assets/:transform_payload` endpoint decodes arbitrary JSON into a `%Transform{}` struct without validating `width` type, range, or maximum. An attacker with a valid asset hash (obtainable from public email image URLs) can craft unlimited variant payloads, forcing repeated native libvips resize attempts and amplifying ETS cache entries. Prior hardening (ml-35, commit `92a36b91`) fixed crashes from malformed payloads but left resource-control validation open. + + + +## Acceptance Criteria + + + +- [ ] #1 Transform.decode/1 rejects payloads with string width (400 Bad Request) +- [ ] #2 Transform.decode/1 rejects payloads with negative width (400 Bad Request) +- [ ] #3 Transform.decode/1 rejects payloads with zero width (400 Bad Request) +- [ ] #4 Transform.decode/1 rejects payloads with float width (400 Bad Request) +- [ ] #5 Transform.decode/1 rejects payloads with width > 2048 (400 Bad Request) +- [ ] #6 Transform.decode/1 accepts width: nil (original size, no resize) +- [ ] #7 Transform.decode/1 accepts widths in 1..2048 (e.g., 96, 150, 480, 300, 2048) +- [ ] #8 Canonical cache key collates variant payloads for the same (hash, width) into a single ETS entry +- [ ] #9 Existing email image URLs (width: 96) continue to serve correctly +- [ ] #10 API and authenticated asset routes continue to work unchanged +- [ ] #11 Existing controller and LiveView tests pass without modification + + +## Implementation Plan + + + +## Objective Alignment + +This plan addresses F1 from the security review by adding two complementary defenses: + +1. **Width validation in `Transform.decode/1` and `decode!/1`** — Rejects payloads where `width` is not `nil` or a positive integer in range `1..2048`, preventing unbounded libvips thumbnail operations. +2. **Canonical cache key derivation** — Derives ETS cache keys from the validated `%Transform{hash, width}` struct instead of the raw payload string, eliminating cache amplification from variant JSON representations. HTTP ETag remains the raw payload to preserve existing test assertions and browser caching behavior. + +Together these close both the resource consumption vector (arbitrary widths → native image processing) and the cache amplification vector (variant payload strings → unbounded ETS entries). + +## Alternatives Considered + +See `doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md` for full analysis. Route 3 (Combined) was selected as the simplest approach that fully addresses the finding. Route 4 (payload signing) is deferred as overkill for the single-operator threat model. + +## Implementation Steps + +### Step 1: Add width validation to `Transform.decode/1` and `decode!/1` + +**File:** `lib/music_library/assets/transform.ex` + +Add a private `valid_width?/1` guard that checks: + +- `nil` → valid (no resize, serve original) +- Positive integer in `1..2048` → valid +- Anything else (string, negative, zero, float, very large) → invalid + +Update `decode/1` to call `valid_width?/1` after parsing params, before constructing the struct. Return `{:error, :invalid_payload}` on validation failure. + +Update `decode!/1` to delegate to `decode/1` and raise on the error tuple, keeping the `!` convention consistent: + +```elixir +def decode!(payload) do + case decode(payload) do + {:ok, transform} -> transform + {:error, :invalid_payload} -> raise ArgumentError, "invalid transform payload" + end +end +``` + +This replaces the current `decode!/1` body which manually decodes base64/JSON without validation. + +**Dependencies:** None +**Verification:** + +- Run doctests: `mix test test/music_library/assets/transform_test.exs` +- The existing doctest (`width: 300`) still passes (300 is in range) +- The `decode!/1` doctest still passes (same valid payload now flows through validation) +- New unit tests (see Step 4) confirm rejection of edge cases + +### Step 2: Add `canonical_key/1` to `Transform` module + +**File:** `lib/music_library/assets/transform.ex` + +Add a public function with a doctest: + +```elixir +@doc """ + iex> Transform.canonical_key(%Transform{hash: "abc123", width: 96}) + "abc123:96" +""" +@spec canonical_key(t()) :: String.t() +def canonical_key(%__MODULE__{hash: hash, width: width}), do: "#{hash}:#{width}" +``` + +**Dependencies:** Step 1 (struct is now guaranteed valid) +**Verification:** Doctest confirms deterministic output + +### Step 3: Update `AssetController` to use canonical cache key (ETS only) + +**File:** `lib/music_library_web/controllers/asset_controller.ex` + +Changes: + +1. In `show/2`, after successful decode, compute `cache_key = Transform.canonical_key(transform)` +2. Pass `cache_key` (not `payload`) as the first argument to `cached_get/3` +3. In `cached_get/3`, use the first argument for `Cache.get/2` and `Cache.set/3` — rename the parameter from `payload` to `cache_key` for clarity +4. Keep `payload` as the HTTP ETag value (unchanged) — the `if-none-match` comparison in `show/2` and the `respond_with_cache/5` call continue to use the raw `payload` string + +The separation is: + +- **ETS cache key:** canonical `"hash:width"` — prevents cache amplification +- **HTTP ETag:** raw payload string — preserves existing test assertions on `get_resp_header(conn, "etag")` + +**Dependencies:** Steps 1 and 2 +**Verification:** + +- Existing controller tests pass without modification: `mix test test/music_library_web/controllers/asset_controller_test.exs` + - ETag assertions all check against the raw `payload`, which is unchanged +- Existing LiveView tests pass (they use Transform for cover URLs): `mix test test/music_library_web/live/collection_live/` + +### Step 4: Add tests + +**File:** `test/music_library/assets/transform_test.exs` + +Add tests for `decode/1` with invalid widths: + +- String width: `%{hash: "abc", width: "300"}` → `{:error, :invalid_payload}` +- Negative width: `%{hash: "abc", width: -1}` → `{:error, :invalid_payload}` +- Zero width: `%{hash: "abc", width: 0}` → `{:error, :invalid_payload}` +- Float width: `%{hash: "abc", width: 300.5}` → `{:error, :invalid_payload}` +- Very large width: `%{hash: "abc", width: 99999}` → `{:error, :invalid_payload}` +- Nil width: `%{hash: "abc", width: nil}` → valid +- Max allowed width: `%{hash: "abc", width: 2048}` → valid + +Add a doctest for `canonical_key/1` (included in Step 2 above). + +**File:** `test/music_library_web/controllers/asset_controller_test.exs` + +Add integration tests: + +- Payload with string width returns 400 +- Payload with negative width returns 400 +- Payload with very large width returns 400 + +Add test for cache amplification fix: + +- Two variant payloads encoding the same (hash, width) produce the same cache entry (verify by checking that the second request does not trigger a new resize — either inspect ETS entry count or verify response timing) + +**Dependencies:** Steps 1-3 +**Verification:** `mix test` — all tests pass + +### Step 5: Update inline module documentation + +**File:** `lib/music_library/assets/transform.ex` + +Update `@moduledoc` to document: + +- The width validation rules: `nil` (original size) or positive integer `1..2048` +- The `canonical_key/1` function and its purpose for cache deduplication + +**File:** `lib/music_library/assets/cache.ex` + +Update the "Cache key" section of `@moduledoc`: + +- Remove: "where `payload` is a transform parameter string (encoding width and asset hash)" +- Replace with: "where the key string is an opaque canonical transform key provided by the caller (currently `"hash:width"` from `Transform.canonical_key/1`)" + +This reflects that Cache no longer knows or cares about the key's origin — it's just an opaque string. + +**Dependencies:** Steps 1-3 +**Verification:** Review updated doc output with `h MusicLibrary.Assets.Transform` and `h MusicLibrary.Assets.Cache` + +## Architecture Impact Analysis + +| Touchpoint | Impact | +| --------------------------------------- | --------------------------------------------------------------------------------------- | +| `MusicLibrary.Assets.Transform` | Adds validation to `decode/1` and `decode!/1` + new `canonical_key/1` function | +| `MusicLibraryWeb.AssetController` | Uses canonical key for ETS Cache; HTTP ETag continues to use raw payload | +| `MusicLibrary.Assets.Cache` | No code change — key format remains `{string, format}`, but the string is now canonical | +| `MusicLibrary.Assets.Image` | No change | +| `MusicLibraryWeb.RecordsOnThisDayEmail` | No change — already uses valid width (96) | +| `MusicLibraryWeb.CollectionJSON` | No change — uses valid widths (nil, 150, 480) | +| All LiveViews using Transform | No change — no width or valid widths only | +| Routes | No change | +| ETS cache | Existing entries invalidated on deploy restart (in-memory, expected) | +| HTTP caching (ETag/If-None-Match) | No change — raw payload remains the ETag, preserving existing browser-cached responses | +| External APIs | Not affected | + +## Performance Profile + +- **`valid_width?/1`:** O(1) — two guard checks (`is_nil` and `is_integer and > 0 and ≤ 2048`) +- **`canonical_key/1`:** O(1) — string concatenation of two fields +- **Cache lookup:** Unchanged — single `:ets.lookup/2` per request +- **Memory:** Reduced — canonical keys collapse variant payloads into single entries +- **N+1 risk:** None — no new queries introduced +- **Latency impact:** Negligible (< 1µs for validation + key concatenation) + +## Benchmarking Requirements + +No benchmarks needed. The change removes work (fewer cache entries, early rejection of invalid payloads) rather than adding it. + +## Cost Profile + +No cost impact — no paid resources are consumed. + +## Production Infrastructure Steps + +No production changes required: + +- No environment variables +- No database migrations +- No service provisioning +- ETS cache is in-memory and auto-clears on application restart during deploy +- No DNS, firewall, or Coolify configuration changes + +## Documentation Updates + +- **`doc-21`** (already created): Contains the full research analysis and route comparison +- **`Transform` `@moduledoc`**: Updated in Step 5 to document width validation rules and `canonical_key/1` +- **`Cache` `@moduledoc`**: Updated in Step 5 to describe the key as opaque rather than raw-payload-specific +- **`docs/architecture.md`**: No update needed — asset serving path is not documented at this granularity +- **`docs/project-conventions.md`**: No update needed +