Merge pull request 'Akkoma fixes: context, polls, media proxy, ConfigDB, API params' (#8047) from lambadalambda/pleroma:fix/akkoma-tier2 into develop

Reviewed-on: https://git.pleroma.social/pleroma/pleroma/pulls/8047

changelog.d/cast-and-validate-many-params.fix

@@ -0,0 +1 @@
+Drop all unexpected query params, not just the first one, when `Pleroma.Web.ApiSpec.CastAndValidate` `strict` is off

changelog.d/config-db-unknown-module.fix

@@ -0,0 +1 @@
+Do not crash at boot or in the AdminAPI config listing when a ConfigDB setting refers to a module that no longer exists

changelog.d/context-local-only.fix

@@ -0,0 +1 @@
+Show local-only replies in status context to local users who don't follow the replier

changelog.d/media-proxy-static-redirect.fix

@@ -0,0 +1 @@
+Media proxy: serve a preview for static=true on non-GIF files instead of redirecting to the main endpoint host

changelog.d/question-note-fixups.fix

@@ -0,0 +1 @@
+Apply the Note fixups to incoming Questions, so polls with a single non-array tag or attachment are accepted

lib/pleroma/config/transfer_task.ex

@@ -48,6 +48,7 @@ defmodule Pleroma.Config.TransferTask do
       settings =
         (Repo.all(ConfigDB) ++ deleted_settings)
         |> Enum.reject(&ConfigDB.static?/1)
+        |> Enum.reject(&(&1.group == :invalid_atom or &1.key == :invalid_atom))
         |> Enum.map(&merge_with_default/1)
 
       started_applications = Application.started_applications()

lib/pleroma/ecto_type/config/atom.ex

@@ -5,6 +5,8 @@
 defmodule Pleroma.EctoType.Config.Atom do
   use Ecto.Type
 
+  require Logger
+
   def type, do: :atom
 
   def cast(key) when is_atom(key) do
@@ -17,8 +19,17 @@ defmodule Pleroma.EctoType.Config.Atom do
 
   def cast(_), do: :error
 
+  # A key or group naming a module that no longer exists (e.g. a removed uploader)
+  # must not crash loading
   def load(key) do
     {:ok, Pleroma.ConfigDB.string_to_elixir_types(key)}
+  rescue
+    ArgumentError ->
+      Logger.warning(
+        "ConfigDB: unknown module #{key} in a setting key or group, the setting is not applied"
+      )
+
+      {:ok, :invalid_atom}
   end
 
   def dump(key) when is_atom(key), do: {:ok, inspect(key)}

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -466,21 +466,28 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
   end
 
   def fetch_activities_for_context_query(context, opts) do
-    public = [Constants.as_public()]
+    user = opts[:user]
 
     recipients =
-      if opts[:user],
-        do: [opts[:user].ap_id | User.following(opts[:user])] ++ public,
-        else: public
+      cond do
+        user && user.local ->
+          [Constants.as_public(), as_local_public(), user.ap_id | User.following(user)]
+
+        user ->
+          [Constants.as_public(), user.ap_id | User.following(user)]
+
+        true ->
+          [Constants.as_public()]
+      end
 
     from(activity in Activity)
     |> maybe_preload_objects(opts)
     |> maybe_preload_bookmarks(opts)
     |> maybe_set_thread_muted_field(opts)
-    |> restrict_unauthenticated(opts[:user])
+    |> restrict_unauthenticated(user)
     |> restrict_blocked(opts)
     |> restrict_blockers_visibility(opts)
-    |> restrict_recipients(recipients, opts[:user])
+    |> restrict_recipients(recipients, user)
     |> restrict_filtered(opts)
     |> where(
       [activity],

lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex

@@ -213,7 +213,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ArticleNotePageValidator do
 
   defp fix_misskey_content(object), do: object
 
-  defp fix(data) do
+  def fix(data) do
     data
     |> CommonFixes.fix_actor()
     |> CommonFixes.fix_object_defaults()

lib/pleroma/web/activity_pub/object_validators/question_validator.ex

@@ -6,10 +6,9 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.QuestionValidator do
   use Ecto.Schema
 
   alias Pleroma.EctoType.ActivityPub.ObjectValidators
-  alias Pleroma.Web.ActivityPub.ObjectValidators.CommonFixes
+  alias Pleroma.Web.ActivityPub.ObjectValidators.ArticleNotePageValidator
   alias Pleroma.Web.ActivityPub.ObjectValidators.CommonValidations
   alias Pleroma.Web.ActivityPub.ObjectValidators.QuestionOptionsValidator
-  alias Pleroma.Web.ActivityPub.Transmogrifier
 
   import Ecto.Changeset
 
@@ -62,11 +61,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.QuestionValidator do
 
   defp fix(data) do
     data
-    |> CommonFixes.fix_actor()
-    |> CommonFixes.fix_object_defaults()
-    |> CommonFixes.fix_quote_url()
-    |> CommonFixes.fix_likes()
-    |> Transmogrifier.fix_emoji()
+    |> ArticleNotePageValidator.fix()
     |> fix_closed()
   end
 

lib/pleroma/web/api_spec/cast_and_validate.ex

@@ -20,6 +20,9 @@ defmodule Pleroma.Web.ApiSpec.CastAndValidate do
 
   require Logger
 
+  # How often to drop offending query params and retry in permissive mode
+  @max_retries 20
+
   @impl Plug
   def init(opts) do
     opts
@@ -114,34 +117,49 @@ defmodule Pleroma.Web.ApiSpec.CastAndValidate do
          false = _strict,
          cast_opts
        ) do
-    case OpenApiSpex.cast_and_validate(spec, operation, conn, content_type, cast_opts) do
-      {:ok, conn} ->
-        {:ok, conn}
+    permissive_cast_and_validate(spec, operation, conn, content_type, cast_opts, @max_retries)
+  end
 
-      # Remove unexpected query params and cast/validate again
-      {:error, errors} ->
-        query_params =
-          Enum.reduce(errors, conn.query_params, fn
-            %{reason: :unexpected_field, name: name, path: [name]}, params ->
-              Map.delete(params, name)
+  # OpenApiSpex reports one offending param at a time, so drop it and cast/validate again
+  defp permissive_cast_and_validate(
+         spec,
+         operation,
+         %Conn{} = conn,
+         content_type,
+         cast_opts,
+         retries
+       ) do
+    with {:error, errors} = error <-
+           OpenApiSpex.cast_and_validate(spec, operation, conn, content_type, cast_opts) do
+      query_params = drop_invalid_query_params(errors, conn.query_params)
 
-            # Filter out empty params
-            %{reason: :invalid_type, path: [name_atom], value: ""}, params ->
-              Map.delete(params, to_string(name_atom))
-
-            %{reason: :invalid_enum, name: nil, path: path, value: value}, params ->
-              path = path |> Enum.reverse() |> tl() |> Enum.reverse() |> list_items_to_string()
-              update_in(params, path, &List.delete(&1, value))
-
-            _, params ->
-              params
-          end)
-
-        conn = %Conn{conn | query_params: query_params}
-        OpenApiSpex.cast_and_validate(spec, operation, conn, content_type, cast_opts)
+      if retries > 0 and query_params != conn.query_params do
+        conn = %{conn | query_params: query_params}
+        permissive_cast_and_validate(spec, operation, conn, content_type, cast_opts, retries - 1)
+      else
+        error
+      end
     end
   end
 
+  defp drop_invalid_query_params(errors, query_params) do
+    Enum.reduce(errors, query_params, fn
+      %{reason: :unexpected_field, name: name, path: [name]}, params ->
+        Map.delete(params, name)
+
+      # Filter out empty params
+      %{reason: :invalid_type, path: [name_atom], value: ""}, params ->
+        Map.delete(params, to_string(name_atom))
+
+      %{reason: :invalid_enum, name: nil, path: path, value: value}, params ->
+        path = path |> Enum.reverse() |> tl() |> Enum.reverse() |> list_items_to_string()
+        update_in(params, path, &List.delete(&1, value))
+
+      _, params ->
+        params
+    end)
+  end
+
   defp list_items_to_string(list) do
     Enum.map(list, fn
       i when is_atom(i) -> to_string(i)

lib/pleroma/web/media_proxy/media_proxy_controller.ex

@@ -7,7 +7,6 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
 
   alias Pleroma.Config
   alias Pleroma.Helpers.MediaHelper
-  alias Pleroma.Helpers.UriHelper
   alias Pleroma.ReverseProxy
   alias Pleroma.Web.MediaProxy
   alias Plug.Conn
@@ -67,9 +66,6 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
         static and content_type == "image/gif" ->
           handle_jpeg_preview(conn, media_proxy_url)
 
-        static ->
-          drop_static_param_and_redirect(conn)
-
         content_type == "image/gif" ->
           conn
           |> put_status(301)
@@ -166,15 +162,6 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     end
   end
 
-  defp drop_static_param_and_redirect(conn) do
-    uri_without_static_param =
-      conn
-      |> current_url()
-      |> UriHelper.modify_uri_params(%{}, ["static"])
-
-    redirect(conn, external: uri_without_static_param)
-  end
-
   defp fallback_on_preview_error(conn, media_proxy_url) do
     redirect(conn, external: media_proxy_url)
   end

test/pleroma/config/transfer_task_test.exs

@@ -96,6 +96,29 @@ defmodule Pleroma.Config.TransferTaskTest do
     assert Application.get_env(:pleroma, Pleroma.Search.ParadeDB.Repo)[:pool_size] == 2
   end
 
+  test "skips settings with an unknown module key" do
+    now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
+
+    Repo.insert_all("config", [
+      %{
+        group: ":pleroma",
+        key: "Pleroma.Nowhere.Module",
+        value: :erlang.term_to_binary(enabled: true),
+        inserted_at: now,
+        updated_at: now
+      }
+    ])
+
+    insert(:config, key: :test_key, value: [live: 2])
+    on_exit(fn -> Application.delete_env(:pleroma, :test_key) end)
+
+    log = capture_log(fn -> assert :ok = TransferTask.load_and_update_env([], false) end)
+
+    assert log =~ "Pleroma.Nowhere.Module"
+    assert Application.get_env(:pleroma, :test_key) == [live: 2]
+    refute Application.get_env(:pleroma, :invalid_atom)
+  end
+
   describe "pleroma restart" do
     setup do
       on_exit(fn ->

test/pleroma/config_db_test.exs

@@ -50,6 +50,22 @@ defmodule Pleroma.ConfigDBTest do
     assert config[:pleroma][:instance] == [name: "Pleroma"]
   end
 
+  test "loads a row with an unknown module key as :invalid_atom" do
+    now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
+
+    Repo.insert_all("config", [
+      %{
+        group: ":pleroma",
+        key: "Pleroma.Nowhere.Module",
+        value: :erlang.term_to_binary(enabled: true),
+        inserted_at: now,
+        updated_at: now
+      }
+    ])
+
+    assert [%ConfigDB{group: :pleroma, key: :invalid_atom}] = Repo.all(ConfigDB)
+  end
+
   describe "update_or_create/1" do
     test "common" do
       config = insert(:config)

test/pleroma/web/activity_pub/transmogrifier/question_handling_test.exs

@@ -103,6 +103,25 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.QuestionHandlingTest do
     assert Enum.sort(object.data["oneOf"]) == Enum.sort(options)
   end
 
+  test "Question activity with a single, non-array tag" do
+    tag = %{
+      "href" => "https://gts.example/tags/caturday",
+      "name" => "#caturday",
+      "type" => "Hashtag"
+    }
+
+    data =
+      File.read!("test/fixtures/mastodon-question-activity.json")
+      |> Jason.decode!()
+      |> Kernel.put_in(["object", "tag"], tag)
+
+    {:ok, %Activity{local: false} = activity} = Transmogrifier.handle_incoming(data)
+    object = Object.normalize(activity, fetch: false)
+
+    assert tag in object.data["tag"]
+    assert Object.hashtags(object) == ["caturday"]
+  end
+
   test "Mastodon Question activity with custom emojis" do
     options = [
       %{

test/pleroma/web/admin_api/controllers/config_controller_test.exs

@@ -60,6 +60,28 @@ defmodule Pleroma.Web.AdminAPI.ConfigControllerTest do
       assert key2 == inspect(config2.key)
     end
 
+    test "does not crash on a setting with an unknown module key", %{conn: conn} do
+      now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
+
+      Pleroma.Repo.insert_all("config", [
+        %{
+          group: ":pleroma",
+          key: "Pleroma.Nowhere.Module",
+          value: :erlang.term_to_binary(enabled: true),
+          inserted_at: now,
+          updated_at: now
+        }
+      ])
+
+      capture_log(fn ->
+        for path <- ["/api/pleroma/admin/config", "/api/pleroma/admin/config?only_db=true"] do
+          conn
+          |> get(path)
+          |> json_response_and_validate_schema(200)
+        end
+      end)
+    end
+
     test "db is added to settings that are in db", %{conn: conn} do
       _config = insert(:config, key: ":instance", value: [name: "Some name"])
 

test/pleroma/web/api_spec/cast_and_validate_test.exs

@@ -0,0 +1,24 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2022 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.ApiSpec.CastAndValidateTest do
+  use Pleroma.Web.ConnCase, async: false
+
+  describe "permissive mode" do
+    setup do: clear_config([Pleroma.Web.ApiSpec.CastAndValidate, :strict], false)
+
+    test "drops several unexpected query params", %{conn: conn} do
+      assert conn
+             |> get("/api/v1/timelines/public?foo=1&bar=2&baz=3&local=true")
+             |> json_response_and_validate_schema(200)
+    end
+
+    test "still rejects an invalid known param", %{conn: conn} do
+      assert %{"error" => _} =
+               conn
+               |> get("/api/v1/timelines/public?foo=1&bar=2&limit=lots")
+               |> json_response(400)
+    end
+  end
+end

test/pleroma/web/mastodon_api/controllers/status_controller_test.exs

@@ -3093,6 +3093,30 @@ defmodule Pleroma.Web.MastodonAPI.StatusControllerTest do
            } = response
   end
 
+  test "context shows local-only replies to local users only" do
+    poster = insert(:user)
+    replier = insert(:user)
+    %{conn: conn} = oauth_access(["read:statuses"])
+    %{conn: remote_conn} = oauth_access(["read:statuses"], user: insert(:user, local: false))
+
+    {:ok, %{id: id1}} = CommonAPI.post(poster, %{status: "1"})
+
+    {:ok, %{id: id2}} =
+      CommonAPI.post(replier, %{status: "2", visibility: "local", in_reply_to_status_id: id1})
+
+    assert %{"descendants" => [%{"id" => ^id2}]} =
+             conn
+             |> get("/api/v1/statuses/#{id1}/context")
+             |> json_response_and_validate_schema(:ok)
+
+    for conn <- [build_conn(), remote_conn] do
+      assert %{"descendants" => []} =
+               conn
+               |> get("/api/v1/statuses/#{id1}/context")
+               |> json_response_and_validate_schema(:ok)
+    end
+  end
+
   test "context doesn't leak priv posts" do
     %{user: user, conn: conn} = oauth_access(["read:statuses"])
     stranger = insert(:user)

test/pleroma/web/media_proxy/media_proxy_controller_test.exs

@@ -252,22 +252,27 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyControllerTest do
       assert redirected_to(response, 301) == media_proxy_url
     end
 
-    test "with `static` param and non-GIF image preview requested, " <>
-           "redirects to media preview proxy URI without `static` param",
+    test "with `static` param and non-GIF image preview requested, responds with a preview",
          %{
            conn: conn,
            url: url,
            media_proxy_url: media_proxy_url
          } do
+      assert_dependencies_installed()
+
       Tesla.Mock.mock(fn
         %{method: :head, url: ^media_proxy_url} ->
           %Tesla.Env{status: 200, body: "", headers: [{"content-type", "image/jpeg"}]}
+
+        %{method: :get, url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: File.read!("test/fixtures/image.jpg")}
       end)
 
       response = get(conn, url <> "?static=true")
 
-      assert response.status == 302
-      assert redirected_to(response) == url
+      assert response.status == 200
+      assert Conn.get_resp_header(response, "content-type") == ["image/jpeg"]
+      assert response.resp_body != ""
     end
 
     test "with :min_content_length setting not matched by Content-Length header, " <>