From 063e6b9841ec72c7e89339c54581d199fa31e675 Mon Sep 17 00:00:00 2001 From: lain Date: Tue, 9 Jun 2020 10:53:40 +0200 Subject: [PATCH 01/16] StatusController: Correctly paginate favorites. Favorites were paginating wrongly, because the pagination headers where using the id of the id of the `Create` activity, while the ordering was by the id of the `Like` activity. This isn't easy to notice in most cases, as they usually have a similar order because people tend to favorite posts as they come in. This commit adds a way to give different pagination ids to the pagination helper, so we can paginate correctly in cases like this. --- lib/pleroma/activity.ex | 4 ++ lib/pleroma/web/activity_pub/activity_pub.ex | 5 +- .../api_spec/operations/status_operation.ex | 3 +- lib/pleroma/web/controller_helper.ex | 56 +++++++++++-------- .../controllers/status_controller_test.exs | 43 ++++++++++++-- 5 files changed, 79 insertions(+), 32 deletions(-) diff --git a/lib/pleroma/activity.ex b/lib/pleroma/activity.ex index 6213d0eb7..f800447fd 100644 --- a/lib/pleroma/activity.ex +++ b/lib/pleroma/activity.ex @@ -41,6 +41,10 @@ defmodule Pleroma.Activity do field(:recipients, {:array, :string}, default: []) field(:thread_muted?, :boolean, virtual: true) + # A field that can be used if you need to join some kind of other + # id to order / paginate this field by + field(:pagination_id, :string, virtual: true) + # This is a fake relation, # do not use outside of with_preloaded_user_actor/with_joined_user_actor has_one(:user_actor, User, on_delete: :nothing, foreign_key: :id) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index eb73c95fe..cc883ccce 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1138,12 +1138,11 @@ def fetch_favourites(user, params \\ %{}, pagination \\ :keyset) do |> Activity.Queries.by_type("Like") |> Activity.with_joined_object() |> Object.with_joined_activity() - |> select([_like, object, activity], %{activity | object: object}) + |> select([like, object, activity], %{activity | object: object, pagination_id: like.id}) |> order_by([like, _, _], desc_nulls_last: like.id) |> Pagination.fetch_paginated( Map.merge(params, %{skip_order: true}), - pagination, - :object_activity + pagination ) end diff --git a/lib/pleroma/web/api_spec/operations/status_operation.ex b/lib/pleroma/web/api_spec/operations/status_operation.ex index ca9db01e5..0b7fad793 100644 --- a/lib/pleroma/web/api_spec/operations/status_operation.ex +++ b/lib/pleroma/web/api_spec/operations/status_operation.ex @@ -333,7 +333,8 @@ def favourites_operation do %Operation{ tags: ["Statuses"], summary: "Favourited statuses", - description: "Statuses the user has favourited", + description: + "Statuses the user has favourited. Please note that you have to use the link headers to paginate this. You can not build the query parameters yourself.", operationId: "StatusController.favourites", parameters: pagination_params(), security: [%{"oAuth" => ["read:favourites"]}], diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index 5d67d75b5..5e33e0810 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -57,35 +57,45 @@ def add_link_headers(conn, activities, extra_params) do end end + defp build_pagination_fields(conn, min_id, max_id, extra_params) do + params = + conn.params + |> Map.drop(Map.keys(conn.path_params)) + |> Map.merge(extra_params) + |> Map.drop(Pagination.page_keys() -- ["limit", "order"]) + + fields = %{ + "next" => current_url(conn, Map.put(params, :max_id, max_id)), + "prev" => current_url(conn, Map.put(params, :min_id, min_id)) + } + + # Generating an `id` without already present pagination keys would + # need a query-restriction with an `q.id >= ^id` or `q.id <= ^id` + # instead of the `q.id > ^min_id` and `q.id < ^max_id`. + # This is because we only have ids present inside of the page, while + # `min_id`, `since_id` and `max_id` requires to know one outside of it. + if Map.take(conn.params, Pagination.page_keys() -- ["limit", "order"]) != [] do + Map.put(fields, "id", current_url(conn, conn.params)) + else + fields + end + end + def get_pagination_fields(conn, activities, extra_params \\ %{}) do case List.last(activities) do - %{id: max_id} -> - params = - conn.params - |> Map.drop(Map.keys(conn.path_params)) - |> Map.merge(extra_params) - |> Map.drop(Pagination.page_keys() -- ["limit", "order"]) - - min_id = + %{pagination_id: max_id} when not is_nil(max_id) -> + %{pagination_id: min_id} = activities |> List.first() - |> Map.get(:id) - fields = %{ - "next" => current_url(conn, Map.put(params, :max_id, max_id)), - "prev" => current_url(conn, Map.put(params, :min_id, min_id)) - } + build_pagination_fields(conn, min_id, max_id, extra_params) - # Generating an `id` without already present pagination keys would - # need a query-restriction with an `q.id >= ^id` or `q.id <= ^id` - # instead of the `q.id > ^min_id` and `q.id < ^max_id`. - # This is because we only have ids present inside of the page, while - # `min_id`, `since_id` and `max_id` requires to know one outside of it. - if Map.take(conn.params, Pagination.page_keys() -- ["limit", "order"]) != [] do - Map.put(fields, "id", current_url(conn, conn.params)) - else - fields - end + %{id: max_id} -> + %{id: min_id} = + activities + |> List.first() + + build_pagination_fields(conn, min_id, max_id, extra_params) _ -> %{} diff --git a/test/web/mastodon_api/controllers/status_controller_test.exs b/test/web/mastodon_api/controllers/status_controller_test.exs index 700c82e4f..648e6f2ce 100644 --- a/test/web/mastodon_api/controllers/status_controller_test.exs +++ b/test/web/mastodon_api/controllers/status_controller_test.exs @@ -1541,14 +1541,49 @@ test "context" do } = response end + test "favorites paginate correctly" do + %{user: user, conn: conn} = oauth_access(["read:favourites"]) + other_user = insert(:user) + {:ok, first_post} = CommonAPI.post(other_user, %{status: "bla"}) + {:ok, second_post} = CommonAPI.post(other_user, %{status: "bla"}) + {:ok, third_post} = CommonAPI.post(other_user, %{status: "bla"}) + + {:ok, _first_favorite} = CommonAPI.favorite(user, third_post.id) + {:ok, _second_favorite} = CommonAPI.favorite(user, first_post.id) + {:ok, third_favorite} = CommonAPI.favorite(user, second_post.id) + + result = + conn + |> get("/api/v1/favourites?limit=1") + + assert [%{"id" => post_id}] = json_response_and_validate_schema(result, 200) + assert post_id == second_post.id + + # Using the header for pagination works correctly + [next, _] = get_resp_header(result, "link") |> hd() |> String.split(", ") + [_, max_id] = Regex.run(~r/max_id=(.*)>;/, next) + + assert max_id == third_favorite.id + + result = + conn + |> get("/api/v1/favourites?max_id=#{max_id}") + + assert [%{"id" => first_post_id}, %{"id" => third_post_id}] = + json_response_and_validate_schema(result, 200) + + assert first_post_id == first_post.id + assert third_post_id == third_post.id + end + test "returns the favorites of a user" do %{user: user, conn: conn} = oauth_access(["read:favourites"]) other_user = insert(:user) {:ok, _} = CommonAPI.post(other_user, %{status: "bla"}) - {:ok, activity} = CommonAPI.post(other_user, %{status: "traps are happy"}) + {:ok, activity} = CommonAPI.post(other_user, %{status: "trees are happy"}) - {:ok, _} = CommonAPI.favorite(user, activity.id) + {:ok, last_like} = CommonAPI.favorite(user, activity.id) first_conn = get(conn, "/api/v1/favourites") @@ -1566,9 +1601,7 @@ test "returns the favorites of a user" do {:ok, _} = CommonAPI.favorite(user, second_activity.id) - last_like = status["id"] - - second_conn = get(conn, "/api/v1/favourites?since_id=#{last_like}") + second_conn = get(conn, "/api/v1/favourites?since_id=#{last_like.id}") assert [second_status] = json_response_and_validate_schema(second_conn, 200) assert second_status["id"] == to_string(second_activity.id) From c4f267b3bef90dcac21b7db2a91f86d3ba5dc7c2 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 08:02:26 +0000 Subject: [PATCH 02/16] Apply suggestion to lib/pleroma/web/controller_helper.ex --- lib/pleroma/web/controller_helper.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index 5e33e0810..6cb19d539 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -57,6 +57,7 @@ def add_link_headers(conn, activities, extra_params) do end end + @id_keys Pagination.page_keys() -- ["limit", "order"] defp build_pagination_fields(conn, min_id, max_id, extra_params) do params = conn.params From be7c322865b2b7aa1c8c25147cc598b6362ab187 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 08:02:35 +0000 Subject: [PATCH 03/16] Apply suggestion to lib/pleroma/web/controller_helper.ex --- lib/pleroma/web/controller_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index 6cb19d539..b7971e940 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -63,7 +63,7 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do conn.params |> Map.drop(Map.keys(conn.path_params)) |> Map.merge(extra_params) - |> Map.drop(Pagination.page_keys() -- ["limit", "order"]) + |> Map.drop(@id_keys) fields = %{ "next" => current_url(conn, Map.put(params, :max_id, max_id)), From b4c50be9df701dc9faf0a25f776f631d2175c99f Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 08:12:29 +0000 Subject: [PATCH 04/16] Apply suggestion to lib/pleroma/web/controller_helper.ex --- lib/pleroma/web/controller_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index b7971e940..ab6e6c61a 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -75,7 +75,7 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do # instead of the `q.id > ^min_id` and `q.id < ^max_id`. # This is because we only have ids present inside of the page, while # `min_id`, `since_id` and `max_id` requires to know one outside of it. - if Map.take(conn.params, Pagination.page_keys() -- ["limit", "order"]) != [] do + if Map.take(conn.params, @id_keys) != %{} do Map.put(fields, "id", current_url(conn, conn.params)) else fields From 86fec45f40dfa45cc89eddc6dcc7799e89d6f461 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 11:09:45 +0200 Subject: [PATCH 05/16] ControllerHelper: Fix wrong comparison. --- lib/pleroma/web/controller_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index ab6e6c61a..88f2cc6f1 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -75,7 +75,7 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do # instead of the `q.id > ^min_id` and `q.id < ^max_id`. # This is because we only have ids present inside of the page, while # `min_id`, `since_id` and `max_id` requires to know one outside of it. - if Map.take(conn.params, @id_keys) != %{} do + if Map.take(conn.params, @id_keys) != [] do Map.put(fields, "id", current_url(conn, conn.params)) else fields From 9e411372d0b7ae286941063956305c0a2eae46a6 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 12:10:09 +0200 Subject: [PATCH 06/16] ActivityPub: Don't show announces of your own objects in timeline. --- lib/pleroma/web/activity_pub/activity_pub.ex | 40 ++++++++++--------- .../controllers/timeline_controller.ex | 1 + test/web/activity_pub/activity_pub_test.exs | 24 +++++++++++ 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index eb73c95fe..4182275bc 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -31,25 +31,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do require Logger require Pleroma.Constants - # For Announce activities, we filter the recipients based on following status for any actors - # that match actual users. See issue #164 for more information about why this is necessary. - defp get_recipients(%{"type" => "Announce"} = data) do - to = Map.get(data, "to", []) - cc = Map.get(data, "cc", []) - bcc = Map.get(data, "bcc", []) - actor = User.get_cached_by_ap_id(data["actor"]) - - recipients = - Enum.filter(Enum.concat([to, cc, bcc]), fn recipient -> - case User.get_cached_by_ap_id(recipient) do - nil -> true - user -> User.following?(user, actor) - end - end) - - {recipients, to, cc} - end - defp get_recipients(%{"type" => "Create"} = data) do to = Map.get(data, "to", []) cc = Map.get(data, "cc", []) @@ -702,6 +683,26 @@ defp user_activities_recipients(%{reading_user: reading_user}) do end end + defp restrict_announce_object_actor(_query, %{announce_filtering_user: _, skip_preload: true}) do + raise "Can't use the child object without preloading!" + end + + defp restrict_announce_object_actor(query, %{announce_filtering_user: %{ap_id: actor}}) do + from( + [activity, object] in query, + where: + fragment( + "?->>'type' != ? or ?->>'actor' != ?", + activity.data, + "Announce", + object.data, + ^actor + ) + ) + end + + defp restrict_announce_object_actor(query, _), do: query + defp restrict_since(query, %{since_id: ""}), do: query defp restrict_since(query, %{since_id: since_id}) do @@ -1113,6 +1114,7 @@ def fetch_activities_query(recipients, opts \\ %{}) do |> restrict_pinned(opts) |> restrict_muted_reblogs(restrict_muted_reblogs_opts) |> restrict_instance(opts) + |> restrict_announce_object_actor(opts) |> Activity.restrict_deactivated_users() |> exclude_poll_votes(opts) |> exclude_invisible_actors(opts) diff --git a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex index 9270ca267..4bdd46d7e 100644 --- a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex @@ -48,6 +48,7 @@ def home(%{assigns: %{user: user}} = conn, params) do |> Map.put(:blocking_user, user) |> Map.put(:muting_user, user) |> Map.put(:reply_filtering_user, user) + |> Map.put(:announce_filtering_user, user) |> Map.put(:user, user) activities = diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 2f65dfc8e..e17cc4ab1 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1643,6 +1643,30 @@ test "home timeline with reply_visibility `self`", %{ assert Enum.all?(visible_ids, &(&1 in activities_ids)) end + + test "filtering out announces where the user is the actor of the announced message" do + user = insert(:user) + other_user = insert(:user) + third_user = insert(:user) + User.follow(user, other_user) + + {:ok, post} = CommonAPI.post(user, %{status: "yo"}) + {:ok, other_post} = CommonAPI.post(third_user, %{status: "yo"}) + {:ok, _announce} = CommonAPI.repeat(post.id, other_user) + {:ok, _announce} = CommonAPI.repeat(post.id, third_user) + {:ok, announce} = CommonAPI.repeat(other_post.id, other_user) + + params = %{ + type: ["Announce"], + announce_filtering_user: user + } + + [result] = + [user.ap_id | User.following(user)] + |> ActivityPub.fetch_activities(params) + + assert result.id == announce.id + end end describe "replies filtering with private messages" do From 600e2ea07396489325e06dee3e8432288e0e13c2 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 12:15:56 +0200 Subject: [PATCH 07/16] ActivityPubTest: Make test easier to understand. --- test/web/activity_pub/activity_pub_test.exs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index e17cc4ab1..6cd3b8d1b 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1656,6 +1656,16 @@ test "filtering out announces where the user is the actor of the announced messa {:ok, _announce} = CommonAPI.repeat(post.id, third_user) {:ok, announce} = CommonAPI.repeat(other_post.id, other_user) + params = %{ + type: ["Announce"] + } + + results = + [user.ap_id | User.following(user)] + |> ActivityPub.fetch_activities(params) + + assert length(results) == 3 + params = %{ type: ["Announce"], announce_filtering_user: user From 570123ae21382c7e78b99442e3c025b0e66b8f6d Mon Sep 17 00:00:00 2001 From: Thibaut Girka Date: Sun, 7 Jun 2020 18:21:11 +0200 Subject: [PATCH 08/16] Add test --- test/web/activity_pub/activity_pub_test.exs | 35 ++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 6cd3b8d1b..72d3f3dfa 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -574,7 +574,7 @@ test "doesn't return transitive interactions concerning blocked users" do refute Enum.member?(activities, activity_four) end - test "doesn't return announce activities concerning blocked users" do + test "doesn't return announce activities with blocked users in 'to'" do blocker = insert(:user) blockee = insert(:user) friend = insert(:user) @@ -596,6 +596,39 @@ test "doesn't return announce activities concerning blocked users" do refute Enum.member?(activities, activity_three.id) end + test "doesn't return announce activities with blocked users in 'cc'" do + blocker = insert(:user) + blockee = insert(:user) + friend = insert(:user) + + {:ok, _user_relationship} = User.block(blocker, blockee) + + {:ok, activity_one} = CommonAPI.post(friend, %{status: "hey!"}) + + {:ok, activity_two} = CommonAPI.post(blockee, %{status: "hey! @#{friend.nickname}"}) + + assert object = Pleroma.Object.normalize(activity_two) + + data = %{ + "actor" => friend.ap_id, + "object" => object.data["id"], + "context" => object.data["context"], + "type" => "Announce", + "to" => ["https://www.w3.org/ns/activitystreams#Public"], + "cc" => [blockee.ap_id] + } + + assert {:ok, activity_three} = ActivityPub.insert(data) + + activities = + ActivityPub.fetch_activities([], %{"blocking_user" => blocker}) + |> Enum.map(fn act -> act.id end) + + assert Enum.member?(activities, activity_one.id) + refute Enum.member?(activities, activity_two.id) + refute Enum.member?(activities, activity_three.id) + end + test "doesn't return activities from blocked domains" do domain = "dogwhistle.zone" domain_user = insert(:user, %{ap_id: "https://#{domain}/@pundit"}) From 5d87405b51efe9f99fea669090a5914db22ca9ed Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 16:55:30 +0200 Subject: [PATCH 09/16] ActivityPubTest: Update test for atomized parameters. --- test/web/activity_pub/activity_pub_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 72d3f3dfa..b239b812f 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -621,7 +621,7 @@ test "doesn't return announce activities with blocked users in 'cc'" do assert {:ok, activity_three} = ActivityPub.insert(data) activities = - ActivityPub.fetch_activities([], %{"blocking_user" => blocker}) + ActivityPub.fetch_activities([], %{blocking_user: blocker}) |> Enum.map(fn act -> act.id end) assert Enum.member?(activities, activity_one.id) From 5e44e9d69871f2e5805a8dddcfce43ae713eb52d Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 10 Jun 2020 18:56:46 +0000 Subject: [PATCH 10/16] Apply suggestion to lib/pleroma/web/controller_helper.ex --- lib/pleroma/web/controller_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index 88f2cc6f1..a5eb3e9e0 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -76,7 +76,7 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do # This is because we only have ids present inside of the page, while # `min_id`, `since_id` and `max_id` requires to know one outside of it. if Map.take(conn.params, @id_keys) != [] do - Map.put(fields, "id", current_url(conn, conn.params)) + Map.put(fields, "id", current_url(conn)) else fields end From 122328b93a708e396b5c0cd1930a4b759e7b7db6 Mon Sep 17 00:00:00 2001 From: normandy Date: Fri, 12 Jun 2020 01:41:09 +0000 Subject: [PATCH 11/16] Update pleroma.nginx to support TLSv1.3 Based on SSL config from https://ssl-config.mozilla.org/ --- installation/pleroma.nginx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/installation/pleroma.nginx b/installation/pleroma.nginx index 688be3e71..d301ca615 100644 --- a/installation/pleroma.nginx +++ b/installation/pleroma.nginx @@ -37,18 +37,17 @@ server { listen 443 ssl http2; listen [::]:443 ssl http2; - ssl_session_timeout 5m; + ssl_session_timeout 1d; + ssl_session_cache shared:MozSSL:10m; # about 40000 sessions + ssl_session_tickets off; ssl_trusted_certificate /etc/letsencrypt/live/example.tld/chain.pem; ssl_certificate /etc/letsencrypt/live/example.tld/fullchain.pem; ssl_certificate_key /etc/letsencrypt/live/example.tld/privkey.pem; - # Add TLSv1.0 to support older devices - ssl_protocols TLSv1.2; - # Uncomment line below if you want to support older devices (Before Android 4.4.2, IE 8, etc.) - # ssl_ciphers "HIGH:!aNULL:!MD5 or HIGH:!aNULL:!MD5:!3DES"; + ssl_protocols TLSv1.2 TLSv1.3; ssl_ciphers "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:!aNULL:!eNULL:!EXPORT:!DES:!MD5:!PSK:!RC4"; - ssl_prefer_server_ciphers on; + ssl_prefer_server_ciphers off; # In case of an old server with an OpenSSL version of 1.0.2 or below, # leave only prime256v1 or comment out the following line. ssl_ecdh_curve X25519:prime256v1:secp384r1:secp521r1; From 520367d6fd8a268e0bc8c145a46aca46a62e8b66 Mon Sep 17 00:00:00 2001 From: Egor Kislitsyn Date: Tue, 9 Jun 2020 21:49:24 +0400 Subject: [PATCH 12/16] Fix atom leak in Rich Media Parser --- .../web/mastodon_api/views/status_view.ex | 14 ++-- lib/pleroma/web/rich_media/helpers.ex | 6 +- lib/pleroma/web/rich_media/parser.ex | 12 +-- .../rich_media/parsers/meta_tags_parser.ex | 8 +- .../web/rich_media/parsers/oembed_parser.ex | 18 ++--- test/web/rich_media/parser_test.exs | 75 ++++++++++--------- .../rich_media/parsers/twitter_card_test.exs | 60 +++++++-------- 7 files changed, 91 insertions(+), 102 deletions(-) diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index 8e3715093..2c49bedb3 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -377,8 +377,8 @@ def render("card.json", %{rich_media: rich_media, page_url: page_url}) do page_url_data = URI.parse(page_url) page_url_data = - if rich_media[:url] != nil do - URI.merge(page_url_data, URI.parse(rich_media[:url])) + if is_binary(rich_media["url"]) do + URI.merge(page_url_data, URI.parse(rich_media["url"])) else page_url_data end @@ -386,11 +386,9 @@ def render("card.json", %{rich_media: rich_media, page_url: page_url}) do page_url = page_url_data |> to_string image_url = - if rich_media[:image] != nil do - URI.merge(page_url_data, URI.parse(rich_media[:image])) + if is_binary(rich_media["image"]) do + URI.merge(page_url_data, URI.parse(rich_media["image"])) |> to_string - else - nil end %{ @@ -399,8 +397,8 @@ def render("card.json", %{rich_media: rich_media, page_url: page_url}) do provider_url: page_url_data.scheme <> "://" <> page_url_data.host, url: page_url, image: image_url |> MediaProxy.url(), - title: rich_media[:title] || "", - description: rich_media[:description] || "", + title: rich_media["title"] || "", + description: rich_media["description"] || "", pleroma: %{ opengraph: rich_media } diff --git a/lib/pleroma/web/rich_media/helpers.ex b/lib/pleroma/web/rich_media/helpers.ex index 9d3d7f978..1729141e9 100644 --- a/lib/pleroma/web/rich_media/helpers.ex +++ b/lib/pleroma/web/rich_media/helpers.ex @@ -9,7 +9,7 @@ defmodule Pleroma.Web.RichMedia.Helpers do alias Pleroma.Object alias Pleroma.Web.RichMedia.Parser - @spec validate_page_url(any()) :: :ok | :error + @spec validate_page_url(URI.t() | binary()) :: :ok | :error defp validate_page_url(page_url) when is_binary(page_url) do validate_tld = Application.get_env(:auto_linker, :opts)[:validate_tld] @@ -18,8 +18,8 @@ defp validate_page_url(page_url) when is_binary(page_url) do |> parse_uri(page_url) end - defp validate_page_url(%URI{host: host, scheme: scheme, authority: authority}) - when scheme == "https" and not is_nil(authority) do + defp validate_page_url(%URI{host: host, scheme: "https", authority: authority}) + when is_binary(authority) do cond do host in Config.get([:rich_media, :ignore_hosts], []) -> :error diff --git a/lib/pleroma/web/rich_media/parser.ex b/lib/pleroma/web/rich_media/parser.ex index 40980def8..d9b5068b1 100644 --- a/lib/pleroma/web/rich_media/parser.ex +++ b/lib/pleroma/web/rich_media/parser.ex @@ -91,7 +91,7 @@ defp parse_url(url) do html |> parse_html() |> maybe_parse() - |> Map.put(:url, url) + |> Map.put("url", url) |> clean_parsed_data() |> check_parsed_data() rescue @@ -111,8 +111,8 @@ defp maybe_parse(html) do end) end - defp check_parsed_data(%{title: title} = data) - when is_binary(title) and byte_size(title) > 0 do + defp check_parsed_data(%{"title" => title} = data) + when is_binary(title) and title != "" do {:ok, data} end @@ -123,11 +123,7 @@ defp check_parsed_data(data) do defp clean_parsed_data(data) do data |> Enum.reject(fn {key, val} -> - with {:ok, _} <- Jason.encode(%{key => val}) do - false - else - _ -> true - end + not match?({:ok, _}, Jason.encode(%{key => val})) end) |> Map.new() end diff --git a/lib/pleroma/web/rich_media/parsers/meta_tags_parser.ex b/lib/pleroma/web/rich_media/parsers/meta_tags_parser.ex index ae0f36702..2762b5902 100644 --- a/lib/pleroma/web/rich_media/parsers/meta_tags_parser.ex +++ b/lib/pleroma/web/rich_media/parsers/meta_tags_parser.ex @@ -29,19 +29,19 @@ defp normalize_attributes(html_node, prefix, key_name, value_name) do {_tag, attributes, _children} = html_node data = - Enum.into(attributes, %{}, fn {name, value} -> + Map.new(attributes, fn {name, value} -> {name, String.trim_leading(value, "#{prefix}:")} end) - %{String.to_atom(data[key_name]) => data[value_name]} + %{data[key_name] => data[value_name]} end - defp maybe_put_title(%{title: _} = meta, _), do: meta + defp maybe_put_title(%{"title" => _} = meta, _), do: meta defp maybe_put_title(meta, html) when meta != %{} do case get_page_title(html) do "" -> meta - title -> Map.put_new(meta, :title, title) + title -> Map.put_new(meta, "title", title) end end diff --git a/lib/pleroma/web/rich_media/parsers/oembed_parser.ex b/lib/pleroma/web/rich_media/parsers/oembed_parser.ex index 8f32bf91b..db8ccf15d 100644 --- a/lib/pleroma/web/rich_media/parsers/oembed_parser.ex +++ b/lib/pleroma/web/rich_media/parsers/oembed_parser.ex @@ -5,7 +5,7 @@ defmodule Pleroma.Web.RichMedia.Parsers.OEmbed do def parse(html, _data) do with elements = [_ | _] <- get_discovery_data(html), - {:ok, oembed_url} <- get_oembed_url(elements), + oembed_url when is_binary(oembed_url) <- get_oembed_url(elements), {:ok, oembed_data} <- get_oembed_data(oembed_url) do {:ok, oembed_data} else @@ -17,19 +17,13 @@ defp get_discovery_data(html) do html |> Floki.find("link[type='application/json+oembed']") end - defp get_oembed_url(nodes) do - {"link", attributes, _children} = nodes |> hd() - - {:ok, Enum.into(attributes, %{})["href"]} + defp get_oembed_url([{"link", attributes, _children} | _]) do + Enum.find_value(attributes, fn {k, v} -> if k == "href", do: v end) end defp get_oembed_data(url) do - {:ok, %Tesla.Env{body: json}} = Pleroma.HTTP.get(url, [], adapter: [pool: :media]) - - {:ok, data} = Jason.decode(json) - - data = data |> Map.new(fn {k, v} -> {String.to_atom(k), v} end) - - {:ok, data} + with {:ok, %Tesla.Env{body: json}} <- Pleroma.HTTP.get(url, [], adapter: [pool: :media]) do + Jason.decode(json) + end end end diff --git a/test/web/rich_media/parser_test.exs b/test/web/rich_media/parser_test.exs index e54a13bc8..420a612c6 100644 --- a/test/web/rich_media/parser_test.exs +++ b/test/web/rich_media/parser_test.exs @@ -60,19 +60,19 @@ test "returns error when no metadata present" do test "doesn't just add a title" do assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/non-ogp") == {:error, - "Found metadata was invalid or incomplete: %{url: \"http://example.com/non-ogp\"}"} + "Found metadata was invalid or incomplete: %{\"url\" => \"http://example.com/non-ogp\"}"} end test "parses ogp" do assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/ogp") == {:ok, %{ - image: "http://ia.media-imdb.com/images/rock.jpg", - title: "The Rock", - description: + "image" => "http://ia.media-imdb.com/images/rock.jpg", + "title" => "The Rock", + "description" => "Directed by Michael Bay. With Sean Connery, Nicolas Cage, Ed Harris, John Spencer.", - type: "video.movie", - url: "http://example.com/ogp" + "type" => "video.movie", + "url" => "http://example.com/ogp" }} end @@ -80,12 +80,12 @@ test "falls back to when ogp:title is missing" do assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/ogp-missing-title") == {:ok, %{ - image: "http://ia.media-imdb.com/images/rock.jpg", - title: "The Rock (1996)", - description: + "image" => "http://ia.media-imdb.com/images/rock.jpg", + "title" => "The Rock (1996)", + "description" => "Directed by Michael Bay. With Sean Connery, Nicolas Cage, Ed Harris, John Spencer.", - type: "video.movie", - url: "http://example.com/ogp-missing-title" + "type" => "video.movie", + "url" => "http://example.com/ogp-missing-title" }} end @@ -93,12 +93,12 @@ test "parses twitter card" do assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/twitter-card") == {:ok, %{ - card: "summary", - site: "@flickr", - image: "https://farm6.staticflickr.com/5510/14338202952_93595258ff_z.jpg", - title: "Small Island Developing States Photo Submission", - description: "View the album on Flickr.", - url: "http://example.com/twitter-card" + "card" => "summary", + "site" => "@flickr", + "image" => "https://farm6.staticflickr.com/5510/14338202952_93595258ff_z.jpg", + "title" => "Small Island Developing States Photo Submission", + "description" => "View the album on Flickr.", + "url" => "http://example.com/twitter-card" }} end @@ -106,27 +106,28 @@ test "parses OEmbed" do assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/oembed") == {:ok, %{ - author_name: "‮‭‬bees‬", - author_url: "https://www.flickr.com/photos/bees/", - cache_age: 3600, - flickr_type: "photo", - height: "768", - html: + "author_name" => "‮‭‬bees‬", + "author_url" => "https://www.flickr.com/photos/bees/", + "cache_age" => 3600, + "flickr_type" => "photo", + "height" => "768", + "html" => "<a data-flickr-embed=\"true\" href=\"https://www.flickr.com/photos/bees/2362225867/\" title=\"Bacon Lollys by ‮‭‬bees‬, on Flickr\"><img src=\"https://farm4.staticflickr.com/3040/2362225867_4a87ab8baf_b.jpg\" width=\"1024\" height=\"768\" alt=\"Bacon Lollys\"></a><script async src=\"https://embedr.flickr.com/assets/client-code.js\" charset=\"utf-8\"></script>", - license: "All Rights Reserved", - license_id: 0, - provider_name: "Flickr", - provider_url: "https://www.flickr.com/", - thumbnail_height: 150, - thumbnail_url: "https://farm4.staticflickr.com/3040/2362225867_4a87ab8baf_q.jpg", - thumbnail_width: 150, - title: "Bacon Lollys", - type: "photo", - url: "http://example.com/oembed", - version: "1.0", - web_page: "https://www.flickr.com/photos/bees/2362225867/", - web_page_short_url: "https://flic.kr/p/4AK2sc", - width: "1024" + "license" => "All Rights Reserved", + "license_id" => 0, + "provider_name" => "Flickr", + "provider_url" => "https://www.flickr.com/", + "thumbnail_height" => 150, + "thumbnail_url" => + "https://farm4.staticflickr.com/3040/2362225867_4a87ab8baf_q.jpg", + "thumbnail_width" => 150, + "title" => "Bacon Lollys", + "type" => "photo", + "url" => "http://example.com/oembed", + "version" => "1.0", + "web_page" => "https://www.flickr.com/photos/bees/2362225867/", + "web_page_short_url" => "https://flic.kr/p/4AK2sc", + "width" => "1024" }} end diff --git a/test/web/rich_media/parsers/twitter_card_test.exs b/test/web/rich_media/parsers/twitter_card_test.exs index 87c767c15..847623535 100644 --- a/test/web/rich_media/parsers/twitter_card_test.exs +++ b/test/web/rich_media/parsers/twitter_card_test.exs @@ -19,11 +19,11 @@ test "parses twitter card with only name attributes" do assert TwitterCard.parse(html, %{}) == {:ok, %{ - "app:id:googleplay": "com.nytimes.android", - "app:name:googleplay": "NYTimes", - "app:url:googleplay": "nytimes://reader/id/100000006583622", - site: nil, - title: + "app:id:googleplay" => "com.nytimes.android", + "app:name:googleplay" => "NYTimes", + "app:url:googleplay" => "nytimes://reader/id/100000006583622", + "site" => nil, + "title" => "She Was Arrested at 14. Then Her Photo Went to a Facial Recognition Database. - The New York Times" }} end @@ -36,15 +36,15 @@ test "parses twitter card with only property attributes" do assert TwitterCard.parse(html, %{}) == {:ok, %{ - card: "summary_large_image", - description: + "card" => "summary_large_image", + "description" => "With little oversight, the N.Y.P.D. has been using powerful surveillance technology on photos of children and teenagers.", - image: + "image" => "https://static01.nyt.com/images/2019/08/01/nyregion/01nypd-juveniles-promo/01nypd-juveniles-promo-videoSixteenByNineJumbo1600.jpg", - "image:alt": "", - title: + "image:alt" => "", + "title" => "She Was Arrested at 14. Then Her Photo Went to a Facial Recognition Database.", - url: + "url" => "https://www.nytimes.com/2019/08/01/nyregion/nypd-facial-recognition-children-teenagers.html" }} end @@ -57,19 +57,19 @@ test "parses twitter card with name & property attributes" do assert TwitterCard.parse(html, %{}) == {:ok, %{ - "app:id:googleplay": "com.nytimes.android", - "app:name:googleplay": "NYTimes", - "app:url:googleplay": "nytimes://reader/id/100000006583622", - card: "summary_large_image", - description: + "app:id:googleplay" => "com.nytimes.android", + "app:name:googleplay" => "NYTimes", + "app:url:googleplay" => "nytimes://reader/id/100000006583622", + "card" => "summary_large_image", + "description" => "With little oversight, the N.Y.P.D. has been using powerful surveillance technology on photos of children and teenagers.", - image: + "image" => "https://static01.nyt.com/images/2019/08/01/nyregion/01nypd-juveniles-promo/01nypd-juveniles-promo-videoSixteenByNineJumbo1600.jpg", - "image:alt": "", - site: nil, - title: + "image:alt" => "", + "site" => nil, + "title" => "She Was Arrested at 14. Then Her Photo Went to a Facial Recognition Database.", - url: + "url" => "https://www.nytimes.com/2019/08/01/nyregion/nypd-facial-recognition-children-teenagers.html" }} end @@ -86,11 +86,11 @@ test "respect only first title tag on the page" do assert TwitterCard.parse(html, %{}) == {:ok, %{ - site: "@atlasobscura", - title: + "site" => "@atlasobscura", + "title" => "The Missing Grave of Margaret Corbin, Revolutionary War Veteran - Atlas Obscura", - card: "summary_large_image", - image: image_path + "card" => "summary_large_image", + "image" => image_path }} end @@ -102,12 +102,12 @@ test "takes first founded title in html head if there is html markup error" do assert TwitterCard.parse(html, %{}) == {:ok, %{ - site: nil, - title: + "site" => nil, + "title" => "She Was Arrested at 14. Then Her Photo Went to a Facial Recognition Database. - The New York Times", - "app:id:googleplay": "com.nytimes.android", - "app:name:googleplay": "NYTimes", - "app:url:googleplay": "nytimes://reader/id/100000006583622" + "app:id:googleplay" => "com.nytimes.android", + "app:name:googleplay" => "NYTimes", + "app:url:googleplay" => "nytimes://reader/id/100000006583622" }} end end From cb7be6eef252216d7ba5d5f72c8005d66b04986c Mon Sep 17 00:00:00 2001 From: href <href@random.sh> Date: Wed, 10 Jun 2020 17:34:23 +0200 Subject: [PATCH 13/16] Remove use of atoms in MRF.UserAllowListPolicy --- config/description.exs | 6 ++--- docs/configuration/cheatsheet.md | 5 ++-- lib/pleroma/config/deprecation_warnings.ex | 25 ++++++++++++++++++- .../mrf/user_allow_list_policy.ex | 2 +- .../mrf/user_allowlist_policy_test.exs | 6 ++--- 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/config/description.exs b/config/description.exs index add1601e2..2f1eaf5f2 100644 --- a/config/description.exs +++ b/config/description.exs @@ -1623,14 +1623,12 @@ # %{ # group: :pleroma, # key: :mrf_user_allowlist, - # type: :group, + # type: :map, # description: # "The keys in this section are the domain names that the policy should apply to." <> # " Each key should be assigned a list of users that should be allowed through by their ActivityPub ID", - # children: [ - # ["example.org": ["https://example.org/users/admin"]], # suggestions: [ - # ["example.org": ["https://example.org/users/admin"]] + # %{"example.org" => ["https://example.org/users/admin"]} # ] # ] # }, diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index 456762151..fad67fc4d 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -138,8 +138,9 @@ their ActivityPub ID. An example: ```elixir -config :pleroma, :mrf_user_allowlist, - "example.org": ["https://example.org/users/admin"] +config :pleroma, :mrf_user_allowlist, %{ + "example.org" => ["https://example.org/users/admin"] +} ``` #### :mrf_object_age diff --git a/lib/pleroma/config/deprecation_warnings.ex b/lib/pleroma/config/deprecation_warnings.ex index c39a8984b..b68ded01f 100644 --- a/lib/pleroma/config/deprecation_warnings.ex +++ b/lib/pleroma/config/deprecation_warnings.ex @@ -4,9 +4,10 @@ defmodule Pleroma.Config.DeprecationWarnings do require Logger + alias Pleroma.Config def check_hellthread_threshold do - if Pleroma.Config.get([:mrf_hellthread, :threshold]) do + if Config.get([:mrf_hellthread, :threshold]) do Logger.warn(""" !!!DEPRECATION WARNING!!! You are using the old configuration mechanism for the hellthread filter. Please check config.md. @@ -14,7 +15,29 @@ def check_hellthread_threshold do end end + def mrf_user_allowlist do + config = Config.get(:mrf_user_allowlist) + + if config && Enum.any?(config, fn {k, _} -> is_atom(k) end) do + rewritten = + Enum.reduce(Config.get(:mrf_user_allowlist), Map.new(), fn {k, v}, acc -> + Map.put(acc, to_string(k), v) + end) + + Config.put(:mrf_user_allowlist, rewritten) + + Logger.error(""" + !!!DEPRECATION WARNING!!! + As of Pleroma 2.0.7, the `mrf_user_allowlist` setting changed of format. + Pleroma 2.1 will remove support for the old format. Please change your configuration to match this: + + config :pleroma, :mrf_user_allowlist, #{inspect(rewritten, pretty: true)} + """) + end + end + def warn do check_hellthread_threshold() + mrf_user_allowlist() end end diff --git a/lib/pleroma/web/activity_pub/mrf/user_allow_list_policy.ex b/lib/pleroma/web/activity_pub/mrf/user_allow_list_policy.ex index a927a4ed8..651aed70f 100644 --- a/lib/pleroma/web/activity_pub/mrf/user_allow_list_policy.ex +++ b/lib/pleroma/web/activity_pub/mrf/user_allow_list_policy.ex @@ -24,7 +24,7 @@ def filter(%{"actor" => actor} = object) do allow_list = Config.get( - [:mrf_user_allowlist, String.to_atom(actor_info.host)], + [:mrf_user_allowlist, actor_info.host], [] ) diff --git a/test/web/activity_pub/mrf/user_allowlist_policy_test.exs b/test/web/activity_pub/mrf/user_allowlist_policy_test.exs index 724bae058..ba1b69658 100644 --- a/test/web/activity_pub/mrf/user_allowlist_policy_test.exs +++ b/test/web/activity_pub/mrf/user_allowlist_policy_test.exs @@ -7,7 +7,7 @@ defmodule Pleroma.Web.ActivityPub.MRF.UserAllowListPolicyTest do alias Pleroma.Web.ActivityPub.MRF.UserAllowListPolicy - setup do: clear_config([:mrf_user_allowlist, :localhost]) + setup do: clear_config(:mrf_user_allowlist) test "pass filter if allow list is empty" do actor = insert(:user) @@ -17,14 +17,14 @@ test "pass filter if allow list is empty" do test "pass filter if allow list isn't empty and user in allow list" do actor = insert(:user) - Pleroma.Config.put([:mrf_user_allowlist, :localhost], [actor.ap_id, "test-ap-id"]) + Pleroma.Config.put([:mrf_user_allowlist], %{"localhost" => [actor.ap_id, "test-ap-id"]}) message = %{"actor" => actor.ap_id} assert UserAllowListPolicy.filter(message) == {:ok, message} end test "rejected if allow list isn't empty and user not in allow list" do actor = insert(:user) - Pleroma.Config.put([:mrf_user_allowlist, :localhost], ["test-ap-id"]) + Pleroma.Config.put([:mrf_user_allowlist], %{"localhost" => ["test-ap-id"]}) message = %{"actor" => actor.ap_id} assert UserAllowListPolicy.filter(message) == {:reject, nil} end From 4b865bba107b0db1de886cefd14227454cbece1e Mon Sep 17 00:00:00 2001 From: lain <lain@soykaf.club> Date: Sat, 13 Jun 2020 10:37:15 +0000 Subject: [PATCH 14/16] Apply suggestion to lib/pleroma/web/controller_helper.ex --- lib/pleroma/web/controller_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index a5eb3e9e0..d5e9c33f5 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -75,7 +75,7 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do # instead of the `q.id > ^min_id` and `q.id < ^max_id`. # This is because we only have ids present inside of the page, while # `min_id`, `since_id` and `max_id` requires to know one outside of it. - if Map.take(conn.params, @id_keys) != [] do + if Map.take(conn.params, @id_keys) != %{} do Map.put(fields, "id", current_url(conn)) else fields From 1d625c29a09cf7c0fb415d5606a91315902efaad Mon Sep 17 00:00:00 2001 From: lain <lain@soykaf.club> Date: Sat, 13 Jun 2020 13:12:43 +0200 Subject: [PATCH 15/16] ControllerHelper: Always return id field. --- lib/pleroma/web/controller_helper.ex | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index d5e9c33f5..69946fb81 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -65,21 +65,11 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do |> Map.merge(extra_params) |> Map.drop(@id_keys) - fields = %{ + %{ "next" => current_url(conn, Map.put(params, :max_id, max_id)), - "prev" => current_url(conn, Map.put(params, :min_id, min_id)) + "prev" => current_url(conn, Map.put(params, :min_id, min_id)), + "id" => current_url(conn) } - - # Generating an `id` without already present pagination keys would - # need a query-restriction with an `q.id >= ^id` or `q.id <= ^id` - # instead of the `q.id > ^min_id` and `q.id < ^max_id`. - # This is because we only have ids present inside of the page, while - # `min_id`, `since_id` and `max_id` requires to know one outside of it. - if Map.take(conn.params, @id_keys) != %{} do - Map.put(fields, "id", current_url(conn)) - else - fields - end end def get_pagination_fields(conn, activities, extra_params \\ %{}) do From b15cfc3d365dcfa5f99159fe06e29de6f8aceb4f Mon Sep 17 00:00:00 2001 From: eugenijm <eugenijm@protonmail.com> Date: Mon, 18 May 2020 18:46:04 +0300 Subject: [PATCH 16/16] Mastodon API: ensure the notification endpoint doesn't return less than the requested amount of records unless it's the last page --- CHANGELOG.md | 1 + lib/pleroma/notification.ex | 19 +++++- lib/pleroma/user.ex | 8 +++ .../mastodon_api/views/notification_view.ex | 68 +++++++++---------- ...ete_notifications_from_invisible_users.exs | 18 +++++ test/notification_test.exs | 8 +++ .../notification_controller_test.exs | 27 ++++++++ .../views/notification_view_test.exs | 4 +- 8 files changed, 112 insertions(+), 41 deletions(-) create mode 100644 priv/repo/migrations/20200527163635_delete_notifications_from_invisible_users.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 9361fa260..b3f2dd10f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Filtering of push notifications on activities from blocked domains - Resolving Peertube accounts with Webfinger - `blob:` urls not being allowed by connect-src CSP +- Mastodon API: fix `GET /api/v1/notifications` not returning the full result set ## [Unreleased (patch)] diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 3386a1933..9ee9606be 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -166,8 +166,16 @@ defp exclude_visibility(query, %{exclude_visibilities: visibility}) query |> join(:left, [n, a], mutated_activity in Pleroma.Activity, on: - fragment("?->>'context'", a.data) == - fragment("?->>'context'", mutated_activity.data) and + fragment( + "COALESCE((?->'object')->>'id', ?->>'object')", + a.data, + a.data + ) == + fragment( + "COALESCE((?->'object')->>'id', ?->>'object')", + mutated_activity.data, + mutated_activity.data + ) and fragment("(?->>'type' = 'Like' or ?->>'type' = 'Announce')", a.data, a.data) and fragment("?->>'type'", mutated_activity.data) == "Create", as: :mutated_activity @@ -541,6 +549,7 @@ def exclude_thread_muter_ap_ids(ap_ids, %Activity{} = activity) do def skip?(%Activity{} = activity, %User{} = user) do [ :self, + :invisible, :followers, :follows, :non_followers, @@ -557,6 +566,12 @@ def skip?(:self, %Activity{} = activity, %User{} = user) do activity.data["actor"] == user.ap_id end + def skip?(:invisible, %Activity{} = activity, _) do + actor = activity.data["actor"] + user = User.get_cached_by_ap_id(actor) + User.invisible?(user) + end + def skip?( :followers, %Activity{} = activity, diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index c5c74d132..52ac9052b 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1488,6 +1488,7 @@ def perform(:delete, %User{} = user) do end) delete_user_activities(user) + delete_notifications_from_user_activities(user) delete_outgoing_pending_follow_requests(user) @@ -1576,6 +1577,13 @@ def follow_import(%User{} = follower, followed_identifiers) }) end + def delete_notifications_from_user_activities(%User{ap_id: ap_id}) do + Notification + |> join(:inner, [n], activity in assoc(n, :activity)) + |> where([n, a], fragment("? = ?", a.actor, ^ap_id)) + |> Repo.delete_all() + end + def delete_user_activities(%User{ap_id: ap_id} = user) do ap_id |> Activity.Queries.by_actor() diff --git a/lib/pleroma/web/mastodon_api/views/notification_view.ex b/lib/pleroma/web/mastodon_api/views/notification_view.ex index b11578623..3865be280 100644 --- a/lib/pleroma/web/mastodon_api/views/notification_view.ex +++ b/lib/pleroma/web/mastodon_api/views/notification_view.ex @@ -46,6 +46,7 @@ def render("index.json", %{notifications: notifications, for: reading_user} = op activities |> Enum.filter(&(&1.data["type"] == "Move")) |> Enum.map(&User.get_cached_by_ap_id(&1.data["target"])) + |> Enum.filter(& &1) actors = activities @@ -84,50 +85,45 @@ def render( # Note: :relationships contain user mutes (needed for :muted flag in :status) status_render_opts = %{relationships: opts[:relationships]} - with %{id: _} = account <- - AccountView.render( - "show.json", - %{user: actor, for: reading_user} - ) do - response = %{ - id: to_string(notification.id), - type: notification.type, - created_at: CommonAPI.Utils.to_masto_date(notification.inserted_at), - account: account, - pleroma: %{ - is_seen: notification.seen - } + account = + AccountView.render( + "show.json", + %{user: actor, for: reading_user} + ) + + response = %{ + id: to_string(notification.id), + type: notification.type, + created_at: CommonAPI.Utils.to_masto_date(notification.inserted_at), + account: account, + pleroma: %{ + is_seen: notification.seen } + } - case notification.type do - "mention" -> - put_status(response, activity, reading_user, status_render_opts) + case notification.type do + "mention" -> + put_status(response, activity, reading_user, status_render_opts) - "favourite" -> - put_status(response, parent_activity_fn.(), reading_user, status_render_opts) + "favourite" -> + put_status(response, parent_activity_fn.(), reading_user, status_render_opts) - "reblog" -> - put_status(response, parent_activity_fn.(), reading_user, status_render_opts) + "reblog" -> + put_status(response, parent_activity_fn.(), reading_user, status_render_opts) - "move" -> - put_target(response, activity, reading_user, %{}) + "move" -> + put_target(response, activity, reading_user, %{}) - "pleroma:emoji_reaction" -> - response - |> put_status(parent_activity_fn.(), reading_user, status_render_opts) - |> put_emoji(activity) + "pleroma:emoji_reaction" -> + response + |> put_status(parent_activity_fn.(), reading_user, status_render_opts) + |> put_emoji(activity) - "pleroma:chat_mention" -> - put_chat_message(response, activity, reading_user, status_render_opts) + "pleroma:chat_mention" -> + put_chat_message(response, activity, reading_user, status_render_opts) - type when type in ["follow", "follow_request"] -> - response - - _ -> - nil - end - else - _ -> nil + type when type in ["follow", "follow_request"] -> + response end end diff --git a/priv/repo/migrations/20200527163635_delete_notifications_from_invisible_users.exs b/priv/repo/migrations/20200527163635_delete_notifications_from_invisible_users.exs new file mode 100644 index 000000000..9e95a8111 --- /dev/null +++ b/priv/repo/migrations/20200527163635_delete_notifications_from_invisible_users.exs @@ -0,0 +1,18 @@ +defmodule Pleroma.Repo.Migrations.DeleteNotificationsFromInvisibleUsers do + use Ecto.Migration + + import Ecto.Query + alias Pleroma.Repo + + def up do + Pleroma.Notification + |> join(:inner, [n], activity in assoc(n, :activity)) + |> where( + [n, a], + fragment("? in (SELECT ap_id FROM users WHERE invisible = true)", a.actor) + ) + |> Repo.delete_all() + end + + def down, do: :ok +end diff --git a/test/notification_test.exs b/test/notification_test.exs index b9bbdceca..526f43fab 100644 --- a/test/notification_test.exs +++ b/test/notification_test.exs @@ -306,6 +306,14 @@ test "it doesn't create subscription notifications if the recipient cannot see t assert {:ok, []} == Notification.create_notifications(status) end + + test "it disables notifications from people who are invisible" do + author = insert(:user, invisible: true) + user = insert(:user) + + {:ok, status} = CommonAPI.post(author, %{status: "hey @#{user.nickname}"}) + refute Notification.create_notification(status, user) + end end describe "follow / follow_request notifications" do diff --git a/test/web/mastodon_api/controllers/notification_controller_test.exs b/test/web/mastodon_api/controllers/notification_controller_test.exs index 698c99711..70ef0e8b5 100644 --- a/test/web/mastodon_api/controllers/notification_controller_test.exs +++ b/test/web/mastodon_api/controllers/notification_controller_test.exs @@ -313,6 +313,33 @@ test "filters notifications for Announce activities" do assert public_activity.id in activity_ids refute unlisted_activity.id in activity_ids end + + test "doesn't return less than the requested amount of records when the user's reply is liked" do + user = insert(:user) + %{user: other_user, conn: conn} = oauth_access(["read:notifications"]) + + {:ok, mention} = + CommonAPI.post(user, %{status: "@#{other_user.nickname}", visibility: "public"}) + + {:ok, activity} = CommonAPI.post(user, %{status: ".", visibility: "public"}) + + {:ok, reply} = + CommonAPI.post(other_user, %{ + status: ".", + visibility: "public", + in_reply_to_status_id: activity.id + }) + + {:ok, _favorite} = CommonAPI.favorite(user, reply.id) + + activity_ids = + conn + |> get("/api/v1/notifications?exclude_visibilities[]=direct&limit=2") + |> json_response_and_validate_schema(200) + |> Enum.map(& &1["status"]["id"]) + + assert [reply.id, mention.id] == activity_ids + end end test "filters notifications using exclude_types" do diff --git a/test/web/mastodon_api/views/notification_view_test.exs b/test/web/mastodon_api/views/notification_view_test.exs index b2fa5b302..9c399b2df 100644 --- a/test/web/mastodon_api/views/notification_view_test.exs +++ b/test/web/mastodon_api/views/notification_view_test.exs @@ -139,9 +139,7 @@ test "Follow notification" do test_notifications_rendering([notification], followed, [expected]) User.perform(:delete, follower) - notification = Notification |> Repo.one() |> Repo.preload(:activity) - - test_notifications_rendering([notification], followed, []) + refute Repo.one(Notification) end @tag capture_log: true