From ad7998361498b08d45ea0971f8b6ecbd8ca0740e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 7 Jan 2021 18:34:30 -0600 Subject: [PATCH 1/9] Fix URL generated for backup files, try to create a source of truth we can reuse throughout the codebase --- lib/pleroma/upload.ex | 26 +++++++++++++++++++ .../web/pleroma_api/views/backup_view.ex | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index db2cc1dae..101cfec98 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -31,6 +31,7 @@ defmodule Pleroma.Upload do """ alias Ecto.UUID + alias Pleroma.Config require Logger @type source :: @@ -228,4 +229,29 @@ defp url_from_spec(%__MODULE__{name: name}, base_url, {:file, path}) do end defp url_from_spec(_upload, _base_url, {:url, url}), do: url + + def base_url do + uploader = Config.get([Pleroma.Upload, :uploader]) + upload_base_url = Config.get([Pleroma.Upload, :base_url]) + + case uploader do + Pleroma.Uploaders.Local -> + cond do + !is_nil(upload_base_url) -> + upload_base_url + + true -> + Pleroma.Web.base_url() <> "/media/" + end + + _ -> + cond do + !is_nil(Config.get([uploader, :public_endpoint])) -> + Config.get([uploader, :public_endpoint]) + + true -> + upload_base_url + end + end + end end diff --git a/lib/pleroma/web/pleroma_api/views/backup_view.ex b/lib/pleroma/web/pleroma_api/views/backup_view.ex index 39affe979..e04c8fc0f 100644 --- a/lib/pleroma/web/pleroma_api/views/backup_view.ex +++ b/lib/pleroma/web/pleroma_api/views/backup_view.ex @@ -24,6 +24,6 @@ def render("index.json", %{backups: backups}) do end def download_url(%Backup{file_name: file_name}) do - Pleroma.Web.Endpoint.url() <> "/media/backups/" <> file_name + Pleroma.Upload.base_url() <> "/backups/" <> file_name end end From 3c936061d55c1c4bd9346471bc498dd123395766 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 8 Jan 2021 10:49:12 -0600 Subject: [PATCH 2/9] Apply Upload.base_url for S3 --- lib/pleroma/uploaders/s3.ex | 2 +- test/pleroma/uploaders/s3_test.exs | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/uploaders/s3.ex b/lib/pleroma/uploaders/s3.ex index 6dbef9085..5a91410d7 100644 --- a/lib/pleroma/uploaders/s3.ex +++ b/lib/pleroma/uploaders/s3.ex @@ -30,7 +30,7 @@ def get_file(file) do {:ok, {:url, Path.join([ - Keyword.fetch!(config, :public_endpoint), + Pleroma.Upload.base_url(), bucket_with_namespace, strict_encode(URI.decode(file)) ])}} diff --git a/test/pleroma/uploaders/s3_test.exs b/test/pleroma/uploaders/s3_test.exs index e7a013dd8..344cf7abe 100644 --- a/test/pleroma/uploaders/s3_test.exs +++ b/test/pleroma/uploaders/s3_test.exs @@ -11,11 +11,16 @@ defmodule Pleroma.Uploaders.S3Test do import Mock import ExUnit.CaptureLog - setup do: - clear_config(Pleroma.Uploaders.S3, - bucket: "test_bucket", - public_endpoint: "https://s3.amazonaws.com" - ) + setup do + clear_config(Pleroma.Upload, + uploader: Pleroma.Uploaders.S3 + ) + + clear_config(Pleroma.Uploaders.S3, + bucket: "test_bucket", + public_endpoint: "https://s3.amazonaws.com" + ) + end describe "get_file/1" do test "it returns path to local folder for files" do From 530fb5b29ebd414781c703e4b41d204135f3efe7 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 8 Jan 2021 16:43:19 -0600 Subject: [PATCH 3/9] Avoid duplicate Config calls --- lib/pleroma/upload.ex | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index 101cfec98..3061b2aed 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -233,6 +233,7 @@ defp url_from_spec(_upload, _base_url, {:url, url}), do: url def base_url do uploader = Config.get([Pleroma.Upload, :uploader]) upload_base_url = Config.get([Pleroma.Upload, :base_url]) + public_endpoint = Config.get([uploader, :public_endpoint]) case uploader do Pleroma.Uploaders.Local -> @@ -246,8 +247,8 @@ def base_url do _ -> cond do - !is_nil(Config.get([uploader, :public_endpoint])) -> - Config.get([uploader, :public_endpoint]) + !is_nil(public_endpoint) -> + public_endpoint true -> upload_base_url From 86dcfb4eb990dc8d06f799663264655ce04d0d5d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 8 Jan 2021 17:05:55 -0600 Subject: [PATCH 4/9] More places we should be using Upload.base_url --- lib/pleroma/web/media_proxy.ex | 14 ++++++++------ lib/pleroma/web/plugs/uploaded_media.ex | 2 +- lib/pleroma/workers/attachments_cleanup_worker.ex | 10 ++-------- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex index 2793cabc1..95e3f4231 100644 --- a/lib/pleroma/web/media_proxy.ex +++ b/lib/pleroma/web/media_proxy.ex @@ -69,7 +69,7 @@ def enabled?, do: Config.get([:media_proxy, :enabled], false) # non-local non-whitelisted URLs through it and be sure that body size constraint is preserved. def preview_enabled?, do: enabled?() and !!Config.get([:media_preview_proxy, :enabled]) - def local?(url), do: String.starts_with?(url, Pleroma.Web.base_url()) + def local?(url), do: String.starts_with?(url, Upload.base_url()) def whitelisted?(url) do %{host: domain} = URI.parse(url) @@ -80,11 +80,13 @@ def whitelisted?(url) do |> Enum.map(&maybe_get_domain_from_url/1) whitelist_domains = - if base_url = Config.get([Upload, :base_url]) do - %{host: base_domain} = URI.parse(base_url) - [base_domain | mediaproxy_whitelist_domains] - else - mediaproxy_whitelist_domains + cond do + Web.base_url() == Upload.base_url() -> + mediaproxy_whitelist_domains + + true -> + %{host: base_domain} = URI.parse(Upload.base_url()) + [base_domain | mediaproxy_whitelist_domains] end domain in whitelist_domains diff --git a/lib/pleroma/web/plugs/uploaded_media.ex b/lib/pleroma/web/plugs/uploaded_media.ex index 94b4c2177..175b4d87d 100644 --- a/lib/pleroma/web/plugs/uploaded_media.ex +++ b/lib/pleroma/web/plugs/uploaded_media.ex @@ -62,7 +62,7 @@ def call(%{request_path: <<"/", @path, "/", file::binary>>} = conn, opts) do def call(conn, _opts), do: conn defp media_is_banned(%{request_path: path} = _conn, {:static_dir, _}) do - MediaProxy.in_banned_urls(Pleroma.Web.base_url() <> path) + MediaProxy.in_banned_urls(Pleroma.Upload.base_url() <> path) end defp media_is_banned(_, {:url, url}), do: MediaProxy.in_banned_urls(url) diff --git a/lib/pleroma/workers/attachments_cleanup_worker.ex b/lib/pleroma/workers/attachments_cleanup_worker.ex index 58226b395..69758e8c1 100644 --- a/lib/pleroma/workers/attachments_cleanup_worker.ex +++ b/lib/pleroma/workers/attachments_cleanup_worker.ex @@ -32,21 +32,15 @@ def perform(%Job{args: %{"op" => "cleanup_attachments", "object" => _object}}), defp do_clean({object_ids, attachment_urls}) do uploader = Pleroma.Config.get([Pleroma.Upload, :uploader]) - prefix = - case Pleroma.Config.get([Pleroma.Upload, :base_url]) do - nil -> "media" - _ -> "" - end - base_url = String.trim_trailing( - Pleroma.Config.get([Pleroma.Upload, :base_url], Pleroma.Web.base_url()), + Pleroma.Upload.base_url(), "/" ) Enum.each(attachment_urls, fn href -> href - |> String.trim_leading("#{base_url}/#{prefix}") + |> String.trim_leading("#{base_url}") |> uploader.delete_file() end) From e8bf060e6e55396e7a0dbb53dacbf470d8f56beb Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 8 Jan 2021 17:24:19 -0600 Subject: [PATCH 5/9] Move construction of S3 base URL with optional namespace and bucket to Upload.base_url/0 Now we should have a correct base URL for S3 hosted objects throughout the codebase. --- lib/pleroma/upload.ex | 23 +++++++++++++++++++++++ lib/pleroma/uploaders/s3.ex | 16 ---------------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index 3061b2aed..a52b698bf 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -245,6 +245,29 @@ def base_url do Pleroma.Web.base_url() <> "/media/" end + Pleroma.Uploaders.S3 -> + bucket = Config.get([Pleroma.Uploaders.S3, :bucket]) + + bucket_with_namespace = + cond do + truncated_namespace = Config.get([Pleroma.Uploaders.S3, :truncated_namespace]) -> + truncated_namespace + + namespace = Config.get([Pleroma.Uploaders.S3, :bucket_namespace]) -> + namespace <> ":" <> bucket + + true -> + bucket + end + + cond do + !is_nil(public_endpoint) -> + Path.join([public_endpoint, bucket_with_namespace]) + + true -> + Path.join([upload_base_url, bucket_with_namespace]) + end + _ -> cond do !is_nil(public_endpoint) -> diff --git a/lib/pleroma/uploaders/s3.ex b/lib/pleroma/uploaders/s3.ex index 5a91410d7..29a1c2861 100644 --- a/lib/pleroma/uploaders/s3.ex +++ b/lib/pleroma/uploaders/s3.ex @@ -12,26 +12,10 @@ defmodule Pleroma.Uploaders.S3 do # links with less strict filenames @impl true def get_file(file) do - config = Config.get([__MODULE__]) - bucket = Keyword.fetch!(config, :bucket) - - bucket_with_namespace = - cond do - truncated_namespace = Keyword.get(config, :truncated_namespace) -> - truncated_namespace - - namespace = Keyword.get(config, :bucket_namespace) -> - namespace <> ":" <> bucket - - true -> - bucket - end - {:ok, {:url, Path.join([ Pleroma.Upload.base_url(), - bucket_with_namespace, strict_encode(URI.decode(file)) ])}} end From fa63f1b55bad4da8d1c8c51e980109ad5352f71e Mon Sep 17 00:00:00 2001 From: feld Date: Sun, 10 Jan 2021 01:34:54 +0000 Subject: [PATCH 6/9] Apply 4 suggestion(s) to 2 file(s) --- lib/pleroma/upload.ex | 26 ++++++-------------------- lib/pleroma/web/media_proxy.ex | 13 ++++++------- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index a52b698bf..51ca97f41 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -237,13 +237,7 @@ def base_url do case uploader do Pleroma.Uploaders.Local -> - cond do - !is_nil(upload_base_url) -> - upload_base_url - - true -> - Pleroma.Web.base_url() <> "/media/" - end + upload_base_url || Pleroma.Web.base_url() <> "/media/" Pleroma.Uploaders.S3 -> bucket = Config.get([Pleroma.Uploaders.S3, :bucket]) @@ -260,22 +254,14 @@ def base_url do bucket end - cond do - !is_nil(public_endpoint) -> - Path.join([public_endpoint, bucket_with_namespace]) - - true -> - Path.join([upload_base_url, bucket_with_namespace]) + if public_endpoint do + Path.join([public_endpoint, bucket_with_namespace]) + else + Path.join([upload_base_url, bucket_with_namespace]) end _ -> - cond do - !is_nil(public_endpoint) -> - public_endpoint - - true -> - upload_base_url - end + public_endpoint || upload_base_url end end end diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex index 95e3f4231..e4d7f8aa8 100644 --- a/lib/pleroma/web/media_proxy.ex +++ b/lib/pleroma/web/media_proxy.ex @@ -80,13 +80,12 @@ def whitelisted?(url) do |> Enum.map(&maybe_get_domain_from_url/1) whitelist_domains = - cond do - Web.base_url() == Upload.base_url() -> - mediaproxy_whitelist_domains - - true -> - %{host: base_domain} = URI.parse(Upload.base_url()) - [base_domain | mediaproxy_whitelist_domains] + base_url = Upload.base_url() + if Web.base_url() == base_url do + mediaproxy_whitelist_domains + else + %{host: base_domain} = URI.parse(base_url) + [base_domain | mediaproxy_whitelist_domains] end domain in whitelist_domains From 9887cdf9be049ca12ea6ba45d38f9072de1b0fc0 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sun, 10 Jan 2021 09:03:42 -0600 Subject: [PATCH 7/9] Formatting --- lib/pleroma/web/media_proxy.ex | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex index e4d7f8aa8..cbe717584 100644 --- a/lib/pleroma/web/media_proxy.ex +++ b/lib/pleroma/web/media_proxy.ex @@ -79,14 +79,14 @@ def whitelisted?(url) do |> Config.get() |> Enum.map(&maybe_get_domain_from_url/1) - whitelist_domains = - base_url = Upload.base_url() - if Web.base_url() == base_url do - mediaproxy_whitelist_domains - else - %{host: base_domain} = URI.parse(base_url) - [base_domain | mediaproxy_whitelist_domains] - end + whitelist_domains = base_url = Upload.base_url() + + if Web.base_url() == base_url do + mediaproxy_whitelist_domains + else + %{host: base_domain} = URI.parse(base_url) + [base_domain | mediaproxy_whitelist_domains] + end domain in whitelist_domains end From 10408810473ad211423cae49db94c33a765dbe33 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 11 Jan 2021 14:01:31 -0600 Subject: [PATCH 8/9] Fix regression in MediaProxy.local?/0 and appending the Upload.base_url to whitelisted domains --- lib/pleroma/web/media_proxy.ex | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex index cbe717584..1dab35d2c 100644 --- a/lib/pleroma/web/media_proxy.ex +++ b/lib/pleroma/web/media_proxy.ex @@ -69,24 +69,24 @@ def enabled?, do: Config.get([:media_proxy, :enabled], false) # non-local non-whitelisted URLs through it and be sure that body size constraint is preserved. def preview_enabled?, do: enabled?() and !!Config.get([:media_preview_proxy, :enabled]) - def local?(url), do: String.starts_with?(url, Upload.base_url()) + def local?(url), do: String.starts_with?(url, Web.base_url()) def whitelisted?(url) do %{host: domain} = URI.parse(url) + %{host: web_domain} = Web.base_url() |> URI.parse() + %{host: upload_domain} = Upload.base_url() |> URI.parse() mediaproxy_whitelist_domains = [:media_proxy, :whitelist] |> Config.get() |> Enum.map(&maybe_get_domain_from_url/1) - whitelist_domains = base_url = Upload.base_url() - - if Web.base_url() == base_url do - mediaproxy_whitelist_domains - else - %{host: base_domain} = URI.parse(base_url) - [base_domain | mediaproxy_whitelist_domains] - end + whitelist_domains = + if web_domain == upload_domain do + mediaproxy_whitelist_domains + else + [upload_domain | mediaproxy_whitelist_domains] + end domain in whitelist_domains end From ef59d998338551f15b7fe782641bfbf443fd66f4 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 11 Jan 2021 14:19:14 -0600 Subject: [PATCH 9/9] Simplify. We will always have a result from Upload.base_url/0, so just add it to the list --- lib/pleroma/web/media_proxy.ex | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex index 1dab35d2c..dcf3b0623 100644 --- a/lib/pleroma/web/media_proxy.ex +++ b/lib/pleroma/web/media_proxy.ex @@ -73,22 +73,14 @@ def local?(url), do: String.starts_with?(url, Web.base_url()) def whitelisted?(url) do %{host: domain} = URI.parse(url) - %{host: web_domain} = Web.base_url() |> URI.parse() - %{host: upload_domain} = Upload.base_url() |> URI.parse() mediaproxy_whitelist_domains = [:media_proxy, :whitelist] |> Config.get() + |> Kernel.++(["#{Upload.base_url()}"]) |> Enum.map(&maybe_get_domain_from_url/1) - whitelist_domains = - if web_domain == upload_domain do - mediaproxy_whitelist_domains - else - [upload_domain | mediaproxy_whitelist_domains] - end - - domain in whitelist_domains + domain in mediaproxy_whitelist_domains end defp maybe_get_domain_from_url("http" <> _ = url) do