From 0c77be9308102cb2e4710fbad02035e9dc7125c3 Mon Sep 17 00:00:00 2001 From: flisk Date: Sun, 12 Mar 2023 18:14:05 +0100 Subject: [PATCH 1/2] don't crash on malformed avatar and banner values weird values in href will cause base64 encoding to fail later down the line, so let's make sure the value we're passing on is somewhat sane, or at the very least a binary this fixes #482 --- lib/pleroma/user.ex | 24 ++++++++++++------------ test/pleroma/user_test.exs | 10 ++++++++++ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index f94202af5..480521984 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -366,21 +366,21 @@ def invisible?(%User{invisible: true}), do: true def invisible?(_), do: false def avatar_url(user, options \\ []) do - case user.avatar do - %{"url" => [%{"href" => href} | _]} -> - href - - _ -> - unless options[:no_default] do - Config.get([:assets, :default_user_avatar], "#{Endpoint.url()}/images/avi.png") - end - end + default = Config.get([:assets, :default_user_avatar], "#{Endpoint.url()}/images/avi.png") + do_optional_url(user.avatar, default, options) end def banner_url(user, options \\ []) do - case user.banner do - %{"url" => [%{"href" => href} | _]} -> href - _ -> !options[:no_default] && "#{Endpoint.url()}/images/banner.png" + do_optional_url(user.banner, "#{Endpoint.url()}/images/banner.png", options) + end + + defp do_optional_url(field, default, options \\ []) do + case field do + %{"url" => [%{"href" => href} | _]} when is_binary(href) -> + href + + _ -> + unless options[:no_default], do: default end end diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index a590946c2..12ccc6bf4 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -2509,6 +2509,16 @@ test "avatar fallback" do assert User.avatar_url(user, no_default: true) == nil end + test "avatar object with nil in href" do + user = insert(:user, avatar: %{"url" => [%{"href" => nil}]}) + assert User.avatar_url(user) != nil + end + + test "banner object with nil in href" do + user = insert(:user, banner: %{"url" => [%{"href" => nil}]}) + assert User.banner_url(user) != nil + end + test "get_host/1" do user = insert(:user, ap_id: "https://lain.com/users/lain", nickname: "lain") assert User.get_host(user) == "lain.com" From 3f76de76dac266ea2909dbd1dc88a9533d972952 Mon Sep 17 00:00:00 2001 From: foxing Date: Sun, 12 Mar 2023 19:13:56 +0000 Subject: [PATCH 2/2] Apply Patch --- lib/pleroma/web/activity_pub/activity_pub.ex | 11 +++++++- .../web/activity_pub/activity_pub_test.exs | 8 ++++++ .../controllers/media_controller_test.exs | 17 +++++++++++ .../mastodon_api/update_credentials_test.exs | 28 +++++++++++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 8e55df0d8..649bf9095 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1502,13 +1502,22 @@ def fetch_activities_bounded( @spec upload(Upload.source(), keyword()) :: {:ok, Object.t()} | {:error, any()} def upload(file, opts \\ []) do - with {:ok, data} <- Upload.store(file, opts) do + with {:ok, data} <- Upload.store(sanitize_upload_file(file), opts) do obj_data = Maps.put_if_present(data, "actor", opts[:actor]) Repo.insert(%Object{data: obj_data}) end end + defp sanitize_upload_file(%Plug.Upload{filename: filename} = upload) when is_binary(filename) do + %Plug.Upload{ + upload + | filename: Path.basename(filename) + } + end + + defp sanitize_upload_file(upload), do: upload + @spec get_actor_url(any()) :: binary() | nil defp get_actor_url(url) when is_binary(url), do: url defp get_actor_url(%{"href" => href}) when is_binary(href), do: href diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index 17c52fc91..e95e4490a 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -1303,6 +1303,14 @@ test "returns reblogs for users for whom reblogs have not been muted" do %{test_file: test_file} end + test "strips / from filename", %{test_file: file} do + file = %Plug.Upload{file | filename: "../../../../../nested/bad.jpg"} + {:ok, %Object{} = object} = ActivityPub.upload(file) + [%{"href" => href}] = object.data["url"] + assert Regex.match?(~r"/bad.jpg$", href) + refute Regex.match?(~r"/nested/", href) + end + test "sets a description if given", %{test_file: file} do {:ok, %Object{} = object} = ActivityPub.upload(file, description: "a cool file") assert object.data["name"] == "a cool file" diff --git a/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs index 50b9febea..7ff8cff6b 100644 --- a/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs @@ -124,6 +124,23 @@ test "/api/v2/media, upload_limit", %{conn: conn, user: user} do assert :ok == File.rm(Path.absname("test/tmp/large_binary.data")) end + + test "Do not allow nested filename", %{conn: conn, image: image} do + image = %Plug.Upload{ + image + | filename: "../../../../../nested/file.jpg" + } + + desc = "Description of the image" + + media = + conn + |> put_req_header("content-type", "multipart/form-data") + |> post("/api/v1/media", %{"file" => image, "description" => desc}) + |> json_response_and_validate_schema(:ok) + + refute Regex.match?(~r"/nested/", media["url"]) + end end describe "Update media description" do diff --git a/test/pleroma/web/mastodon_api/update_credentials_test.exs b/test/pleroma/web/mastodon_api/update_credentials_test.exs index e9b8825bf..4aec31eac 100644 --- a/test/pleroma/web/mastodon_api/update_credentials_test.exs +++ b/test/pleroma/web/mastodon_api/update_credentials_test.exs @@ -396,6 +396,34 @@ test "updates the user's background, upload_limit, returns a HTTP 413", %{ assert :ok == File.rm(Path.absname("test/tmp/large_binary.data")) end + test "Strip / from upload files", %{user: user, conn: conn} do + new_image = %Plug.Upload{ + content_type: "image/jpeg", + path: Path.absname("test/fixtures/image.jpg"), + filename: "../../../../nested/an_image.jpg" + } + + assert user.avatar == %{} + + res = + patch(conn, "/api/v1/accounts/update_credentials", %{ + "avatar" => new_image, + "header" => new_image, + "pleroma_background_image" => new_image + }) + + assert user_response = json_response_and_validate_schema(res, 200) + assert user_response["avatar"] + assert user_response["header"] + assert user_response["pleroma"]["background_image"] + refute Regex.match?(~r"/nested/", user_response["avatar"]) + refute Regex.match?(~r"/nested/", user_response["header"]) + refute Regex.match?(~r"/nested/", user_response["pleroma"]["background_image"]) + + user = User.get_by_id(user.id) + refute user.avatar == %{} + end + test "requires 'write:accounts' permission" do token1 = insert(:oauth_token, scopes: ["read"]) token2 = insert(:oauth_token, scopes: ["write", "follow"])