From a83916fdacac7b11ca478ef9a61b32dd269c8fd2 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Fri, 4 Sep 2020 19:05:08 +0300 Subject: [PATCH 01/12] adapter options unification not needed options deletion --- config/config.exs | 10 +++++----- config/description.exs | 8 +++++++- docs/configuration/cheatsheet.md | 4 ++-- lib/mix/tasks/pleroma/frontend.ex | 4 +--- lib/pleroma/gun/conn.ex | 8 ++++---- lib/pleroma/http/adapter_helper.ex | 2 +- lib/pleroma/http/adapter_helper/gun.ex | 14 ++++++-------- lib/pleroma/http/adapter_helper/hackney.ex | 14 ++++++++++---- .../mrf/media_proxy_warming_policy.ex | 12 +++--------- lib/pleroma/web/rel_me.ex | 15 +++------------ lib/pleroma/web/rich_media/helpers.ex | 19 +++++-------------- 11 files changed, 47 insertions(+), 63 deletions(-) diff --git a/config/config.exs b/config/config.exs index d631c3962..2426fbd52 100644 --- a/config/config.exs +++ b/config/config.exs @@ -735,28 +735,28 @@ config :pleroma, :connections_pool, max_connections: 250, max_idle_time: 30_000, retry: 0, - await_up_timeout: 5_000 + connect_timeout: 5_000 config :pleroma, :pools, federation: [ size: 50, max_waiting: 10, - timeout: 10_000 + recv_timeout: 10_000 ], media: [ size: 50, max_waiting: 10, - timeout: 10_000 + recv_timeout: 10_000 ], upload: [ size: 25, max_waiting: 5, - timeout: 15_000 + recv_timeout: 15_000 ], default: [ size: 10, max_waiting: 2, - timeout: 5_000 + recv_timeout: 5_000 ] config :pleroma, :hackney_pools, diff --git a/config/description.exs b/config/description.exs index 18c133f02..eac97ad64 100644 --- a/config/description.exs +++ b/config/description.exs @@ -3377,7 +3377,7 @@ config :pleroma, :config_description, [ suggestions: [250] }, %{ - key: :await_up_timeout, + key: :connect_timeout, type: :integer, description: "Timeout while `gun` will wait until connection is up. Default: 5000ms.", suggestions: [5000] @@ -3415,6 +3415,12 @@ config :pleroma, :config_description, [ description: "Maximum number of requests waiting for other requests to finish. After this number is reached, the pool will start returning errrors when a new request is made", suggestions: [10] + }, + %{ + key: :recv_timeout, + type: :integer, + description: "Timeout for the pool while gun will wait for response", + suggestions: [10_000] } ] } diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index a9a650fab..7f0725b48 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -498,7 +498,7 @@ Settings for HTTP connection pool. * `:connection_acquisition_wait` - Timeout to acquire a connection from pool.The total max time is this value multiplied by the number of retries. * `connection_acquisition_retries` - Number of attempts to acquire the connection from the pool if it is overloaded. Each attempt is timed `:connection_acquisition_wait` apart. * `:max_connections` - Maximum number of connections in the pool. -* `:await_up_timeout` - Timeout to connect to the host. +* `:connect_timeout` - Timeout to connect to the host. * `:reclaim_multiplier` - Multiplied by `:max_connections` this will be the maximum number of idle connections that will be reclaimed in case the pool is overloaded. ### :pools @@ -517,7 +517,7 @@ There are four pools used: For each pool, the options are: * `:size` - limit to how much requests can be concurrently executed. -* `:timeout` - timeout while `gun` will wait for response +* `:recv_timeout` - timeout while `gun` will wait for response * `:max_waiting` - limit to how much requests can be waiting for others to finish, after this is reached, subsequent requests will be dropped. ## Captcha diff --git a/lib/mix/tasks/pleroma/frontend.ex b/lib/mix/tasks/pleroma/frontend.ex index 1957b1d84..73df67439 100644 --- a/lib/mix/tasks/pleroma/frontend.ex +++ b/lib/mix/tasks/pleroma/frontend.ex @@ -124,9 +124,7 @@ defmodule Mix.Tasks.Pleroma.Frontend do url = String.replace(frontend_info["build_url"], "${ref}", frontend_info["ref"]) with {:ok, %{status: 200, body: zip_body}} <- - Pleroma.HTTP.get(url, [], - adapter: [pool: :media, timeout: 120_000, recv_timeout: 120_000] - ) do + Pleroma.HTTP.get(url, [], adapter: [pool: :media, recv_timeout: 120_000]) do unzip(zip_body, dest) else e -> {:error, e} diff --git a/lib/pleroma/gun/conn.ex b/lib/pleroma/gun/conn.ex index a3f75a4bb..75b1ffc0a 100644 --- a/lib/pleroma/gun/conn.ex +++ b/lib/pleroma/gun/conn.ex @@ -13,7 +13,7 @@ defmodule Pleroma.Gun.Conn do opts = opts |> Enum.into(%{}) - |> Map.put_new(:await_up_timeout, pool_opts[:await_up_timeout] || 5_000) + |> Map.put_new(:connect_timeout, pool_opts[:connect_timeout] || 5_000) |> Map.put_new(:supervise, false) |> maybe_add_tls_opts(uri) @@ -50,7 +50,7 @@ defmodule Pleroma.Gun.Conn do with open_opts <- Map.delete(opts, :tls_opts), {:ok, conn} <- Gun.open(proxy_host, proxy_port, open_opts), - {:ok, _} <- Gun.await_up(conn, opts[:await_up_timeout]), + {:ok, _} <- Gun.await_up(conn, opts[:connect_timeout]), stream <- Gun.connect(conn, connect_opts), {:response, :fin, 200, _} <- Gun.await(conn, stream) do {:ok, conn} @@ -88,7 +88,7 @@ defmodule Pleroma.Gun.Conn do |> Map.put(:socks_opts, socks_opts) with {:ok, conn} <- Gun.open(proxy_host, proxy_port, opts), - {:ok, _} <- Gun.await_up(conn, opts[:await_up_timeout]) do + {:ok, _} <- Gun.await_up(conn, opts[:connect_timeout]) do {:ok, conn} else error -> @@ -106,7 +106,7 @@ defmodule Pleroma.Gun.Conn do host = Pleroma.HTTP.AdapterHelper.parse_host(host) with {:ok, conn} <- Gun.open(host, port, opts), - {:ok, _} <- Gun.await_up(conn, opts[:await_up_timeout]) do + {:ok, _} <- Gun.await_up(conn, opts[:connect_timeout]) do {:ok, conn} else error -> diff --git a/lib/pleroma/http/adapter_helper.ex b/lib/pleroma/http/adapter_helper.ex index d72297323..08b51578a 100644 --- a/lib/pleroma/http/adapter_helper.ex +++ b/lib/pleroma/http/adapter_helper.ex @@ -6,7 +6,7 @@ defmodule Pleroma.HTTP.AdapterHelper do @moduledoc """ Configure Tesla.Client with default and customized adapter options. """ - @defaults [pool: :federation] + @defaults [pool: :federation, connect_timeout: 5_000, recv_timeout: 5_000] @type proxy_type() :: :socks4 | :socks5 @type host() :: charlist() | :inet.ip_address() diff --git a/lib/pleroma/http/adapter_helper/gun.ex b/lib/pleroma/http/adapter_helper/gun.ex index 4a967d8f2..1dbb71362 100644 --- a/lib/pleroma/http/adapter_helper/gun.ex +++ b/lib/pleroma/http/adapter_helper/gun.ex @@ -11,12 +11,8 @@ defmodule Pleroma.HTTP.AdapterHelper.Gun do require Logger @defaults [ - connect_timeout: 5_000, - domain_lookup_timeout: 5_000, - tls_handshake_timeout: 5_000, retry: 1, - retry_timeout: 1000, - await_up_timeout: 5_000 + retry_timeout: 1_000 ] @type pool() :: :federation | :upload | :media | :default @@ -45,15 +41,17 @@ defmodule Pleroma.HTTP.AdapterHelper.Gun do end defp put_timeout(opts) do + {recv_timeout, opts} = Keyword.pop(opts, :recv_timeout, pool_timeout(opts[:pool])) # this is the timeout to receive a message from Gun - Keyword.put_new(opts, :timeout, pool_timeout(opts[:pool])) + # `:timeout` key is used in Tesla + Keyword.put(opts, :timeout, recv_timeout) end @spec pool_timeout(pool()) :: non_neg_integer() def pool_timeout(pool) do - default = Config.get([:pools, :default, :timeout], 5_000) + default = Config.get([:pools, :default, :recv_timeout], 5_000) - Config.get([:pools, pool, :timeout], default) + Config.get([:pools, pool, :recv_timeout], default) end @prefix Pleroma.Gun.ConnectionPool diff --git a/lib/pleroma/http/adapter_helper/hackney.ex b/lib/pleroma/http/adapter_helper/hackney.ex index 42e3acfec..ef84553c1 100644 --- a/lib/pleroma/http/adapter_helper/hackney.ex +++ b/lib/pleroma/http/adapter_helper/hackney.ex @@ -2,11 +2,8 @@ defmodule Pleroma.HTTP.AdapterHelper.Hackney do @behaviour Pleroma.HTTP.AdapterHelper @defaults [ - connect_timeout: 10_000, - recv_timeout: 20_000, follow_redirect: true, - force_redirect: true, - pool: :federation + force_redirect: true ] @spec options(keyword(), URI.t()) :: keyword() @@ -19,6 +16,7 @@ defmodule Pleroma.HTTP.AdapterHelper.Hackney do |> Keyword.merge(config_opts) |> Keyword.merge(connection_opts) |> add_scheme_opts(uri) + |> maybe_add_with_body() |> Pleroma.HTTP.AdapterHelper.maybe_add_proxy(proxy) end @@ -27,4 +25,12 @@ defmodule Pleroma.HTTP.AdapterHelper.Hackney do end defp add_scheme_opts(opts, _), do: opts + + defp maybe_add_with_body(opts) do + if opts[:max_body] do + Keyword.put(opts, :with_body, true) + else + opts + end + end end diff --git a/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex b/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex index dfab105a3..a203405a0 100644 --- a/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex +++ b/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex @@ -13,22 +13,16 @@ defmodule Pleroma.Web.ActivityPub.MRF.MediaProxyWarmingPolicy do require Logger @options [ - pool: :media + pool: :media, + recv_timeout: 10_000 ] def perform(:prefetch, url) do Logger.debug("Prefetching #{inspect(url)}") - opts = - if Application.get_env(:tesla, :adapter) == Tesla.Adapter.Hackney do - Keyword.put(@options, :recv_timeout, 10_000) - else - @options - end - url |> MediaProxy.url() - |> HTTP.get([], adapter: opts) + |> HTTP.get([], adapter: @options) end def perform(:preload, %{"object" => %{"attachment" => attachments}} = _message) do diff --git a/lib/pleroma/web/rel_me.ex b/lib/pleroma/web/rel_me.ex index 8e2b51508..32bce3c1b 100644 --- a/lib/pleroma/web/rel_me.ex +++ b/lib/pleroma/web/rel_me.ex @@ -5,7 +5,8 @@ defmodule Pleroma.Web.RelMe do @options [ pool: :media, - max_body: 2_000_000 + max_body: 2_000_000, + recv_timeout: 2_000 ] if Pleroma.Config.get(:env) == :test do @@ -23,18 +24,8 @@ defmodule Pleroma.Web.RelMe do def parse(_), do: {:error, "No URL provided"} defp parse_url(url) do - opts = - if Application.get_env(:tesla, :adapter) == Tesla.Adapter.Hackney do - Keyword.merge(@options, - recv_timeout: 2_000, - with_body: true - ) - else - @options - end - with {:ok, %Tesla.Env{body: html, status: status}} when status in 200..299 <- - Pleroma.HTTP.get(url, [], adapter: opts), + Pleroma.HTTP.get(url, [], adapter: @options), {:ok, html_tree} <- Floki.parse_document(html), data <- Floki.attribute(html_tree, "link[rel~=me]", "href") ++ diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index 752ca9f81..084a66466 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -9,14 +9,15 @@ defmodule Pleroma.Web.RichMedia.Helpers do alias Pleroma.Object alias Pleroma.Web.RichMedia.Parser - @rich_media_options [ + @options [ pool: :media, - max_body: 2_000_000 + max_body: 2_000_000, + recv_timeout: 2_000 ] @spec validate_page_url(URI.t() | binary()) :: :ok | :error defp validate_page_url(page_url) when is_binary(page_url) do - validate_tld = Pleroma.Config.get([Pleroma.Formatter, :validate_tld]) + validate_tld = Config.get([Pleroma.Formatter, :validate_tld]) page_url |> Linkify.Parser.url?(validate_tld: validate_tld) @@ -86,16 +87,6 @@ defmodule Pleroma.Web.RichMedia.Helpers do def rich_media_get(url) do headers = [{"user-agent", Pleroma.Application.user_agent() <> "; Bot"}] - options = - if Application.get_env(:tesla, :adapter) == Tesla.Adapter.Hackney do - Keyword.merge(@rich_media_options, - recv_timeout: 2_000, - with_body: true - ) - else - @rich_media_options - end - - Pleroma.HTTP.get(url, headers, adapter: options) + Pleroma.HTTP.get(url, headers, adapter: @options) end end From 8a3d43044a06488545490a89e29c8349d914f164 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Sat, 5 Sep 2020 12:41:01 +0300 Subject: [PATCH 02/12] migrations for renaming gun timeout options --- ...e_await_up_timeout_in_connections_pool.exs | 13 +++++++++++++ ...20200905091427_rename_timeout_in_pools.exs | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 priv/repo/migrations/20200905082737_rename_await_up_timeout_in_connections_pool.exs create mode 100644 priv/repo/migrations/20200905091427_rename_timeout_in_pools.exs diff --git a/priv/repo/migrations/20200905082737_rename_await_up_timeout_in_connections_pool.exs b/priv/repo/migrations/20200905082737_rename_await_up_timeout_in_connections_pool.exs new file mode 100644 index 000000000..22c40663c --- /dev/null +++ b/priv/repo/migrations/20200905082737_rename_await_up_timeout_in_connections_pool.exs @@ -0,0 +1,13 @@ +defmodule Pleroma.Repo.Migrations.RenameAwaitUpTimeoutInConnectionsPool do + use Ecto.Migration + + def change do + with %Pleroma.ConfigDB{} = config <- + Pleroma.ConfigDB.get_by_params(%{group: :pleroma, key: :connections_pool}), + {timeout, value} when is_integer(timeout) <- Keyword.pop(config.value, :await_up_timeout) do + config + |> Ecto.Changeset.change(value: Keyword.put(value, :connect_timeout, timeout)) + |> Pleroma.Repo.update() + end + end +end diff --git a/priv/repo/migrations/20200905091427_rename_timeout_in_pools.exs b/priv/repo/migrations/20200905091427_rename_timeout_in_pools.exs new file mode 100644 index 000000000..bb2f50ecc --- /dev/null +++ b/priv/repo/migrations/20200905091427_rename_timeout_in_pools.exs @@ -0,0 +1,19 @@ +defmodule Pleroma.Repo.Migrations.RenameTimeoutInPools do + use Ecto.Migration + + def change do + with %Pleroma.ConfigDB{} = config <- + Pleroma.ConfigDB.get_by_params(%{group: :pleroma, key: :pools}) do + updated_value = + Enum.map(config.value, fn {pool, pool_value} -> + with {timeout, value} when is_integer(timeout) <- Keyword.pop(pool_value, :timeout) do + {pool, Keyword.put(value, :recv_timeout, timeout)} + end + end) + + config + |> Ecto.Changeset.change(value: updated_value) + |> Pleroma.Repo.update() + end + end +end From 696bf09433aa7f33cf580c71cb7f1f3367d4c124 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Mon, 7 Sep 2020 16:57:42 +0300 Subject: [PATCH 03/12] passing adapter options directly without adapter key --- config/test.exs | 2 +- lib/mix/tasks/pleroma/benchmark.ex | 11 ++--- lib/mix/tasks/pleroma/frontend.ex | 2 +- lib/pleroma/http/ex_aws.ex | 2 +- lib/pleroma/http/http.ex | 2 +- lib/pleroma/http/tzdata.ex | 4 +- .../mrf/media_proxy_warming_policy.ex | 2 +- lib/pleroma/web/rel_me.ex | 2 +- lib/pleroma/web/rich_media/helpers.ex | 2 +- test/web/instances/instance_test.exs | 2 + .../mastodon_api/views/account_view_test.exs | 40 ++++++++++--------- 11 files changed, 36 insertions(+), 35 deletions(-) diff --git a/config/test.exs b/config/test.exs index f0358e384..e9c2273e8 100644 --- a/config/test.exs +++ b/config/test.exs @@ -114,7 +114,7 @@ config :pleroma, Pleroma.Plugs.RemoteIp, enabled: false config :pleroma, Pleroma.Web.ApiSpec.CastAndValidate, strict: true -config :pleroma, :instances_favicons, enabled: true +config :pleroma, :instances_favicons, enabled: false config :pleroma, Pleroma.Uploaders.S3, bucket: nil, diff --git a/lib/mix/tasks/pleroma/benchmark.ex b/lib/mix/tasks/pleroma/benchmark.ex index dd2b9c8f2..a607d5d4f 100644 --- a/lib/mix/tasks/pleroma/benchmark.ex +++ b/lib/mix/tasks/pleroma/benchmark.ex @@ -91,20 +91,17 @@ defmodule Mix.Tasks.Pleroma.Benchmark do "Without conn and without pool" => fn -> {:ok, %Tesla.Env{}} = Pleroma.HTTP.get("https://httpbin.org/stream-bytes/1500", [], - adapter: [pool: :no_pool, receive_conn: false] + pool: :no_pool, + receive_conn: false ) end, "Without conn and with pool" => fn -> {:ok, %Tesla.Env{}} = - Pleroma.HTTP.get("https://httpbin.org/stream-bytes/1500", [], - adapter: [receive_conn: false] - ) + Pleroma.HTTP.get("https://httpbin.org/stream-bytes/1500", [], receive_conn: false) end, "With reused conn and without pool" => fn -> {:ok, %Tesla.Env{}} = - Pleroma.HTTP.get("https://httpbin.org/stream-bytes/1500", [], - adapter: [pool: :no_pool] - ) + Pleroma.HTTP.get("https://httpbin.org/stream-bytes/1500", [], pool: :no_pool) end, "With reused conn and with pool" => fn -> {:ok, %Tesla.Env{}} = Pleroma.HTTP.get("https://httpbin.org/stream-bytes/1500") diff --git a/lib/mix/tasks/pleroma/frontend.ex b/lib/mix/tasks/pleroma/frontend.ex index 73df67439..cbce81ab9 100644 --- a/lib/mix/tasks/pleroma/frontend.ex +++ b/lib/mix/tasks/pleroma/frontend.ex @@ -124,7 +124,7 @@ defmodule Mix.Tasks.Pleroma.Frontend do url = String.replace(frontend_info["build_url"], "${ref}", frontend_info["ref"]) with {:ok, %{status: 200, body: zip_body}} <- - Pleroma.HTTP.get(url, [], adapter: [pool: :media, recv_timeout: 120_000]) do + Pleroma.HTTP.get(url, [], pool: :media, recv_timeout: 120_000) do unzip(zip_body, dest) else e -> {:error, e} diff --git a/lib/pleroma/http/ex_aws.ex b/lib/pleroma/http/ex_aws.ex index c3f335c73..5cac3532f 100644 --- a/lib/pleroma/http/ex_aws.ex +++ b/lib/pleroma/http/ex_aws.ex @@ -11,7 +11,7 @@ defmodule Pleroma.HTTP.ExAws do @impl true def request(method, url, body \\ "", headers \\ [], http_opts \\ []) do - http_opts = Keyword.put_new(http_opts, :adapter, pool: :upload) + http_opts = Keyword.put_new(http_opts, :pool, :upload) case HTTP.request(method, url, body, headers, http_opts) do {:ok, env} -> diff --git a/lib/pleroma/http/http.ex b/lib/pleroma/http/http.ex index 7bc73f4a0..052597191 100644 --- a/lib/pleroma/http/http.ex +++ b/lib/pleroma/http/http.ex @@ -60,7 +60,7 @@ defmodule Pleroma.HTTP do {:ok, Env.t()} | {:error, any()} def request(method, url, body, headers, options) when is_binary(url) do uri = URI.parse(url) - adapter_opts = AdapterHelper.options(uri, options[:adapter] || []) + adapter_opts = AdapterHelper.options(uri, options || []) options = put_in(options[:adapter], adapter_opts) params = options[:params] || [] diff --git a/lib/pleroma/http/tzdata.ex b/lib/pleroma/http/tzdata.ex index 4539ac359..09cfdadf7 100644 --- a/lib/pleroma/http/tzdata.ex +++ b/lib/pleroma/http/tzdata.ex @@ -11,7 +11,7 @@ defmodule Pleroma.HTTP.Tzdata do @impl true def get(url, headers, options) do - options = Keyword.put_new(options, :adapter, pool: :default) + options = Keyword.put_new(options, :pool, :default) with {:ok, %Tesla.Env{} = env} <- HTTP.get(url, headers, options) do {:ok, {env.status, env.headers, env.body}} @@ -20,7 +20,7 @@ defmodule Pleroma.HTTP.Tzdata do @impl true def head(url, headers, options) do - options = Keyword.put_new(options, :adapter, pool: :default) + options = Keyword.put_new(options, :pool, :default) with {:ok, %Tesla.Env{} = env} <- HTTP.head(url, headers, options) do {:ok, {env.status, env.headers}} diff --git a/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex b/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex index a203405a0..98d595469 100644 --- a/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex +++ b/lib/pleroma/web/activity_pub/mrf/media_proxy_warming_policy.ex @@ -22,7 +22,7 @@ defmodule Pleroma.Web.ActivityPub.MRF.MediaProxyWarmingPolicy do url |> MediaProxy.url() - |> HTTP.get([], adapter: @options) + |> HTTP.get([], @options) end def perform(:preload, %{"object" => %{"attachment" => attachments}} = _message) do diff --git a/lib/pleroma/web/rel_me.ex b/lib/pleroma/web/rel_me.ex index 32bce3c1b..28f75b18d 100644 --- a/lib/pleroma/web/rel_me.ex +++ b/lib/pleroma/web/rel_me.ex @@ -25,7 +25,7 @@ defmodule Pleroma.Web.RelMe do defp parse_url(url) do with {:ok, %Tesla.Env{body: html, status: status}} when status in 200..299 <- - Pleroma.HTTP.get(url, [], adapter: @options), + Pleroma.HTTP.get(url, [], @options), {:ok, html_tree} <- Floki.parse_document(html), data <- Floki.attribute(html_tree, "link[rel~=me]", "href") ++ diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index 084a66466..bd7f03cbe 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -87,6 +87,6 @@ defmodule Pleroma.Web.RichMedia.Helpers do def rich_media_get(url) do headers = [{"user-agent", Pleroma.Application.user_agent() <> "; Bot"}] - Pleroma.HTTP.get(url, headers, adapter: @options) + Pleroma.HTTP.get(url, headers, @options) end end diff --git a/test/web/instances/instance_test.exs b/test/web/instances/instance_test.exs index dc6ace843..5d4efcebe 100644 --- a/test/web/instances/instance_test.exs +++ b/test/web/instances/instance_test.exs @@ -112,6 +112,8 @@ defmodule Pleroma.Instances.InstanceTest do end test "Returns nil on too long favicon URLs" do + clear_config([:instances_favicons, :enabled], true) + long_favicon_url = "https://Lorem.ipsum.dolor.sit.amet/consecteturadipiscingelit/Praesentpharetrapurusutaliquamtempus/Mauriseulaoreetarcu/atfacilisisorci/Nullamporttitor/nequesedfeugiatmollis/dolormagnaefficiturlorem/nonpretiumsapienorcieurisus/Nullamveleratsem/Maecenassedaccumsanexnam/favicon.png" diff --git a/test/web/mastodon_api/views/account_view_test.exs b/test/web/mastodon_api/views/account_view_test.exs index 8f37efa3c..68a5d0091 100644 --- a/test/web/mastodon_api/views/account_view_test.exs +++ b/test/web/mastodon_api/views/account_view_test.exs @@ -5,7 +5,6 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do use Pleroma.DataCase - alias Pleroma.Config alias Pleroma.User alias Pleroma.UserRelationship alias Pleroma.Web.CommonAPI @@ -19,8 +18,6 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do :ok end - setup do: clear_config([:instances_favicons, :enabled]) - test "Represent a user account" do background_image = %{ "url" => [%{"href" => "https://example.com/images/asuka_hospital.png"}] @@ -78,8 +75,7 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do pleroma: %{ ap_id: user.ap_id, background_image: "https://example.com/images/asuka_hospital.png", - favicon: - "https://shitposter.club/plugins/Qvitter/img/gnusocial-favicons/favicon-16x16.png", + favicon: nil, confirmation_pending: false, tags: [], is_admin: false, @@ -98,22 +94,29 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do assert expected == AccountView.render("show.json", %{user: user, skip_visibility_check: true}) end - test "Favicon is nil when :instances_favicons is disabled" do - user = insert(:user) + describe "favicon" do + setup do + [user: insert(:user)] + end - Config.put([:instances_favicons, :enabled], true) + test "is parsed when :instance_favicons is enabled", %{user: user} do + clear_config([:instances_favicons, :enabled], true) - assert %{ - pleroma: %{ - favicon: - "https://shitposter.club/plugins/Qvitter/img/gnusocial-favicons/favicon-16x16.png" - } - } = AccountView.render("show.json", %{user: user, skip_visibility_check: true}) + assert %{ + pleroma: %{ + favicon: + "https://shitposter.club/plugins/Qvitter/img/gnusocial-favicons/favicon-16x16.png" + } + } = AccountView.render("show.json", %{user: user, skip_visibility_check: true}) + end - Config.put([:instances_favicons, :enabled], false) + test "is nil when :instances_favicons is disabled", %{user: user} do + assert %{pleroma: %{favicon: nil}} = + AccountView.render("show.json", %{user: user, skip_visibility_check: true}) + end + end - assert %{pleroma: %{favicon: nil}} = - AccountView.render("show.json", %{user: user, skip_visibility_check: true}) + test "Favicon when :instance_favicons is enabled" do end test "Represent the user account for the account owner" do @@ -173,8 +176,7 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do pleroma: %{ ap_id: user.ap_id, background_image: nil, - favicon: - "https://shitposter.club/plugins/Qvitter/img/gnusocial-favicons/favicon-16x16.png", + favicon: nil, confirmation_pending: false, tags: [], is_admin: false, From 18d21aed00dcbdaabd7db25b8b7d0c88141ec98a Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Mon, 7 Sep 2020 19:04:16 +0300 Subject: [PATCH 04/12] deprecation warnings --- lib/pleroma/config/deprecation_warnings.ex | 43 ++++++++++++++++ test/config/deprecation_warnings_test.exs | 59 +++++++++++++++++++--- 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/config/deprecation_warnings.ex b/lib/pleroma/config/deprecation_warnings.ex index 0f52eb210..2bfe4ddba 100644 --- a/lib/pleroma/config/deprecation_warnings.ex +++ b/lib/pleroma/config/deprecation_warnings.ex @@ -56,6 +56,7 @@ defmodule Pleroma.Config.DeprecationWarnings do check_old_mrf_config() check_media_proxy_whitelist_config() check_welcome_message_config() + check_gun_pool_options() end def check_welcome_message_config do @@ -115,4 +116,46 @@ defmodule Pleroma.Config.DeprecationWarnings do """) end end + + def check_gun_pool_options do + pool_config = Config.get(:connections_pool) + + if timeout = pool_config[:await_up_timeout] do + Logger.warn(""" + !!!DEPRECATION WARNING!!! + Your config is using old setting name `await_up_timeout` instead of `connect_timeout`. Setting should work for now, but you are advised to change format to scheme with port to prevent possible issues later. + """) + + Config.put(:connections_pool, Keyword.put_new(pool_config, :connect_timeout, timeout)) + end + + pools_configs = Config.get(:pools) + + warning_preface = """ + !!!DEPRECATION WARNING!!! + Your config is using old setting name `timeout` instead of `recv_timeout` in pool settings. Setting should work for now, but you are advised to change format to scheme with port to prevent possible issues later. + """ + + updated_config = + Enum.reduce(pools_configs, [], fn {pool_name, config}, acc -> + if timeout = config[:timeout] do + Keyword.put(acc, pool_name, Keyword.put_new(config, :recv_timeout, timeout)) + else + acc + end + end) + + if updated_config != [] do + pool_warnings = + updated_config + |> Keyword.keys() + |> Enum.map(fn pool_name -> + "\n* `:timeout` options in #{pool_name} pool is now `:recv_timeout`" + end) + + Logger.warn(Enum.join([warning_preface | pool_warnings])) + + Config.put(:pools, updated_config) + end + end end diff --git a/test/config/deprecation_warnings_test.exs b/test/config/deprecation_warnings_test.exs index 555661a71..e22052404 100644 --- a/test/config/deprecation_warnings_test.exs +++ b/test/config/deprecation_warnings_test.exs @@ -4,12 +4,15 @@ defmodule Pleroma.Config.DeprecationWarningsTest do import ExUnit.CaptureLog + alias Pleroma.Config + alias Pleroma.Config.DeprecationWarnings + test "check_old_mrf_config/0" do clear_config([:instance, :rewrite_policy], Pleroma.Web.ActivityPub.MRF.NoOpPolicy) clear_config([:instance, :mrf_transparency], true) clear_config([:instance, :mrf_transparency_exclusions], []) - assert capture_log(fn -> Pleroma.Config.DeprecationWarnings.check_old_mrf_config() end) =~ + assert capture_log(fn -> DeprecationWarnings.check_old_mrf_config() end) =~ """ !!!DEPRECATION WARNING!!! Your config is using old namespaces for MRF configuration. They should work for now, but you are advised to change to new namespaces to prevent possible issues later: @@ -44,22 +47,66 @@ defmodule Pleroma.Config.DeprecationWarningsTest do ] assert capture_log(fn -> - Pleroma.Config.DeprecationWarnings.move_namespace_and_warn( + DeprecationWarnings.move_namespace_and_warn( config_map, "Warning preface" ) end) =~ "Warning preface\n error :key\n error :key2\n error :key3" - assert Pleroma.Config.get(new_group1) == 1 - assert Pleroma.Config.get(new_group2) == 2 - assert Pleroma.Config.get(new_group3) == 3 + assert Config.get(new_group1) == 1 + assert Config.get(new_group2) == 2 + assert Config.get(new_group3) == 3 end test "check_media_proxy_whitelist_config/0" do clear_config([:media_proxy, :whitelist], ["https://example.com", "example2.com"]) assert capture_log(fn -> - Pleroma.Config.DeprecationWarnings.check_media_proxy_whitelist_config() + DeprecationWarnings.check_media_proxy_whitelist_config() end) =~ "Your config is using old format (only domain) for MediaProxy whitelist option" end + + describe "check_gun_pool_options/0" do + test "await_up_timeout" do + config = Config.get(:connections_pool) + clear_config(:connections_pool, Keyword.put(config, :await_up_timeout, 5_000)) + + assert capture_log(fn -> + DeprecationWarnings.check_gun_pool_options() + end) =~ + "Your config is using old setting name `await_up_timeout` instead of `connect_timeout`" + end + + test "pool timeout" do + old_config = [ + federation: [ + size: 50, + max_waiting: 10, + timeout: 10_000 + ], + media: [ + size: 50, + max_waiting: 10, + timeout: 10_000 + ], + upload: [ + size: 25, + max_waiting: 5, + timeout: 15_000 + ], + default: [ + size: 10, + max_waiting: 2, + timeout: 5_000 + ] + ] + + clear_config(:pools, old_config) + + assert capture_log(fn -> + DeprecationWarnings.check_gun_pool_options() + end) =~ + "Your config is using old setting name `timeout` instead of `recv_timeout` in pool settings" + end + end end From 7ad1732ed2444d4d7bb1a89e66d46e5d52536e0f Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Mon, 7 Sep 2020 19:55:14 +0300 Subject: [PATCH 05/12] changelog entry --- CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46f1e859b..f303e22a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased +### Changed + +- Settings renaming: `:await_up_timeout` in `:connections_pool` namespace to `connect_timeout`, `timeout` in `pools` namespace to `recv_timeout`. + ### Removed - **Breaking:** Removed `Pleroma.Workers.Cron.StatsWorker` setting from Oban `:crontab`. - ## unreleased-patch - ??? ### Added @@ -25,7 +28,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Mastodon API: Cards being wrong for preview statuses due to cache key collision - Password resets no longer processed for deactivated accounts - ## [2.1.0] - 2020-08-28 ### Changed From d8a48f838697ce6f1d37f5504c1e51b316aed037 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Tue, 8 Sep 2020 12:23:08 +0300 Subject: [PATCH 06/12] CHANGELOG.md: Split settings renaming to 2 separate entries --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f303e22a7..e6180a6da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Changed -- Settings renaming: `:await_up_timeout` in `:connections_pool` namespace to `connect_timeout`, `timeout` in `pools` namespace to `recv_timeout`. +- Renamed `:await_up_timeout` in `:connections_pool` namespace to `:connect_timeout`, old name is deprecated. +- Renamed `:timeout` in `pools` namespace to `:recv_timeout`, old name is deprecated. ### Removed From fa347b9c2f416cd2c402e3e6eebb561dfc0ee8a8 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 26 Aug 2020 13:32:03 -0500 Subject: [PATCH 07/12] Fix uploading webp image files when Exiftool Upload Filter is enabled --- CHANGELOG.md | 1 + lib/pleroma/upload/filter/exiftool.ex | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46f1e859b..cfe0651c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Fix whole_word always returning false on filter get requests - Migrations not working on OTP releases if the database was connected over ssl - Fix relay following +- Fixed uploading webp images when the Exiftool Upload Filter is enabled ## [2.0.7] - 2020-06-13 diff --git a/lib/pleroma/upload/filter/exiftool.ex b/lib/pleroma/upload/filter/exiftool.ex index ea8798fe3..a91bd5e24 100644 --- a/lib/pleroma/upload/filter/exiftool.ex +++ b/lib/pleroma/upload/filter/exiftool.ex @@ -10,9 +10,20 @@ defmodule Pleroma.Upload.Filter.Exiftool do @behaviour Pleroma.Upload.Filter @spec filter(Pleroma.Upload.t()) :: :ok | {:error, String.t()} - def filter(%Pleroma.Upload{tempfile: file, content_type: "image" <> _}) do + def filter(%Pleroma.Upload{name: file, tempfile: path, content_type: "image" <> _}) do + # webp is not compatible with exiftool at this time + if Regex.match?(~r/\.(webp)$/i, file) do + :ok + else + strip_exif(path) + end + end + + def filter(_), do: :ok + + defp strip_exif(path) do try do - case System.cmd("exiftool", ["-overwrite_original", "-gps:all=", file], parallelism: true) do + case System.cmd("exiftool", ["-overwrite_original", "-gps:all=", path], parallelism: true) do {_response, 0} -> :ok {error, 1} -> {:error, error} end @@ -21,6 +32,4 @@ defmodule Pleroma.Upload.Filter.Exiftool do {:error, "exiftool command not found"} end end - - def filter(_), do: :ok end From 2165a249744a1ad4386a9d237871abe88e298942 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 4 Sep 2020 17:40:59 -0500 Subject: [PATCH 08/12] Improve upload filter return values so we can identify when filters make no changes to the input --- lib/pleroma/upload/filter.ex | 13 ++++++++++--- lib/pleroma/upload/filter/anonymize_filename.ex | 4 +++- lib/pleroma/upload/filter/dedupe.ex | 4 ++-- lib/pleroma/upload/filter/exiftool.ex | 8 ++++---- lib/pleroma/upload/filter/mogrifun.ex | 6 +++--- lib/pleroma/upload/filter/mogrify.ex | 6 +++--- test/upload/filter/anonymize_filename_test.exs | 6 +++--- test/upload/filter/dedupe_test.exs | 1 + test/upload/filter/exiftool_test.exs | 2 +- test/upload/filter/mogrifun_test.exs | 2 +- test/upload/filter/mogrify_test.exs | 2 +- 11 files changed, 32 insertions(+), 22 deletions(-) diff --git a/lib/pleroma/upload/filter.ex b/lib/pleroma/upload/filter.ex index dbdadc97f..661135634 100644 --- a/lib/pleroma/upload/filter.ex +++ b/lib/pleroma/upload/filter.ex @@ -15,7 +15,11 @@ defmodule Pleroma.Upload.Filter do require Logger - @callback filter(Pleroma.Upload.t()) :: :ok | {:ok, Pleroma.Upload.t()} | {:error, any()} + @callback filter(Pleroma.Upload.t()) :: + {:ok, :filtered} + | {:ok, :noop} + | {:ok, :filtered, Pleroma.Upload.t()} + | {:error, any()} @spec filter([module()], Pleroma.Upload.t()) :: {:ok, Pleroma.Upload.t()} | {:error, any()} @@ -25,10 +29,13 @@ defmodule Pleroma.Upload.Filter do def filter([filter | rest], upload) do case filter.filter(upload) do - :ok -> + {:ok, :filtered} -> filter(rest, upload) - {:ok, upload} -> + {:ok, :filtered, upload} -> + filter(rest, upload) + + {:ok, :noop} -> filter(rest, upload) error -> diff --git a/lib/pleroma/upload/filter/anonymize_filename.ex b/lib/pleroma/upload/filter/anonymize_filename.ex index 07ead8203..fc456e4f4 100644 --- a/lib/pleroma/upload/filter/anonymize_filename.ex +++ b/lib/pleroma/upload/filter/anonymize_filename.ex @@ -16,9 +16,11 @@ defmodule Pleroma.Upload.Filter.AnonymizeFilename do def filter(%Upload{name: name} = upload) do extension = List.last(String.split(name, ".")) name = predefined_name(extension) || random(extension) - {:ok, %Upload{upload | name: name}} + {:ok, :filtered, %Upload{upload | name: name}} end + def filter(_), do: {:ok, :noop} + @spec predefined_name(String.t()) :: String.t() | nil defp predefined_name(extension) do with name when not is_nil(name) <- Config.get([__MODULE__, :text]), diff --git a/lib/pleroma/upload/filter/dedupe.ex b/lib/pleroma/upload/filter/dedupe.ex index 41218a918..86cbc8996 100644 --- a/lib/pleroma/upload/filter/dedupe.ex +++ b/lib/pleroma/upload/filter/dedupe.ex @@ -17,8 +17,8 @@ defmodule Pleroma.Upload.Filter.Dedupe do |> Base.encode16(case: :lower) filename = shasum <> "." <> extension - {:ok, %Upload{upload | id: shasum, path: filename}} + {:ok, :filtered, %Upload{upload | id: shasum, path: filename}} end - def filter(_), do: :ok + def filter(_), do: {:ok, :noop} end diff --git a/lib/pleroma/upload/filter/exiftool.ex b/lib/pleroma/upload/filter/exiftool.ex index a91bd5e24..94d12c01b 100644 --- a/lib/pleroma/upload/filter/exiftool.ex +++ b/lib/pleroma/upload/filter/exiftool.ex @@ -9,22 +9,22 @@ defmodule Pleroma.Upload.Filter.Exiftool do """ @behaviour Pleroma.Upload.Filter - @spec filter(Pleroma.Upload.t()) :: :ok | {:error, String.t()} + @spec filter(Pleroma.Upload.t()) :: {:ok, any()} | {:error, String.t()} def filter(%Pleroma.Upload{name: file, tempfile: path, content_type: "image" <> _}) do # webp is not compatible with exiftool at this time if Regex.match?(~r/\.(webp)$/i, file) do - :ok + {:ok, :noop} else strip_exif(path) end end - def filter(_), do: :ok + def filter(_), do: {:ok, :noop} defp strip_exif(path) do try do case System.cmd("exiftool", ["-overwrite_original", "-gps:all=", path], parallelism: true) do - {_response, 0} -> :ok + {_response, 0} -> {:ok, :filtered} {error, 1} -> {:error, error} end rescue diff --git a/lib/pleroma/upload/filter/mogrifun.ex b/lib/pleroma/upload/filter/mogrifun.ex index c8fa7b190..363e5cf0f 100644 --- a/lib/pleroma/upload/filter/mogrifun.ex +++ b/lib/pleroma/upload/filter/mogrifun.ex @@ -38,16 +38,16 @@ defmodule Pleroma.Upload.Filter.Mogrifun do [{"fill", "yellow"}, {"tint", "40"}] ] - @spec filter(Pleroma.Upload.t()) :: :ok | {:error, String.t()} + @spec filter(Pleroma.Upload.t()) :: {:ok, atom()} | {:error, String.t()} def filter(%Pleroma.Upload{tempfile: file, content_type: "image" <> _}) do try do Filter.Mogrify.do_filter(file, [Enum.random(@filters)]) - :ok + {:ok, :filtered} rescue _e in ErlangError -> {:error, "mogrify command not found"} end end - def filter(_), do: :ok + def filter(_), do: {:ok, :noop} end diff --git a/lib/pleroma/upload/filter/mogrify.ex b/lib/pleroma/upload/filter/mogrify.ex index 7a45add5a..71968fd9c 100644 --- a/lib/pleroma/upload/filter/mogrify.ex +++ b/lib/pleroma/upload/filter/mogrify.ex @@ -8,18 +8,18 @@ defmodule Pleroma.Upload.Filter.Mogrify do @type conversion :: action :: String.t() | {action :: String.t(), opts :: String.t()} @type conversions :: conversion() | [conversion()] - @spec filter(Pleroma.Upload.t()) :: :ok | {:error, String.t()} + @spec filter(Pleroma.Upload.t()) :: {:ok, :atom} | {:error, String.t()} def filter(%Pleroma.Upload{tempfile: file, content_type: "image" <> _}) do try do do_filter(file, Pleroma.Config.get!([__MODULE__, :args])) - :ok + {:ok, :filtered} rescue _e in ErlangError -> {:error, "mogrify command not found"} end end - def filter(_), do: :ok + def filter(_), do: {:ok, :noop} def do_filter(file, filters) do file diff --git a/test/upload/filter/anonymize_filename_test.exs b/test/upload/filter/anonymize_filename_test.exs index adff70f57..19b915cc8 100644 --- a/test/upload/filter/anonymize_filename_test.exs +++ b/test/upload/filter/anonymize_filename_test.exs @@ -24,18 +24,18 @@ defmodule Pleroma.Upload.Filter.AnonymizeFilenameTest do test "it replaces filename on pre-defined text", %{upload_file: upload_file} do Config.put([Upload.Filter.AnonymizeFilename, :text], "custom-file.png") - {:ok, %Upload{name: name}} = Upload.Filter.AnonymizeFilename.filter(upload_file) + {:ok, :filtered, %Upload{name: name}} = Upload.Filter.AnonymizeFilename.filter(upload_file) assert name == "custom-file.png" end test "it replaces filename on pre-defined text expression", %{upload_file: upload_file} do Config.put([Upload.Filter.AnonymizeFilename, :text], "custom-file.{extension}") - {:ok, %Upload{name: name}} = Upload.Filter.AnonymizeFilename.filter(upload_file) + {:ok, :filtered, %Upload{name: name}} = Upload.Filter.AnonymizeFilename.filter(upload_file) assert name == "custom-file.jpg" end test "it replaces filename on random text", %{upload_file: upload_file} do - {:ok, %Upload{name: name}} = Upload.Filter.AnonymizeFilename.filter(upload_file) + {:ok, :filtered, %Upload{name: name}} = Upload.Filter.AnonymizeFilename.filter(upload_file) assert <<_::bytes-size(14)>> <> ".jpg" = name refute name == "an… image.jpg" end diff --git a/test/upload/filter/dedupe_test.exs b/test/upload/filter/dedupe_test.exs index 966c353f7..75c7198e1 100644 --- a/test/upload/filter/dedupe_test.exs +++ b/test/upload/filter/dedupe_test.exs @@ -25,6 +25,7 @@ defmodule Pleroma.Upload.Filter.DedupeTest do assert { :ok, + :filtered, %Pleroma.Upload{id: @shasum, path: @shasum <> ".jpg"} } = Dedupe.filter(upload) end diff --git a/test/upload/filter/exiftool_test.exs b/test/upload/filter/exiftool_test.exs index 8ed7d650b..094253a25 100644 --- a/test/upload/filter/exiftool_test.exs +++ b/test/upload/filter/exiftool_test.exs @@ -21,7 +21,7 @@ defmodule Pleroma.Upload.Filter.ExiftoolTest do tempfile: Path.absname("test/fixtures/DSCN0010_tmp.jpg") } - assert Filter.Exiftool.filter(upload) == :ok + assert Filter.Exiftool.filter(upload) == {:ok, :filtered} {exif_original, 0} = System.cmd("exiftool", ["test/fixtures/DSCN0010.jpg"]) {exif_filtered, 0} = System.cmd("exiftool", ["test/fixtures/DSCN0010_tmp.jpg"]) diff --git a/test/upload/filter/mogrifun_test.exs b/test/upload/filter/mogrifun_test.exs index 2426a8496..dc1e9e78f 100644 --- a/test/upload/filter/mogrifun_test.exs +++ b/test/upload/filter/mogrifun_test.exs @@ -36,7 +36,7 @@ defmodule Pleroma.Upload.Filter.MogrifunTest do save: fn _f, _o -> :ok end ]} ]) do - assert Filter.Mogrifun.filter(upload) == :ok + assert Filter.Mogrifun.filter(upload) == {:ok, :filtered} end Task.await(task) diff --git a/test/upload/filter/mogrify_test.exs b/test/upload/filter/mogrify_test.exs index 62ca30487..bf64b96b3 100644 --- a/test/upload/filter/mogrify_test.exs +++ b/test/upload/filter/mogrify_test.exs @@ -33,7 +33,7 @@ defmodule Pleroma.Upload.Filter.MogrifyTest do custom: fn _m, _a -> :ok end, custom: fn m, a, o -> send(task.pid, {:apply_filter, {m, a, o}}) end, save: fn _f, _o -> :ok end do - assert Filter.Mogrify.filter(upload) == :ok + assert Filter.Mogrify.filter(upload) == {:ok, :filtered} end Task.await(task) From 3a98960c2684229435c5d04e4f3e0611098ceea2 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 4 Sep 2020 17:50:16 -0500 Subject: [PATCH 09/12] Verify webp files are not processed with exiftool --- test/upload/filter/exiftool_test.exs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/upload/filter/exiftool_test.exs b/test/upload/filter/exiftool_test.exs index 094253a25..fe24036d9 100644 --- a/test/upload/filter/exiftool_test.exs +++ b/test/upload/filter/exiftool_test.exs @@ -30,4 +30,15 @@ defmodule Pleroma.Upload.Filter.ExiftoolTest do assert String.match?(exif_original, ~r/GPS/) refute String.match?(exif_filtered, ~r/GPS/) end + + test "verify webp files are skipped" do + upload = %Pleroma.Upload{ + name: "sample.webp", + content_type: "image/webp", + path: Path.absname("/dev/null"), + tempfile: Path.absname("/dev/null") + } + + assert Filter.Exiftool.filter(upload) == {:ok, :noop} + end end From 216c84a8f4d82649110ffaa2bc9d02b879805c5f Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 4 Sep 2020 17:56:05 -0500 Subject: [PATCH 10/12] Bypass the filter based on content-type as well in case a webp image is uploaded with the wrong file extension. --- lib/pleroma/upload/filter/exiftool.ex | 5 ++++- test/upload/filter/exiftool_test.exs | 10 +++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/upload/filter/exiftool.ex b/lib/pleroma/upload/filter/exiftool.ex index 94d12c01b..b07a671ac 100644 --- a/lib/pleroma/upload/filter/exiftool.ex +++ b/lib/pleroma/upload/filter/exiftool.ex @@ -10,8 +10,11 @@ defmodule Pleroma.Upload.Filter.Exiftool do @behaviour Pleroma.Upload.Filter @spec filter(Pleroma.Upload.t()) :: {:ok, any()} | {:error, String.t()} + + # webp is not compatible with exiftool at this time + def filter(%Pleroma.Upload{content_type: "image/webp"}), do: {:ok, :noop} + def filter(%Pleroma.Upload{name: file, tempfile: path, content_type: "image" <> _}) do - # webp is not compatible with exiftool at this time if Regex.match?(~r/\.(webp)$/i, file) do {:ok, :noop} else diff --git a/test/upload/filter/exiftool_test.exs b/test/upload/filter/exiftool_test.exs index fe24036d9..84a3f8b30 100644 --- a/test/upload/filter/exiftool_test.exs +++ b/test/upload/filter/exiftool_test.exs @@ -34,11 +34,15 @@ defmodule Pleroma.Upload.Filter.ExiftoolTest do test "verify webp files are skipped" do upload = %Pleroma.Upload{ name: "sample.webp", - content_type: "image/webp", - path: Path.absname("/dev/null"), - tempfile: Path.absname("/dev/null") + content_type: "image/webp" + } + + bad_type = %Pleroma.Upload{ + name: "sample.webp", + content_type: "image/jpeg" } assert Filter.Exiftool.filter(upload) == {:ok, :noop} + assert Filter.Exiftool.filter(bad_type) == {:ok, :noop} end end From 4ea07f74e9416da8f97a12cfdc24da82e1c00d91 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 4 Sep 2020 22:18:01 -0500 Subject: [PATCH 11/12] Revert/simplify. We only need to check the content-type. There's no chance a webp file will get mismatched as another image type. --- lib/pleroma/upload/filter/exiftool.ex | 16 ++++------------ test/upload/filter/exiftool_test.exs | 6 ------ 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/lib/pleroma/upload/filter/exiftool.ex b/lib/pleroma/upload/filter/exiftool.ex index b07a671ac..1fd0cfdaa 100644 --- a/lib/pleroma/upload/filter/exiftool.ex +++ b/lib/pleroma/upload/filter/exiftool.ex @@ -14,19 +14,9 @@ defmodule Pleroma.Upload.Filter.Exiftool do # webp is not compatible with exiftool at this time def filter(%Pleroma.Upload{content_type: "image/webp"}), do: {:ok, :noop} - def filter(%Pleroma.Upload{name: file, tempfile: path, content_type: "image" <> _}) do - if Regex.match?(~r/\.(webp)$/i, file) do - {:ok, :noop} - else - strip_exif(path) - end - end - - def filter(_), do: {:ok, :noop} - - defp strip_exif(path) do + def filter(%Pleroma.Upload{tempfile: file, content_type: "image" <> _}) do try do - case System.cmd("exiftool", ["-overwrite_original", "-gps:all=", path], parallelism: true) do + case System.cmd("exiftool", ["-overwrite_original", "-gps:all=", file], parallelism: true) do {_response, 0} -> {:ok, :filtered} {error, 1} -> {:error, error} end @@ -35,4 +25,6 @@ defmodule Pleroma.Upload.Filter.Exiftool do {:error, "exiftool command not found"} end end + + def filter(_), do: {:ok, :noop} end diff --git a/test/upload/filter/exiftool_test.exs b/test/upload/filter/exiftool_test.exs index 84a3f8b30..d4cd4ba11 100644 --- a/test/upload/filter/exiftool_test.exs +++ b/test/upload/filter/exiftool_test.exs @@ -37,12 +37,6 @@ defmodule Pleroma.Upload.Filter.ExiftoolTest do content_type: "image/webp" } - bad_type = %Pleroma.Upload{ - name: "sample.webp", - content_type: "image/jpeg" - } - assert Filter.Exiftool.filter(upload) == {:ok, :noop} - assert Filter.Exiftool.filter(bad_type) == {:ok, :noop} end end From 0f27211dd0b9177abf0eb8b10665c6351f07756a Mon Sep 17 00:00:00 2001 From: rinpatch Date: Tue, 8 Sep 2020 12:28:38 +0300 Subject: [PATCH 12/12] CHANGELOG.md: move the exiftool webp entry to a proper section Also clarify how it was fixed --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfe0651c8..527df8438 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed - Possible OOM errors with the default HTTP adapter +- Fixed uploading webp images when the Exiftool Upload Filter is enabled by skipping them - Mastodon API: Search parameter `following` now correctly returns the followings rather than the followers - Mastodon API: Timelines hanging for (`number of posts with links * rich media timeout`) in the worst case. Reduced to just rich media timeout. @@ -138,7 +139,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Fix whole_word always returning false on filter get requests - Migrations not working on OTP releases if the database was connected over ssl - Fix relay following -- Fixed uploading webp images when the Exiftool Upload Filter is enabled ## [2.0.7] - 2020-06-13