ML-181: harden assets endpoints

backlog/tasks/ml-181 - Harden-public-asset-transforms-against-unbounded-resource-consumption-and-cache-amplification.md

@@ -3,10 +3,10 @@ id: ML-181
 title: >-
   Harden public asset transforms against unbounded resource consumption and
   cache amplification
-status: To Do
+status: Done
 assignee: []
 created_date: "2026-05-13 18:33"
-updated_date: "2026-05-13 19:36"
+updated_date: "2026-05-22 06:52"
 labels:
   - security
   - api
@@ -19,6 +19,12 @@ references:
     backlog/completed/ml-35 -
     Harden-the-public-asset-endpoint-against-invalid-payloads.md
   - doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md
+modified_files:
+  - lib/music_library/assets/transform.ex
+  - lib/music_library/assets/cache.ex
+  - lib/music_library_web/controllers/asset_controller.ex
+  - test/music_library/assets/transform_test.exs
+  - test/music_library_web/controllers/asset_controller_test.exs
 priority: medium
 ordinal: 9000
 ---
@@ -35,17 +41,17 @@ The unauthenticated `GET /public/assets/:transform_payload` endpoint decodes arb
 
 <!-- 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
+- [x] #1 Transform.decode/1 rejects payloads with string width (400 Bad Request)
+- [x] #2 Transform.decode/1 rejects payloads with negative width (400 Bad Request)
+- [x] #3 Transform.decode/1 rejects payloads with zero width (400 Bad Request)
+- [x] #4 Transform.decode/1 rejects payloads with float width (400 Bad Request)
+- [x] #5 Transform.decode/1 rejects payloads with width > 2048 (400 Bad Request)
+- [x] #6 Transform.decode/1 accepts width: nil (original size, no resize)
+- [x] #7 Transform.decode/1 accepts widths in 1..2048 (e.g., 96, 150, 480, 300, 2048)
+- [x] #8 Canonical cache key collates variant payloads for the same (hash, width) into a single ETS entry
+- [x] #9 Existing email image URLs (width: 96) continue to serve correctly
+- [x] #10 API and authenticated asset routes continue to work unchanged
+- [x] #11 Existing controller and LiveView tests pass without modification
 <!-- AC:END -->
 
 ## Implementation Plan
@@ -244,3 +250,29 @@ No production changes required:
 - **`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 -->
+
+## Final Summary
+
+<!-- SECTION:FINAL_SUMMARY:BEGIN -->
+
+### Summary
+
+Added two complementary defenses to the public asset transform endpoint:
+
+1. **Width validation in `Transform.decode/1` and `decode!/1`** — Rejects payloads where `width` is not `nil` or a positive integer in `1..2048`. Invalid payloads return `{:error, :invalid_payload}` (400 Bad Request at the controller level). A private `valid_width?/1` guard handles nil, valid integer range, and all invalid cases (string, negative, zero, float, very large).
+
+2. **Canonical cache key derivation** — Added `Transform.canonical_key/1` producing `"hash:width"` strings. `AssetController` now uses this as the ETS cache key while continuing to use the raw payload string for HTTP ETags. This collapses variant JSON payloads encoding the same (hash, width) into a single cache entry.
+
+### Changes
+
+- `lib/music_library/assets/transform.ex` — Added `@max_width 2048`, `valid_width?/1`, `canonical_key/1`, updated `decode/1`, `decode!/1`, and `@moduledoc`
+- `lib/music_library/assets/cache.ex` — Updated `@moduledoc` Cache key section to describe key as opaque
+- `lib/music_library_web/controllers/asset_controller.ex` — Uses `Transform.canonical_key/1` for ETS cache; HTTP ETag remains raw payload
+- `test/music_library/assets/transform_test.exs` — 8 new unit tests for width validation edge cases + canonical_key doctest
+- `test/music_library_web/controllers/asset_controller_test.exs` — 4 new integration tests (string/negative/large width rejection, cache amplification fix)
+
+### Verification
+
+- Full test suite: **1115 passed** (44 doctests, 1071 tests), 0 failures
+- Existing controller and LiveView tests pass without modification
+<!-- SECTION:FINAL_SUMMARY:END -->

lib/music_library/assets/cache.ex

@@ -4,9 +4,10 @@ defmodule MusicLibrary.Assets.Cache do
 
   ## Cache key
 
-  Each entry is keyed by `{payload, format}` where `payload` is a transform
-  parameter string (encoding width and asset hash) and `format` is the output
-  MIME type (e.g. `"image/webp"`).
+  Each entry is keyed by `{key, format}` where `key` is an opaque canonical
+  transform key provided by the caller (currently `"hash:width"` from
+  `Transform.canonical_key/1`) and `format` is the output MIME type
+  (e.g. `"image/webp"`).
 
   ## TTL and pruning
 

lib/music_library/assets/transform.ex

@@ -1,6 +1,18 @@
 defmodule MusicLibrary.Assets.Transform do
   @moduledoc """
   Represents an image transformation (hash + target width) for asset serving.
+
+  ## Width validation
+
+  `decode/1` validates the `width` field: it must be `nil` (serve original size)
+  or a positive integer in `1..2048`. Any other value (string, negative, zero,
+  float, or very large) returns `{:error, :invalid_payload}`.
+
+  ## Canonical cache key
+
+  `canonical_key/1` produces a deterministic `"hash:width"` string used as the
+  ETS cache key in `MusicLibraryWeb.AssetController`, collapsing variant JSON
+  payloads that encode the same (hash, width) into a single cache entry.
   """
 
   @derive JSON.Encoder
@@ -39,11 +51,19 @@ defmodule MusicLibrary.Assets.Transform do
     iex> Transform.decode("!!!invalid")
     {:error, :invalid_payload}
   """
+  @max_width 2048
+
   @spec decode(payload()) :: {:ok, t()} | {:error, :invalid_payload}
   def decode(payload) do
     with {:ok, decoded} <- Base.url_decode64(payload, padding: false),
          {:ok, params} when is_map(params) <- JSON.decode(decoded) do
-      {:ok, struct!(__MODULE__, %{hash: params["hash"], width: params["width"]})}
+      width = params["width"]
+
+      if valid_width?(width) do
+        {:ok, struct!(__MODULE__, %{hash: params["hash"], width: width})}
+      else
+        {:error, :invalid_payload}
+      end
     else
       _ -> {:error, :invalid_payload}
     end
@@ -57,16 +77,23 @@ defmodule MusicLibrary.Assets.Transform do
   """
   @spec decode!(payload()) :: t()
   def decode!(payload) do
-    params =
-      payload
-      |> Base.url_decode64!(padding: false)
-      |> JSON.decode!()
-
-    struct!(__MODULE__, %{
-      hash: params["hash"],
-      width: params["width"]
-    })
+    case decode(payload) do
+      {:ok, transform} -> transform
+      {:error, :invalid_payload} -> raise ArgumentError, "invalid transform payload"
+    end
   end
+
+  @doc """
+    iex> alias MusicLibrary.Assets.Transform
+    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}"
+
+  defp valid_width?(nil), do: true
+  defp valid_width?(width) when is_integer(width) and width > 0 and width <= @max_width, do: true
+  defp valid_width?(_), do: false
 end
 
 defimpl Phoenix.Param, for: MusicLibrary.Assets.Transform do

lib/music_library_web/controllers/asset_controller.ex

@@ -17,7 +17,9 @@ defmodule MusicLibraryWeb.AssetController do
         bad_request(conn)
 
       {:ok, transform} ->
-        case cached_get(payload, transform, format) do
+        cache_key = Transform.canonical_key(transform)
+
+        case cached_get(cache_key, transform, format) do
           nil ->
             not_found(conn)
 
@@ -30,12 +32,12 @@ defmodule MusicLibraryWeb.AssetController do
     end
   end
 
-  defp cached_get(_payload, transform, _format) when is_nil(transform.hash) do
+  defp cached_get(_cache_key, transform, _format) when is_nil(transform.hash) do
     nil
   end
 
-  defp cached_get(payload, transform, format) do
-    case Cache.get(payload, format) do
+  defp cached_get(cache_key, transform, format) do
+    case Cache.get(cache_key, format) do
       :not_found ->
         if asset = Assets.get(transform.hash) do
           result =
@@ -47,7 +49,7 @@ defmodule MusicLibraryWeb.AssetController do
 
           case result do
             {:ok, image_data} ->
-              Cache.set(payload, format, image_data)
+              Cache.set(cache_key, format, image_data)
               image_data
 
             {:error, reason} ->

test/music_library/assets/transform_test.exs

@@ -2,4 +2,52 @@ defmodule MusicLibrary.Assets.TransformTest do
   use ExUnit.Case, async: true
 
   doctest MusicLibrary.Assets.Transform
+
+  alias MusicLibrary.Assets.Transform
+
+  defp make_payload(params) do
+    params |> JSON.encode!() |> Base.url_encode64(padding: false)
+  end
+
+  describe "decode/1 width validation" do
+    test "rejects string width" do
+      payload = make_payload(%{hash: "abc", width: "300"})
+      assert {:error, :invalid_payload} = Transform.decode(payload)
+    end
+
+    test "rejects negative width" do
+      payload = make_payload(%{hash: "abc", width: -1})
+      assert {:error, :invalid_payload} = Transform.decode(payload)
+    end
+
+    test "rejects zero width" do
+      payload = make_payload(%{hash: "abc", width: 0})
+      assert {:error, :invalid_payload} = Transform.decode(payload)
+    end
+
+    test "rejects float width" do
+      payload = make_payload(%{hash: "abc", width: 300.5})
+      assert {:error, :invalid_payload} = Transform.decode(payload)
+    end
+
+    test "rejects very large width" do
+      payload = make_payload(%{hash: "abc", width: 99_999})
+      assert {:error, :invalid_payload} = Transform.decode(payload)
+    end
+
+    test "accepts nil width" do
+      payload = make_payload(%{hash: "abc", width: nil})
+      assert {:ok, %Transform{hash: "abc", width: nil}} = Transform.decode(payload)
+    end
+
+    test "accepts max allowed width 2048" do
+      payload = make_payload(%{hash: "abc", width: 2048})
+      assert {:ok, %Transform{hash: "abc", width: 2048}} = Transform.decode(payload)
+    end
+
+    test "accepts width 1" do
+      payload = make_payload(%{hash: "abc", width: 1})
+      assert {:ok, %Transform{hash: "abc", width: 1}} = Transform.decode(payload)
+    end
+  end
 end

test/music_library_web/controllers/asset_controller_test.exs

@@ -87,6 +87,67 @@ defmodule MusicLibraryWeb.AssetControllerTest do
       assert text_response(conn, 404) == "Not found"
     end
 
+    test "400s when payload has string width", %{conn: conn, asset: asset} do
+      payload =
+        %{hash: asset.hash, width: "300"}
+        |> JSON.encode!()
+        |> Base.url_encode64(padding: false)
+
+      conn = get(conn, ~p"/assets/#{payload}")
+      assert text_response(conn, 400) == "Bad request"
+    end
+
+    test "400s when payload has negative width", %{conn: conn, asset: asset} do
+      payload =
+        %{hash: asset.hash, width: -1}
+        |> JSON.encode!()
+        |> Base.url_encode64(padding: false)
+
+      conn = get(conn, ~p"/assets/#{payload}")
+      assert text_response(conn, 400) == "Bad request"
+    end
+
+    test "400s when payload has very large width", %{conn: conn, asset: asset} do
+      payload =
+        %{hash: asset.hash, width: 99_999}
+        |> JSON.encode!()
+        |> Base.url_encode64(padding: false)
+
+      conn = get(conn, ~p"/assets/#{payload}")
+      assert text_response(conn, 400) == "Bad request"
+    end
+
+    test "canonical cache key collapses variant payloads into single ETS entry", %{
+      conn: conn,
+      asset: asset
+    } do
+      # Two variant payloads encoding the same (hash, width)
+      payload_a =
+        %{hash: asset.hash, width: 480}
+        |> JSON.encode!()
+        |> Base.url_encode64(padding: false)
+
+      # Same hash/width but different JSON key order / whitespace
+      payload_b =
+        %{width: 480, hash: asset.hash}
+        |> JSON.encode!()
+        |> Base.url_encode64(padding: false)
+
+      # First request populates the cache
+      conn_a = get(conn, ~p"/assets/#{payload_a}")
+      assert conn_a.status == 200
+      assert get_resp_header(conn_a, "etag") == [payload_a]
+
+      # Second request with different payload but same canonical key
+      # should serve from cache (same content, correct ETag for this payload)
+      conn_b = get(conn, ~p"/assets/#{payload_b}")
+      assert conn_b.status == 200
+      assert get_resp_header(conn_b, "etag") == [payload_b]
+
+      # Both should return the same image content
+      assert conn_a.resp_body == conn_b.resp_body
+    end
+
     @tag :capture_log
     test "404s when asset content is corrupt", %{conn: conn} do
       {:ok, asset} = Assets.store(%{content: "not an image", format: "image/jpeg"})