From fc450fdefc2df2bbec20a79fb2c60a95e7f41833 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 28 Aug 2024 15:45:13 -0400 Subject: [PATCH 01/14] 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 d4db97b63..7dce02a5f 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -56,17 +56,20 @@ defmodule Pleroma.Workers.ReceiverWorker do 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 33be91085..640cefb78 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -51,6 +51,54 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 02/14] 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 640cefb78..2c0da8887 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -51,52 +51,56 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 03/14] 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 2c0da8887..085108e37 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -65,6 +65,12 @@ defmodule Pleroma.Workers.ReceiverWorkerTest do status: 404, body: "" } + + %{url: "https://springfield.social/users/hankscorpio"} -> + %Tesla.Env{ + status: 410, + body: "" + } end) end @@ -101,6 +107,23 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 04/14] 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 6b47e7db9..000000000 --- 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 3c3fb3fd2..19dc0087f 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 085108e37..cb434f52e 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -128,23 +128,6 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 05/14] 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 000000000..1ed0c5bb1 --- /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 06/14] 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 cb434f52e..995f765a1 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -177,4 +177,21 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 07/14] 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 7dce02a5f..80518f6fd 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,7 +33,8 @@ defmodule Pleroma.Workers.ReceiverWorker do 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 @@ defmodule Pleroma.Workers.ReceiverWorker 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 995f765a1..adf90ec86 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 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 08/14] 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 adf90ec86..779e83eaa 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -220,4 +220,29 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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 09/14] 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 80518f6fd..4b1f74a27 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -61,17 +61,21 @@ defmodule Pleroma.Workers.ReceiverWorker do 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 10/14] :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 2df716556..e812b1a46 100644 --- a/lib/pleroma/web/federator.ex +++ b/lib/pleroma/web/federator.ex @@ -121,11 +121,6 @@ defmodule Pleroma.Web.Federator 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 4b1f74a27..c7e6bc5ea 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -74,7 +74,6 @@ defmodule Pleroma.Workers.ReceiverWorker 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 11/14] 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 c7e6bc5ea..810fda67c 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -69,6 +69,8 @@ defmodule Pleroma.Workers.ReceiverWorker 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 12/14] 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 810fda67c..6787a59ef 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -75,8 +75,11 @@ defmodule Pleroma.Workers.ReceiverWorker 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 13/14] 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 000000000..f91502ed2 --- /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 1ed0c5bb1..000000000 --- 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 14/14] 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 e812b1a46..58260afa8 100644 --- a/lib/pleroma/web/federator.ex +++ b/lib/pleroma/web/federator.ex @@ -102,7 +102,8 @@ defmodule Pleroma.Web.Federator 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 6787a59ef..0373ec15f 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,8 +33,7 @@ defmodule Pleroma.Workers.ReceiverWorker do 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 @@ defmodule Pleroma.Workers.ReceiverWorker 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 779e83eaa..4d53c44ed 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -138,15 +138,13 @@ defmodule Pleroma.Workers.ReceiverWorkerTest 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