From 35ef470d000c53e21c6f867d53ca3a83260d93b8 Mon Sep 17 00:00:00 2001 From: Sadposter Date: Mon, 2 Sep 2019 12:15:21 +0100 Subject: [PATCH 01/10] truncate fields for remote users instead --- lib/pleroma/user/info.ex | 7 +++++++ test/user_test.exs | 17 +++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/user/info.ex b/lib/pleroma/user/info.ex index 779bfbc18..0beb2f721 100644 --- a/lib/pleroma/user/info.ex +++ b/lib/pleroma/user/info.ex @@ -242,6 +242,7 @@ def set_keys(info, keys) do end def remote_user_creation(info, params) do + params = Map.put(params, "fields", Enum.map(params["fields"], &truncate_field/1)) info |> cast(params, [ :ap_enabled, @@ -326,6 +327,12 @@ defp valid_field?(%{"name" => name, "value" => value}) do defp valid_field?(_), do: false + defp truncate_field(%{"name" => name, "value" => value}) do + {name, _chopped} = String.split_at(name, Pleroma.Config.get([:instance, :account_field_name_length], 255)) + {value, _chopped} = String.split_at(value, Pleroma.Config.get([:instance, :account_field_value_length], 255)) + %{"name" => name, "value" => value} + end + @spec confirmation_changeset(Info.t(), keyword()) :: Changeset.t() def confirmation_changeset(info, opts) do need_confirmation? = Keyword.get(opts, :need_confirmation) diff --git a/test/user_test.exs b/test/user_test.exs index 2cbc1f525..68a469fe3 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1117,11 +1117,20 @@ test "get_public_key_for_ap_id fetches a user that's not in the db" do assert {:ok, _key} = User.get_public_key_for_ap_id("http://mastodon.example.org/users/admin") end - test "insert or update a user from given data" do - user = insert(:user, %{nickname: "nick@name.de"}) - data = %{ap_id: user.ap_id <> "xxx", name: user.name, nickname: user.nickname} + describe "insert or update a user from given data" do + test "with normal data" do + user = insert(:user, %{nickname: "nick@name.de"}) + data = %{ap_id: user.ap_id <> "xxx", name: user.name, nickname: user.nickname} - assert {:ok, %User{}} = User.insert_or_update_user(data) + assert {:ok, %User{}} = User.insert_or_update_user(data) + end + + test "with overly long fields" do + current_max_length = Pleroma.Config.get([:instance, :account_field_value_length], 255) + user = insert(:user, nickname: "nickname@supergood.domain") + data = %{ap_id: user.ap_id, info: %{ fields: [%{"name" => "myfield", "value" => String.duplicate("h", current_max_length + 1)}] }} + assert {:ok, %User{}} = User.insert_or_update_user(data) + end end describe "per-user rich-text filtering" do From 05c935c3961e4c1a20c7713611920318d45d4b57 Mon Sep 17 00:00:00 2001 From: Sadposter Date: Mon, 2 Sep 2019 12:15:40 +0100 Subject: [PATCH 02/10] mix format --- lib/pleroma/user/info.ex | 9 +++++++-- test/user_test.exs | 23 ++++++++++++++++------- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/pleroma/user/info.ex b/lib/pleroma/user/info.ex index 0beb2f721..ca1282d02 100644 --- a/lib/pleroma/user/info.ex +++ b/lib/pleroma/user/info.ex @@ -243,6 +243,7 @@ def set_keys(info, keys) do def remote_user_creation(info, params) do params = Map.put(params, "fields", Enum.map(params["fields"], &truncate_field/1)) + info |> cast(params, [ :ap_enabled, @@ -328,8 +329,12 @@ defp valid_field?(%{"name" => name, "value" => value}) do defp valid_field?(_), do: false defp truncate_field(%{"name" => name, "value" => value}) do - {name, _chopped} = String.split_at(name, Pleroma.Config.get([:instance, :account_field_name_length], 255)) - {value, _chopped} = String.split_at(value, Pleroma.Config.get([:instance, :account_field_value_length], 255)) + {name, _chopped} = + String.split_at(name, Pleroma.Config.get([:instance, :account_field_name_length], 255)) + + {value, _chopped} = + String.split_at(value, Pleroma.Config.get([:instance, :account_field_value_length], 255)) + %{"name" => name, "value" => value} end diff --git a/test/user_test.exs b/test/user_test.exs index 68a469fe3..0ca310331 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1119,17 +1119,26 @@ test "get_public_key_for_ap_id fetches a user that's not in the db" do describe "insert or update a user from given data" do test "with normal data" do - user = insert(:user, %{nickname: "nick@name.de"}) - data = %{ap_id: user.ap_id <> "xxx", name: user.name, nickname: user.nickname} + user = insert(:user, %{nickname: "nick@name.de"}) + data = %{ap_id: user.ap_id <> "xxx", name: user.name, nickname: user.nickname} - assert {:ok, %User{}} = User.insert_or_update_user(data) + assert {:ok, %User{}} = User.insert_or_update_user(data) end test "with overly long fields" do - current_max_length = Pleroma.Config.get([:instance, :account_field_value_length], 255) - user = insert(:user, nickname: "nickname@supergood.domain") - data = %{ap_id: user.ap_id, info: %{ fields: [%{"name" => "myfield", "value" => String.duplicate("h", current_max_length + 1)}] }} - assert {:ok, %User{}} = User.insert_or_update_user(data) + current_max_length = Pleroma.Config.get([:instance, :account_field_value_length], 255) + user = insert(:user, nickname: "nickname@supergood.domain") + + data = %{ + ap_id: user.ap_id, + info: %{ + fields: [ + %{"name" => "myfield", "value" => String.duplicate("h", current_max_length + 1)} + ] + } + } + + assert {:ok, %User{}} = User.insert_or_update_user(data) end end From d0f07e55d28d25684130cb1090d0bdbb48807548 Mon Sep 17 00:00:00 2001 From: Sadposter Date: Mon, 2 Sep 2019 12:31:23 +0100 Subject: [PATCH 03/10] use atom key for fields --- lib/pleroma/user/info.ex | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/user/info.ex b/lib/pleroma/user/info.ex index ca1282d02..151e025de 100644 --- a/lib/pleroma/user/info.ex +++ b/lib/pleroma/user/info.ex @@ -242,7 +242,12 @@ def set_keys(info, keys) do end def remote_user_creation(info, params) do - params = Map.put(params, "fields", Enum.map(params["fields"], &truncate_field/1)) + params = + if Map.has_key?(params, :fields) do + Map.put(params, :fields, Enum.map(params[:fields], &truncate_field/1)) + else + params + end info |> cast(params, [ From e73685834c1797404c943f66417ffa30add87e04 Mon Sep 17 00:00:00 2001 From: Sadposter Date: Mon, 2 Sep 2019 12:35:55 +0100 Subject: [PATCH 04/10] add mandatory fields for user update --- test/user_test.exs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/user_test.exs b/test/user_test.exs index 0ca310331..92a48f630 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1131,6 +1131,8 @@ test "with overly long fields" do data = %{ ap_id: user.ap_id, + name: user.name, + nickname: user.nickname, info: %{ fields: [ %{"name" => "myfield", "value" => String.duplicate("h", current_max_length + 1)} From 2975da284b75c846a99a56ce70a91ebc3cc43f33 Mon Sep 17 00:00:00 2001 From: Sadposter Date: Wed, 4 Sep 2019 15:45:40 +0100 Subject: [PATCH 05/10] truncate remote user bio/display name --- lib/pleroma/user.ex | 16 +++++++++++++++- test/user_test.exs | 45 +++++++++++++++++++++++++++++---------------- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 29fd6d2ea..87e56b5b4 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -174,11 +174,25 @@ def following_count(%User{} = user) do |> Repo.aggregate(:count, :id) end + defp truncate_if_exists(params, key, max_length) do + if Map.has_key?(params, key) do + {value, _chopped} = String.split_at(params[key], max_length) + Map.put(params, key, value) + else + params + end + end + def remote_user_creation(params) do bio_limit = Pleroma.Config.get([:instance, :user_bio_length], 5000) name_limit = Pleroma.Config.get([:instance, :user_name_length], 100) - params = Map.put(params, :info, params[:info] || %{}) + params = + params + |> Map.put(:info, params[:info] || %{}) + |> truncate_if_exists(:name, name_limit) + |> truncate_if_exists(:bio, bio_limit) + info_cng = User.Info.remote_user_creation(%User.Info{}, params[:info]) changes = diff --git a/test/user_test.exs b/test/user_test.exs index 92a48f630..45f998ff8 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -570,22 +570,6 @@ test "it has required fields" do refute cs.valid? end) end - - test "it restricts some sizes" do - bio_limit = Pleroma.Config.get([:instance, :user_bio_length], 5000) - name_limit = Pleroma.Config.get([:instance, :user_name_length], 100) - - [bio: bio_limit, name: name_limit] - |> Enum.each(fn {field, size} -> - string = String.pad_leading(".", size) - cs = User.remote_user_creation(Map.put(@valid_remote, field, string)) - assert cs.valid? - - string = String.pad_leading(".", size + 1) - cs = User.remote_user_creation(Map.put(@valid_remote, field, string)) - refute cs.valid? - end) - end end describe "followers and friends" do @@ -1142,6 +1126,35 @@ test "with overly long fields" do assert {:ok, %User{}} = User.insert_or_update_user(data) end + + test "with an overly long bio" do + current_max_length = Pleroma.Config.get([:instance, :user_bio_length], 5000) + user = insert(:user, nickname: "nickname@supergood.domain") + + data = %{ + ap_id: user.ap_id, + name: user.name, + nickname: user.nickname, + bio: String.duplicate("h", current_max_length + 1), + info: %{} + } + + assert {:ok, %User{}} = User.insert_or_update_user(data) + end + + test "with an overly long display name" do + current_max_length = Pleroma.Config.get([:instance, :user_name_length], 100) + user = insert(:user, nickname: "nickname@supergood.domain") + + data = %{ + ap_id: user.ap_id, + name: String.duplicate("h", current_max_length + 1), + nickname: user.nickname, + info: %{} + } + + assert {:ok, %User{}} = User.insert_or_update_user(data) + end end describe "per-user rich-text filtering" do From cb99cfcc65f57f0044117ebd12d040488343d9ef Mon Sep 17 00:00:00 2001 From: Sadposter Date: Wed, 4 Sep 2019 15:57:42 +0100 Subject: [PATCH 06/10] don't try to truncate non-strings --- lib/pleroma/user.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 87e56b5b4..e2ebce6fc 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -175,7 +175,7 @@ def following_count(%User{} = user) do end defp truncate_if_exists(params, key, max_length) do - if Map.has_key?(params, key) do + if Map.has_key?(params, key) and is_binary(params[key]) do {value, _chopped} = String.split_at(params[key], max_length) Map.put(params, key, value) else From 5c5ebd38619bb853a58374918fd8983569ba7c0b Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 14 Sep 2019 01:50:15 +0300 Subject: [PATCH 07/10] Mastodon API: Respect post privacy in favourited/reblogged endpoints --- CHANGELOG.md | 1 + .../controllers/mastodon_api_controller.ex | 4 ++ .../mastodon_api_controller_test.exs | 53 ++++++++++++++++++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fe3bf687..0c5e43123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Security - OStatus: eliminate the possibility of a protocol downgrade attack. - OStatus: prevent following locked accounts, bypassing the approval process. +- Mastodon API: respect post privacy in `/api/v1/statuses/:id/{favourited,reblogged}_by` ### Removed - **Breaking:** GNU Social API with Qvitter extensions support diff --git a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex index 0940e07a6..060137b80 100644 --- a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex @@ -842,6 +842,7 @@ def get_mascot(%{assigns: %{user: user}} = conn, _params) do def favourited_by(%{assigns: %{user: user}} = conn, %{"id" => id}) do with %Activity{} = activity <- Activity.get_by_id_with_object(id), + {:visible, true} <- {:visible, Visibility.visible_for_user?(activity, user)}, %Object{data: %{"likes" => likes}} <- Object.normalize(activity) do q = from(u in User, where: u.ap_id in ^likes) @@ -853,12 +854,14 @@ def favourited_by(%{assigns: %{user: user}} = conn, %{"id" => id}) do |> put_view(AccountView) |> render("accounts.json", %{for: user, users: users, as: :user}) else + {:visible, false} -> {:error, :not_found} _ -> json(conn, []) end end def reblogged_by(%{assigns: %{user: user}} = conn, %{"id" => id}) do with %Activity{} = activity <- Activity.get_by_id_with_object(id), + {:visible, true} <- {:visible, Visibility.visible_for_user?(activity, user)}, %Object{data: %{"announcements" => announces}} <- Object.normalize(activity) do q = from(u in User, where: u.ap_id in ^announces) @@ -870,6 +873,7 @@ def reblogged_by(%{assigns: %{user: user}} = conn, %{"id" => id}) do |> put_view(AccountView) |> render("accounts.json", %{for: user, users: users, as: :user}) else + {:visible, false} -> {:error, :not_found} _ -> json(conn, []) end end diff --git a/test/web/mastodon_api/mastodon_api_controller_test.exs b/test/web/mastodon_api/mastodon_api_controller_test.exs index 806ae7e69..c9bce1439 100644 --- a/test/web/mastodon_api/mastodon_api_controller_test.exs +++ b/test/web/mastodon_api/mastodon_api_controller_test.exs @@ -3698,7 +3698,7 @@ test "returns 404 when poll is private and not available for user", %{conn: conn build_conn() |> assign(:user, user) - [conn: conn, activity: activity] + [conn: conn, activity: activity, user: user] end test "returns users who have favorited the status", %{conn: conn, activity: activity} do @@ -3758,6 +3758,32 @@ test "does not fail on an unauthenticated request", %{conn: conn, activity: acti [%{"id" => id}] = response assert id == other_user.id end + + test "requires authentifucation for private posts", %{conn: conn, user: user} do + other_user = insert(:user) + + {:ok, activity} = + CommonAPI.post(user, %{ + "status" => "@#{other_user.nickname} wanna get some #cofe together?", + "visibility" => "direct" + }) + + {:ok, _, _} = CommonAPI.favorite(activity.id, other_user) + + conn + |> assign(:user, nil) + |> get("/api/v1/statuses/#{activity.id}/favourited_by") + |> json_response(404) + + response = + build_conn() + |> assign(:user, other_user) + |> get("/api/v1/statuses/#{activity.id}/favourited_by") + |> json_response(200) + + [%{"id" => id}] = response + assert id == other_user.id + end end describe "GET /api/v1/statuses/:id/reblogged_by" do @@ -3769,7 +3795,7 @@ test "does not fail on an unauthenticated request", %{conn: conn, activity: acti build_conn() |> assign(:user, user) - [conn: conn, activity: activity] + [conn: conn, activity: activity, user: user] end test "returns users who have reblogged the status", %{conn: conn, activity: activity} do @@ -3829,6 +3855,29 @@ test "does not fail on an unauthenticated request", %{conn: conn, activity: acti [%{"id" => id}] = response assert id == other_user.id end + + test "requires authentifucation for private posts", %{conn: conn, user: user} do + other_user = insert(:user) + + {:ok, activity} = + CommonAPI.post(user, %{ + "status" => "@#{other_user.nickname} wanna get some #cofe together?", + "visibility" => "direct" + }) + + conn + |> assign(:user, nil) + |> get("/api/v1/statuses/#{activity.id}/reblogged_by") + |> json_response(404) + + response = + build_conn() + |> assign(:user, other_user) + |> get("/api/v1/statuses/#{activity.id}/reblogged_by") + |> json_response(200) + + assert [] == response + end end describe "POST /auth/password, with valid parameters" do From 85b6144ffd9e96fc79608847fe739a40ca094207 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 14 Sep 2019 10:46:35 +0000 Subject: [PATCH 08/10] Apply suggestion to test/web/mastodon_api/mastodon_api_controller_test.exs --- test/web/mastodon_api/mastodon_api_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/web/mastodon_api/mastodon_api_controller_test.exs b/test/web/mastodon_api/mastodon_api_controller_test.exs index c9bce1439..013a838b8 100644 --- a/test/web/mastodon_api/mastodon_api_controller_test.exs +++ b/test/web/mastodon_api/mastodon_api_controller_test.exs @@ -3759,7 +3759,7 @@ test "does not fail on an unauthenticated request", %{conn: conn, activity: acti assert id == other_user.id end - test "requires authentifucation for private posts", %{conn: conn, user: user} do + test "requires authentification for private posts", %{conn: conn, user: user} do other_user = insert(:user) {:ok, activity} = From a78a7ee455c4e8f4c2aab15a15626237b2b90399 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 14 Sep 2019 10:50:08 +0000 Subject: [PATCH 09/10] Apply suggestion to test/web/mastodon_api/mastodon_api_controller_test.exs --- test/web/mastodon_api/mastodon_api_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/web/mastodon_api/mastodon_api_controller_test.exs b/test/web/mastodon_api/mastodon_api_controller_test.exs index 013a838b8..061c3a8ad 100644 --- a/test/web/mastodon_api/mastodon_api_controller_test.exs +++ b/test/web/mastodon_api/mastodon_api_controller_test.exs @@ -3856,7 +3856,7 @@ test "does not fail on an unauthenticated request", %{conn: conn, activity: acti assert id == other_user.id end - test "requires authentifucation for private posts", %{conn: conn, user: user} do + test "requires authentification for private posts", %{conn: conn, user: user} do other_user = insert(:user) {:ok, activity} = From b870ae08fd19acd7c40e4353e657e6a1ed1b34c5 Mon Sep 17 00:00:00 2001 From: Egor Kislitsyn Date: Sat, 14 Sep 2019 19:31:20 +0700 Subject: [PATCH 10/10] Fix `Activity.all_by_actor_and_id/2` test --- test/activity_test.exs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/activity_test.exs b/test/activity_test.exs index f9f789a76..275cab81e 100644 --- a/test/activity_test.exs +++ b/test/activity_test.exs @@ -213,7 +213,11 @@ test "all_by_actor_and_id/2" do assert [] == Activity.all_by_actor_and_id(user, []) - assert [%Activity{id: ^id2}, %Activity{id: ^id1}] = - Activity.all_by_actor_and_id(user.ap_id, [id1, id2]) + activities = + user.ap_id + |> Activity.all_by_actor_and_id([id1, id2]) + |> Enum.sort(&(&1.id < &2.id)) + + assert [%Activity{id: ^id1}, %Activity{id: ^id2}] = activities end end