diff --git a/CHANGELOG.md b/CHANGELOG.md index 1efe1ecfc..9130a81ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Changed - `Pleroma.Upload, :base_url` now MUST be configured explicitly; use of the same domain as the instance is **strongly** discouraged +- The `Dedupe` upload filter is now always active; + `AnonymizeFilenames` is again opt-in ## Fixed - Critical security issue allowing Akkoma to be used as a vector for diff --git a/config/config.exs b/config/config.exs index 723d173ec..a366961c0 100644 --- a/config/config.exs +++ b/config/config.exs @@ -61,7 +61,7 @@ # Upload configuration config :pleroma, Pleroma.Upload, uploader: Pleroma.Uploaders.Local, - filters: [Pleroma.Upload.Filter.Dedupe], + filters: [], link_name: false, proxy_remote: false, filename_display_max_length: 30, diff --git a/docs/docs/configuration/cheatsheet.md b/docs/docs/configuration/cheatsheet.md index a04160a1d..9c5bb9901 100644 --- a/docs/docs/configuration/cheatsheet.md +++ b/docs/docs/configuration/cheatsheet.md @@ -597,7 +597,7 @@ the source code is here: [kocaptcha](https://github.com/koto-bank/kocaptcha). Th * `uploader`: Which one of the [uploaders](#uploaders) to use. * `filters`: List of [upload filters](#upload-filters) to use. -* `link_name`: When enabled Akkoma will add a `name` parameter to the url of the upload, for example `https://instance.tld/media/corndog.png?name=corndog.png`. This is needed to provide the correct filename in Content-Disposition headers when using filters like `Pleroma.Upload.Filter.Dedupe` +* `link_name`: When enabled Akkoma will add a `name` parameter to the url of the upload, for example `https://instance.tld/media/corndog.png?name=corndog.png`. This is needed to provide the correct filename in Content-Disposition headers * `base_url`: The base URL to access a user-uploaded file; MUST be configured explicitly. Using a (sub)domain distinct from the instance endpoint is **strongly** recommended. * `proxy_remote`: If you're using a remote uploader, Akkoma will proxy media requests instead of redirecting to it. @@ -639,17 +639,18 @@ config :ex_aws, :s3, ### Upload filters -#### Pleroma.Upload.Filter.AnonymizeFilename - -This filter replaces the filename (not the path) of an upload. For complete obfuscation, add -`Pleroma.Upload.Filter.Dedupe` before AnonymizeFilename. - -* `text`: Text to replace filenames in links. If empty, `{random}.extension` will be used. You can get the original filename extension by using `{extension}`, for example `custom-file-name.{extension}`. - #### Pleroma.Upload.Filter.Dedupe +**Always** active; cannot be turned off. +Renames files to their hash and prevents duplicate files filling up the disk. No specific configuration. +#### Pleroma.Upload.Filter.AnonymizeFilename + +This filter replaces the declared filename (not the path) of an upload. + +* `text`: Text to replace filenames in links. If empty, `{random}.extension` will be used. You can get the original filename extension by using `{extension}`, for example `custom-file-name.{extension}`. + #### Pleroma.Upload.Filter.Exiftool This filter only strips the GPS and location metadata with Exiftool leaving color profiles and attributes intact. diff --git a/lib/mix/tasks/pleroma/instance.ex b/lib/mix/tasks/pleroma/instance.ex index b442fdb5b..44f6b6e70 100644 --- a/lib/mix/tasks/pleroma/instance.ex +++ b/lib/mix/tasks/pleroma/instance.ex @@ -36,8 +36,7 @@ def run(["gen" | rest]) do listen_ip: :string, listen_port: :string, strip_uploads: :string, - anonymize_uploads: :string, - dedupe_uploads: :string + anonymize_uploads: :string ], aliases: [ o: :output, @@ -195,14 +194,6 @@ def run(["gen" | rest]) do "n" ) === "y" - dedupe_uploads = - get_option( - options, - :dedupe_uploads, - "Do you want to deduplicate uploaded files? (y/n)", - "n" - ) === "y" - Config.put([:instance, :static_dir], static_dir) secret = :crypto.strong_rand_bytes(64) |> Base.encode64() |> binary_part(0, 64) @@ -240,8 +231,7 @@ def run(["gen" | rest]) do upload_filters: upload_filters(%{ strip: strip_uploads, - anonymize: anonymize_uploads, - dedupe: dedupe_uploads + anonymize: anonymize_uploads }) ) @@ -329,13 +319,6 @@ defp upload_filters(filters) when is_map(filters) do enabled_filters end - enabled_filters = - if filters.dedupe do - enabled_filters ++ [Pleroma.Upload.Filter.Dedupe] - else - enabled_filters - end - enabled_filters end end diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index 974d12533..1158e9449 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -66,7 +66,7 @@ defmodule Pleroma.Upload do path: String.t() } - @always_enabled_filters [Pleroma.Upload.Filter.AnonymizeFilename] + @always_enabled_filters [Pleroma.Upload.Filter.Dedupe] defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path] diff --git a/test/mix/tasks/pleroma/instance_test.exs b/test/mix/tasks/pleroma/instance_test.exs index 522319371..17b2e3267 100644 --- a/test/mix/tasks/pleroma/instance_test.exs +++ b/test/mix/tasks/pleroma/instance_test.exs @@ -71,8 +71,6 @@ test "running gen" do "./test/../test/instance/static/", "--strip-uploads", "y", - "--dedupe-uploads", - "n", "--anonymize-uploads", "n" ]) diff --git a/test/mix/tasks/pleroma/uploads_test.exs b/test/mix/tasks/pleroma/uploads_test.exs index 67fb642c1..d00e25a37 100644 --- a/test/mix/tasks/pleroma/uploads_test.exs +++ b/test/mix/tasks/pleroma/uploads_test.exs @@ -16,7 +16,6 @@ defmodule Mix.Tasks.Pleroma.UploadsTest do Mix.shell(Mix.Shell.IO) end) - File.mkdir_p!("test/uploads") :ok end diff --git a/test/pleroma/object_test.exs b/test/pleroma/object_test.exs index 8320660a5..4b0fec1bd 100644 --- a/test/pleroma/object_test.exs +++ b/test/pleroma/object_test.exs @@ -22,6 +22,13 @@ defmodule Pleroma.ObjectTest do :ok end + # Only works for a single attachment but that's all we need here + defp get_attachment_filepath(note, uploads_dir) do + %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = note + filename = href |> Path.basename() + "#{uploads_dir}/#{filename}" + end + test "returns an object by it's AP id" do object = insert(:note) found_object = Object.get_by_ap_id(object.data["id"]) @@ -95,14 +102,13 @@ test "Disabled via config" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -111,7 +117,7 @@ test "Disabled via config" do assert Object.get_by_id(note.id).data["deleted"] refute Object.get_by_id(attachment.id) == nil - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") end test "in subdirectories" do @@ -129,14 +135,13 @@ test "in subdirectories" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -145,7 +150,7 @@ test "in subdirectories" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, []} == File.ls("#{uploads_dir}/#{path}") + refute File.exists?("#{path}") end test "with dedupe enabled" do @@ -168,13 +173,11 @@ test "with dedupe enabled" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) - filename = Path.basename(href) + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, files} = File.ls(uploads_dir) - assert filename in files + assert File.exists?("#{path}") Object.delete(note) @@ -182,8 +185,8 @@ test "with dedupe enabled" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, files} = File.ls(uploads_dir) - refute filename in files + # what if another test runs concurrently using the same image file? + refute File.exists?("#{path}") end test "with objects that have legacy data.url attribute" do @@ -203,14 +206,13 @@ test "with objects that have legacy data.url attribute" do {:ok, %Object{}} = Object.create(%{url: "https://google.com", actor: user.ap_id}) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -219,7 +221,7 @@ test "with objects that have legacy data.url attribute" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, []} == File.ls("#{uploads_dir}/#{path}") + refute File.exists?("#{path}") end test "With custom base_url" do @@ -238,14 +240,13 @@ test "With custom base_url" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -254,7 +255,7 @@ test "With custom base_url" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, []} == File.ls("#{uploads_dir}/#{path}") + refute File.exists?("#{path}") end end diff --git a/test/pleroma/upload_test.exs b/test/pleroma/upload_test.exs index ad6065b43..27a2d1b97 100644 --- a/test/pleroma/upload_test.exs +++ b/test/pleroma/upload_test.exs @@ -188,7 +188,7 @@ test "copies the file to the configured folder with anonymizing filename" do refute data["name"] == "an [image.jpg" end - test "escapes invalid characters in url" do + test "mangles the filename" do File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg") file = %Plug.Upload{ @@ -200,23 +200,8 @@ test "escapes invalid characters in url" do {:ok, data} = Upload.store(file) [attachment_url | _] = data["url"] - assert Path.basename(attachment_url["href"]) == "an%E2%80%A6%20image.jpg" - end - - test "escapes reserved uri characters" do - File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg") - - file = %Plug.Upload{ - content_type: "image/jpeg", - path: Path.absname("test/fixtures/image_tmp.jpg"), - filename: ":?#[]@!$&\\'()*+,;=.jpg" - } - - {:ok, data} = Upload.store(file) - [attachment_url | _] = data["url"] - - assert Path.basename(attachment_url["href"]) == - "%3A%3F%23%5B%5D%40%21%24%26%5C%27%28%29%2A%2B%2C%3B%3D.jpg" + refute Path.basename(attachment_url["href"]) == "an%E2%80%A6%20image.jpg" + refute Path.basename(attachment_url["href"]) == "an… image.jpg" end end diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index 5d5388cf5..69b4ac257 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -1304,14 +1304,6 @@ 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/pleroma_api/controllers/mascot_controller_test.exs b/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs index 7f02bff8f..8829597eb 100644 --- a/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs +++ b/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs @@ -64,6 +64,10 @@ test "mascot retrieving" do assert json_response_and_validate_schema(ret_conn, 200) + %{"url" => uploaded_url} = Jason.decode!(ret_conn.resp_body) + + assert uploaded_url != nil and is_binary(uploaded_url) + user = User.get_cached_by_id(user.id) conn = @@ -72,6 +76,6 @@ test "mascot retrieving" do |> get("/api/v1/pleroma/mascot") assert %{"url" => url, "type" => "image"} = json_response_and_validate_schema(conn, 200) - assert url =~ "an_image" + assert url == uploaded_url end end diff --git a/test/test_helper.exs b/test/test_helper.exs index 0fc7a86b9..22a0f33ee 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -9,6 +9,10 @@ {:ok, _} = Application.ensure_all_started(:ex_machina) +# Prepare and later automatically cleanup upload dir +uploads_dir = Pleroma.Config.get([Pleroma.Uploaders.Local, :uploads], "test/uploads") +File.mkdir_p!(uploads_dir) + ExUnit.after_suite(fn _results -> uploads = Pleroma.Config.get([Pleroma.Uploaders.Local, :uploads], "test/uploads") File.rm_rf!(uploads)