Harden public asset endpoint against invalid payloads

Closes #143

lib/music_library/assets/image.ex

@@ -22,14 +22,17 @@ defmodule MusicLibrary.Assets.Image do
       [:music_library, :assets, :image, :resize],
       %{},
       fn ->
-        {:ok, thumb} = Operation.thumbnail_buffer(cover_data, size)
+        with {:ok, thumb} <- Operation.thumbnail_buffer(cover_data, size),
-        result = Image.write_to_buffer(thumb, extension(format))
+             {:ok, _binary} = result <- Image.write_to_buffer(thumb, extension(format)) do
-        {result, %{}}
+          {result, %{}}
+        else
+          {:error, _reason} = error -> {error, %{}}
+        end
       end
     )
   end
 
-  @spec convert(binary(), String.t(), String.t()) :: {:ok, binary()}
+  @spec convert(binary(), String.t(), String.t()) :: {:ok, binary()} | {:error, term()}
   def convert(cover_data, data_format, target_format) do
     if data_format == target_format do
       {:ok, cover_data}
@@ -38,9 +41,12 @@ defmodule MusicLibrary.Assets.Image do
         [:music_library, :assets, :image, :convert],
         %{},
         fn ->
-          {:ok, image} = Image.new_from_buffer(cover_data)
+          with {:ok, image} <- Image.new_from_buffer(cover_data),
-          result = Image.write_to_buffer(image, extension(target_format))
+               {:ok, _binary} = result <- Image.write_to_buffer(image, extension(target_format)) do
-          {result, %{}}
+            {result, %{}}
+          else
+            {:error, _reason} = error -> {error, %{}}
+          end
         end
       )
     end

lib/music_library/assets/transform.ex

@@ -25,6 +25,30 @@ defmodule MusicLibrary.Assets.Transform do
     |> Base.url_encode64(padding: false)
   end
 
+  @doc """
+  Decodes a Base64-encoded JSON payload into a transform struct.
+
+  Returns `{:error, :invalid_payload}` if the payload is not valid Base64 or JSON.
+
+    iex> alias MusicLibrary.Assets.Transform
+    iex> payload = "eyJoYXNoIjoiYWJjMTIzIiwid2lkdGgiOjMwMH0"
+    iex> Transform.decode(payload)
+    {:ok, %Transform{hash: "abc123", width: 300}}
+
+    iex> alias MusicLibrary.Assets.Transform
+    iex> Transform.decode("!!!invalid")
+    {:error, :invalid_payload}
+  """
+  @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"]})}
+    else
+      _ -> {:error, :invalid_payload}
+    end
+  end
+
   @doc """
     iex> alias MusicLibrary.Assets.Transform
     iex> payload = "eyJoYXNoIjoiYWJjMTIzIiwid2lkdGgiOjMwMH0"

lib/music_library_web/controllers/asset_controller.ex

@@ -1,6 +1,8 @@
 defmodule MusicLibraryWeb.AssetController do
   use MusicLibraryWeb, :controller
 
+  require Logger
+
   alias MusicLibrary.Assets
   alias MusicLibrary.Assets.{Cache, Image, Transform}
 
@@ -9,16 +11,21 @@ defmodule MusicLibraryWeb.AssetController do
 
   def show(conn, %{"transform_payload" => payload}) do
     format = pick_format(conn)
-    transform = Transform.decode!(payload)
 
-    case cached_get(payload, transform, format) do
+    case Transform.decode(payload) do
-      nil ->
+      {:error, :invalid_payload} ->
-        not_found(conn)
+        bad_request(conn)
 
-      content when is_binary(content) ->
+      {:ok, transform} ->
-        case get_req_header(conn, "if-none-match") do
+        case cached_get(payload, transform, format) do
-          [^payload] -> extend_cache(conn)
+          nil ->
-          _ -> respond_with_cache(conn, content, format, payload)
+            not_found(conn)
+
+          content when is_binary(content) ->
+            case get_req_header(conn, "if-none-match") do
+              [^payload] -> extend_cache(conn)
+              _ -> respond_with_cache(conn, content, format, payload)
+            end
         end
     end
   end
@@ -31,15 +38,22 @@ defmodule MusicLibraryWeb.AssetController do
     case Cache.get(payload, format) do
       :not_found ->
         if asset = Assets.get(transform.hash) do
-          {:ok, image_data} =
+          result =
             if transform.width do
               Image.resize(asset.content, transform.width, format)
             else
               Image.convert(asset.content, asset.format, format)
             end
 
-          Cache.set(payload, format, image_data)
+          case result do
-          image_data
+            {:ok, image_data} ->
+              Cache.set(payload, format, image_data)
+              image_data
+
+            {:error, reason} ->
+              Logger.error("Asset transform failed for #{transform.hash}: #{inspect(reason)}")
+              nil
+          end
         end
 
       {:found, content} ->
@@ -61,6 +75,12 @@ defmodule MusicLibraryWeb.AssetController do
     end
   end
 
+  defp bad_request(conn) do
+    conn
+    |> put_status(:bad_request)
+    |> text("Bad request")
+  end
+
   defp not_found(conn) do
     conn
     |> put_status(:not_found)

test/music_library_web/controllers/asset_controller_test.exs

@@ -70,6 +70,33 @@ defmodule MusicLibraryWeb.AssetControllerTest do
       assert conn.resp_body == <<>>
     end
 
+    test "400s on invalid base64 payload", %{conn: conn} do
+      conn = get(conn, ~p"/assets/!!!invalid-base64!!!")
+      assert text_response(conn, 400) == "Bad request"
+    end
+
+    test "400s on valid base64 but invalid JSON payload", %{conn: conn} do
+      payload = Base.url_encode64("not json", padding: false)
+      conn = get(conn, ~p"/assets/#{payload}")
+      assert text_response(conn, 400) == "Bad request"
+    end
+
+    test "404s when payload has null hash", %{conn: conn} do
+      payload = Base.url_encode64(JSON.encode!(%{hash: nil, width: nil}), padding: false)
+      conn = get(conn, ~p"/assets/#{payload}")
+      assert text_response(conn, 404) == "Not found"
+    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"})
+      transform = %Transform{hash: asset.hash, width: 480}
+      payload = Transform.encode!(transform)
+
+      conn = get(conn, ~p"/assets/#{payload}")
+      assert text_response(conn, 404) == "Not found"
+    end
+
     test "handles transforms with width", %{conn: conn, asset: asset} do
       transform = %Transform{hash: asset.hash, width: 480}
       payload = Transform.encode!(transform)