Merge branch 'fix/optimistic-inbox' into 'develop'

Fix Optimistic Inbox for failed signatures

See merge request pleroma/pleroma!4193

5 files changed, 83 insertions, 55 deletions

diff --git a/lib/pleroma/signature.ex b/lib/pleroma/signature.ex
index 900d40c4b..195513478 100644
--- a/lib/pleroma/signature.ex
+++ b/lib/pleroma/signature.ex
@@ -10,6 +10,14 @@ defmodule Pleroma.Signature do
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.ActivityPub
 
+  import Plug.Conn, only: [put_req_header: 3]
+
+  @http_signatures_impl Application.compile_env(
+                          :pleroma,
+                          [__MODULE__, :http_signatures_impl],
+                          HTTPSignatures
+                        )
+
   @known_suffixes ["/publickey", "/main-key"]
 
   def key_id_to_actor_id(key_id) do
@@ -85,4 +93,48 @@ defmodule Pleroma.Signature do
   def signed_date(%NaiveDateTime{} = date) do
     Timex.format!(date, "{WDshort}, {0D} {Mshort} {YYYY} {h24}:{m}:{s} GMT")
   end
+
+  @spec validate_signature(Plug.Conn.t(), String.t()) :: boolean()
+  def validate_signature(%Plug.Conn{} = 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)
+
+    @http_signatures_impl.validate_conn(conn)
+  end
+
+  @spec validate_signature(Plug.Conn.t()) :: boolean()
+  def validate_signature(%Plug.Conn{} = 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 = Enum.join([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 = Enum.join([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
 end
diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex
index e6161455d..cdd054e1a 100644
--- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex
+++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex
@@ -293,8 +293,15 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubController do
     json(conn, "ok")
   end
 
-  def inbox(%{assigns: %{valid_signature: false}, req_headers: req_headers} = conn, params) do
-    Federator.incoming_ap_doc(%{req_headers: req_headers, params: params})
+  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
+    })
+
     json(conn, "ok")
   end
 
diff --git a/lib/pleroma/web/federator.ex b/lib/pleroma/web/federator.ex
index 4b30fd21d..3d3101d61 100644
--- a/lib/pleroma/web/federator.ex
+++ b/lib/pleroma/web/federator.ex
@@ -35,10 +35,12 @@ defmodule Pleroma.Web.Federator do
   end
 
   # Client API
-  def incoming_ap_doc(%{params: params, req_headers: req_headers}) do
+  def incoming_ap_doc(%{params: _params, req_headers: _req_headers} = args) do
+    job_args = Enum.into(args, %{}, fn {k, v} -> {Atom.to_string(k), v} end)
+
     ReceiverWorker.enqueue(
       "incoming_ap_doc",
-      %{"req_headers" => req_headers, "params" => params, "timeout" => :timer.seconds(20)},
+      Map.put(job_args, "timeout", :timer.seconds(20)),
       priority: 2
     )
   end
diff --git a/lib/pleroma/web/plugs/http_signature_plug.ex b/lib/pleroma/web/plugs/http_signature_plug.ex
index 6bf2dd432..67974599a 100644
--- a/lib/pleroma/web/plugs/http_signature_plug.ex
+++ b/lib/pleroma/web/plugs/http_signature_plug.ex
@@ -8,16 +8,12 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlug do
   import Plug.Conn
   import Phoenix.Controller, only: [get_format: 1, text: 2]
 
+  alias Pleroma.Signature
   alias Pleroma.Web.ActivityPub.MRF
 
   require Logger
 
   @config_impl Application.compile_env(:pleroma, [__MODULE__, :config_impl], Pleroma.Config)
-  @http_signatures_impl Application.compile_env(
-                          :pleroma,
-                          [__MODULE__, :http_signatures_impl],
-                          HTTPSignatures
-                        )
 
   def init(options) do
     options
@@ -39,48 +35,6 @@ 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)
-
-    @http_signatures_impl.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
       # we replace the digest header with the one we computed in DigestPlug
@@ -90,7 +44,7 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlug do
           conn -> conn
         end
 
-      assign(conn, :valid_signature, validate_signature(conn))
+      assign(conn, :valid_signature, Signature.validate_signature(conn))
     else
       Logger.debug("No signature header!")
       conn
diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
index d01813c25..fd5c13fca 100644
--- a/lib/pleroma/workers/receiver_worker.ex
+++ b/lib/pleroma/workers/receiver_worker.ex
@@ -12,17 +12,30 @@ defmodule Pleroma.Workers.ReceiverWorker do
   @impl Oban.Worker
 
   def perform(%Job{
-        args: %{"op" => "incoming_ap_doc", "req_headers" => req_headers, "params" => params}
+        args: %{
+          "op" => "incoming_ap_doc",
+          "method" => method,
+          "params" => params,
+          "req_headers" => req_headers,
+          "request_path" => request_path,
+          "query_string" => query_string
+        }
       }) do
     # Oban's serialization converts our tuple headers to lists.
     # Revert it for the signature validation.
     req_headers = Enum.into(req_headers, [], &List.to_tuple(&1))
 
-    conn_data = %{params: params, req_headers: req_headers}
+    conn_data = %Plug.Conn{
+      method: method,
+      params: params,
+      req_headers: req_headers,
+      request_path: request_path,
+      query_string: query_string
+    }
 
     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, HTTPSignatures.validate_conn(conn_data)},
+         {:signature, true} <- {:signature, Signature.validate_signature(conn_data)},
          {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do
       {:ok, res}
     else