From 010edcbcb51dfddc83d5a3810c257c1678429c2d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 21 Aug 2024 14:50:19 -0400 Subject: [PATCH 01/48] Use Map.filter now that minimum Elixir version is 1.13 --- lib/pleroma/maps.ex | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/pleroma/maps.ex b/lib/pleroma/maps.ex index 5020a8ff8d..1afbde4840 100644 --- a/lib/pleroma/maps.ex +++ b/lib/pleroma/maps.ex @@ -20,15 +20,13 @@ def safe_put_in(data, keys, value) when is_map(data) and is_list(keys) do end def filter_empty_values(data) do - # TODO: Change to Map.filter in Elixir 1.13+ data - |> Enum.filter(fn + |> Map.filter(fn {_k, nil} -> false {_k, ""} -> false {_k, []} -> false {_k, %{} = v} -> Map.keys(v) != [] {_k, _v} -> true end) - |> Map.new() end end From e65555e8c5cacb36a404579f56fb501a7fba0781 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 21 Aug 2024 15:11:41 -0400 Subject: [PATCH 02/48] Remove workaround for URI.merge bug on nil fields before Elixir 1.13 https://github.com/elixir-lang/elixir/issues/10771 --- lib/pleroma/web/mastodon_api/views/status_view.ex | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index 747638c53a..1b78477d0c 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -803,19 +803,7 @@ defp build_application(%{"type" => _type, "name" => name, "url" => url}), defp build_application(_), do: nil - # Workaround for Elixir issue #10771 - # Avoid applying URI.merge unless necessary - # TODO: revert to always attempting URI.merge(image_url_data, page_url_data) - # when Elixir 1.12 is the minimum supported version - @spec build_image_url(struct() | nil, struct()) :: String.t() | nil - defp build_image_url( - %URI{scheme: image_scheme, host: image_host} = image_url_data, - %URI{} = _page_url_data - ) - when not is_nil(image_scheme) and not is_nil(image_host) do - image_url_data |> to_string - end - + @spec build_image_url(URI.t(), URI.t()) :: String.t() defp build_image_url(%URI{} = image_url_data, %URI{} = page_url_data) do URI.merge(page_url_data, image_url_data) |> to_string end From 5138a4984ba8cf04e0b6015a7a9253a9013e013e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 21 Aug 2024 15:24:33 -0400 Subject: [PATCH 03/48] Skip changelog --- changelog.d/todo-cleanup.skip | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 changelog.d/todo-cleanup.skip diff --git a/changelog.d/todo-cleanup.skip b/changelog.d/todo-cleanup.skip new file mode 100644 index 0000000000..e69de29bb2 From 5f6506d864239408e9fa3705c5dd7b241307241a Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 20:39:32 -0400 Subject: [PATCH 04/48] Pleroma.HTTP: option stream: true will return a stream as the body for Gun adapter --- lib/pleroma/http/adapter_helper/gun.ex | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/pleroma/http/adapter_helper/gun.ex b/lib/pleroma/http/adapter_helper/gun.ex index 1fe8dd4b26..f9a8180f25 100644 --- a/lib/pleroma/http/adapter_helper/gun.ex +++ b/lib/pleroma/http/adapter_helper/gun.ex @@ -32,6 +32,7 @@ def options(incoming_opts \\ [], %URI{} = uri) do |> AdapterHelper.maybe_add_proxy(proxy) |> Keyword.merge(incoming_opts) |> put_timeout() + |> maybe_stream() end defp add_scheme_opts(opts, %{scheme: "http"}), do: opts @@ -47,6 +48,14 @@ defp put_timeout(opts) do Keyword.put(opts, :timeout, recv_timeout) end + # Tesla Gun adapter uses body_as: :stream + defp maybe_stream(opts) do + case Keyword.pop(opts, :stream, nil) do + {true, opts} -> Keyword.put(opts, :body_as, :stream) + {_, opts} -> opts + end + end + @spec pool_timeout(pool()) :: non_neg_integer() def pool_timeout(pool) do default = Config.get([:pools, :default, :recv_timeout], 5_000) From bb279c28025522764272468e3177a5f6701bc155 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 21:08:25 -0400 Subject: [PATCH 05/48] Pleroma.HTTP add AdapterHelper.can_stream? to assist with discovering if the current adapter supports returning a Stream body --- lib/pleroma/http/adapter_helper.ex | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/pleroma/http/adapter_helper.ex b/lib/pleroma/http/adapter_helper.ex index dcb27a29d0..f8bde2ac34 100644 --- a/lib/pleroma/http/adapter_helper.ex +++ b/lib/pleroma/http/adapter_helper.ex @@ -118,4 +118,13 @@ def format_host(host) do host_charlist end end + + #TODO add Finch support once we have an AdapterHelper for it + @spec can_stream? :: bool() + def can_stream? do + case Application.get_env(:tesla, :adapter) do + Tesla.Adapter.Gun -> true + _ -> false + end + end end From ec8db9d4eedfade5a8b74425b21b07b3f4e44992 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 21:09:15 -0400 Subject: [PATCH 06/48] RichMedia: skip the HTTP HEAD request for adapters that support streaming the response body --- lib/pleroma/web/rich_media/helpers.ex | 38 +++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index e2889b3516..88bfbae680 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -11,16 +11,39 @@ defmodule Pleroma.Web.RichMedia.Helpers do @spec rich_media_get(String.t()) :: {:ok, String.t()} | get_errors() def rich_media_get(url) do - headers = [{"user-agent", Pleroma.Application.user_agent() <> "; Bot"}] + case Pleroma.HTTP.AdapterHelper.can_stream?() do + true -> stream(url) + false -> head_first(url) + end + |> handle_result(url) + end + defp stream(url) do + with {_, {:ok, %Tesla.Env{status: 200, body: stream_body, headers: headers}}} <- + {:head, Pleroma.HTTP.get(url, req_headers(), http_options())}, + {_, :ok} <- {:content_type, check_content_type(headers)}, + {_, :ok} <- {:content_length, check_content_length(headers)} do + body = Enum.into(stream_body, <<>>) + {:ok, body} + end + end + + defp head_first(url) do with {_, {:ok, %Tesla.Env{status: 200, headers: headers}}} <- - {:head, Pleroma.HTTP.head(url, headers, http_options())}, + {:head, Pleroma.HTTP.head(url, req_headers(), http_options())}, {_, :ok} <- {:content_type, check_content_type(headers)}, {_, :ok} <- {:content_length, check_content_length(headers)}, {_, {:ok, %Tesla.Env{status: 200, body: body}}} <- - {:get, Pleroma.HTTP.get(url, headers, http_options())} do + {:get, Pleroma.HTTP.get(url, req_headers(), http_options())} do {:ok, body} - else + end + end + + defp handle_result(result, url) do + case result do + {:ok, body} -> + {:ok, body} + {:head, _} -> Logger.debug("Rich media error for #{url}: HTTP HEAD failed") {:error, :head} @@ -74,7 +97,12 @@ defp http_options do [ pool: :rich_media, max_body: Config.get([:rich_media, :max_body], 5_000_000), - tesla_middleware: [{Tesla.Middleware.Timeout, timeout: timeout}] + tesla_middleware: [{Tesla.Middleware.Timeout, timeout: timeout}], + stream: true ] end + + defp req_headers do + [{"user-agent", Pleroma.Application.user_agent() <> "; Bot"}] + end end From 0a86d2b3ac9c90a16aec1237019ecfcb1e680728 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 21:22:59 -0400 Subject: [PATCH 07/48] Handle streaming response errors --- lib/pleroma/http/adapter_helper.ex | 2 +- lib/pleroma/web/rich_media/helpers.ex | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/pleroma/http/adapter_helper.ex b/lib/pleroma/http/adapter_helper.ex index f8bde2ac34..4dbcccdcc1 100644 --- a/lib/pleroma/http/adapter_helper.ex +++ b/lib/pleroma/http/adapter_helper.ex @@ -119,7 +119,7 @@ def format_host(host) do end end - #TODO add Finch support once we have an AdapterHelper for it + # TODO add Finch support once we have an AdapterHelper for it @spec can_stream? :: bool() def can_stream? do case Application.get_env(:tesla, :adapter) do diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index 88bfbae680..db1310b23f 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -11,10 +11,10 @@ defmodule Pleroma.Web.RichMedia.Helpers do @spec rich_media_get(String.t()) :: {:ok, String.t()} | get_errors() def rich_media_get(url) do - case Pleroma.HTTP.AdapterHelper.can_stream?() do - true -> stream(url) - false -> head_first(url) - end + case Pleroma.HTTP.AdapterHelper.can_stream?() do + true -> stream(url) + false -> head_first(url) + end |> handle_result(url) end @@ -22,8 +22,8 @@ defp stream(url) do with {_, {:ok, %Tesla.Env{status: 200, body: stream_body, headers: headers}}} <- {:head, Pleroma.HTTP.get(url, req_headers(), http_options())}, {_, :ok} <- {:content_type, check_content_type(headers)}, - {_, :ok} <- {:content_length, check_content_length(headers)} do - body = Enum.into(stream_body, <<>>) + {_, :ok} <- {:content_length, check_content_length(headers)}, + body <- Enum.into(stream_body, <<>>) do {:ok, body} end end @@ -59,6 +59,10 @@ defp handle_result(result, url) do {:get, _} -> Logger.debug("Rich media error for #{url}: HTTP GET failed") {:error, :get} + + {:error, :recv_chunk_timeout} -> + Logger.debug("Rich media error for #{url}: HTTP streaming response failed") + {:error, :get} end end From 116fe77b77eedd2feb073d3be256fea08169c95b Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 21:55:06 -0400 Subject: [PATCH 08/48] Tesla.Middleware.Timeout breaks streaming bodies These are executed by Oban now and Oban can enforce the timeout if the regular HTTP timeout is not sufficient. --- lib/pleroma/web/rich_media/helpers.ex | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index db1310b23f..880d19218f 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -96,12 +96,9 @@ defp check_content_length(headers) do end defp http_options do - timeout = Config.get!([:rich_media, :timeout]) - [ pool: :rich_media, max_body: Config.get([:rich_media, :max_body], 5_000_000), - tesla_middleware: [{Tesla.Middleware.Timeout, timeout: timeout}], stream: true ] end From 44901502ffd7713d498976e2d2b9a55c298f1876 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 21:56:02 -0400 Subject: [PATCH 09/48] Fix incorrect identifier for the with statement --- lib/pleroma/web/rich_media/helpers.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index 880d19218f..b819843433 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -20,7 +20,7 @@ def rich_media_get(url) do defp stream(url) do with {_, {:ok, %Tesla.Env{status: 200, body: stream_body, headers: headers}}} <- - {:head, Pleroma.HTTP.get(url, req_headers(), http_options())}, + {:get, Pleroma.HTTP.get(url, req_headers(), http_options())}, {_, :ok} <- {:content_type, check_content_type(headers)}, {_, :ok} <- {:content_length, check_content_length(headers)}, body <- Enum.into(stream_body, <<>>) do From 0804b73c0ae5846a133386c09970546375e3d918 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 27 Aug 2024 22:08:29 -0400 Subject: [PATCH 10/48] This error is not returned by Tesla Upstream has a bug filed for this as they aren't handling this error internally, so it was raising --- lib/pleroma/web/rich_media/helpers.ex | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index b819843433..a242ca640a 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -59,10 +59,6 @@ defp handle_result(result, url) do {:get, _} -> Logger.debug("Rich media error for #{url}: HTTP GET failed") {:error, :get} - - {:error, :recv_chunk_timeout} -> - Logger.debug("Rich media error for #{url}: HTTP streaming response failed") - {:error, :get} end end From 3419e2cbdd3bf1c21e3bbf44ea5313ecfd03c989 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?marcin=20miko=C5=82ajczak?= Date: Wed, 28 Aug 2024 17:37:42 +0200 Subject: [PATCH 11/48] Correct response in AdminAPI docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: marcin mikołajczak --- changelog.d/docs-fix.skip | 0 docs/development/API/admin_api.md | 31 +++++++++++++++++-------------- 2 files changed, 17 insertions(+), 14 deletions(-) create mode 100644 changelog.d/docs-fix.skip diff --git a/changelog.d/docs-fix.skip b/changelog.d/docs-fix.skip new file mode 100644 index 0000000000..e69de29bb2 diff --git a/docs/development/API/admin_api.md b/docs/development/API/admin_api.md index 5b373b8e1f..409e78a1e8 100644 --- a/docs/development/API/admin_api.md +++ b/docs/development/API/admin_api.md @@ -433,7 +433,7 @@ Response: * On success: URL of the unfollowed relay ```json -{"https://example.com/relay"} +"https://example.com/relay" ``` ## `POST /api/v1/pleroma/admin/users/invite_token` @@ -1193,20 +1193,23 @@ Loads json generated from `config/descriptions.exs`. - Response: ```json -[ - { - "id": 1234, - "data": { - "actor": { - "id": 1, - "nickname": "lain" +{ + "items": [ + { + "id": 1234, + "data": { + "actor": { + "id": 1, + "nickname": "lain" + }, + "action": "relay_follow" }, - "action": "relay_follow" - }, - "time": 1502812026, // timestamp - "message": "[2017-08-15 15:47:06] @nick0 followed relay: https://example.org/relay" // log message - } -] + "time": 1502812026, // timestamp + "message": "[2017-08-15 15:47:06] @nick0 followed relay: https://example.org/relay" // log message + } + ], + "total": 1 +} ``` ## `POST /api/v1/pleroma/admin/reload_emoji` From fc450fdefc2df2bbec20a79fb2c60a95e7f41833 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 15:45:13 -0400 Subject: [PATCH 12/48] ReceiverWorker: cancel job if user fetch is forbidden An instance block with authenticated fetch being required can cause this as we couldn't get the user to find their public key to verify the signature. Commonly observed if someone boosts/Announces a post from an instance that blocked you. --- lib/pleroma/workers/receiver_worker.ex | 5 +- test/pleroma/workers/receiver_worker_test.exs | 48 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index d4db97b639..7dce02a5fc 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -56,17 +56,20 @@ def timeout(%_{args: %{"timeout" => timeout}}), do: timeout def timeout(_job), do: :timer.seconds(5) + defp process_errors({:error, {:error, _} = error}), do: process_errors(error) + defp process_errors(errors) do case errors do {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} {:error, :already_present} -> {:cancel, :already_present} {:error, {:validate_object, _} = reason} -> {:cancel, reason} - {:error, {:error, {:validate, {:error, _changeset} = reason}}} -> {:cancel, reason} + {:error, {:validate, {:error, _changeset} = reason}} -> {:cancel, reason} {:error, {:reject, _} = reason} -> {:cancel, reason} {:signature, false} -> {:cancel, :invalid_signature} {:error, "Object has been deleted"} = reason -> {:cancel, reason} {:error, {:side_effects, {:error, :no_object_actor}} = reason} -> {:cancel, reason} {:error, :not_found} = reason -> {:cancel, reason} + {:error, :forbidden} = reason -> {:cancel, reason} {:error, _} = e -> e e -> {:error, e} end diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 33be910853..640cefb785 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -51,6 +51,54 @@ test "it does not retry duplicates" do }) end + test "it does not retry if a user fetch fails with a 403" do + Tesla.Mock.mock(fn + %{url: "https://simpsons.com/users/bart"} -> + %Tesla.Env{ + status: 403, + body: "" + } + end) + + params = + %{ + "@context" => [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1" + ], + "actor" => "https://simpsons.com/users/bart", + "cc" => [], + "id" => "https://simpsons.com/activity/eat-my-shorts", + "object" => %{}, + "to" => ["https://www.w3.org/ns/activitystreams#Public"], + "type" => "Create" + } + + req_headers = [ + ["accept-encoding", "gzip"], + ["content-length", "31337"], + ["content-type", "application/activity+json"], + ["date", "Wed, 28 Aug 2024 15:36:31 GMT"], + ["digest", "SHA-256=ouge/6HP2/QryG6F3JNtZ6vzs/hSwMk67xdxe87eH7A="], + ["host", "bikeshed.party"], + [ + "signature", + "does not matter as user needs to be fetched first" + ] + ] + + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: req_headers, + request_path: "/inbox", + params: params, + query_string: "" + }) + + assert {:cancel, {:error, :forbidden}} = ReceiverWorker.perform(oban_job) + end + test "it can validate the signature" do Tesla.Mock.mock(fn %{url: "https://mastodon.social/users/bastianallgeier"} -> From 60101e240dea53c3496eda548dbe269fc22b2f72 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 15:54:49 -0400 Subject: [PATCH 13/48] Add test confirming cancellation for activity by a deleted user --- test/pleroma/workers/receiver_worker_test.exs | 88 ++++++++++--------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 640cefb785..2c0da88872 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -51,52 +51,56 @@ test "it does not retry duplicates" do }) end - test "it does not retry if a user fetch fails with a 403" do - Tesla.Mock.mock(fn - %{url: "https://simpsons.com/users/bart"} -> - %Tesla.Env{ - status: 403, - body: "" - } - end) + describe "cancels on a failed user fetch" do + setup do + Tesla.Mock.mock(fn + %{url: "https://springfield.social/users/bart"} -> + %Tesla.Env{ + status: 403, + body: "" + } - params = - %{ - "@context" => [ - "https://www.w3.org/ns/activitystreams", - "https://w3id.org/security/v1" - ], - "actor" => "https://simpsons.com/users/bart", - "cc" => [], - "id" => "https://simpsons.com/activity/eat-my-shorts", - "object" => %{}, - "to" => ["https://www.w3.org/ns/activitystreams#Public"], - "type" => "Create" - } + %{url: "https://springfield.social/users/troymcclure"} -> + %Tesla.Env{ + status: 404, + body: "" + } + end) + end - req_headers = [ - ["accept-encoding", "gzip"], - ["content-length", "31337"], - ["content-type", "application/activity+json"], - ["date", "Wed, 28 Aug 2024 15:36:31 GMT"], - ["digest", "SHA-256=ouge/6HP2/QryG6F3JNtZ6vzs/hSwMk67xdxe87eH7A="], - ["host", "bikeshed.party"], - [ - "signature", - "does not matter as user needs to be fetched first" - ] - ] + test "when request returns a 403" do + params = + insert(:note_activity).data + |> Map.put("actor", "https://springfield.social/users/bart") - {:ok, oban_job} = - Federator.incoming_ap_doc(%{ - method: "POST", - req_headers: req_headers, - request_path: "/inbox", - params: params, - query_string: "" - }) + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: [], + request_path: "/inbox", + params: params, + query_string: "" + }) - assert {:cancel, {:error, :forbidden}} = ReceiverWorker.perform(oban_job) + assert {:cancel, {:error, :forbidden}} = ReceiverWorker.perform(oban_job) + end + + test "when request returns a 404" do + params = + insert(:note_activity).data + |> Map.put("actor", "https://springfield.social/users/troymcclure") + + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: [], + request_path: "/inbox", + params: params, + query_string: "" + }) + + assert {:cancel, {:error, :not_found}} = ReceiverWorker.perform(oban_job) + end end test "it can validate the signature" do From 66e1b4089528dcd5bcdb61343f111cea03f17ab8 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 16:04:12 -0400 Subject: [PATCH 14/48] Cancel if the User fetch resulted in a 410 --- test/pleroma/workers/receiver_worker_test.exs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 2c0da88872..085108e372 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -65,6 +65,12 @@ test "it does not retry duplicates" do status: 404, body: "" } + + %{url: "https://springfield.social/users/hankscorpio"} -> + %Tesla.Env{ + status: 410, + body: "" + } end) end @@ -101,6 +107,23 @@ test "when request returns a 404" do assert {:cancel, {:error, :not_found}} = ReceiverWorker.perform(oban_job) end + + test "when request returns a 410" do + params = + insert(:note_activity).data + |> Map.put("actor", "https://springfield.social/users/hankscorpio") + + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: [], + request_path: "/inbox", + params: params, + query_string: "" + }) + + assert {:cancel, {:error, :not_found}} = ReceiverWorker.perform(oban_job) + end end test "it can validate the signature" do From 48a46618858c9b0dee5ade61c0d9113c521be289 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 16:22:38 -0400 Subject: [PATCH 15/48] Simplify test, move data into a json fixture By removing the inReplyTo, tags, and cc we can simplify the test and it still passes signature validation --- test/fixtures/bastianallgeier.json | 117 -------------- .../receiver_worker_signature_activity.json | 127 ++++++++++----- test/pleroma/workers/receiver_worker_test.exs | 147 +----------------- 3 files changed, 89 insertions(+), 302 deletions(-) delete mode 100644 test/fixtures/bastianallgeier.json diff --git a/test/fixtures/bastianallgeier.json b/test/fixtures/bastianallgeier.json deleted file mode 100644 index 6b47e7db9b..0000000000 --- a/test/fixtures/bastianallgeier.json +++ /dev/null @@ -1,117 +0,0 @@ -{ - "@context": [ - "https://www.w3.org/ns/activitystreams", - "https://w3id.org/security/v1", - { - "Curve25519Key": "toot:Curve25519Key", - "Device": "toot:Device", - "Ed25519Key": "toot:Ed25519Key", - "Ed25519Signature": "toot:Ed25519Signature", - "EncryptedMessage": "toot:EncryptedMessage", - "PropertyValue": "schema:PropertyValue", - "alsoKnownAs": { - "@id": "as:alsoKnownAs", - "@type": "@id" - }, - "cipherText": "toot:cipherText", - "claim": { - "@id": "toot:claim", - "@type": "@id" - }, - "deviceId": "toot:deviceId", - "devices": { - "@id": "toot:devices", - "@type": "@id" - }, - "discoverable": "toot:discoverable", - "featured": { - "@id": "toot:featured", - "@type": "@id" - }, - "featuredTags": { - "@id": "toot:featuredTags", - "@type": "@id" - }, - "fingerprintKey": { - "@id": "toot:fingerprintKey", - "@type": "@id" - }, - "focalPoint": { - "@container": "@list", - "@id": "toot:focalPoint" - }, - "identityKey": { - "@id": "toot:identityKey", - "@type": "@id" - }, - "indexable": "toot:indexable", - "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", - "memorial": "toot:memorial", - "messageFranking": "toot:messageFranking", - "messageType": "toot:messageType", - "movedTo": { - "@id": "as:movedTo", - "@type": "@id" - }, - "publicKeyBase64": "toot:publicKeyBase64", - "schema": "http://schema.org#", - "suspended": "toot:suspended", - "toot": "http://joinmastodon.org/ns#", - "value": "schema:value" - } - ], - "attachment": [ - { - "name": "Website", - "type": "PropertyValue", - "value": "https://bastianallgeier.com" - }, - { - "name": "Project", - "type": "PropertyValue", - "value": "https://getkirby.com" - }, - { - "name": "Github", - "type": "PropertyValue", - "value": "https://github.com/bastianallgeier" - } - ], - "devices": "https://mastodon.social/users/bastianallgeier/collections/devices", - "discoverable": true, - "endpoints": { - "sharedInbox": "https://mastodon.social/inbox" - }, - "featured": "https://mastodon.social/users/bastianallgeier/collections/featured", - "featuredTags": "https://mastodon.social/users/bastianallgeier/collections/tags", - "followers": "https://mastodon.social/users/bastianallgeier/followers", - "following": "https://mastodon.social/users/bastianallgeier/following", - "icon": { - "mediaType": "image/jpeg", - "type": "Image", - "url": "https://files.mastodon.social/accounts/avatars/000/007/393/original/0180a20079617c71.jpg" - }, - "id": "https://mastodon.social/users/bastianallgeier", - "image": { - "mediaType": "image/jpeg", - "type": "Image", - "url": "https://files.mastodon.social/accounts/headers/000/007/393/original/13d644ab46d50478.jpeg" - }, - "inbox": "https://mastodon.social/users/bastianallgeier/inbox", - "indexable": false, - "manuallyApprovesFollowers": false, - "memorial": false, - "name": "Bastian Allgeier", - "outbox": "https://mastodon.social/users/bastianallgeier/outbox", - "preferredUsername": "bastianallgeier", - "publicKey": { - "id": "https://mastodon.social/users/bastianallgeier#main-key", - "owner": "https://mastodon.social/users/bastianallgeier", - "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA3fz+hpgVztO9z6HUhyzv\nwP++ERBBoIwSLKf1TyIM8bvzGFm2YXaO5uxu1HvumYFTYc3ACr3q4j8VUb7NMxkQ\nlzu4QwPjOFJ43O+fY+HSPORXEDW5fXDGC5DGpox4+i08LxRmx7L6YPRUSUuPN8nI\nWyq1Qsq1zOQrNY/rohMXkBdSXxqC3yIRqvtLt4otCgay/5tMogJWkkS6ZKyFhb9z\nwVVy1fsbV10c9C+SHy4NH26CKaTtpTYLRBMjhTCS8bX8iDSjGIf2aZgYs1ir7gEz\n9wf5CvLiENmVWGwm64t6KSEAkA4NJ1hzgHUZPCjPHZE2SmhO/oHaxokTzqtbbENJ\n1QIDAQAB\n-----END PUBLIC KEY-----\n" - }, - "published": "2016-11-01T00:00:00Z", - "summary": "

Designer & developer. Creator of Kirby CMS

", - "tag": [], - "type": "Person", - "url": "https://mastodon.social/@bastianallgeier" -} diff --git a/test/fixtures/receiver_worker_signature_activity.json b/test/fixtures/receiver_worker_signature_activity.json index 3c3fb3fd2f..19dc0087f3 100644 --- a/test/fixtures/receiver_worker_signature_activity.json +++ b/test/fixtures/receiver_worker_signature_activity.json @@ -1,62 +1,109 @@ { "@context": [ "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", { + "claim": { + "@id": "toot:claim", + "@type": "@id" + }, + "memorial": "toot:memorial", "atomUri": "ostatus:atomUri", + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", "blurhash": "toot:blurhash", - "conversation": "ostatus:conversation", + "ostatus": "http://ostatus.org#", + "discoverable": "toot:discoverable", "focalPoint": { "@container": "@list", "@id": "toot:focalPoint" }, - "inReplyToAtomUri": "ostatus:inReplyToAtomUri", - "ostatus": "http://ostatus.org#", + "votersCount": "toot:votersCount", + "Hashtag": "as:Hashtag", + "Emoji": "toot:Emoji", + "alsoKnownAs": { + "@id": "as:alsoKnownAs", + "@type": "@id" + }, "sensitive": "as:sensitive", + "movedTo": { + "@id": "as:movedTo", + "@type": "@id" + }, + "inReplyToAtomUri": "ostatus:inReplyToAtomUri", + "conversation": "ostatus:conversation", + "Device": "toot:Device", + "schema": "http://schema.org#", "toot": "http://joinmastodon.org/ns#", - "votersCount": "toot:votersCount" + "cipherText": "toot:cipherText", + "suspended": "toot:suspended", + "messageType": "toot:messageType", + "featuredTags": { + "@id": "toot:featuredTags", + "@type": "@id" + }, + "Curve25519Key": "toot:Curve25519Key", + "deviceId": "toot:deviceId", + "Ed25519Signature": "toot:Ed25519Signature", + "featured": { + "@id": "toot:featured", + "@type": "@id" + }, + "devices": { + "@id": "toot:devices", + "@type": "@id" + }, + "value": "schema:value", + "PropertyValue": "schema:PropertyValue", + "messageFranking": "toot:messageFranking", + "publicKeyBase64": "toot:publicKeyBase64", + "identityKey": { + "@id": "toot:identityKey", + "@type": "@id" + }, + "Ed25519Key": "toot:Ed25519Key", + "indexable": "toot:indexable", + "EncryptedMessage": "toot:EncryptedMessage", + "fingerprintKey": { + "@id": "toot:fingerprintKey", + "@type": "@id" + } } ], - "atomUri": "https://chaos.social/users/distantnative/statuses/109336635639931467", - "attachment": [ - { - "blurhash": "UAK1zS00OXIUxuMxIUM{?b-:-;W:Di?b%2M{", - "height": 960, - "mediaType": "image/jpeg", - "name": null, - "type": "Document", - "url": "https://assets.chaos.social/media_attachments/files/109/336/634/286/114/657/original/2e6122063d8bfb26.jpeg", - "width": 346 - } - ], - "attributedTo": "https://chaos.social/users/distantnative", - "cc": [ - "https://chaos.social/users/distantnative/followers" - ], - "content": "

Favorite piece of anthropology meta discourse.

", - "contentMap": { - "en": "

Favorite piece of anthropology meta discourse.

" - }, - "conversation": "tag:chaos.social,2022-11-13:objectId=71843781:objectType=Conversation", - "id": "https://chaos.social/users/distantnative/statuses/109336635639931467", + "actor": "https://phpc.social/users/denniskoch", + "cc": [], + "id": "https://phpc.social/users/denniskoch/statuses/112847382711461301/activity", "inReplyTo": null, "inReplyToAtomUri": null, - "published": "2022-11-13T13:04:20Z", - "replies": { - "first": { - "items": [], - "next": "https://chaos.social/users/distantnative/statuses/109336635639931467/replies?only_other_accounts=true&page=true", - "partOf": "https://chaos.social/users/distantnative/statuses/109336635639931467/replies", - "type": "CollectionPage" + "object": { + "atomUri": "https://phpc.social/users/denniskoch/statuses/112847382711461301", + "attachment": [], + "attributedTo": "https://phpc.social/users/denniskoch", + "cc": [], + "content": "

@bastianallgeier @distantnative @kev Another main argument: Discord is popular. Many people have an account, so you can just join an server quickly. Also you know the app and how to get around.

", + "contentMap": { + "en": "

@bastianallgeier @distantnative @kev Another main argument: Discord is popular. Many people have an account, so you can just join an server quickly. Also you know the app and how to get around.

" }, - "id": "https://chaos.social/users/distantnative/statuses/109336635639931467/replies", - "type": "Collection" + "conversation": "tag:mastodon.social,2024-07-25:objectId=760068442:objectType=Conversation", + "id": "https://phpc.social/users/denniskoch/statuses/112847382711461301", + "published": "2024-07-25T13:33:29Z", + "replies": null, + "sensitive": false, + "tag": [], + "to": [ + "https://www.w3.org/ns/activitystreams#Public" + ], + "type": "Note", + "url": "https://phpc.social/@denniskoch/112847382711461301" + }, + "published": "2024-07-25T13:33:29Z", + "signature": { + "created": "2024-07-25T13:33:29Z", + "creator": "https://phpc.social/users/denniskoch#main-key", + "signatureValue": "slz9BKJzd2n1S44wdXGOU+bV/wsskdgAaUpwxj8R16mYOL8+DTpE6VnfSKoZGsBBJT8uG5gnVfVEz1YsTUYtymeUgLMh7cvd8VnJnZPS+oixbmBRVky/Myf91TEgQQE7G4vDmTdB4ii54hZrHcOOYYf5FKPNRSkMXboKA6LMqNtekhbI+JTUJYIB02WBBK6PUyo15f6B1RJ6HGWVgud9NE0y1EZXfrkqUt682p8/9D49ORf7AwjXUJibKic2RbPvhEBj70qUGfBm4vvgdWhSUn1IG46xh+U0+NrTSUED82j1ZVOeua/2k/igkGs8cSBkY35quXTkPz6gbqCCH66CuA==", + "type": "RsaSignature2017" }, - "sensitive": false, - "summary": null, - "tag": [], "to": [ "https://www.w3.org/ns/activitystreams#Public" ], - "type": "Note", - "url": "https://chaos.social/@distantnative/109336635639931467" + "type": "Create" } diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 085108e372..cb434f52ec 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -128,23 +128,6 @@ test "when request returns a 410" do test "it can validate the signature" do Tesla.Mock.mock(fn - %{url: "https://mastodon.social/users/bastianallgeier"} -> - %Tesla.Env{ - status: 200, - body: File.read!("test/fixtures/bastianallgeier.json"), - headers: [{"content-type", "application/activity+json"}] - } - - %{url: "https://mastodon.social/users/bastianallgeier/collections/featured"} -> - %Tesla.Env{ - status: 200, - headers: [{"content-type", "application/activity+json"}], - body: - File.read!("test/fixtures/users_mock/masto_featured.json") - |> String.replace("{{domain}}", "mastodon.social") - |> String.replace("{{nickname}}", "bastianallgeier") - } - %{url: "https://phpc.social/users/denniskoch"} -> %Tesla.Env{ status: 200, @@ -161,136 +144,10 @@ test "it can validate the signature" do |> String.replace("{{domain}}", "phpc.social") |> String.replace("{{nickname}}", "denniskoch") } - - %{url: "https://mastodon.social/users/bastianallgeier/statuses/112846516276907281"} -> - %Tesla.Env{ - status: 200, - headers: [{"content-type", "application/activity+json"}], - body: File.read!("test/fixtures/receiver_worker_signature_activity.json") - } end) - params = %{ - "@context" => [ - "https://www.w3.org/ns/activitystreams", - "https://w3id.org/security/v1", - %{ - "claim" => %{"@id" => "toot:claim", "@type" => "@id"}, - "memorial" => "toot:memorial", - "atomUri" => "ostatus:atomUri", - "manuallyApprovesFollowers" => "as:manuallyApprovesFollowers", - "blurhash" => "toot:blurhash", - "ostatus" => "http://ostatus.org#", - "discoverable" => "toot:discoverable", - "focalPoint" => %{"@container" => "@list", "@id" => "toot:focalPoint"}, - "votersCount" => "toot:votersCount", - "Hashtag" => "as:Hashtag", - "Emoji" => "toot:Emoji", - "alsoKnownAs" => %{"@id" => "as:alsoKnownAs", "@type" => "@id"}, - "sensitive" => "as:sensitive", - "movedTo" => %{"@id" => "as:movedTo", "@type" => "@id"}, - "inReplyToAtomUri" => "ostatus:inReplyToAtomUri", - "conversation" => "ostatus:conversation", - "Device" => "toot:Device", - "schema" => "http://schema.org#", - "toot" => "http://joinmastodon.org/ns#", - "cipherText" => "toot:cipherText", - "suspended" => "toot:suspended", - "messageType" => "toot:messageType", - "featuredTags" => %{"@id" => "toot:featuredTags", "@type" => "@id"}, - "Curve25519Key" => "toot:Curve25519Key", - "deviceId" => "toot:deviceId", - "Ed25519Signature" => "toot:Ed25519Signature", - "featured" => %{"@id" => "toot:featured", "@type" => "@id"}, - "devices" => %{"@id" => "toot:devices", "@type" => "@id"}, - "value" => "schema:value", - "PropertyValue" => "schema:PropertyValue", - "messageFranking" => "toot:messageFranking", - "publicKeyBase64" => "toot:publicKeyBase64", - "identityKey" => %{"@id" => "toot:identityKey", "@type" => "@id"}, - "Ed25519Key" => "toot:Ed25519Key", - "indexable" => "toot:indexable", - "EncryptedMessage" => "toot:EncryptedMessage", - "fingerprintKey" => %{"@id" => "toot:fingerprintKey", "@type" => "@id"} - } - ], - "actor" => "https://phpc.social/users/denniskoch", - "cc" => [ - "https://phpc.social/users/denniskoch/followers", - "https://mastodon.social/users/bastianallgeier", - "https://chaos.social/users/distantnative", - "https://fosstodon.org/users/kev" - ], - "id" => "https://phpc.social/users/denniskoch/statuses/112847382711461301/activity", - "object" => %{ - "atomUri" => "https://phpc.social/users/denniskoch/statuses/112847382711461301", - "attachment" => [], - "attributedTo" => "https://phpc.social/users/denniskoch", - "cc" => [ - "https://phpc.social/users/denniskoch/followers", - "https://mastodon.social/users/bastianallgeier", - "https://chaos.social/users/distantnative", - "https://fosstodon.org/users/kev" - ], - "content" => - "

@bastianallgeier @distantnative @kev Another main argument: Discord is popular. Many people have an account, so you can just join an server quickly. Also you know the app and how to get around.

", - "contentMap" => %{ - "en" => - "

@bastianallgeier @distantnative @kev Another main argument: Discord is popular. Many people have an account, so you can just join an server quickly. Also you know the app and how to get around.

" - }, - "conversation" => - "tag:mastodon.social,2024-07-25:objectId=760068442:objectType=Conversation", - "id" => "https://phpc.social/users/denniskoch/statuses/112847382711461301", - "inReplyTo" => - "https://mastodon.social/users/bastianallgeier/statuses/112846516276907281", - "inReplyToAtomUri" => - "https://mastodon.social/users/bastianallgeier/statuses/112846516276907281", - "published" => "2024-07-25T13:33:29Z", - "replies" => %{ - "first" => %{ - "items" => [], - "next" => - "https://phpc.social/users/denniskoch/statuses/112847382711461301/replies?only_other_accounts=true&page=true", - "partOf" => - "https://phpc.social/users/denniskoch/statuses/112847382711461301/replies", - "type" => "CollectionPage" - }, - "id" => "https://phpc.social/users/denniskoch/statuses/112847382711461301/replies", - "type" => "Collection" - }, - "sensitive" => false, - "tag" => [ - %{ - "href" => "https://mastodon.social/users/bastianallgeier", - "name" => "@bastianallgeier@mastodon.social", - "type" => "Mention" - }, - %{ - "href" => "https://chaos.social/users/distantnative", - "name" => "@distantnative@chaos.social", - "type" => "Mention" - }, - %{ - "href" => "https://fosstodon.org/users/kev", - "name" => "@kev@fosstodon.org", - "type" => "Mention" - } - ], - "to" => ["https://www.w3.org/ns/activitystreams#Public"], - "type" => "Note", - "url" => "https://phpc.social/@denniskoch/112847382711461301" - }, - "published" => "2024-07-25T13:33:29Z", - "signature" => %{ - "created" => "2024-07-25T13:33:29Z", - "creator" => "https://phpc.social/users/denniskoch#main-key", - "signatureValue" => - "slz9BKJzd2n1S44wdXGOU+bV/wsskdgAaUpwxj8R16mYOL8+DTpE6VnfSKoZGsBBJT8uG5gnVfVEz1YsTUYtymeUgLMh7cvd8VnJnZPS+oixbmBRVky/Myf91TEgQQE7G4vDmTdB4ii54hZrHcOOYYf5FKPNRSkMXboKA6LMqNtekhbI+JTUJYIB02WBBK6PUyo15f6B1RJ6HGWVgud9NE0y1EZXfrkqUt682p8/9D49ORf7AwjXUJibKic2RbPvhEBj70qUGfBm4vvgdWhSUn1IG46xh+U0+NrTSUED82j1ZVOeua/2k/igkGs8cSBkY35quXTkPz6gbqCCH66CuA==", - "type" => "RsaSignature2017" - }, - "to" => ["https://www.w3.org/ns/activitystreams#Public"], - "type" => "Create" - } + params = + File.read!("test/fixtures/receiver_worker_signature_activity.json") |> Jason.decode!() req_headers = [ ["accept-encoding", "gzip"], From 3dadb9ed086fb63a3e664a43be3bf30f9ffbfb2d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 16:37:46 -0400 Subject: [PATCH 16/48] Changelog --- changelog.d/oban-recevier-user-error.fix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/oban-recevier-user-error.fix diff --git a/changelog.d/oban-recevier-user-error.fix b/changelog.d/oban-recevier-user-error.fix new file mode 100644 index 0000000000..1ed0c5bb15 --- /dev/null +++ b/changelog.d/oban-recevier-user-error.fix @@ -0,0 +1 @@ +ReceiverWorker will cancel processing jobs instead of retrying if the user cannot be fetched due to 403, 404, or 410 errors. From bb2f4a76b3af4ad5f0e2950ef8dc2567c6ad69ff Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 17:01:30 -0400 Subject: [PATCH 17/48] Add test for origin containment failures --- test/pleroma/workers/receiver_worker_test.exs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index cb434f52ec..995f765a13 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -177,4 +177,21 @@ test "it can validate the signature" do assert {:ok, %Pleroma.Activity{}} = ReceiverWorker.perform(oban_job) end + + test "cancels due to origin containment" do + params = + insert(:note_activity).data + |> Map.put("id", "https://notorigindomain.com/activity") + + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: [], + request_path: "/inbox", + params: params, + query_string: "" + }) + + assert {:cancel, :origin_containment_failed} = ReceiverWorker.perform(oban_job) + end end From 6ae629cfe072d236453d256017618fe9a8c44755 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 17:24:59 -0400 Subject: [PATCH 18/48] Cancel ReceiverWorker jobs if the user account has been disabled / deactivated --- lib/pleroma/workers/receiver_worker.ex | 4 ++- test/pleroma/workers/receiver_worker_test.exs | 26 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 7dce02a5fc..80518f6fd9 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,7 +33,8 @@ def perform(%Job{ query_string: query_string } - with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + with {:ok, %User{} = actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + {:user_active, true} <- {:user_active, match?(true, actor.is_active)}, {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do @@ -70,6 +71,7 @@ defp process_errors(errors) do {:error, {:side_effects, {:error, :no_object_actor}} = reason} -> {:cancel, reason} {:error, :not_found} = reason -> {:cancel, reason} {:error, :forbidden} = reason -> {:cancel, reason} + {:user_active, false} = reason -> {:cancel, reason} {:error, _} = e -> e e -> {:error, e} end diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 995f765a13..adf90ec862 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -9,6 +9,7 @@ defmodule Pleroma.Workers.ReceiverWorkerTest do import Mock import Pleroma.Factory + alias Pleroma.User alias Pleroma.Web.Federator alias Pleroma.Workers.ReceiverWorker @@ -124,6 +125,31 @@ test "when request returns a 410" do assert {:cancel, {:error, :not_found}} = ReceiverWorker.perform(oban_job) end + + test "when user account is disabled" do + user = insert(:user) + + fake_activity = URI.parse(user.ap_id) |> Map.put(:path, "/fake-activity") |> to_string + + params = + insert(:note_activity, user: user).data + |> Map.put("id", fake_activity) + + {:ok, %User{}} = User.set_activation(user, false) + + {:ok, oban_job} = + ReceiverWorker.new(%{ + "op" => "incoming_ap_doc", + "method" => "POST", + "req_headers" => [], + "request_path" => "/inbox", + "params" => params, + "query_string" => "" + }) + |> Oban.insert() + + assert {:cancel, {:user_active, false}} = ReceiverWorker.perform(oban_job) + end end test "it can validate the signature" do From 2e9515578a689428027ca7084d5c9b0d0b4a60ba Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 17:38:13 -0400 Subject: [PATCH 19/48] ReceiverWorker job canceled due to deleted object --- test/pleroma/workers/receiver_worker_test.exs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index adf90ec862..779e83eaa4 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -220,4 +220,29 @@ test "cancels due to origin containment" do assert {:cancel, :origin_containment_failed} = ReceiverWorker.perform(oban_job) end + + test "canceled due to deleted object" do + params = + insert(:announce_activity).data + |> Map.put("object", "http://localhost:4001/deleted") + + Tesla.Mock.mock(fn + %{url: "http://localhost:4001/deleted"} -> + %Tesla.Env{ + status: 404, + body: "" + } + end) + + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: [], + request_path: "/inbox", + params: params, + query_string: "" + }) + + assert {:cancel, _} = ReceiverWorker.perform(oban_job) + end end From 2346807ac93d5acb9901823cceaffe5c305c1e20 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 17:44:33 -0400 Subject: [PATCH 20/48] Annotate error cases --- lib/pleroma/workers/receiver_worker.ex | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 80518f6fd9..4b1f74a275 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -61,17 +61,21 @@ defp process_errors({:error, {:error, _} = error}), do: process_errors(error) defp process_errors(errors) do case errors do - {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} - {:error, :already_present} -> {:cancel, :already_present} - {:error, {:validate_object, _} = reason} -> {:cancel, reason} - {:error, {:validate, {:error, _changeset} = reason}} -> {:cancel, reason} - {:error, {:reject, _} = reason} -> {:cancel, reason} - {:signature, false} -> {:cancel, :invalid_signature} - {:error, "Object has been deleted"} = reason -> {:cancel, reason} - {:error, {:side_effects, {:error, :no_object_actor}} = reason} -> {:cancel, reason} + # User fetch failures {:error, :not_found} = reason -> {:cancel, reason} {:error, :forbidden} = reason -> {:cancel, reason} + # Inactive user {:user_active, false} = reason -> {:cancel, reason} + # Validator will error and return a changeset error + # e.g., duplicate activities or if the object was deleted + {:error, {:validate, {:error, _changeset} = reason}} -> {:cancel, reason} + # MRFs will return a reject + {:error, {:reject, _} = reason} -> {:cancel, reason} + # HTTP Sigs + {:signature, false} -> {:cancel, :invalid_signature} + {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} + {:error, {:validate_object, _} = reason} -> {:cancel, reason} + {:error, {:side_effects, {:error, :no_object_actor}} = reason} -> {:cancel, reason} {:error, _} = e -> e e -> {:error, e} end From 380a6a6df31a16a89f5c5cc497ddc1360cea3854 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 17:45:31 -0400 Subject: [PATCH 21/48] :validate_object is not a real error returned from anywhere --- lib/pleroma/web/federator.ex | 5 ----- lib/pleroma/workers/receiver_worker.ex | 1 - 2 files changed, 6 deletions(-) diff --git a/lib/pleroma/web/federator.ex b/lib/pleroma/web/federator.ex index 2df7165566..e812b1a466 100644 --- a/lib/pleroma/web/federator.ex +++ b/lib/pleroma/web/federator.ex @@ -121,11 +121,6 @@ def perform(:incoming_ap_doc, params) do Logger.debug("Unhandled actor #{actor}, #{inspect(e)}") {:error, e} - {:error, {:validate_object, _}} = e -> - Logger.error("Incoming AP doc validation error: #{inspect(e)}") - Logger.debug(Jason.encode!(params, pretty: true)) - e - e -> # Just drop those for now Logger.debug(fn -> "Unhandled activity\n" <> Jason.encode!(params, pretty: true) end) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 4b1f74a275..c7e6bc5eab 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -74,7 +74,6 @@ defp process_errors(errors) do # HTTP Sigs {:signature, false} -> {:cancel, :invalid_signature} {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} - {:error, {:validate_object, _} = reason} -> {:cancel, reason} {:error, {:side_effects, {:error, :no_object_actor}} = reason} -> {:cancel, reason} {:error, _} = e -> e e -> {:error, e} From c5ca806aa0023e25755947a3bf0d54242e45f65a Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 17:57:34 -0400 Subject: [PATCH 22/48] Add back one of the duplicate checks to fix a test, document where it comes from --- lib/pleroma/workers/receiver_worker.ex | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index c7e6bc5eab..810fda67cd 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -69,6 +69,8 @@ defp process_errors(errors) do # Validator will error and return a changeset error # e.g., duplicate activities or if the object was deleted {:error, {:validate, {:error, _changeset} = reason}} -> {:cancel, reason} + # Duplicate detection during Normalization + {:error, :already_present} -> {:cancel, :already_present} # MRFs will return a reject {:error, {:reject, _} = reason} -> {:cancel, reason} # HTTP Sigs From 8a3efa7152488460934c1fadc8ab86efd7d47c04 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 18:02:35 -0400 Subject: [PATCH 23/48] More error annotations --- lib/pleroma/workers/receiver_worker.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 810fda67cd..6787a59ef2 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -75,8 +75,11 @@ defp process_errors(errors) do {:error, {:reject, _} = reason} -> {:cancel, reason} # HTTP Sigs {:signature, false} -> {:cancel, :invalid_signature} + # Origin / URL validation failed somewhere possibly due to spoofing {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} + # Unclear if this can be reached {:error, {:side_effects, {:error, :no_object_actor}} = reason} -> {:cancel, reason} + # Catchall {:error, _} = e -> e e -> {:error, e} end From e498d252e44ddc1a85288b80dc65beefcd60edf2 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 18:03:33 -0400 Subject: [PATCH 24/48] Changelog update --- changelog.d/oban-recevier-improvements.fix | 1 + changelog.d/oban-recevier-user-error.fix | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 changelog.d/oban-recevier-improvements.fix delete mode 100644 changelog.d/oban-recevier-user-error.fix diff --git a/changelog.d/oban-recevier-improvements.fix b/changelog.d/oban-recevier-improvements.fix new file mode 100644 index 0000000000..f91502ed25 --- /dev/null +++ b/changelog.d/oban-recevier-improvements.fix @@ -0,0 +1 @@ +ReceiverWorker will cancel processing jobs instead of retrying if the user cannot be fetched due to 403, 404, or 410 errors or if the account is disabled locally. diff --git a/changelog.d/oban-recevier-user-error.fix b/changelog.d/oban-recevier-user-error.fix deleted file mode 100644 index 1ed0c5bb15..0000000000 --- a/changelog.d/oban-recevier-user-error.fix +++ /dev/null @@ -1 +0,0 @@ -ReceiverWorker will cancel processing jobs instead of retrying if the user cannot be fetched due to 403, 404, or 410 errors. From 1821ef4f157980bdf64f7540ee5aa8e26fa3102e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 18:35:01 -0400 Subject: [PATCH 25/48] Move user active check into Federator.perform/1 --- lib/pleroma/web/federator.ex | 3 ++- lib/pleroma/workers/receiver_worker.ex | 5 ++--- test/pleroma/workers/receiver_worker_test.exs | 14 ++++++-------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/pleroma/web/federator.ex b/lib/pleroma/web/federator.ex index e812b1a466..58260afa88 100644 --- a/lib/pleroma/web/federator.ex +++ b/lib/pleroma/web/federator.ex @@ -102,7 +102,8 @@ def perform(:incoming_ap_doc, params) do # NOTE: we use the actor ID to do the containment, this is fine because an # actor shouldn't be acting on objects outside their own AP server. - with {_, {:ok, _user}} <- {:actor, User.get_or_fetch_by_ap_id(actor)}, + with {_, {:ok, user}} <- {:actor, User.get_or_fetch_by_ap_id(actor)}, + {:user_active, true} <- {:user_active, match?(true, user.is_active)}, nil <- Activity.normalize(params["id"]), {_, :ok} <- {:correct_origin?, Containment.contain_origin_from_id(actor, params)}, diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 6787a59ef2..0373ec15f8 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,8 +33,7 @@ def perform(%Job{ query_string: query_string } - with {:ok, %User{} = actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), - {:user_active, true} <- {:user_active, match?(true, actor.is_active)}, + with {:ok, %User{}} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do @@ -65,7 +64,7 @@ defp process_errors(errors) do {:error, :not_found} = reason -> {:cancel, reason} {:error, :forbidden} = reason -> {:cancel, reason} # Inactive user - {:user_active, false} = reason -> {:cancel, reason} + {:error, {:user_active, false} = reason} -> {:cancel, reason} # Validator will error and return a changeset error # e.g., duplicate activities or if the object was deleted {:error, {:validate, {:error, _changeset} = reason}} -> {:cancel, reason} diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 779e83eaa4..4d53c44ed8 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -138,15 +138,13 @@ test "when user account is disabled" do {:ok, %User{}} = User.set_activation(user, false) {:ok, oban_job} = - ReceiverWorker.new(%{ - "op" => "incoming_ap_doc", - "method" => "POST", - "req_headers" => [], - "request_path" => "/inbox", - "params" => params, - "query_string" => "" + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: [], + request_path: "/inbox", + params: params, + query_string: "" }) - |> Oban.insert() assert {:cancel, {:user_active, false}} = ReceiverWorker.perform(oban_job) end From 0bf82a1745a38a3752f5b7df645a7d266b8fd9c8 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 19:50:51 -0400 Subject: [PATCH 26/48] Add an AdapterHelper for Finch so we can support streaming request bodies --- lib/pleroma/http/adapter_helper.ex | 2 ++ lib/pleroma/http/adapter_helper/finch.ex | 33 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 lib/pleroma/http/adapter_helper/finch.ex diff --git a/lib/pleroma/http/adapter_helper.ex b/lib/pleroma/http/adapter_helper.ex index 4dbcccdcc1..be00ba78a6 100644 --- a/lib/pleroma/http/adapter_helper.ex +++ b/lib/pleroma/http/adapter_helper.ex @@ -52,6 +52,7 @@ defp adapter_helper do case adapter() do Tesla.Adapter.Gun -> AdapterHelper.Gun Tesla.Adapter.Hackney -> AdapterHelper.Hackney + {Tesla.Adapter.Finch, _} -> AdapterHelper.Finch _ -> AdapterHelper.Default end end @@ -124,6 +125,7 @@ def format_host(host) do def can_stream? do case Application.get_env(:tesla, :adapter) do Tesla.Adapter.Gun -> true + {Tesla.Adapter.Finch, _} -> true _ -> false end end diff --git a/lib/pleroma/http/adapter_helper/finch.ex b/lib/pleroma/http/adapter_helper/finch.ex new file mode 100644 index 0000000000..10a988901e --- /dev/null +++ b/lib/pleroma/http/adapter_helper/finch.ex @@ -0,0 +1,33 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2022 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.HTTP.AdapterHelper.Finch do + @behaviour Pleroma.HTTP.AdapterHelper + + alias Pleroma.Config + alias Pleroma.HTTP.AdapterHelper + + @spec options(keyword(), URI.t()) :: keyword() + def options(incoming_opts \\ [], %URI{} = _uri) do + proxy = + [:http, :proxy_url] + |> Config.get() + |> AdapterHelper.format_proxy() + + config_opts = Config.get([:http, :adapter], []) + + config_opts + |> Keyword.merge(incoming_opts) + |> AdapterHelper.maybe_add_proxy(proxy) + |> maybe_stream() + end + + # Tesla Finch adapter uses response: :stream + defp maybe_stream(opts) do + case Keyword.pop(opts, :stream, nil) do + {true, opts} -> Keyword.put(opts, :response, :stream) + {_, opts} -> opts + end + end +end From 8ab4dd20dfdd0cc92c18ade7d84bfb5364785a15 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 19:52:29 -0400 Subject: [PATCH 27/48] Update comments, remove solved TODO --- lib/pleroma/http/adapter_helper.ex | 1 - lib/pleroma/http/adapter_helper/finch.ex | 2 +- lib/pleroma/http/adapter_helper/gun.ex | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/http/adapter_helper.ex b/lib/pleroma/http/adapter_helper.ex index be00ba78a6..32c1080f7e 100644 --- a/lib/pleroma/http/adapter_helper.ex +++ b/lib/pleroma/http/adapter_helper.ex @@ -120,7 +120,6 @@ def format_host(host) do end end - # TODO add Finch support once we have an AdapterHelper for it @spec can_stream? :: bool() def can_stream? do case Application.get_env(:tesla, :adapter) do diff --git a/lib/pleroma/http/adapter_helper/finch.ex b/lib/pleroma/http/adapter_helper/finch.ex index 10a988901e..181caed7e1 100644 --- a/lib/pleroma/http/adapter_helper/finch.ex +++ b/lib/pleroma/http/adapter_helper/finch.ex @@ -23,7 +23,7 @@ def options(incoming_opts \\ [], %URI{} = _uri) do |> maybe_stream() end - # Tesla Finch adapter uses response: :stream + # Finch uses [response: :stream] defp maybe_stream(opts) do case Keyword.pop(opts, :stream, nil) do {true, opts} -> Keyword.put(opts, :response, :stream) diff --git a/lib/pleroma/http/adapter_helper/gun.ex b/lib/pleroma/http/adapter_helper/gun.ex index f9a8180f25..30ba26765b 100644 --- a/lib/pleroma/http/adapter_helper/gun.ex +++ b/lib/pleroma/http/adapter_helper/gun.ex @@ -48,7 +48,7 @@ defp put_timeout(opts) do Keyword.put(opts, :timeout, recv_timeout) end - # Tesla Gun adapter uses body_as: :stream + # Gun uses [body_as: :stream] defp maybe_stream(opts) do case Keyword.pop(opts, :stream, nil) do {true, opts} -> Keyword.put(opts, :body_as, :stream) From d01569822e0dc45349c321ad306f6e19b4e967af Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 19:56:09 -0400 Subject: [PATCH 28/48] Changelog --- changelog.d/rich-media-no-heads.change | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/rich-media-no-heads.change diff --git a/changelog.d/rich-media-no-heads.change b/changelog.d/rich-media-no-heads.change new file mode 100644 index 0000000000..0bab323aa5 --- /dev/null +++ b/changelog.d/rich-media-no-heads.change @@ -0,0 +1 @@ +Rich Media preview fetching will skip making an HTTP HEAD request to check a URL for allowed content type and length if the Tesla adapter is Gun or Finch From c17a78c55a6b288c271923f730dc69aaf27e6556 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 09:37:10 -0400 Subject: [PATCH 29/48] Rich Media: add stream byte counting as an extra protection against malicious URLs --- lib/pleroma/web/rich_media/helpers.ex | 34 +++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index a242ca640a..d4be979578 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -23,7 +23,7 @@ defp stream(url) do {:get, Pleroma.HTTP.get(url, req_headers(), http_options())}, {_, :ok} <- {:content_type, check_content_type(headers)}, {_, :ok} <- {:content_length, check_content_length(headers)}, - body <- Enum.into(stream_body, <<>>) do + {:read_stream, {:ok, body}} <- {:read_stream, read_stream(stream_body)} do {:ok, body} end end @@ -52,8 +52,12 @@ defp handle_result(result, url) do Logger.debug("Rich media error for #{url}: content-type is #{type}") {:error, :content_type} - {:content_length, {_, length}} -> - Logger.debug("Rich media error for #{url}: content-length is #{length}") + {:content_length, :error} -> + Logger.debug("Rich media error for #{url}: content-length exceeded") + {:error, :body_too_large} + + {:read_stream, :error} -> + Logger.debug("Rich media error for #{url}: content-length exceeded") {:error, :body_too_large} {:get, _} -> @@ -82,7 +86,7 @@ defp check_content_length(headers) do {_, maybe_content_length} -> case Integer.parse(maybe_content_length) do {content_length, ""} when content_length <= max_body -> :ok - {_, ""} -> {:error, maybe_content_length} + {_, ""} -> :error _ -> :ok end @@ -91,6 +95,28 @@ defp check_content_length(headers) do end end + defp read_stream(stream) do + max_body = Keyword.get(http_options(), :max_body) + + try do + result = + Stream.transform(stream, 0, fn chunk, total_bytes -> + new_total = total_bytes + byte_size(chunk) + + if new_total > max_body do + raise("Exceeds max body limit of #{max_body}") + else + {[chunk], new_total} + end + end) + |> Enum.into(<<>>) + + {:ok, result} + rescue + _ -> :error + end + end + defp http_options do [ pool: :rich_media, From ceffb8a8918b83d482e9c1da64fec22b428a61f3 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 23 Aug 2024 13:52:19 -0400 Subject: [PATCH 30/48] Drop incoming Delete activities from unknown actors --- changelog.d/drop-unknown-deletes.change | 1 + lib/pleroma/workers/receiver_worker.ex | 16 +++++++++++++- test/pleroma/workers/receiver_worker_test.exs | 22 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 changelog.d/drop-unknown-deletes.change diff --git a/changelog.d/drop-unknown-deletes.change b/changelog.d/drop-unknown-deletes.change new file mode 100644 index 0000000000..0be7f54502 --- /dev/null +++ b/changelog.d/drop-unknown-deletes.change @@ -0,0 +1 @@ +Drop incoming Delete activities from unknown actors diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index d4db97b639..ea86a3a12e 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,7 +33,8 @@ def perform(%Job{ query_string: query_string } - with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + with {_, false} <- {:unknown_delete, unknown_delete?(params)}, + User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do @@ -58,6 +59,7 @@ def timeout(_job), do: :timer.seconds(5) defp process_errors(errors) do case errors do + {:unknown_delete, true} -> {:cancel, "Delete from unknown actor"} {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} {:error, :already_present} -> {:cancel, :already_present} {:error, {:validate_object, _} = reason} -> {:cancel, reason} @@ -71,4 +73,16 @@ defp process_errors(errors) do e -> {:error, e} end end + + defp unknown_delete?(%{ + "type" => "Delete", + "actor" => actor + }) do + case User.get_cached_by_ap_id(actor) do + %User{} -> false + _ -> true + end + end + + defp unknown_delete?(_), do: false end diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 33be910853..91fbb1fe88 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -245,4 +245,26 @@ test "it can validate the signature" do assert {:ok, %Pleroma.Activity{}} = ReceiverWorker.perform(oban_job) end + + # When activity is delivered to the inbox and we cannot immediately verify signature + # we capture all the params and process it later in the Oban job. + # This requires we replicate the same scenario by including additional fields in the params + test "Deletes cancelled for an unknown actor" do + params = %{ + "type" => "Delete", + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + + assert {:cancel, "Delete from unknown actor"} = + ReceiverWorker.perform(%Oban.Job{ + args: %{ + "op" => "incoming_ap_doc", + "method" => :post, + "req_headers" => [], + "request_path" => "/inbox", + "query_string" => "", + "params" => params + } + }) + end end From 4bc6f334f49c27e1f466052fee6fa2f3d5d2ee74 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 23 Aug 2024 14:18:04 -0400 Subject: [PATCH 31/48] Revert unintentional change --- lib/pleroma/workers/receiver_worker.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index ea86a3a12e..4033fec0fb 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -34,7 +34,7 @@ def perform(%Job{ } with {_, false} <- {:unknown_delete, unknown_delete?(params)}, - User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do From 1c394dd18c5d61dc716a9b9cda981a359de32456 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 23 Aug 2024 14:24:04 -0400 Subject: [PATCH 32/48] Move the check to the inbox --- .../activity_pub/activity_pub_controller.ex | 32 +++++++++++++++---- lib/pleroma/workers/receiver_worker.ex | 15 +-------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index cdd054e1a3..a24dcfc9c9 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -294,13 +294,19 @@ def inbox(%{assigns: %{valid_signature: true}} = conn, params) do end def inbox(%{assigns: %{valid_signature: false}} = conn, params) do - Federator.incoming_ap_doc(%{ - method: conn.method, - req_headers: conn.req_headers, - request_path: conn.request_path, - params: params, - query_string: conn.query_string - }) + case unknown_delete?(params) do + true -> + :ok + + false -> + Federator.incoming_ap_doc(%{ + method: conn.method, + req_headers: conn.req_headers, + request_path: conn.request_path, + params: params, + query_string: conn.query_string + }) + end json(conn, "ok") end @@ -558,4 +564,16 @@ def pinned(conn, %{"nickname" => nickname}) do |> json(UserView.render("featured.json", %{user: user})) end end + + defp unknown_delete?(%{ + "type" => "Delete", + "actor" => actor + }) do + case User.get_cached_by_ap_id(actor) do + %User{} -> false + _ -> true + end + end + + defp unknown_delete?(_), do: false end diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 4033fec0fb..ce92ef9ddf 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,8 +33,7 @@ def perform(%Job{ query_string: query_string } - with {_, false} <- {:unknown_delete, unknown_delete?(params)}, - {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do @@ -73,16 +72,4 @@ defp process_errors(errors) do e -> {:error, e} end end - - defp unknown_delete?(%{ - "type" => "Delete", - "actor" => actor - }) do - case User.get_cached_by_ap_id(actor) do - %User{} -> false - _ -> true - end - end - - defp unknown_delete?(_), do: false end From 27fcc421719062d5de9bf4dc90f3349595eb278d Mon Sep 17 00:00:00 2001 From: feld Date: Sat, 24 Aug 2024 16:53:22 +0000 Subject: [PATCH 33/48] Use Pleroma.Object.Containment.get_actor/1 to reliably find the actor of an incoming activity or object --- lib/pleroma/web/activity_pub/activity_pub_controller.ex | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index a24dcfc9c9..77ec26f148 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -567,9 +567,8 @@ def pinned(conn, %{"nickname" => nickname}) do defp unknown_delete?(%{ "type" => "Delete", - "actor" => actor - }) do - case User.get_cached_by_ap_id(actor) do + } = data) do + case data |> Pleroma.Object.Containment.get_actor() |> User.get_cached_by_ap_id() do %User{} -> false _ -> true end From 7bcc21ad6f1fdf9dbc16990e9891f9de7a21011d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sat, 24 Aug 2024 13:01:28 -0400 Subject: [PATCH 34/48] Switch test to the inbox --- .../activity_pub_controller_test.exs | 21 ++++++++++++++++++ test/pleroma/workers/receiver_worker_test.exs | 22 ------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index af1a32fed8..b9067533c4 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -684,6 +684,27 @@ test "without valid signature, " <> |> json_response(400) end + # When activity is delivered to the inbox and we cannot immediately verify signature + # we capture all the params and process it later in the Oban job. + # Once we begin processing it through Oban we risk fetching the actor to validate the + # activity which just leads to inserting a new user to process a Delete not relevant to us. + test "Deletes from an unknown actor are discarded", %{conn: conn} do + params = + %{ + "type" => "Delete", + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + |> Jason.encode!() + + conn + |> assign(:valid_signature, false) + |> put_req_header("content-type", "application/activity+json") + |> post("/inbox", params) + |> json_response(200) + + assert all_enqueued() == [] + end + test "accepts Add/Remove activities", %{conn: conn} do object_id = "c61d6733-e256-4fe1-ab13-1e369789423f" diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 91fbb1fe88..33be910853 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -245,26 +245,4 @@ test "it can validate the signature" do assert {:ok, %Pleroma.Activity{}} = ReceiverWorker.perform(oban_job) end - - # When activity is delivered to the inbox and we cannot immediately verify signature - # we capture all the params and process it later in the Oban job. - # This requires we replicate the same scenario by including additional fields in the params - test "Deletes cancelled for an unknown actor" do - params = %{ - "type" => "Delete", - "actor" => "https://unknown.mastodon.instance/users/somebody" - } - - assert {:cancel, "Delete from unknown actor"} = - ReceiverWorker.perform(%Oban.Job{ - args: %{ - "op" => "incoming_ap_doc", - "method" => :post, - "req_headers" => [], - "request_path" => "/inbox", - "query_string" => "", - "params" => params - } - }) - end end From 06deacd58e6aa676847530f24c6799162ed06777 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sat, 24 Aug 2024 20:03:50 -0400 Subject: [PATCH 35/48] Formatting --- lib/pleroma/web/activity_pub/activity_pub_controller.ex | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index 77ec26f148..4c83d19274 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -565,9 +565,11 @@ def pinned(conn, %{"nickname" => nickname}) do end end - defp unknown_delete?(%{ - "type" => "Delete", - } = data) do + defp unknown_delete?( + %{ + "type" => "Delete" + } = data + ) do case data |> Pleroma.Object.Containment.get_actor() |> User.get_cached_by_ap_id() do %User{} -> false _ -> true From 16a9b34876f3a9289c02253e802bffdac4901ac0 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:16:59 -0400 Subject: [PATCH 36/48] Convert to an Plug called InboxGuard --- lib/pleroma/constants.ex | 12 ++++ .../activity_pub/activity_pub_controller.ex | 33 ++-------- lib/pleroma/web/plugs/inbox_guard_plug.ex | 66 +++++++++++++++++++ lib/pleroma/web/router.ex | 6 +- .../activity_pub_controller_test.exs | 2 +- 5 files changed, 91 insertions(+), 28 deletions(-) create mode 100644 lib/pleroma/web/plugs/inbox_guard_plug.ex diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index 3a5e353012..f969bfdbb0 100644 --- a/lib/pleroma/constants.ex +++ b/lib/pleroma/constants.ex @@ -85,6 +85,18 @@ defmodule Pleroma.Constants do ] ) + const(allowed_activity_types_from_strangers, + do: [ + "Block", + "Create", + "Flag", + "Follow", + "Like", + "Move", + "React" + ] + ) + # basic regex, just there to weed out potential mistakes # https://datatracker.ietf.org/doc/html/rfc2045#section-5.1 const(mime_regex, diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index 4c83d19274..cdd054e1a3 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -294,19 +294,13 @@ def inbox(%{assigns: %{valid_signature: true}} = conn, params) do end def inbox(%{assigns: %{valid_signature: false}} = conn, params) do - case unknown_delete?(params) do - true -> - :ok - - false -> - Federator.incoming_ap_doc(%{ - method: conn.method, - req_headers: conn.req_headers, - request_path: conn.request_path, - params: params, - query_string: conn.query_string - }) - end + Federator.incoming_ap_doc(%{ + method: conn.method, + req_headers: conn.req_headers, + request_path: conn.request_path, + params: params, + query_string: conn.query_string + }) json(conn, "ok") end @@ -564,17 +558,4 @@ def pinned(conn, %{"nickname" => nickname}) do |> json(UserView.render("featured.json", %{user: user})) end end - - defp unknown_delete?( - %{ - "type" => "Delete" - } = data - ) do - case data |> Pleroma.Object.Containment.get_actor() |> User.get_cached_by_ap_id() do - %User{} -> false - _ -> true - end - end - - defp unknown_delete?(_), do: false end diff --git a/lib/pleroma/web/plugs/inbox_guard_plug.ex b/lib/pleroma/web/plugs/inbox_guard_plug.ex new file mode 100644 index 0000000000..643b586d45 --- /dev/null +++ b/lib/pleroma/web/plugs/inbox_guard_plug.ex @@ -0,0 +1,66 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2022 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.Plugs.InboxGuardPlug do + import Plug.Conn + import Pleroma.Constants, only: [allowed_activity_types_from_strangers: 0] + + alias Pleroma.Config + alias Pleroma.User + + def init(options) do + options + end + + def call(%{assigns: %{valid_signature: true}} = conn, _opts) do + conn + end + + def call(conn, _opts) do + with {_, true} <- {:federating, Config.get!([:instance, :federating])}, + true <- known_actor?(conn) do + conn + else + {:federating, false} -> + conn + |> json(403, "Not federating") + + _ -> + conn + |> filter_from_strangers() + end + end + + # If signature failed but we know this actor we should + # accept it as we may only need to refetch their public key + # during processing + defp known_actor?(%{body_params: data}) do + case Pleroma.Object.Containment.get_actor(data) |> User.get_cached_by_ap_id() do + %User{} -> true + _ -> false + end + end + + # Only permit a subset of activity types from strangers + # or else it will add actors you've never interacted with + # to the database + defp filter_from_strangers(%{body_params: %{"type" => type}} = conn) do + with true <- type in allowed_activity_types_from_strangers() do + conn + else + _ -> + conn + |> json(400, "Invalid activity type for an unknown actor") + end + end + + defp json(conn, status, resp) do + json_resp = Jason.encode!(resp) + + conn + |> put_resp_content_type("application/json") + |> resp(status, json_resp) + |> halt() + end +end diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index 6492e38619..9b9ee421cf 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -217,6 +217,10 @@ defmodule Pleroma.Web.Router do plug(Pleroma.Web.Plugs.MappedSignatureToIdentityPlug) end + pipeline :inbox_guard do + plug(Pleroma.Web.Plugs.InboxGuardPlug) + end + pipeline :static_fe do plug(Pleroma.Web.Plugs.StaticFEPlug) end @@ -920,7 +924,7 @@ defmodule Pleroma.Web.Router do end scope "/", Pleroma.Web.ActivityPub do - pipe_through(:activitypub) + pipe_through([:activitypub, :inbox_guard]) post("/inbox", ActivityPubController, :inbox) post("/users/:nickname/inbox", ActivityPubController, :inbox) end diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index b9067533c4..1b959f3245 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -700,7 +700,7 @@ test "Deletes from an unknown actor are discarded", %{conn: conn} do |> assign(:valid_signature, false) |> put_req_header("content-type", "application/activity+json") |> post("/inbox", params) - |> json_response(200) + |> json_response(400) assert all_enqueued() == [] end From e2cdae2c88eb22588209923d83c2a9c52d16c48c Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:23:07 -0400 Subject: [PATCH 37/48] Change relay inbox response when not federating to a 403 for consistency --- lib/pleroma/web/activity_pub/activity_pub_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index cdd054e1a3..a08eda5f47 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -311,7 +311,7 @@ def inbox(conn, %{"type" => "Create"} = params) do post_inbox_relayed_create(conn, params) else conn - |> put_status(:bad_request) + |> put_status(403) |> json("Not federating") end end From 990b2058df11cc32f260d0ffcf7dd0f342d435b4 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:30:23 -0400 Subject: [PATCH 38/48] Remove unnecessary error match in ReceiverWorker --- lib/pleroma/workers/receiver_worker.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index ce92ef9ddf..d4db97b639 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -58,7 +58,6 @@ def timeout(_job), do: :timer.seconds(5) defp process_errors(errors) do case errors do - {:unknown_delete, true} -> {:cancel, "Delete from unknown actor"} {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} {:error, :already_present} -> {:cancel, :already_present} {:error, {:validate_object, _} = reason} -> {:cancel, reason} From 2b39956acbc3ccd87a43cd4ddbd5976adcac5936 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:40:31 -0400 Subject: [PATCH 39/48] Fix test title to be more specific as it has a broader but incorrect meaning --- test/pleroma/web/activity_pub/activity_pub_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 1b959f3245..762fca0a1f 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -657,7 +657,7 @@ test "accept follow activity", %{conn: conn} do end test "without valid signature, " <> - "it only accepts Create activities and requires enabled federation", + "it accepts Create activities and requires enabled federation", %{conn: conn} do data = File.read!("test/fixtures/mastodon-post-activity.json") |> Jason.decode!() non_create_data = File.read!("test/fixtures/mastodon-announce.json") |> Jason.decode!() From 012132303f79c0d693a8fba7236433443261b757 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:40:45 -0400 Subject: [PATCH 40/48] Test more types we do not want to receive from strangers --- .../activity_pub_controller_test.exs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 762fca0a1f..453dbaf0cd 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -688,21 +688,25 @@ test "without valid signature, " <> # we capture all the params and process it later in the Oban job. # Once we begin processing it through Oban we risk fetching the actor to validate the # activity which just leads to inserting a new user to process a Delete not relevant to us. - test "Deletes from an unknown actor are discarded", %{conn: conn} do - params = - %{ - "type" => "Delete", - "actor" => "https://unknown.mastodon.instance/users/somebody" - } - |> Jason.encode!() + test "Activities of certain types from an unknown actor are discarded", %{conn: conn} do + example_bad_types = ["Announce", "Delete", "Undo"] - conn - |> assign(:valid_signature, false) - |> put_req_header("content-type", "application/activity+json") - |> post("/inbox", params) - |> json_response(400) + Enum.each(example_bad_types, fn bad_type -> + params = + %{ + "type" => bad_type, + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + |> Jason.encode!() - assert all_enqueued() == [] + conn + |> assign(:valid_signature, false) + |> put_req_header("content-type", "application/activity+json") + |> post("/inbox", params) + |> json_response(400) + + assert all_enqueued() == [] + end) end test "accepts Add/Remove activities", %{conn: conn} do From 094da5d6343507eb1540f0a6357accc67573e02e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:44:52 -0400 Subject: [PATCH 41/48] Update changelog --- changelog.d/drop-unknown-deletes.change | 1 - changelog.d/drop-unwanted.change | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/drop-unknown-deletes.change create mode 100644 changelog.d/drop-unwanted.change diff --git a/changelog.d/drop-unknown-deletes.change b/changelog.d/drop-unknown-deletes.change deleted file mode 100644 index 0be7f54502..0000000000 --- a/changelog.d/drop-unknown-deletes.change +++ /dev/null @@ -1 +0,0 @@ -Drop incoming Delete activities from unknown actors diff --git a/changelog.d/drop-unwanted.change b/changelog.d/drop-unwanted.change new file mode 100644 index 0000000000..59447d68f6 --- /dev/null +++ b/changelog.d/drop-unwanted.change @@ -0,0 +1 @@ +Restrict incoming activities from unknown actors to a subset that does not imply a previous relationship From 5205e846eb5cfbd0adfa5031ad75e96fccbc86d8 Mon Sep 17 00:00:00 2001 From: feld Date: Fri, 30 Aug 2024 13:14:05 +0000 Subject: [PATCH 42/48] Update allowed activity types from strangers Move is emitted from the old account EmojiReact is ~ Like Announced TBD --- lib/pleroma/constants.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index f969bfdbb0..bbd1836831 100644 --- a/lib/pleroma/constants.ex +++ b/lib/pleroma/constants.ex @@ -92,8 +92,8 @@ defmodule Pleroma.Constants do "Flag", "Follow", "Like", - "Move", - "React" + "EmojiReact", + "Announce" ] ) From e38f5f1a817d6da30e9a128ec74a2a7c78faf174 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 30 Aug 2024 09:35:04 -0400 Subject: [PATCH 43/48] Add recognized activity types to a constant and use it in the test --- lib/pleroma/constants.ex | 18 ++++++++++++++++++ .../activity_pub_controller_test.exs | 4 +++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index bbd1836831..5268ebe7a2 100644 --- a/lib/pleroma/constants.ex +++ b/lib/pleroma/constants.ex @@ -85,6 +85,24 @@ defmodule Pleroma.Constants do ] ) + const(activity_types, + do: [ + "Create", + "Update", + "Delete", + "Follow", + "Accept", + "Reject", + "Add", + "Remove", + "Like", + "Announce", + "Undo", + "Flag", + "EmojiReact" + ] + ) + const(allowed_activity_types_from_strangers, do: [ "Block", diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 453dbaf0cd..c32f6c1a37 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -689,7 +689,9 @@ test "without valid signature, " <> # Once we begin processing it through Oban we risk fetching the actor to validate the # activity which just leads to inserting a new user to process a Delete not relevant to us. test "Activities of certain types from an unknown actor are discarded", %{conn: conn} do - example_bad_types = ["Announce", "Delete", "Undo"] + example_bad_types = + Pleroma.Constants.activity_types() -- + Pleroma.Constants.allowed_activity_types_from_strangers() Enum.each(example_bad_types, fn bad_type -> params = From 11ee94ae17094a2bc33505a31671b8c705f768a4 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 30 Aug 2024 09:46:10 -0400 Subject: [PATCH 44/48] InboxGuardPlug: Add early rejection of unknown activity types --- lib/pleroma/web/plugs/inbox_guard_plug.ex | 31 ++++++++++++++++--- .../activity_pub_controller_test.exs | 21 +++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/web/plugs/inbox_guard_plug.ex b/lib/pleroma/web/plugs/inbox_guard_plug.ex index 643b586d45..0064cce766 100644 --- a/lib/pleroma/web/plugs/inbox_guard_plug.ex +++ b/lib/pleroma/web/plugs/inbox_guard_plug.ex @@ -4,7 +4,7 @@ defmodule Pleroma.Web.Plugs.InboxGuardPlug do import Plug.Conn - import Pleroma.Constants, only: [allowed_activity_types_from_strangers: 0] + import Pleroma.Constants, only: [activity_types: 0, allowed_activity_types_from_strangers: 0] alias Pleroma.Config alias Pleroma.User @@ -14,24 +14,46 @@ def init(options) do end def call(%{assigns: %{valid_signature: true}} = conn, _opts) do - conn + with {_, true} <- {:federating, Config.get!([:instance, :federating])} do + conn + |> filter_activity_types() + else + {:federating, false} -> + conn + |> json(403, "Not federating") + |> halt() + end end def call(conn, _opts) do with {_, true} <- {:federating, Config.get!([:instance, :federating])}, - true <- known_actor?(conn) do + conn = filter_activity_types(conn), + {:known, true} <- {:known, known_actor?(conn)} do conn else {:federating, false} -> conn |> json(403, "Not federating") + |> halt() - _ -> + {:known, false} -> conn |> filter_from_strangers() end end + # Early rejection of unrecognized types + defp filter_activity_types(%{body_params: %{"type" => type}} = conn) do + with true <- type in activity_types() do + conn + else + _ -> + conn + |> json(400, "Invalid activity type") + |> halt() + end + end + # If signature failed but we know this actor we should # accept it as we may only need to refetch their public key # during processing @@ -52,6 +74,7 @@ defp filter_from_strangers(%{body_params: %{"type" => type}} = conn) do _ -> conn |> json(400, "Invalid activity type for an unknown actor") + |> halt() end end diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index c32f6c1a37..3bd589f490 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -711,6 +711,27 @@ test "Activities of certain types from an unknown actor are discarded", %{conn: end) end + test "Unknown activity types are discarded", %{conn: conn} do + unknown_types = ["Poke", "Read", "Dazzle"] + + Enum.each(unknown_types, fn bad_type -> + params = + %{ + "type" => bad_type, + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + |> Jason.encode!() + + conn + |> assign(:valid_signature, true) + |> put_req_header("content-type", "application/activity+json") + |> post("/inbox", params) + |> json_response(400) + + assert all_enqueued() == [] + end) + end + test "accepts Add/Remove activities", %{conn: conn} do object_id = "c61d6733-e256-4fe1-ab13-1e369789423f" From bb235f913fb88f925abc791285808afe63d14bca Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 30 Aug 2024 10:03:51 -0400 Subject: [PATCH 45/48] Update changelog --- changelog.d/drop-unwanted.change | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/drop-unwanted.change b/changelog.d/drop-unwanted.change index 59447d68f6..459d4bfe64 100644 --- a/changelog.d/drop-unwanted.change +++ b/changelog.d/drop-unwanted.change @@ -1 +1 @@ -Restrict incoming activities from unknown actors to a subset that does not imply a previous relationship +Restrict incoming activities from unknown actors to a subset that does not imply a previous relationship and early rejection of unrecognized activity types. From 5a1144208d1007af2a2d2279c582adf9d2fa7246 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sun, 1 Sep 2024 12:26:59 -0400 Subject: [PATCH 46/48] Prevent OAuth App flow from creating duplicate entries --- changelog.d/oauth-app.fix | 1 + .../controllers/app_controller.ex | 4 +- .../controllers/app_controller_test.exs | 47 +++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 changelog.d/oauth-app.fix diff --git a/changelog.d/oauth-app.fix b/changelog.d/oauth-app.fix new file mode 100644 index 0000000000..eb917462fd --- /dev/null +++ b/changelog.d/oauth-app.fix @@ -0,0 +1 @@ +Prevent OAuth App flow from creating duplicate entries diff --git a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex index 844673ae01..e5e8ea8f58 100644 --- a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex @@ -33,11 +33,9 @@ def create(%{body_params: params} = conn, _params) do app_attrs = params |> Map.take([:client_name, :redirect_uris, :website]) - |> Map.put(:scopes, scopes) |> Maps.put_if_present(:user_id, user_id) - with cs <- App.register_changeset(%App{}, app_attrs), - {:ok, app} <- Repo.insert(cs) do + with {:ok, app} <- App.get_or_make(app_attrs, scopes) do render(conn, "show.json", app: app) end end diff --git a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs index bc9d4048c4..1e2e687910 100644 --- a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs @@ -89,4 +89,51 @@ test "creates an oauth app with a user", %{conn: conn} do assert expected == json_response_and_validate_schema(conn, 200) assert app.user_id == user.id end + + test "creates an oauth app without a user", %{conn: conn} do + app_attrs = build(:oauth_app) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: app_attrs.client_name, + redirect_uris: app_attrs.redirect_uris + }) + + [app] = Repo.all(App) + + expected = %{ + "name" => app.client_name, + "website" => app.website, + "client_id" => app.client_id, + "client_secret" => app.client_secret, + "id" => app.id |> to_string(), + "redirect_uri" => app.redirect_uris, + "vapid_key" => Push.vapid_config() |> Keyword.get(:public_key) + } + + assert expected == json_response_and_validate_schema(conn, 200) + end + + test "does not duplicate apps with the same client name", %{conn: conn} do + client_name = "BleromaSE" + redirect_uris = "https://bleroma.app/oauth-callback" + + for _i <- 1..3 do + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: client_name, + redirect_uris: redirect_uris + }) + |> json_response_and_validate_schema(200) + end + + apps = Repo.all(App) + + assert length(apps) == 1 + assert List.first(apps).client_name == client_name + assert List.first(apps).redirect_uris == redirect_uris + end end From e3a7c1d906698d2f36661b60de4bdab5a475b871 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sun, 1 Sep 2024 12:37:59 -0400 Subject: [PATCH 47/48] Test that app scopes can be updated --- .../controllers/app_controller_test.exs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs index 1e2e687910..26c431673c 100644 --- a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs @@ -136,4 +136,37 @@ test "does not duplicate apps with the same client name", %{conn: conn} do assert List.first(apps).client_name == client_name assert List.first(apps).redirect_uris == redirect_uris end + + test "app scopes can be updated", %{conn: conn} do + client_name = "BleromaSE" + redirect_uris = "https://bleroma.app/oauth-callback" + website = "https://bleromase.com" + scopes = "read write" + + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: client_name, + redirect_uris: redirect_uris, + website: website, + scopes: scopes + }) + |> json_response_and_validate_schema(200) + + assert List.first(Repo.all(App)).scopes == String.split(scopes, " ") + + updated_scopes = "read write push" + + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: client_name, + redirect_uris: redirect_uris, + website: website, + scopes: updated_scopes + }) + |> json_response_and_validate_schema(200) + + assert List.first(Repo.all(App)).scopes == String.split(updated_scopes, " ") + end end From 751d63d4bb05caececf52a3a3b134182e57a059d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sun, 1 Sep 2024 13:34:30 -0400 Subject: [PATCH 48/48] Support OAuth App updating the website URL --- .../controllers/app_controller.ex | 3 +- lib/pleroma/web/o_auth/app.ex | 24 +++++---------- .../controllers/app_controller_test.exs | 30 +++++++++++++++++++ test/pleroma/web/o_auth/app_test.exs | 15 ++++++---- 4 files changed, 49 insertions(+), 23 deletions(-) diff --git a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex index e5e8ea8f58..4677ac40aa 100644 --- a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex @@ -33,9 +33,10 @@ def create(%{body_params: params} = conn, _params) do app_attrs = params |> Map.take([:client_name, :redirect_uris, :website]) + |> Map.put(:scopes, scopes) |> Maps.put_if_present(:user_id, user_id) - with {:ok, app} <- App.get_or_make(app_attrs, scopes) do + with {:ok, app} <- App.get_or_make(app_attrs) do render(conn, "show.json", app: app) end end diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index d1bf6dd18f..889850c73c 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -67,35 +67,27 @@ def update(id, params) do with %__MODULE__{} = app <- Repo.get(__MODULE__, id) do app |> changeset(params) + |> validate_required([:scopes]) |> Repo.update() end end @doc """ - Gets app by attrs or create new with attrs. - And updates the scopes if need. + Gets app by attrs or create new with attrs. + Updates the attrs if needed. """ - @spec get_or_make(map(), list(String.t())) :: {:ok, t()} | {:error, Ecto.Changeset.t()} - def get_or_make(attrs, scopes) do - with %__MODULE__{} = app <- Repo.get_by(__MODULE__, attrs) do - update_scopes(app, scopes) + @spec get_or_make(map()) :: {:ok, t()} | {:error, Ecto.Changeset.t()} + def get_or_make(attrs) do + with %__MODULE__{} = app <- Repo.get_by(__MODULE__, client_name: attrs.client_name) do + __MODULE__.update(app.id, Map.take(attrs, [:scopes, :website])) else _e -> %__MODULE__{} - |> register_changeset(Map.put(attrs, :scopes, scopes)) + |> register_changeset(attrs) |> Repo.insert() end end - defp update_scopes(%__MODULE__{} = app, []), do: {:ok, app} - defp update_scopes(%__MODULE__{scopes: scopes} = app, scopes), do: {:ok, app} - - defp update_scopes(%__MODULE__{} = app, scopes) do - app - |> change(%{scopes: scopes}) - |> Repo.update() - end - @spec search(map()) :: {:ok, [t()], non_neg_integer()} def search(params) do query = from(a in __MODULE__) diff --git a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs index 26c431673c..df28f20108 100644 --- a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs @@ -169,4 +169,34 @@ test "app scopes can be updated", %{conn: conn} do assert List.first(Repo.all(App)).scopes == String.split(updated_scopes, " ") end + + test "app website URL can be updated", %{conn: conn} do + client_name = "BleromaSE" + redirect_uris = "https://bleroma.app/oauth-callback" + website = "https://bleromase.com" + + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: client_name, + redirect_uris: redirect_uris, + website: website + }) + |> json_response_and_validate_schema(200) + + assert List.first(Repo.all(App)).website == website + + updated_website = "https://bleromase2ultimateedition.com" + + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: client_name, + redirect_uris: redirect_uris, + website: updated_website + }) + |> json_response_and_validate_schema(200) + + assert List.first(Repo.all(App)).website == updated_website + end end diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 96a67de6bd..423b660ea7 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -12,20 +12,23 @@ defmodule Pleroma.Web.OAuth.AppTest do test "gets exist app" do attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]})) - {:ok, %App{} = exist_app} = App.get_or_make(attrs, []) + {:ok, %App{} = exist_app} = App.get_or_make(attrs) assert exist_app == app end test "make app" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} - {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) + attrs = %{client_name: "Mastodon-Local", redirect_uris: ".", scopes: ["write"]} + {:ok, %App{} = app} = App.get_or_make(attrs) assert app.scopes == ["write"] end test "gets exist app and updates scopes" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} - app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]})) - {:ok, %App{} = exist_app} = App.get_or_make(attrs, ["read", "write", "follow", "push"]) + attrs = %{client_name: "Mastodon-Local", redirect_uris: ".", scopes: ["read", "write"]} + app = insert(:oauth_app, attrs) + + {:ok, %App{} = exist_app} = + App.get_or_make(%{attrs | scopes: ["read", "write", "follow", "push"]}) + assert exist_app.id == app.id assert exist_app.scopes == ["read", "write", "follow", "push"] end