From 92a36b915ad0fa611a852f5ad979313f9fbd5802 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Mon, 30 Mar 2026 14:56:40 +0100 Subject: [PATCH] Harden public asset endpoint against invalid payloads Closes #143 --- lib/music_library/assets/image.ex | 20 +++++---- lib/music_library/assets/transform.ex | 24 +++++++++++ .../controllers/asset_controller.ex | 42 ++++++++++++++----- .../controllers/asset_controller_test.exs | 27 ++++++++++++ 4 files changed, 95 insertions(+), 18 deletions(-) diff --git a/lib/music_library/assets/image.ex b/lib/music_library/assets/image.ex index daa2844d..2f5c8073 100644 --- a/lib/music_library/assets/image.ex +++ b/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) - result = Image.write_to_buffer(thumb, extension(format)) - {result, %{}} + with {:ok, thumb} <- Operation.thumbnail_buffer(cover_data, size), + {:ok, _binary} = result <- Image.write_to_buffer(thumb, extension(format)) do + {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) - result = Image.write_to_buffer(image, extension(target_format)) - {result, %{}} + with {:ok, image} <- Image.new_from_buffer(cover_data), + {:ok, _binary} = result <- Image.write_to_buffer(image, extension(target_format)) do + {result, %{}} + else + {:error, _reason} = error -> {error, %{}} + end end ) end diff --git a/lib/music_library/assets/transform.ex b/lib/music_library/assets/transform.ex index eb342dde..7b97e6a7 100644 --- a/lib/music_library/assets/transform.ex +++ b/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" diff --git a/lib/music_library_web/controllers/asset_controller.ex b/lib/music_library_web/controllers/asset_controller.ex index 548122be..40bcccee 100644 --- a/lib/music_library_web/controllers/asset_controller.ex +++ b/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 - nil -> - not_found(conn) + case Transform.decode(payload) do + {:error, :invalid_payload} -> + bad_request(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) + {:ok, transform} -> + case cached_get(payload, transform, format) do + nil -> + 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) - image_data + case result do + {: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) diff --git a/test/music_library_web/controllers/asset_controller_test.exs b/test/music_library_web/controllers/asset_controller_test.exs index eb1073b5..31399958 100644 --- a/test/music_library_web/controllers/asset_controller_test.exs +++ b/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)