From 9f45f939499b39026ffa4162d1662a163306f9a7 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Tue, 18 Jun 2019 17:00:49 +0300 Subject: [PATCH 01/10] Added more `redirect_uri` checks to prevent redirect to not explicitly listed URI. --- lib/pleroma/web/oauth/oauth_controller.ex | 46 +++++-- test/web/oauth/oauth_controller_test.exs | 150 +++++++++++++++++----- 2 files changed, 153 insertions(+), 43 deletions(-) diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 35a7c582e..60e5665fd 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -64,26 +64,34 @@ defp do_authorize(%Plug.Conn{} = conn, params) do defp handle_existing_authorization( %Plug.Conn{assigns: %{token: %Token{} = token}} = conn, - params + %{"redirect_uri" => @oob_token_redirect_uri} ) do - token = Repo.preload(token, :app) + render(conn, "oob_token_exists.html", %{token: token}) + end + + defp handle_existing_authorization( + %Plug.Conn{assigns: %{token: %Token{} = token}} = conn, + %{} = params + ) do + app = Repo.preload(token, :app).app redirect_uri = if is_binary(params["redirect_uri"]) do params["redirect_uri"] else - default_redirect_uri(token.app) + default_redirect_uri(app) end - redirect_uri = redirect_uri(conn, redirect_uri) - - if redirect_uri == @oob_token_redirect_uri do - render(conn, "oob_token_exists.html", %{token: token}) - else + if redirect_uri in String.split(app.redirect_uris) do + redirect_uri = redirect_uri(conn, redirect_uri) url_params = %{access_token: token.token} url_params = UriHelper.append_param_if_present(url_params, :state, params["state"]) url = UriHelper.append_uri_params(redirect_uri, url_params) redirect(conn, external: url) + else + conn + |> put_flash(:error, "Unlisted redirect_uri.") + |> redirect(external: redirect_uri(conn, redirect_uri)) end end @@ -100,18 +108,28 @@ def create_authorization( end end + def after_create_authorization(%Plug.Conn{} = conn, %Authorization{} = auth, %{ + "authorization" => %{"redirect_uri" => @oob_token_redirect_uri} + }) do + render(conn, "oob_authorization_created.html", %{auth: auth}) + end + def after_create_authorization(%Plug.Conn{} = conn, %Authorization{} = auth, %{ "authorization" => %{"redirect_uri" => redirect_uri} = auth_attrs }) do - redirect_uri = redirect_uri(conn, redirect_uri) + app = Repo.preload(auth, :app).app - if redirect_uri == @oob_token_redirect_uri do - render(conn, "oob_authorization_created.html", %{auth: auth}) - else + # An extra safety measure before we redirect (the same check is being performed in `do_create_authorization/2`) + if redirect_uri in String.split(app.redirect_uris) do + redirect_uri = redirect_uri(conn, redirect_uri) url_params = %{code: auth.token} url_params = UriHelper.append_param_if_present(url_params, :state, auth_attrs["state"]) url = UriHelper.append_uri_params(redirect_uri, url_params) redirect(conn, external: url) + else + conn + |> put_flash(:error, "Unlisted redirect_uri.") + |> redirect(external: redirect_uri(conn, redirect_uri)) end end @@ -324,7 +342,7 @@ def callback(%Plug.Conn{} = conn, params) do }) conn - |> put_session(:registration_id, registration.id) + |> put_session_registration_id(registration.id) |> registration_details(%{"authorization" => registration_params}) end else @@ -445,7 +463,7 @@ defp validate_scopes(app, params) do |> Scopes.validates(app.scopes) end - defp default_redirect_uri(%App{} = app) do + def default_redirect_uri(%App{} = app) do app.redirect_uris |> String.split() |> Enum.at(0) diff --git a/test/web/oauth/oauth_controller_test.exs b/test/web/oauth/oauth_controller_test.exs index 242b7fdb3..aae34804d 100644 --- a/test/web/oauth/oauth_controller_test.exs +++ b/test/web/oauth/oauth_controller_test.exs @@ -10,6 +10,7 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest do alias Pleroma.Registration alias Pleroma.Repo alias Pleroma.Web.OAuth.Authorization + alias Pleroma.Web.OAuth.OAuthController alias Pleroma.Web.OAuth.Token @oauth_config_path [:oauth2, :issue_new_refresh_token] @@ -49,7 +50,7 @@ test "GET /oauth/authorize renders auth forms, including OAuth consumer form", % %{ "response_type" => "code", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "scope" => "read" } ) @@ -72,7 +73,7 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", % "authorization" => %{ "scope" => "read follow", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "a_state" } } @@ -98,11 +99,12 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", % test "with user-bound registration, GET /oauth//callback redirects to `redirect_uri` with `code`", %{app: app, conn: conn} do registration = insert(:registration) + redirect_uri = OAuthController.default_redirect_uri(app) state_params = %{ "scope" => Enum.join(app.scopes, " "), "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "state" => "" } @@ -121,7 +123,7 @@ test "with user-bound registration, GET /oauth//callback redirects to ) assert response = html_response(conn, 302) - assert redirected_to(conn) =~ ~r/#{app.redirect_uris}\?code=.+/ + assert redirected_to(conn) =~ ~r/#{redirect_uri}\?code=.+/ end end @@ -132,7 +134,7 @@ test "with user-unbound registration, GET /oauth//callback renders reg state_params = %{ "scope" => "read write", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "a_state" } @@ -165,7 +167,7 @@ test "on authentication error, GET /oauth//callback redirects to `redi state_params = %{ "scope" => Enum.join(app.scopes, " "), "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "" } @@ -199,7 +201,7 @@ test "GET /oauth/registration_details renders registration details form", %{ "authorization" => %{ "scopes" => app.scopes, "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "a_state", "nickname" => nil, "email" => "john@doe.com" @@ -218,6 +220,7 @@ test "with valid params, POST /oauth/register?op=register redirects to `redirect conn: conn } do registration = insert(:registration, user: nil, info: %{"nickname" => nil, "email" => nil}) + redirect_uri = OAuthController.default_redirect_uri(app) conn = conn @@ -229,7 +232,7 @@ test "with valid params, POST /oauth/register?op=register redirects to `redirect "authorization" => %{ "scopes" => app.scopes, "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "state" => "a_state", "nickname" => "availablenick", "email" => "available@email.com" @@ -238,7 +241,36 @@ test "with valid params, POST /oauth/register?op=register redirects to `redirect ) assert response = html_response(conn, 302) - assert redirected_to(conn) =~ ~r/#{app.redirect_uris}\?code=.+/ + assert redirected_to(conn) =~ ~r/#{redirect_uri}\?code=.+/ + end + + test "with unlisted `redirect_uri`, POST /oauth/register?op=register results in HTTP 401", + %{ + app: app, + conn: conn + } do + registration = insert(:registration, user: nil, info: %{"nickname" => nil, "email" => nil}) + unlisted_redirect_uri = "http://cross-site-request.com" + + conn = + conn + |> put_session(:registration_id, registration.id) + |> post( + "/oauth/register", + %{ + "op" => "register", + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => unlisted_redirect_uri, + "state" => "a_state", + "nickname" => "availablenick", + "email" => "available@email.com" + } + } + ) + + assert response = html_response(conn, 401) end test "with invalid params, POST /oauth/register?op=register renders registration_details page", @@ -254,7 +286,7 @@ test "with invalid params, POST /oauth/register?op=register renders registration "authorization" => %{ "scopes" => app.scopes, "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "a_state", "nickname" => "availablenickname", "email" => "available@email.com" @@ -286,6 +318,7 @@ test "with valid params, POST /oauth/register?op=connect redirects to `redirect_ } do user = insert(:user, password_hash: Comeonin.Pbkdf2.hashpwsalt("testpassword")) registration = insert(:registration, user: nil) + redirect_uri = OAuthController.default_redirect_uri(app) conn = conn @@ -297,7 +330,7 @@ test "with valid params, POST /oauth/register?op=connect redirects to `redirect_ "authorization" => %{ "scopes" => app.scopes, "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "state" => "a_state", "name" => user.nickname, "password" => "testpassword" @@ -306,7 +339,37 @@ test "with valid params, POST /oauth/register?op=connect redirects to `redirect_ ) assert response = html_response(conn, 302) - assert redirected_to(conn) =~ ~r/#{app.redirect_uris}\?code=.+/ + assert redirected_to(conn) =~ ~r/#{redirect_uri}\?code=.+/ + end + + test "with unlisted `redirect_uri`, POST /oauth/register?op=connect results in HTTP 401`", + %{ + app: app, + conn: conn + } do + user = insert(:user, password_hash: Comeonin.Pbkdf2.hashpwsalt("testpassword")) + registration = insert(:registration, user: nil) + unlisted_redirect_uri = "http://cross-site-request.com" + + conn = + conn + |> put_session(:registration_id, registration.id) + |> post( + "/oauth/register", + %{ + "op" => "connect", + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => unlisted_redirect_uri, + "state" => "a_state", + "name" => user.nickname, + "password" => "testpassword" + } + } + ) + + assert response = html_response(conn, 401) end test "with invalid params, POST /oauth/register?op=connect renders registration_details page", @@ -322,7 +385,7 @@ test "with invalid params, POST /oauth/register?op=connect renders registration_ "authorization" => %{ "scopes" => app.scopes, "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "a_state", "name" => user.nickname, "password" => "wrong password" @@ -358,7 +421,7 @@ test "renders authentication page", %{app: app, conn: conn} do %{ "response_type" => "code", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "scope" => "read" } ) @@ -378,7 +441,7 @@ test "properly handles internal calls with `authorization`-wrapped params", %{ "authorization" => %{ "response_type" => "code", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "scope" => "read" } } @@ -399,7 +462,7 @@ test "renders authentication page if user is already authenticated but `force_lo %{ "response_type" => "code", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "scope" => "read", "force_login" => "true" } @@ -423,7 +486,7 @@ test "with existing authentication and non-OOB `redirect_uri`, redirects to app %{ "response_type" => "code", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "state" => "specific_client_state", "scope" => "read" } @@ -433,6 +496,31 @@ test "with existing authentication and non-OOB `redirect_uri`, redirects to app "https://redirect.url?access_token=#{token.token}&state=specific_client_state" end + test "with existing authentication and unlisted non-OOB `redirect_uri`, redirects without credentials", + %{ + app: app, + conn: conn + } do + unlisted_redirect_uri = "http://cross-site-request.com" + token = insert(:oauth_token, app_id: app.id) + + conn = + conn + |> put_session(:oauth_token, token.token) + |> get( + "/oauth/authorize", + %{ + "response_type" => "code", + "client_id" => app.client_id, + "redirect_uri" => unlisted_redirect_uri, + "state" => "specific_client_state", + "scope" => "read" + } + ) + + assert redirected_to(conn) == unlisted_redirect_uri + end + test "with existing authentication and OOB `redirect_uri`, redirects to app with `token` and `state` params", %{ app: app, @@ -461,6 +549,7 @@ test "with existing authentication and OOB `redirect_uri`, redirects to app with test "redirects with oauth authorization" do user = insert(:user) app = insert(:oauth_app, scopes: ["read", "write", "follow"]) + redirect_uri = OAuthController.default_redirect_uri(app) conn = build_conn() @@ -469,14 +558,14 @@ test "redirects with oauth authorization" do "name" => user.nickname, "password" => "test", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "scope" => "read write", "state" => "statepassed" } }) target = redirected_to(conn) - assert target =~ app.redirect_uris + assert target =~ redirect_uri query = URI.parse(target).query |> URI.query_decoder() |> Map.new() @@ -489,6 +578,7 @@ test "redirects with oauth authorization" do test "returns 401 for wrong credentials", %{conn: conn} do user = insert(:user) app = insert(:oauth_app) + redirect_uri = OAuthController.default_redirect_uri(app) result = conn @@ -497,7 +587,7 @@ test "returns 401 for wrong credentials", %{conn: conn} do "name" => user.nickname, "password" => "wrong", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "state" => "statepassed", "scope" => Enum.join(app.scopes, " ") } @@ -506,7 +596,7 @@ test "returns 401 for wrong credentials", %{conn: conn} do # Keep the details assert result =~ app.client_id - assert result =~ app.redirect_uris + assert result =~ redirect_uri # Error message assert result =~ "Invalid Username/Password" @@ -515,6 +605,7 @@ test "returns 401 for wrong credentials", %{conn: conn} do test "returns 401 for missing scopes", %{conn: conn} do user = insert(:user) app = insert(:oauth_app) + redirect_uri = OAuthController.default_redirect_uri(app) result = conn @@ -523,7 +614,7 @@ test "returns 401 for missing scopes", %{conn: conn} do "name" => user.nickname, "password" => "test", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "state" => "statepassed", "scope" => "" } @@ -532,7 +623,7 @@ test "returns 401 for missing scopes", %{conn: conn} do # Keep the details assert result =~ app.client_id - assert result =~ app.redirect_uris + assert result =~ redirect_uri # Error message assert result =~ "This action is outside the authorized scopes" @@ -541,6 +632,7 @@ test "returns 401 for missing scopes", %{conn: conn} do test "returns 401 for scopes beyond app scopes", %{conn: conn} do user = insert(:user) app = insert(:oauth_app, scopes: ["read", "write"]) + redirect_uri = OAuthController.default_redirect_uri(app) result = conn @@ -549,7 +641,7 @@ test "returns 401 for scopes beyond app scopes", %{conn: conn} do "name" => user.nickname, "password" => "test", "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => redirect_uri, "state" => "statepassed", "scope" => "read write follow" } @@ -558,7 +650,7 @@ test "returns 401 for scopes beyond app scopes", %{conn: conn} do # Keep the details assert result =~ app.client_id - assert result =~ app.redirect_uris + assert result =~ redirect_uri # Error message assert result =~ "This action is outside the authorized scopes" @@ -577,7 +669,7 @@ test "issues a token for an all-body request" do |> post("/oauth/token", %{ "grant_type" => "authorization_code", "code" => auth.token, - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "client_id" => app.client_id, "client_secret" => app.client_secret }) @@ -631,7 +723,7 @@ test "issues a token for request with HTTP basic auth client credentials" do |> post("/oauth/token", %{ "grant_type" => "authorization_code", "code" => auth.token, - "redirect_uri" => app.redirect_uris + "redirect_uri" => OAuthController.default_redirect_uri(app) }) assert %{"access_token" => token, "scope" => scope} = json_response(conn, 200) @@ -676,7 +768,7 @@ test "rejects token exchange with invalid client credentials" do |> post("/oauth/token", %{ "grant_type" => "authorization_code", "code" => auth.token, - "redirect_uri" => app.redirect_uris + "redirect_uri" => OAuthController.default_redirect_uri(app) }) assert resp = json_response(conn, 400) @@ -755,7 +847,7 @@ test "rejects an invalid authorization code" do |> post("/oauth/token", %{ "grant_type" => "authorization_code", "code" => "Imobviouslyinvalid", - "redirect_uri" => app.redirect_uris, + "redirect_uri" => OAuthController.default_redirect_uri(app), "client_id" => app.client_id, "client_secret" => app.client_secret }) From 64bc7ac6192164d116df0f306442a5a36dc60416 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Tue, 18 Jun 2019 17:15:26 +0300 Subject: [PATCH 02/10] Minor edit (comment). --- lib/pleroma/web/oauth/oauth_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 60e5665fd..3f8e3b074 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -119,7 +119,7 @@ def after_create_authorization(%Plug.Conn{} = conn, %Authorization{} = auth, %{ }) do app = Repo.preload(auth, :app).app - # An extra safety measure before we redirect (the same check is being performed in `do_create_authorization/2`) + # An extra safety measure before we redirect (also done in `do_create_authorization/2`) if redirect_uri in String.split(app.redirect_uris) do redirect_uri = redirect_uri(conn, redirect_uri) url_params = %{code: auth.token} From 736d8ad6be1ccb1514a189ccf2384e9699ea107e Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Wed, 19 Jun 2019 15:57:44 +0000 Subject: [PATCH 03/10] implement anti link spam MRF --- CHANGELOG.md | 1 + docs/config.md | 1 + .../activity_pub/mrf/anti_link_spam_policy.ex | 46 +++++++ .../mrf/anti_link_spam_policy_test.exs | 120 ++++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100644 lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex create mode 100644 test/web/activity_pub/mrf/anti_link_spam_policy_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b7e5c9a1..ced0573f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - MRF: Support for running subchains. - Configuration: `skip_thread_containment` option - Configuration: `rate_limit` option. See `Pleroma.Plugs.RateLimiter` documentation for details. +- MRF: Support for filtering out likely spam messages using naive heuristics. ### Changed - **Breaking:** Configuration: move from Pleroma.Mailer to Pleroma.Emails.Mailer diff --git a/docs/config.md b/docs/config.md index ed8e465c6..4e9697afc 100644 --- a/docs/config.md +++ b/docs/config.md @@ -90,6 +90,7 @@ config :pleroma, Pleroma.Emails.Mailer, * `Pleroma.Web.ActivityPub.MRF.SubchainPolicy`: Selectively runs other MRF policies when messages match (see ``:mrf_subchain`` section) * `Pleroma.Web.ActivityPub.MRF.RejectNonPublic`: Drops posts with non-public visibility settings (See ``:mrf_rejectnonpublic`` section) * `Pleroma.Web.ActivityPub.MRF.EnsureRePrepended`: Rewrites posts to ensure that replies to posts with subjects do not have an identical subject and instead begin with re:. + * `Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicy`: Rejects posts from likely spambots using naive heuristics. * `public`: Makes the client API in authentificated mode-only except for user-profiles. Useful for disabling the Local Timeline and The Whole Known Network. * `quarantined_instances`: List of ActivityPub instances where private(DMs, followers-only) activities will not be send. * `managed_config`: Whenether the config for pleroma-fe is configured in this config or in ``static/config.json`` diff --git a/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex b/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex new file mode 100644 index 000000000..33ea61f5c --- /dev/null +++ b/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex @@ -0,0 +1,46 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2019 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicy do + alias Pleroma.User + + require Logger + + # has the user successfully posted before? + defp user_has_posted_before?(%User{} = u) do + u.info.note_count > 0 || u.info.follower_count > 0 + end + + # does the post contain links? + defp contains_links?(%{"content" => content} = _object) do + content + |> Floki.filter_out("a.mention,a.hashtag,a[rel~=\"tag\"],a.zrl") + |> Floki.attribute("a", "href") + |> length() > 0 + end + + def filter(%{"type" => "Create", "actor" => actor, "object" => object} = message) do + with {:ok, %User{} = u} <- User.get_or_fetch_by_ap_id(actor), + {:contains_links, true} <- {:contains_links, contains_links?(object)}, + {:posted_before, true} <- {:posted_before, user_has_posted_before?(u)} do + {:ok, message} + else + {:contains_links, false} -> + {:ok, message} + + {:posted_before, false} -> + {:reject, nil} + + {:error, _} -> + {:reject, nil} + + e -> + Logger.warn("[MRF anti-link-spam] WTF: unhandled error #{inspect(e)}") + {:reject, nil} + end + end + + # in all other cases, pass through + def filter(message), do: {:ok, message} +end diff --git a/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs b/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs new file mode 100644 index 000000000..a456e863c --- /dev/null +++ b/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs @@ -0,0 +1,120 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2019 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicyTest do + use Pleroma.DataCase + import Pleroma.Factory + + alias Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicy + + @linkless_message %{ + "type" => "Create", + "object" => %{ + "content" => "hi world!" + } + } + + @linkful_message %{ + "type" => "Create", + "object" => %{ + "content" => "hi world!" + } + } + + describe "with new user" do + test "it allows posts without links" do + user = insert(:user) + + assert user.info.note_count == 0 + + message = + @linkless_message + |> Map.put("actor", user.ap_id) + + {:ok, _message} = AntiLinkSpamPolicy.filter(message) + end + + test "it disallows posts with links" do + user = insert(:user) + + assert user.info.note_count == 0 + + message = + @linkful_message + |> Map.put("actor", user.ap_id) + + {:reject, _} = AntiLinkSpamPolicy.filter(message) + end + end + + describe "with old user" do + test "it allows posts without links" do + user = insert(:user, info: %{note_count: 1}) + + assert user.info.note_count == 1 + + message = + @linkless_message + |> Map.put("actor", user.ap_id) + + {:ok, _message} = AntiLinkSpamPolicy.filter(message) + end + + test "it allows posts with links" do + user = insert(:user, info: %{note_count: 1}) + + assert user.info.note_count == 1 + + message = + @linkful_message + |> Map.put("actor", user.ap_id) + + {:ok, _message} = AntiLinkSpamPolicy.filter(message) + end + end + + describe "with followed new user" do + test "it allows posts without links" do + user = insert(:user, info: %{follower_count: 1}) + + assert user.info.follower_count == 1 + + message = + @linkless_message + |> Map.put("actor", user.ap_id) + + {:ok, _message} = AntiLinkSpamPolicy.filter(message) + end + + test "it allows posts with links" do + user = insert(:user, info: %{follower_count: 1}) + + assert user.info.follower_count == 1 + + message = + @linkful_message + |> Map.put("actor", user.ap_id) + + {:ok, _message} = AntiLinkSpamPolicy.filter(message) + end + end + + describe "with unknown actors" do + test "it rejects posts without links" do + message = + @linkless_message + |> Map.put("actor", "http://invalid.actor") + + {:reject, _} = AntiLinkSpamPolicy.filter(message) + end + + test "it rejects posts with links" do + message = + @linkful_message + |> Map.put("actor", "http://invalid.actor") + + {:reject, _} = AntiLinkSpamPolicy.filter(message) + end + end +end From 21dacd4b15f92726f8a26fb4ec7b06b7f98d97f1 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Wed, 19 Jun 2019 16:33:49 +0000 Subject: [PATCH 04/10] unbreak polls --- .../activity_pub/mrf/anti_link_spam_policy.ex | 2 ++ .../mrf/anti_link_spam_policy_test.exs | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex b/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex index 33ea61f5c..14e5955ee 100644 --- a/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex +++ b/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex @@ -20,6 +20,8 @@ defp contains_links?(%{"content" => content} = _object) do |> length() > 0 end + defp contains_links?(_), do: false + def filter(%{"type" => "Create", "actor" => actor, "object" => object} = message) do with {:ok, %User{} = u} <- User.get_or_fetch_by_ap_id(actor), {:contains_links, true} <- {:contains_links, contains_links?(object)}, diff --git a/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs b/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs index a456e863c..284c13336 100644 --- a/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs +++ b/test/web/activity_pub/mrf/anti_link_spam_policy_test.exs @@ -22,6 +22,14 @@ defmodule Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicyTest do } } + @response_message %{ + "type" => "Create", + "object" => %{ + "name" => "yes", + "type" => "Answer" + } + } + describe "with new user" do test "it allows posts without links" do user = insert(:user) @@ -117,4 +125,16 @@ test "it rejects posts with links" do {:reject, _} = AntiLinkSpamPolicy.filter(message) end end + + describe "with contentless-objects" do + test "it does not reject them or error out" do + user = insert(:user, info: %{note_count: 1}) + + message = + @response_message + |> Map.put("actor", user.ap_id) + + {:ok, _message} = AntiLinkSpamPolicy.filter(message) + end + end end From 630ac6a921bc80a93f5f994731c1085fd1b9d3e8 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Thu, 20 Jun 2019 03:01:03 +0000 Subject: [PATCH 05/10] docs: better description for mrf anti link spam --- CHANGELOG.md | 2 +- docs/config.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ced0573f6..0dc8b547d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,7 +62,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - MRF: Support for running subchains. - Configuration: `skip_thread_containment` option - Configuration: `rate_limit` option. See `Pleroma.Plugs.RateLimiter` documentation for details. -- MRF: Support for filtering out likely spam messages using naive heuristics. +- MRF: Support for filtering out likely spam messages by rejecting posts from new users that contain links. ### Changed - **Breaking:** Configuration: move from Pleroma.Mailer to Pleroma.Emails.Mailer diff --git a/docs/config.md b/docs/config.md index 4e9697afc..b75193545 100644 --- a/docs/config.md +++ b/docs/config.md @@ -90,7 +90,7 @@ config :pleroma, Pleroma.Emails.Mailer, * `Pleroma.Web.ActivityPub.MRF.SubchainPolicy`: Selectively runs other MRF policies when messages match (see ``:mrf_subchain`` section) * `Pleroma.Web.ActivityPub.MRF.RejectNonPublic`: Drops posts with non-public visibility settings (See ``:mrf_rejectnonpublic`` section) * `Pleroma.Web.ActivityPub.MRF.EnsureRePrepended`: Rewrites posts to ensure that replies to posts with subjects do not have an identical subject and instead begin with re:. - * `Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicy`: Rejects posts from likely spambots using naive heuristics. + * `Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicy`: Rejects posts from likely spambots by rejecting posts from new users that contain links. * `public`: Makes the client API in authentificated mode-only except for user-profiles. Useful for disabling the Local Timeline and The Whole Known Network. * `quarantined_instances`: List of ActivityPub instances where private(DMs, followers-only) activities will not be send. * `managed_config`: Whenether the config for pleroma-fe is configured in this config or in ``static/config.json`` From fc6e661672c17a57991ad94754b005be05f68621 Mon Sep 17 00:00:00 2001 From: Egor Kislitsyn Date: Fri, 21 Jun 2019 16:47:16 +0700 Subject: [PATCH 06/10] Fix rate limiter tests --- test/plugs/rate_limiter_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/plugs/rate_limiter_test.exs b/test/plugs/rate_limiter_test.exs index b3798bf03..b8d6aff89 100644 --- a/test/plugs/rate_limiter_test.exs +++ b/test/plugs/rate_limiter_test.exs @@ -20,7 +20,7 @@ test "ip/1" do end test "it restricts by opts" do - scale = 100 + scale = 1000 limit = 5 Pleroma.Config.put([:rate_limit, @limiter_name], {scale, limit}) @@ -64,7 +64,7 @@ test "it restricts by opts" do test "optional limits for authenticated users" do Ecto.Adapters.SQL.Sandbox.checkout(Pleroma.Repo) - scale = 100 + scale = 1000 limit = 5 Pleroma.Config.put([:rate_limit, @limiter_name], [{1, 10}, {scale, limit}]) From b6af80f769195b5047ee8da07166f022c2e29b0a Mon Sep 17 00:00:00 2001 From: feld Date: Fri, 21 Jun 2019 11:36:32 +0000 Subject: [PATCH 07/10] Revert "Merge branch 'fix/ogp-title' into 'develop'" This reverts merge request !1277 --- .../rich_media/parsers/meta_tags_parser.ex | 33 +++++-------------- .../rich_media/ogp-missing-title.html | 12 ------- test/web/rich_media/parser_test.exs | 22 ------------- 3 files changed, 8 insertions(+), 59 deletions(-) delete mode 100644 test/fixtures/rich_media/ogp-missing-title.html 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 82f1cce29..4a7c5eae0 100644 --- a/lib/pleroma/web/rich_media/parsers/meta_tags_parser.ex +++ b/lib/pleroma/web/rich_media/parsers/meta_tags_parser.ex @@ -1,19 +1,15 @@ defmodule Pleroma.Web.RichMedia.Parsers.MetaTagsParser do def parse(html, data, prefix, error_message, key_name, value_name \\ "content") do - meta_data = - html - |> get_elements(key_name, prefix) - |> Enum.reduce(data, fn el, acc -> - attributes = normalize_attributes(el, prefix, key_name, value_name) + with elements = [_ | _] <- get_elements(html, key_name, prefix), + meta_data = + Enum.reduce(elements, data, fn el, acc -> + attributes = normalize_attributes(el, prefix, key_name, value_name) - Map.merge(acc, attributes) - end) - |> maybe_put_title(html) - - if Enum.empty?(meta_data) do - {:error, error_message} - else + Map.merge(acc, attributes) + end) do {:ok, meta_data} + else + _e -> {:error, error_message} end end @@ -31,17 +27,4 @@ defp normalize_attributes(html_node, prefix, key_name, value_name) do %{String.to_atom(data[key_name]) => data[value_name]} end - - defp maybe_put_title(%{title: _} = meta, _), do: meta - - defp maybe_put_title(meta, html) do - case get_page_title(html) do - "" -> meta - title -> Map.put_new(meta, :title, title) - end - end - - defp get_page_title(html) do - Floki.find(html, "title") |> Floki.text() - end end diff --git a/test/fixtures/rich_media/ogp-missing-title.html b/test/fixtures/rich_media/ogp-missing-title.html deleted file mode 100644 index fcdbedfc6..000000000 --- a/test/fixtures/rich_media/ogp-missing-title.html +++ /dev/null @@ -1,12 +0,0 @@ - - - - The Rock (1996) - - - - - - - diff --git a/test/web/rich_media/parser_test.exs b/test/web/rich_media/parser_test.exs index a49ba9549..3a9cc1854 100644 --- a/test/web/rich_media/parser_test.exs +++ b/test/web/rich_media/parser_test.exs @@ -9,15 +9,6 @@ defmodule Pleroma.Web.RichMedia.ParserTest do } -> %Tesla.Env{status: 200, body: File.read!("test/fixtures/rich_media/ogp.html")} - %{ - method: :get, - url: "http://example.com/ogp-missing-title" - } -> - %Tesla.Env{ - status: 200, - body: File.read!("test/fixtures/rich_media/ogp-missing-title.html") - } - %{ method: :get, url: "http://example.com/twitter-card" @@ -60,19 +51,6 @@ test "parses ogp" do }} end - 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: - "Directed by Michael Bay. With Sean Connery, Nicolas Cage, Ed Harris, John Spencer.", - type: "video.movie", - url: "http://www.imdb.com/title/tt0117500/" - }} - end - test "parses twitter card" do assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/twitter-card") == {:ok, From e76115989a0867d5a37a869b560153c2e7c060fd Mon Sep 17 00:00:00 2001 From: rinpatch <rinpatch@sdf.org> Date: Fri, 21 Jun 2019 19:30:25 +0300 Subject: [PATCH 08/10] Move config templates to priv so they can be found in releases --- lib/mix/tasks/pleroma/instance.ex | 11 ++++++----- .../tasks/pleroma => priv/templates}/robots_txt.eex | 0 .../pleroma => priv/templates}/sample_config.eex | 0 .../tasks/pleroma => priv/templates}/sample_psql.eex | 0 4 files changed, 6 insertions(+), 5 deletions(-) rename {lib/mix/tasks/pleroma => priv/templates}/robots_txt.eex (100%) rename {lib/mix/tasks/pleroma => priv/templates}/sample_config.eex (100%) rename {lib/mix/tasks/pleroma => priv/templates}/sample_psql.eex (100%) diff --git a/lib/mix/tasks/pleroma/instance.ex b/lib/mix/tasks/pleroma/instance.ex index 2c4e414cf..c6738dbcc 100644 --- a/lib/mix/tasks/pleroma/instance.ex +++ b/lib/mix/tasks/pleroma/instance.ex @@ -155,10 +155,11 @@ def run(["gen" | rest]) do secret = :crypto.strong_rand_bytes(64) |> Base.encode64() |> binary_part(0, 64) signing_salt = :crypto.strong_rand_bytes(8) |> Base.encode64() |> binary_part(0, 8) {web_push_public_key, web_push_private_key} = :crypto.generate_key(:ecdh, :prime256v1) + template_dir = Application.app_dir(:pleroma, "priv") <> "/templates" result_config = EEx.eval_file( - "sample_config.eex" |> Path.expand(__DIR__), + template_dir <> "/sample_config.eex", domain: domain, port: port, email: email, @@ -179,7 +180,7 @@ def run(["gen" | rest]) do result_psql = EEx.eval_file( - "sample_psql.eex" |> Path.expand(__DIR__), + template_dir <> "/sample_psql.eex", dbname: dbname, dbuser: dbuser, dbpass: dbpass @@ -193,7 +194,7 @@ def run(["gen" | rest]) do shell_info("Writing #{psql_path}.") File.write(psql_path, result_psql) - write_robots_txt(indexable) + write_robots_txt(indexable, template_dir) shell_info( "\n" <> @@ -217,10 +218,10 @@ def run(["gen" | rest]) do end end - defp write_robots_txt(indexable) do + defp write_robots_txt(indexable, template_dir) do robots_txt = EEx.eval_file( - Path.expand("robots_txt.eex", __DIR__), + template_dir <> "/robots_txt.eex", indexable: indexable ) diff --git a/lib/mix/tasks/pleroma/robots_txt.eex b/priv/templates/robots_txt.eex similarity index 100% rename from lib/mix/tasks/pleroma/robots_txt.eex rename to priv/templates/robots_txt.eex diff --git a/lib/mix/tasks/pleroma/sample_config.eex b/priv/templates/sample_config.eex similarity index 100% rename from lib/mix/tasks/pleroma/sample_config.eex rename to priv/templates/sample_config.eex diff --git a/lib/mix/tasks/pleroma/sample_psql.eex b/priv/templates/sample_psql.eex similarity index 100% rename from lib/mix/tasks/pleroma/sample_psql.eex rename to priv/templates/sample_psql.eex From 960d6b54e8575b828d34fcf0b69e634dc4d33fe0 Mon Sep 17 00:00:00 2001 From: rinpatch <rinpatch@sdf.org> Date: Fri, 21 Jun 2019 21:56:49 +0300 Subject: [PATCH 09/10] use Config in generated config when available Mix.Config is deprecated and does not work on OTP releases. However we can't fully switch to Config because it is not present in Elixir < 1.9. I tried to evaluate if Config is available at runtime, but for some weird reason OTP releases crash if I do that. --- priv/templates/sample_config.eex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/priv/templates/sample_config.eex b/priv/templates/sample_config.eex index 8b45acb05..526593d0a 100644 --- a/priv/templates/sample_config.eex +++ b/priv/templates/sample_config.eex @@ -3,7 +3,11 @@ # NOTE: This file should not be committed to a repo or otherwise made public # without removing sensitive information. -use Mix.Config +<%= if Code.ensure_loaded?(Config) do + "import Config" +else + "use Mix.Config" +end %> config :pleroma, Pleroma.Web.Endpoint, url: [host: "<%= domain %>", scheme: "https", port: <%= port %>], From 127a5a7d6567124b834a1f5399a0032c1c1f849d Mon Sep 17 00:00:00 2001 From: William Pitcock <nenolod@dereferenced.org> Date: Fri, 21 Jun 2019 22:27:14 +0000 Subject: [PATCH 10/10] change the anti-link-spam MRF implementation to use old_user? instead of the previous name --- lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex b/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex index 14e5955ee..2da3eac2f 100644 --- a/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex +++ b/lib/pleroma/web/activity_pub/mrf/anti_link_spam_policy.ex @@ -8,7 +8,7 @@ defmodule Pleroma.Web.ActivityPub.MRF.AntiLinkSpamPolicy do require Logger # has the user successfully posted before? - defp user_has_posted_before?(%User{} = u) do + defp old_user?(%User{} = u) do u.info.note_count > 0 || u.info.follower_count > 0 end @@ -25,13 +25,13 @@ defp contains_links?(_), do: false def filter(%{"type" => "Create", "actor" => actor, "object" => object} = message) do with {:ok, %User{} = u} <- User.get_or_fetch_by_ap_id(actor), {:contains_links, true} <- {:contains_links, contains_links?(object)}, - {:posted_before, true} <- {:posted_before, user_has_posted_before?(u)} do + {:old_user, true} <- {:old_user, old_user?(u)} do {:ok, message} else {:contains_links, false} -> {:ok, message} - {:posted_before, false} -> + {:old_user, false} -> {:reject, nil} {:error, _} ->