Drop media base_url default and recommend different domain

Same-domain setups enabled now at least two exploits,
so they ought to be discouraged and definitely not be the default.

CHANGELOG.md

@@ -9,6 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Added
 
 ## Changed
+- `Pleroma.Upload, :base_url` now MUST be configured explicitly;
+  use of the same domain as the instance is **strongly** discouraged
 
 ## Fixed
 - Critical security issue allowing Akkoma to be used as a vector for

docs/docs/configuration/cheatsheet.md

@@ -598,7 +598,8 @@ 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`
-* `base_url`: The base URL to access a user-uploaded file. Useful when you want to host the media files via another domain or are using a 3rd party S3 provider.
+* `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.
 * `proxy_opts`: Proxy options, see `Pleroma.ReverseProxy` documentation.
 * `filename_display_max_length`: Set max length of a filename to display. 0 = no limit. Default: 30.

docs/docs/configuration/hardening.md

@@ -17,6 +17,16 @@ This sets the Akkoma application server to only listen to the localhost interfac
 
 This sets the `secure` flag on Akkoma’s session cookie. This makes sure, that the cookie is only accepted over encrypted HTTPs connections. This implicitly renames the cookie from `pleroma_key` to `__Host-pleroma-key` which enforces some restrictions. (see [cookie prefixes](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Cookie_prefixes))
 
+### `Pleroma.Upload, :uploader, :base_url`
+
+> Recommended value: *anything on a different domain than the instance endpoint; e.g. https://media.myinstance.net/*
+
+Uploads are user controlled and (unless you’re running a true single-user
+instance) should therefore not be considered trusted. But the domain is used
+as a pivilege boundary e.g. by HTTP content security policy and ActivityPub.
+Having uploads on the same domain enabled several past vulnerabilities
+able to be exploited by malicious users.
+
 ### `:http_security`
 
 > Recommended value: `true`

lib/mix/tasks/pleroma/instance.ex

@@ -20,6 +20,7 @@ def run(["gen" | rest]) do
           output: :string,
           output_psql: :string,
           domain: :string,
+          media_url: :string,
           instance_name: :string,
           admin_email: :string,
           notify_email: :string,
@@ -64,6 +65,14 @@ def run(["gen" | rest]) do
           ":"
         ) ++ [443]
 
+      media_url =
+        get_option(
+          options,
+          :media_url,
+          "What base url will uploads use? (e.g https://media.example.com/media)\n" <>
+            "  Generally this should NOT use the same domain as the instance       "
+        )
+
       name =
         get_option(
           options,
@@ -207,6 +216,7 @@ def run(["gen" | rest]) do
         EEx.eval_file(
           template_dir <> "/sample_config.eex",
           domain: domain,
+          media_url: media_url,
           port: port,
           email: email,
           notify_email: notify_email,

lib/pleroma/upload.ex

@@ -39,6 +39,8 @@ defmodule Pleroma.Upload do
   alias Pleroma.Web.ActivityPub.Utils
   require Logger
 
+  @mix_env Mix.env()
+
   @type source ::
           Plug.Upload.t()
           | (data_uri_string :: String.t())
@@ -228,6 +230,13 @@ defp url_from_spec(%__MODULE__{name: name}, base_url, {:file, path}) do
 
   defp url_from_spec(_upload, _base_url, {:url, url}), do: url
 
+  if @mix_env == :test do
+    defp choose_base_url(prim, sec \\ nil),
+      do: prim || sec || Pleroma.Web.Endpoint.url() <> "/media/"
+  else
+    defp choose_base_url(prim, sec \\ nil), do: prim || sec
+  end
+
   def base_url do
     uploader = Config.get([Pleroma.Upload, :uploader])
     upload_base_url = Config.get([Pleroma.Upload, :base_url])
@@ -235,7 +244,7 @@ def base_url do
 
     case uploader do
       Pleroma.Uploaders.Local ->
-        upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/"
+        choose_base_url(upload_base_url)
 
       Pleroma.Uploaders.S3 ->
         bucket = Config.get([Pleroma.Uploaders.S3, :bucket])
@@ -261,7 +270,7 @@ def base_url do
         end
 
       _ ->
-        public_endpoint || upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/"
+        choose_base_url(public_endpoint, upload_base_url)
     end
   end
 end

priv/templates/sample_config.eex

@@ -78,6 +78,8 @@ config :joken, default_signer: "<%= jwt_secret %>"
 
 config :pleroma, configurable_from_database: <%= db_configurable? %>
 
+config :pleroma, Pleroma.Upload,
 <%= if Kernel.length(upload_filters) > 0 do
-"config :pleroma, Pleroma.Upload, filters: #{inspect(upload_filters)}"
+"  filters: #{inspect(upload_filters)},"
 end %>
+  base_url: "<%= media_url %>"

test/mix/tasks/pleroma/instance_test.exs

@@ -39,6 +39,8 @@ test "running gen" do
         tmp_path() <> "setup.psql",
         "--domain",
         "test.pleroma.social",
+        "--media-url",
+        "https://media.pleroma.social/media",
         "--instance-name",
         "Pleroma",
         "--admin-email",
@@ -92,6 +94,7 @@ test "running gen" do
     assert generated_config =~ "configurable_from_database: true"
     assert generated_config =~ "http: [ip: {127, 0, 0, 1}, port: 4000]"
     assert generated_config =~ "filters: [Pleroma.Upload.Filter.Exiftool]"
+    assert generated_config =~ "base_url: \"https://media.pleroma.social/media\""
     assert File.read!(tmp_path() <> "setup.psql") == generated_setup_psql()
     assert File.exists?(Path.expand("./test/instance/static/robots.txt"))
   end