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

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,

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.

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

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]
 

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"
       ])

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
 

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
 

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
 

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"

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

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)