Merge branch 'fix/gts-federation' into 'develop'

GoToSocial federation fixes

See merge request pleroma/pleroma!3725

5 files changed, 82 insertions, 17 deletions

diff --git a/lib/pleroma/signature.ex b/lib/pleroma/signature.ex
index dbe6fd209..ff0c56856 100644
--- a/lib/pleroma/signature.ex
+++ b/lib/pleroma/signature.ex
@@ -10,17 +10,14 @@ defmodule Pleroma.Signature do
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.ActivityPub
 
+  @known_suffixes ["/publickey", "/main-key"]
+
   def key_id_to_actor_id(key_id) do
     uri =
-      URI.parse(key_id)
+      key_id
+      |> URI.parse()
       |> Map.put(:fragment, nil)
-
-    uri =
-      if not is_nil(uri.path) and String.ends_with?(uri.path, "/publickey") do
-        Map.put(uri, :path, String.replace(uri.path, "/publickey", ""))
-      else
-        uri
-      end
+      |> remove_suffix(@known_suffixes)
 
     maybe_ap_id = URI.to_string(uri)
 
@@ -36,6 +33,16 @@ defmodule Pleroma.Signature do
     end
   end
 
+  defp remove_suffix(uri, [test | rest]) do
+    if not is_nil(uri.path) and String.ends_with?(uri.path, test) do
+      Map.put(uri, :path, String.replace(uri.path, test, ""))
+    else
+      remove_suffix(uri, rest)
+    end
+  end
+
+  defp remove_suffix(uri, []), do: uri
+
   def fetch_public_key(conn) do
     with %{"keyId" => kid} <- HTTPSignatures.signature_for_conn(conn),
          {:ok, actor_id} <- key_id_to_actor_id(kid),
diff --git a/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex b/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex
index 5647156f5..2670e3f17 100644
--- a/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex
@@ -63,7 +63,10 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ArticleNotePageValidator do
   defp fix_replies(%{"replies" => %{"items" => replies}} = data) when is_list(replies),
     do: Map.put(data, "replies", replies)
 
-  defp fix_replies(%{"replies" => replies} = data) when is_bitstring(replies),
+  # TODO: Pleroma does not have any support for Collections at the moment.
+  # If the `replies` field is not something the ObjectID validator can handle,
+  # the activity/object would be rejected, which is bad behavior.
+  defp fix_replies(%{"replies" => replies} = data) when not is_list(replies),
     do: Map.drop(data, ["replies"])
 
   defp fix_replies(data), do: data
diff --git a/lib/pleroma/web/plugs/http_signature_plug.ex b/lib/pleroma/web/plugs/http_signature_plug.ex
index d023754a6..4bf325218 100644
--- a/lib/pleroma/web/plugs/http_signature_plug.ex
+++ b/lib/pleroma/web/plugs/http_signature_plug.ex
@@ -25,21 +25,58 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlug do
     end
   end
 
+  defp validate_signature(conn, request_target) do
+    # Newer drafts for HTTP signatures now use @request-target instead of the
+    # old (request-target). We'll now support both for incoming signatures.
+    conn =
+      conn
+      |> put_req_header("(request-target)", request_target)
+      |> put_req_header("@request-target", request_target)
+
+    HTTPSignatures.validate_conn(conn)
+  end
+
+  defp validate_signature(conn) do
+    # This (request-target) is non-standard, but many implementations do it
+    # this way due to a misinterpretation of
+    # https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-06
+    # "path" was interpreted as not having the query, though later examples
+    # show that it must be the absolute path + query. This behavior is kept to
+    # make sure most software (Pleroma itself, Mastodon, and probably others)
+    # do not break.
+    request_target = String.downcase("#{conn.method}") <> " #{conn.request_path}"
+
+    # This is the proper way to build the @request-target, as expected by
+    # many HTTP signature libraries, clarified in the following draft:
+    # https://www.ietf.org/archive/id/draft-ietf-httpbis-message-signatures-11.html#section-2.2.6
+    # It is the same as before, but containing the query part as well.
+    proper_target = request_target <> "?#{conn.query_string}"
+
+    cond do
+      # Normal, non-standard behavior but expected by Pleroma and more.
+      validate_signature(conn, request_target) ->
+        true
+
+      # Has query string and the previous one failed: let's try the standard.
+      conn.query_string != "" ->
+        validate_signature(conn, proper_target)
+
+      # If there's no query string and signature fails, it's rotten.
+      true ->
+        false
+    end
+  end
+
   defp maybe_assign_valid_signature(conn) do
     if has_signature_header?(conn) do
-      # set (request-target) header to the appropriate value
-      # we also replace the digest header with the one we computed
-      request_target = String.downcase("#{conn.method}") <> " #{conn.request_path}"
-
+      # we replace the digest header with the one we computed in DigestPlug
       conn =
-        conn
-        |> put_req_header("(request-target)", request_target)
-        |> case do
+        case conn do
           %{assigns: %{digest: digest}} = conn -> put_req_header(conn, "digest", digest)
           conn -> conn
         end
 
-      assign(conn, :valid_signature, HTTPSignatures.validate_conn(conn))
+      assign(conn, :valid_signature, validate_signature(conn))
     else
       Logger.debug("No signature header!")
       conn
diff --git a/test/pleroma/signature_test.exs b/test/pleroma/signature_test.exs
index 92d05f26c..b849cbee7 100644
--- a/test/pleroma/signature_test.exs
+++ b/test/pleroma/signature_test.exs
@@ -109,6 +109,11 @@ defmodule Pleroma.SignatureTest do
                {:ok, "https://example.com/users/1234"}
     end
 
+    test "it deduces the actor id for gotoSocial" do
+      assert Signature.key_id_to_actor_id("https://example.com/users/1234/main-key") ==
+               {:ok, "https://example.com/users/1234"}
+    end
+
     test "it calls webfinger for 'acct:' accounts" do
       with_mock(Pleroma.Web.WebFinger,
         finger: fn _ -> %{"ap_id" => "https://gensokyo.2hu/users/raymoo"} end
diff --git a/test/pleroma/web/activity_pub/object_validators/article_note_page_validator_test.exs b/test/pleroma/web/activity_pub/object_validators/article_note_page_validator_test.exs
index 781093b1d..c7a62be18 100644
--- a/test/pleroma/web/activity_pub/object_validators/article_note_page_validator_test.exs
+++ b/test/pleroma/web/activity_pub/object_validators/article_note_page_validator_test.exs
@@ -103,4 +103,17 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ArticleNotePageValidatorTest
 
     %{valid?: true} = ArticleNotePageValidator.cast_and_validate(note)
   end
+
+  test "a Note without replies/first/items validates" do
+    insert(:user, ap_id: "https://mastodon.social/users/emelie")
+
+    note =
+      "test/fixtures/tesla_mock/status.emelie.json"
+      |> File.read!()
+      |> Jason.decode!()
+      |> pop_in(["replies", "first", "items"])
+      |> elem(1)
+
+    %{valid?: true} = ArticleNotePageValidator.cast_and_validate(note)
+  end
 end