From a6c2fb97a7c275dc47dc9ce84efac0adfcb86853 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 17:06:00 +0100 Subject: [PATCH 1/7] Add sobelow Signed-off-by: Thomas Citharel --- .sobelow-conf | 12 ++++++++++++ mix.exs | 3 ++- mix.lock | 1 + 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 .sobelow-conf diff --git a/.sobelow-conf b/.sobelow-conf new file mode 100644 index 000000000..207a0df18 --- /dev/null +++ b/.sobelow-conf @@ -0,0 +1,12 @@ +[ + verbose: true, + private: false, + skip: true, + router: "", + exit: "false", + format: "txt", + out: "", + threshold: "low", + ignore: ["Config.Secrets", "XSS", "Config.HTTPS"], + ignore_files: [""] +] diff --git a/mix.exs b/mix.exs index 6c05cedda..36427b793 100644 --- a/mix.exs +++ b/mix.exs @@ -154,7 +154,8 @@ defmodule Mobilizon.Mixfile do {:mock, "~> 0.3.4", only: :test}, {:elixir_feed_parser, "~> 2.1.0", only: :test}, {:mox, "~> 1.0", only: :test}, - {:junit_formatter, "~> 3.1", only: [:test]} + {:junit_formatter, "~> 3.1", only: [:test]}, + {:sobelow, "~> 0.8", only: [:dev, :test]} ] ++ oauth_deps() end diff --git a/mix.lock b/mix.lock index 9e4bb8ffb..3c2025d6b 100644 --- a/mix.lock +++ b/mix.lock @@ -116,6 +116,7 @@ "sitemapper": {:hex, :sitemapper, "0.5.0", "23b0bb7b3888f03d4e4e5bedb7034e6d2979e169366372d960d6f433112b9bdf", [:mix], [{:ex_aws_s3, "~> 2.0", [hex: :ex_aws_s3, repo: "hexpm", optional: true]}, {:xml_builder, "~> 2.1.1", [hex: :xml_builder, repo: "hexpm", optional: false]}], "hexpm", "be7acff8d0245aa7ca125b9c4d0751009bbbca26ef866d888fef4fdf98670e41"}, "sleeplocks": {:hex, :sleeplocks, "1.1.1", "3d462a0639a6ef36cc75d6038b7393ae537ab394641beb59830a1b8271faeed3", [:rebar3], [], "hexpm", "84ee37aeff4d0d92b290fff986d6a95ac5eedf9b383fadfd1d88e9b84a1c02e1"}, "slugger": {:hex, :slugger, "0.3.0", "efc667ab99eee19a48913ccf3d038b1fb9f165fa4fbf093be898b8099e61b6ed", [:mix], [], "hexpm", "20d0ded0e712605d1eae6c5b4889581c3460d92623a930ddda91e0e609b5afba"}, + "sobelow": {:hex, :sobelow, "0.11.0", "cdc17e3a9f1ea78dc55dbe0a03121cb6767fef737c6d9f1e62ee7e78730abccc", [:mix], [{:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "c57807bfe6f231338b657781f89ef0320b66a0dbe779aa911d6ed27cfa14ae6e"}, "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.6", "cf344f5692c82d2cd7554f5ec8fd961548d4fd09e7d22f5b62482e5aeaebd4b0", [:make, :mix, :rebar3], [], "hexpm", "bdb0d2471f453c88ff3908e7686f86f9be327d065cc1ec16fa4540197ea04680"}, "telemetry": {:hex, :telemetry, "0.4.2", "2808c992455e08d6177322f14d3bdb6b625fbcfd233a73505870d8738a2f4599", [:rebar3], [], "hexpm", "2d1419bd9dda6a206d7b5852179511722e2b18812310d304620c7bd92a13fcef"}, "tesla": {:hex, :tesla, "1.4.0", "1081bef0124b8bdec1c3d330bbe91956648fb008cf0d3950a369cda466a31a87", [:mix], [{:castore, "~> 0.1", [hex: :castore, repo: "hexpm", optional: true]}, {:exjsx, ">= 3.0.0", [hex: :exjsx, repo: "hexpm", optional: true]}, {:finch, "~> 0.3", [hex: :finch, repo: "hexpm", optional: true]}, {:fuse, "~> 2.4", [hex: :fuse, repo: "hexpm", optional: true]}, {:gun, "~> 1.3", [hex: :gun, repo: "hexpm", optional: true]}, {:hackney, "~> 1.6", [hex: :hackney, repo: "hexpm", optional: true]}, {:ibrowse, "~> 4.4.0", [hex: :ibrowse, repo: "hexpm", optional: true]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: true]}, {:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:mint, "~> 1.0", [hex: :mint, repo: "hexpm", optional: true]}, {:poison, ">= 1.0.0", [hex: :poison, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: true]}], "hexpm", "bf1374a5569f5fca8e641363b63f7347d680d91388880979a33bc12a6eb3e0aa"}, From f0141c97e8cbcd51e7a5664fe2d637014e3aee0c Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 18:14:52 +0100 Subject: [PATCH 2/7] Refactor rich media parsers to restrict the allowed properties Signed-off-by: Thomas Citharel --- .../rich_media/parsers/meta_tags_parser.ex | 26 +++++++++--- .../rich_media/parsers/oembed_parser.ex | 23 +++++++++- lib/service/rich_media/parsers/ogp.ex | 32 ++++++++++++-- .../rich_media/parsers/twitter_card.ex | 42 +++++++++++++++---- 4 files changed, 103 insertions(+), 20 deletions(-) diff --git a/lib/service/rich_media/parsers/meta_tags_parser.ex b/lib/service/rich_media/parsers/meta_tags_parser.ex index 2ac44a787..bd383db71 100644 --- a/lib/service/rich_media/parsers/meta_tags_parser.ex +++ b/lib/service/rich_media/parsers/meta_tags_parser.ex @@ -7,12 +7,21 @@ defmodule Mobilizon.Service.RichMedia.Parsers.MetaTagsParser do @moduledoc """ Module to parse meta tags data in HTML pages """ - def parse(html, data, prefix, error_message, key_name, value_name \\ "content") do + + def parse( + html, + data, + prefix, + error_message, + key_name, + value_name \\ :content, + allowed_attributes \\ [] + ) do meta_data = html |> get_elements(key_name, prefix) |> Enum.reduce(data, fn el, acc -> - attributes = normalize_attributes(el, prefix, key_name, value_name) + attributes = normalize_attributes(el, prefix, key_name, value_name, allowed_attributes) Map.merge(acc, attributes) end) @@ -27,18 +36,23 @@ defmodule Mobilizon.Service.RichMedia.Parsers.MetaTagsParser do end defp get_elements(html, key_name, prefix) do - html |> Floki.find("meta[#{key_name}^='#{prefix}:']") + html |> Floki.find("meta[#{to_string(key_name)}^='#{prefix}:']") end - defp normalize_attributes(html_node, prefix, key_name, value_name) do + defp normalize_attributes(html_node, prefix, key_name, value_name, allowed_attributes) do {_tag, attributes, _children} = html_node data = - Enum.into(attributes, %{}, fn {name, value} -> + attributes + |> Enum.into(%{}, fn {name, value} -> {name, String.trim_leading(value, "#{prefix}:")} end) - %{String.to_atom(data[key_name]) => data[value_name]} + if data[to_string(key_name)] in Enum.map(allowed_attributes, &to_string/1) do + %{String.to_existing_atom(data[to_string(key_name)]) => data[to_string(value_name)]} + else + %{} + end end defp maybe_put_title(%{title: _} = meta, _), do: meta diff --git a/lib/service/rich_media/parsers/oembed_parser.ex b/lib/service/rich_media/parsers/oembed_parser.ex index dcc2d3e0b..ce450d0d9 100644 --- a/lib/service/rich_media/parsers/oembed_parser.ex +++ b/lib/service/rich_media/parsers/oembed_parser.ex @@ -41,10 +41,31 @@ defmodule Mobilizon.Service.RichMedia.Parsers.OEmbed do {:ok, Enum.into(attributes, %{})["href"]} end + @oembed_allowed_attributes [ + :type, + :version, + :html, + :width, + :height, + :title, + :author_name, + :author_url, + :provider_name, + :provider_url, + :cache_age, + :thumbnail_url, + :thumbnail_width, + :thumbnail_height, + :url + ] + defp get_oembed_data(url) do with {:ok, %{body: json}} <- Tesla.get(url, opts: @http_options), {:ok, data} <- Jason.decode(json), - data <- data |> Map.new(fn {k, v} -> {String.to_atom(k), v} end) do + data <- + data + |> Map.new(fn {k, v} -> {String.to_existing_atom(k), v} end) + |> Map.take(@oembed_allowed_attributes) do {:ok, data} end end diff --git a/lib/service/rich_media/parsers/ogp.ex b/lib/service/rich_media/parsers/ogp.ex index 7bfcf88ed..531f79bf4 100644 --- a/lib/service/rich_media/parsers/ogp.ex +++ b/lib/service/rich_media/parsers/ogp.ex @@ -10,6 +10,26 @@ defmodule Mobilizon.Service.RichMedia.Parsers.OGP do require Logger alias Mobilizon.Service.RichMedia.Parsers.MetaTagsParser + @opengraph_properties [ + :title, + :type, + :image, + :url, + :audio, + :description, + :determiner, + :locale, + :"locale:alternate", + :site_name, + :video, + :"image:url", + :"image.secure_url", + :"image:type", + :"image:width", + :"image:height", + :"image:alt" + ] + def parse(html, data) do Logger.debug("Using OpenGraph card parser") @@ -19,7 +39,9 @@ defmodule Mobilizon.Service.RichMedia.Parsers.OGP do data, "og", "No OGP metadata found", - "property" + :property, + :content, + @opengraph_properties ) do data = transform_tags(data) Logger.debug("Data found with OpenGraph card parser") @@ -29,9 +51,11 @@ defmodule Mobilizon.Service.RichMedia.Parsers.OGP do defp transform_tags(data) do data - |> Map.put(:image_remote_url, Map.get(data, :image)) - |> Map.put(:width, get_integer_value(data, :"image:width")) - |> Map.put(:height, get_integer_value(data, :"image:height")) + |> Enum.reject(fn {_, v} -> is_nil(v) end) + |> Map.new() + |> Map.update(:image_remote_url, Map.get(data, :image), & &1) + |> Map.update(:width, get_integer_value(data, :"image:width"), & &1) + |> Map.update(:height, get_integer_value(data, :"image:height"), & &1) end @spec get_integer_value(map(), atom()) :: integer() | nil diff --git a/lib/service/rich_media/parsers/twitter_card.ex b/lib/service/rich_media/parsers/twitter_card.ex index 2399d72aa..6d2d5297d 100644 --- a/lib/service/rich_media/parsers/twitter_card.ex +++ b/lib/service/rich_media/parsers/twitter_card.ex @@ -10,25 +10,49 @@ defmodule Mobilizon.Service.RichMedia.Parsers.TwitterCard do alias Mobilizon.Service.RichMedia.Parsers.MetaTagsParser require Logger + @twitter_card_properties [ + :card, + :site, + :creator, + :title, + :description, + :image, + :"image:alt" + ] + @spec parse(String.t(), map()) :: {:ok, map()} | {:error, String.t()} def parse(html, data) do Logger.debug("Using Twitter card parser") - res = + with {:ok, data} <- parse_name_attrs(data, html), + {:ok, data} <- parse_property_attrs(data, html), + data <- transform_tags(data) do + Logger.debug("Data found with Twitter card parser") + Logger.debug(inspect(data)) data - |> parse_name_attrs(html) - |> parse_property_attrs(html) - - Logger.debug("Data found with Twitter card parser") - Logger.debug(inspect(res)) - res + end end defp parse_name_attrs(data, html) do - MetaTagsParser.parse(html, data, "twitter", %{}, "name") + MetaTagsParser.parse(html, data, "twitter", %{}, :name, :content, [:"twitter:card"]) end defp parse_property_attrs({_, data}, html) do - MetaTagsParser.parse(html, data, "twitter", "No twitter card metadata found", "property") + MetaTagsParser.parse( + html, + data, + "twitter", + "No twitter card metadata found", + :property, + :content, + @twitter_card_properties + ) + end + + defp transform_tags(data) do + data + |> Enum.reject(fn {_, v} -> is_nil(v) end) + |> Map.new() + |> Map.update(:image_remote_url, Map.get(data, :image), & &1) end end From b7915a6467f2f1f009763c378a1d4200b44fd0ae Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 18:15:21 +0100 Subject: [PATCH 3/7] Add some CSP headers Signed-off-by: Thomas Citharel --- lib/web/router.ex | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/web/router.ex b/lib/web/router.ex index 76bde3ceb..b8aa2690e 100644 --- a/lib/web/router.ex +++ b/lib/web/router.ex @@ -4,6 +4,12 @@ defmodule Mobilizon.Web.Router do """ use Mobilizon.Web, :router + @csp if Application.fetch_env!(:mobilizon, :env) != :dev, + do: "default-src 'self';", + else: + "default-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'" + @headers %{"content-security-policy" => @csp} + pipeline :graphql do # plug(:accepts, ["json"]) plug(Mobilizon.Web.Auth.Pipeline) @@ -30,6 +36,7 @@ defmodule Mobilizon.Web.Router do pipeline :activity_pub_and_html do plug(:accepts, ["html", "activity-json"]) + plug(:put_secure_browser_headers, @headers) plug(Cldr.Plug.AcceptLanguage, cldr_backend: Mobilizon.Cldr @@ -37,6 +44,7 @@ defmodule Mobilizon.Web.Router do end pipeline :atom_and_ical do + plug(:put_secure_browser_headers, @headers) plug(:accepts, ["atom", "ics", "html"]) end @@ -48,10 +56,7 @@ defmodule Mobilizon.Web.Router do ) plug(:accepts, ["html"]) - plug(:fetch_session) - plug(:fetch_flash) - plug(:protect_from_forgery) - plug(:put_secure_browser_headers) + plug(:put_secure_browser_headers, @headers) end pipeline :remote_media do @@ -158,6 +163,8 @@ defmodule Mobilizon.Web.Router do get("/interact", PageController, :interact) get("/auth/:provider", AuthController, :request) + # sobelow_skip ["Config.CSRFRoute"] + # Possibly related to https://github.com/ueberauth/ueberauth/issues/125 get("/auth/:provider/callback", AuthController, :callback) post("/auth/:provider/callback", AuthController, :callback) end From 21698f754d42e9537469ab8ba691020f7e9a239b Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 18:16:00 +0100 Subject: [PATCH 4/7] Change some String.to_atom/1 to String.to_existing_atom/1 Signed-off-by: Thomas Citharel --- lib/graphql/resolvers/admin.ex | 8 ++++---- lib/mobilizon/actors/actors.ex | 12 ------------ 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/graphql/resolvers/admin.ex b/lib/graphql/resolvers/admin.ex index 21698b06c..b8485d0ae 100644 --- a/lib/graphql/resolvers/admin.ex +++ b/lib/graphql/resolvers/admin.ex @@ -123,7 +123,7 @@ defmodule Mobilizon.GraphQL.Resolvers.Admin do # Changes are stored as %{"key" => "value"} so we need to convert them back as struct defp convert_changes_to_struct(struct, %{"report_id" => _report_id} = changes) do - with data <- for({key, val} <- changes, into: %{}, do: {String.to_atom(key), val}), + with data <- for({key, val} <- changes, into: %{}, do: {String.to_existing_atom(key), val}), data <- Map.put(data, :report, Mobilizon.Reports.get_report(data.report_id)) do struct(struct, data) end @@ -135,7 +135,7 @@ defmodule Mobilizon.GraphQL.Resolvers.Admin do for( {key, val} <- changes, into: %{}, - do: {String.to_atom(key), process_eventual_type(changeset, key, val)} + do: {String.to_existing_atom(key), process_eventual_type(changeset, key, val)} ) do struct(struct, data) end @@ -144,11 +144,11 @@ defmodule Mobilizon.GraphQL.Resolvers.Admin do # datetimes are not unserialized as DateTime/NaiveDateTime so we do it manually with changeset data defp process_eventual_type(changeset, key, val) do cond do - changeset[String.to_atom(key)] == :utc_datetime and not is_nil(val) -> + changeset[String.to_existing_atom(key)] == :utc_datetime and not is_nil(val) -> {:ok, datetime, _} = DateTime.from_iso8601(val) datetime - changeset[String.to_atom(key)] == :naive_datetime and not is_nil(val) -> + changeset[String.to_existing_atom(key)] == :naive_datetime and not is_nil(val) -> {:ok, datetime} = NaiveDateTime.from_iso8601(val) datetime diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index 763d2abb5..306ac56a2 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/lib/mobilizon/actors/actors.ex @@ -296,18 +296,6 @@ defmodule Mobilizon.Actors do end end - # defp transform_media_file(nil), do: nil - - # defp transform_media_file(file) do - # file = for({key, val} <- file, into: %{}, do: {String.to_atom(key), val}) - - # if is_nil(file) do - # nil - # else - # struct(Mobilizon.Medias.File, file) - # end - # end - @delete_actor_default_options [reserve_username: true, suspension: false] def delete_actor(%Actor{} = actor, options \\ @delete_actor_default_options) do From 7b91367145ac4795c8bf0cfcb1684fb07028201b Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 19:11:13 +0100 Subject: [PATCH 5/7] Some sobelow fixes Signed-off-by: Thomas Citharel --- .sobelow-conf | 10 +++++----- .sobelow-skips | 2 ++ lib/mobilizon.ex | 3 ++- lib/mobilizon/discussions/discussions.ex | 1 + lib/mobilizon/resources/resources.ex | 1 + lib/web/auth/error_handler.ex | 1 + lib/web/proxy/reverse_proxy.ex | 2 ++ lib/web/router.ex | 4 ++-- lib/web/views/utils.ex | 1 + 9 files changed, 17 insertions(+), 8 deletions(-) create mode 100644 .sobelow-skips diff --git a/.sobelow-conf b/.sobelow-conf index 207a0df18..06cca8933 100644 --- a/.sobelow-conf +++ b/.sobelow-conf @@ -2,11 +2,11 @@ verbose: true, private: false, skip: true, - router: "", - exit: "false", + router: "lib/web/router.ex", + exit: "low", format: "txt", out: "", - threshold: "low", - ignore: ["Config.Secrets", "XSS", "Config.HTTPS"], - ignore_files: [""] + threshold: "medium", + ignore: ["Config.HTTPS"], + ignore_files: ["config/dev.1.secret.exs", "config/dev.2.secret.exs", "config/dev.3.secret.exs", "config/dev.secret.exs", "config/e2e.secret.exs", "config/prod.secret.exs", "config/test.secret.exs"] ] diff --git a/.sobelow-skips b/.sobelow-skips new file mode 100644 index 000000000..8a0f398e7 --- /dev/null +++ b/.sobelow-skips @@ -0,0 +1,2 @@ + +AACA51671C4B3C803ACBCA3FADE84CDE \ No newline at end of file diff --git a/lib/mobilizon.ex b/lib/mobilizon.ex index b4e581399..a1ca09962 100644 --- a/lib/mobilizon.ex +++ b/lib/mobilizon.ex @@ -76,10 +76,11 @@ defmodule Mobilizon do :ok end + # sobelow_skip ["DOS.StringToAtom"] @spec cachex_spec(atom, integer, integer, integer, function | nil) :: Supervisor.child_spec() defp cachex_spec(name, limit, default, interval, fallback \\ nil) do %{ - id: :"cache_#{name}", + id: String.to_atom("cache_#{to_string(name)}"), start: {Cachex, :start_link, [ diff --git a/lib/mobilizon/discussions/discussions.ex b/lib/mobilizon/discussions/discussions.ex index 9ecddffc0..35d3b47dd 100644 --- a/lib/mobilizon/discussions/discussions.ex +++ b/lib/mobilizon/discussions/discussions.ex @@ -58,6 +58,7 @@ defmodule Mobilizon.Discussions do @doc """ Callback for Absinthe Ecto Dataloader """ + # sobelow_skip ["SQL.Query"] @spec data :: Dataloader.Ecto.t() def data do Dataloader.Ecto.new(Repo, query: &query/2) diff --git a/lib/mobilizon/resources/resources.ex b/lib/mobilizon/resources/resources.ex index 23f15046c..b4cb43ae2 100644 --- a/lib/mobilizon/resources/resources.ex +++ b/lib/mobilizon/resources/resources.ex @@ -185,6 +185,7 @@ defmodule Mobilizon.Resources do end) end + # sobelow_skip ["SQL.Query"] @spec update_children(Multi.t(), Resource.t(), map()) :: Multi.t() defp update_children( %Multi{} = multi, diff --git a/lib/web/auth/error_handler.ex b/lib/web/auth/error_handler.ex index 6207477d6..edf760bb8 100644 --- a/lib/web/auth/error_handler.ex +++ b/lib/web/auth/error_handler.ex @@ -4,6 +4,7 @@ defmodule Mobilizon.Web.Auth.ErrorHandler do """ import Plug.Conn + # sobelow_skip ["XSS.SendResp"] def auth_error(conn, {type, _reason}, _opts) do body = Jason.encode!(%{message: to_string(type)}) send_resp(conn, 401, body) diff --git a/lib/web/proxy/reverse_proxy.ex b/lib/web/proxy/reverse_proxy.ex index 121e6a277..c3e5993f0 100644 --- a/lib/web/proxy/reverse_proxy.ex +++ b/lib/web/proxy/reverse_proxy.ex @@ -145,6 +145,7 @@ defmodule Mobilizon.Web.ReverseProxy do end end + # sobelow_skip ["XSS.SendResp"] def call(conn, _, _) do conn |> send_resp(400, Conn.Status.reason_phrase(400)) @@ -223,6 +224,7 @@ defmodule Mobilizon.Web.ReverseProxy do |> send_resp(code, "") end + # sobelow_skip ["XSS.SendResp"] defp error_or_redirect(conn, url, code, body, opts) do if Keyword.get(opts, :redirect_on_failure, false) do conn diff --git a/lib/web/router.ex b/lib/web/router.ex index b8aa2690e..75aaa638d 100644 --- a/lib/web/router.ex +++ b/lib/web/router.ex @@ -163,8 +163,8 @@ defmodule Mobilizon.Web.Router do get("/interact", PageController, :interact) get("/auth/:provider", AuthController, :request) - # sobelow_skip ["Config.CSRFRoute"] - # Possibly related to https://github.com/ueberauth/ueberauth/issues/125 + # Have a look at https://github.com/ueberauth/ueberauth/issues/125 some day + # Also possible CSRF issue get("/auth/:provider/callback", AuthController, :callback) post("/auth/:provider/callback", AuthController, :callback) end diff --git a/lib/web/views/utils.ex b/lib/web/views/utils.ex index 34eaa45c4..f1fba1796 100644 --- a/lib/web/views/utils.ex +++ b/lib/web/views/utils.ex @@ -5,6 +5,7 @@ defmodule Mobilizon.Web.Views.Utils do alias Mobilizon.Service.Metadata.Utils, as: MetadataUtils + # sobelow_skip ["Traversal.FileModule"] @spec inject_tags(Enum.t(), String.t()) :: {:safe, String.t()} def inject_tags(tags, locale \\ "en") do with {:ok, index_content} <- File.read(index_file_path()) do From e6da458b4943965d4c8e61befedd719081befdd3 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 19:31:27 +0100 Subject: [PATCH 6/7] Add sobelow to CI Signed-off-by: Thomas Citharel --- .gitlab-ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 1cbf2120c..d1d0156a0 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -55,8 +55,9 @@ lint-elixir: - mix deps.get script: - export EXITVALUE=0 - - mix credo --strict -a || export EXITVALUE=1 - mix format --check-formatted --dry-run || export EXITVALUE=1 + - mix credo --strict -a || export EXITVALUE=1 + - mix sobelow --config --skip || export EXITVALUE=1 - exit $EXITVALUE lint-front: From a4545bcf672b04efa914cf72b6b3aa2c8b02f41a Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 22 Jan 2021 19:44:37 +0100 Subject: [PATCH 7/7] Exclude persons from being followed Signed-off-by: Thomas Citharel --- lib/federation/activity_pub/activity_pub.ex | 2 +- lib/federation/activity_pub/types/actors.ex | 5 ++- .../transmogrifier/follow_test.exs | 33 ++++++++++++++----- .../activity_pub/transmogrifier/undo_test.exs | 2 +- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/lib/federation/activity_pub/activity_pub.ex b/lib/federation/activity_pub/activity_pub.ex index 3cf173441..cf7a9e54c 100644 --- a/lib/federation/activity_pub/activity_pub.ex +++ b/lib/federation/activity_pub/activity_pub.ex @@ -338,7 +338,7 @@ defmodule Mobilizon.Federation.ActivityPub do :ok <- maybe_federate(activity) do {:ok, activity, follower} else - {:error, err, msg} when err in [:already_following, :suspended] -> + {:error, err, msg} when err in [:already_following, :suspended, :no_person] -> {:error, msg} {:different_actors, _} -> diff --git a/lib/federation/activity_pub/types/actors.ex b/lib/federation/activity_pub/types/actors.ex index 1e0cdc673..66f7fd781 100644 --- a/lib/federation/activity_pub/types/actors.ex +++ b/lib/federation/activity_pub/types/actors.ex @@ -131,7 +131,8 @@ defmodule Mobilizon.Federation.ActivityPub.Types.Actors do end end - def follow(%Actor{} = follower_actor, %Actor{} = followed, _local, additional) do + def follow(%Actor{} = follower_actor, %Actor{type: type} = followed, _local, additional) + when type != :Person do with {:ok, %Follower{} = follower} <- Mobilizon.Actors.follow(followed, follower_actor, additional["activity_id"], false), :ok <- FollowMailer.send_notification_to_admins(follower), @@ -140,6 +141,8 @@ defmodule Mobilizon.Federation.ActivityPub.Types.Actors do end end + def follow(_, _, _, _), do: {:error, :no_person, "Only group and instances can be followed"} + defp prepare_args_for_actor(args) do args |> maybe_sanitize_username() diff --git a/test/federation/activity_pub/transmogrifier/follow_test.exs b/test/federation/activity_pub/transmogrifier/follow_test.exs index db99d54ab..10a2666cd 100644 --- a/test/federation/activity_pub/transmogrifier/follow_test.exs +++ b/test/federation/activity_pub/transmogrifier/follow_test.exs @@ -1,6 +1,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do use Mobilizon.DataCase + import ExUnit.CaptureLog import Mobilizon.Factory alias Mobilizon.Actors alias Mobilizon.Actors.Follower @@ -8,9 +9,25 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do alias Mobilizon.Federation.ActivityPub.{Activity, Transmogrifier} describe "handle incoming follow requests" do - test "it works for incoming follow requests" do + test "it works only for groups" do actor = insert(:actor) + data = + File.read!("test/fixtures/mastodon-follow-activity.json") + |> Jason.decode!() + |> Map.put("object", actor.url) + + assert capture_log(fn -> + :error = Transmogrifier.handle_incoming(data) + end) =~ "Only group and instances can be followed" + + actor = Actors.get_actor_with_preload(actor.id) + refute Actors.is_following(Actors.get_actor_by_url!(data["actor"], true), actor) + end + + test "it works for incoming follow requests" do + actor = insert(:group) + data = File.read!("test/fixtures/mastodon-follow-activity.json") |> Jason.decode!() @@ -27,7 +44,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do end test "it rejects activities without a valid ID" do - actor = insert(:actor) + actor = insert(:group) data = File.read!("test/fixtures/mastodon-follow-activity.json") @@ -59,7 +76,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do describe "handle incoming follow accept activities" do test "it works for incoming accepts" do follower = insert(:actor) - followed = insert(:actor, manually_approves_followers: false) + followed = insert(:group, manually_approves_followers: false) refute Actors.is_following(follower, followed) @@ -91,7 +108,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do test "it works for incoming accepts which were pre-accepted" do follower = insert(:actor) - followed = insert(:actor, manually_approves_followers: true) + followed = insert(:group, manually_approves_followers: true) refute Actors.is_following(follower, followed) @@ -126,7 +143,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do test "it works for incoming accepts which are referenced by IRI only" do follower = insert(:actor) - followed = insert(:actor, manually_approves_followers: true) + followed = insert(:group, manually_approves_followers: true) {:ok, follow_activity, _} = ActivityPub.follow(follower, followed) @@ -148,7 +165,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do test "it fails for incoming accepts which cannot be correlated" do follower = insert(:actor) - followed = insert(:actor) + followed = insert(:group) accept_data = File.read!("test/fixtures/mastodon-accept-activity.json") @@ -169,7 +186,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do describe "handle incoming follow reject activities" do test "it fails for incoming rejects which cannot be correlated" do follower = insert(:actor) - followed = insert(:actor) + followed = insert(:group) accept_data = File.read!("test/fixtures/mastodon-reject-activity.json") @@ -188,7 +205,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.FollowTest do test "it works for incoming rejects which are referenced by IRI only" do follower = insert(:actor) - followed = insert(:actor) + followed = insert(:group) {:ok, follow_activity, _} = ActivityPub.follow(follower, followed) diff --git a/test/federation/activity_pub/transmogrifier/undo_test.exs b/test/federation/activity_pub/transmogrifier/undo_test.exs index 777bba458..d24ee52c6 100644 --- a/test/federation/activity_pub/transmogrifier/undo_test.exs +++ b/test/federation/activity_pub/transmogrifier/undo_test.exs @@ -46,7 +46,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.UndoTest do end test "it works for incomming unfollows with an existing follow" do - actor = insert(:actor) + actor = insert(:group) follow_data = File.read!("test/fixtures/mastodon-follow-activity.json")