ML-181: hardening asset routes (plan)
This commit is contained in:
Claudio Ortolina
+245
@@ -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
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
|
||||
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.
|
||||
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
<!-- AC:BEGIN -->
|
||||
|
||||
- [ ] #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
|
||||
<!-- AC:END -->
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
<!-- SECTION:PLAN:BEGIN -->
|
||||
|
||||
## 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
|
||||
<!-- SECTION:PLAN:END -->
|
||||
Reference in New Issue
Block a user