From b2e817bbdd37e21e0f926ee6111c4a912b2032d1 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 24 Jun 2020 16:33:59 +0200 Subject: [PATCH] Drop HTMLSanitizeEx and fix title sanitizing Signed-off-by: Thomas Citharel --- CHANGELOG.md | 7 ++++++- lib/federation/activity_pub/activity_pub.ex | 9 +++++---- lib/graphql/api/groups.ex | 3 ++- lib/service/export/icalendar.ex | 3 ++- lib/service/formatter/html.ex | 10 ++++++++++ lib/service/metadata/event.ex | 7 ++++--- lib/service/metadata/instance.ex | 3 ++- lib/service/workers/build_search.ex | 3 ++- lib/web/templates/email/report.html.eex | 2 +- mix.exs | 1 - test/graphql/api/report_test.exs | 3 ++- test/graphql/resolvers/event_test.exs | 6 ++++-- 12 files changed, 40 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baec80c89..224ac4ad4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,9 +6,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Changed + +- Completely replaced HTMLSanitizeEx with FastSanitize [!490](https://framagit.org/framasoft/mobilizon/-/merge_requests/490) + ### Fixed -- Fixed notification scheduler (!486)[https://framagit.org/framasoft/mobilizon/-/merge_requests/486] +- Fixed notification scheduler [!486](https://framagit.org/framasoft/mobilizon/-/merge_requests/486) +- Fixed event title escaping [!490](https://framagit.org/framasoft/mobilizon/-/merge_requests/490) ## [1.0.0-beta.3] - 2020-06-24 diff --git a/lib/federation/activity_pub/activity_pub.ex b/lib/federation/activity_pub/activity_pub.ex index 1895d0afb..d6b489d92 100644 --- a/lib/federation/activity_pub/activity_pub.ex +++ b/lib/federation/activity_pub/activity_pub.ex @@ -45,6 +45,7 @@ defmodule Mobilizon.Federation.ActivityPub do alias Mobilizon.Federation.WebFinger alias Mobilizon.GraphQL.API.Utils, as: APIUtils + alias Mobilizon.Service.Formatter.HTML alias Mobilizon.Service.Notifications.Scheduler alias Mobilizon.Service.RichMedia.Parser @@ -499,7 +500,7 @@ defmodule Mobilizon.Federation.ActivityPub do metadata: additional |> Map.get(:metadata, %{}) - |> Map.update(:message, nil, &String.trim(HtmlSanitizeEx.strip_tags(&1))) + |> Map.update(:message, nil, &String.trim(HTML.strip_tags(&1))) }), join_data <- Convertible.model_to_as(participant), audience <- @@ -1257,7 +1258,7 @@ defmodule Mobilizon.Federation.ActivityPub do # If title is not set: we are not updating it args = if Map.has_key?(args, :title) && !is_nil(args.title), - do: Map.update(args, :title, "", &String.trim(HtmlSanitizeEx.strip_tags(&1))), + do: Map.update(args, :title, "", &String.trim/1), else: args # If we've been given a description (we might not get one if updating) @@ -1344,7 +1345,7 @@ defmodule Mobilizon.Federation.ActivityPub do defp prepare_args_for_group(args) do with preferred_username <- - args |> Map.get(:preferred_username) |> HtmlSanitizeEx.strip_tags() |> String.trim(), + args |> Map.get(:preferred_username) |> HTML.strip_tags() |> String.trim(), summary <- args |> Map.get(:summary, "") |> String.trim(), {summary, _mentions, _tags} <- summary |> String.trim() |> APIUtils.make_content_html([], "text/html") do @@ -1357,7 +1358,7 @@ defmodule Mobilizon.Federation.ActivityPub do {:reporter, Actors.get_actor!(args.reporter_id)}, {:reported, %Actor{} = reported_actor} <- {:reported, Actors.get_actor!(args.reported_id)}, - content <- HtmlSanitizeEx.strip_tags(args.content), + content <- HTML.strip_tags(args.content), event <- Conversations.get_comment(Map.get(args, :event_id)), {:get_report_comments, comments} <- {:get_report_comments, diff --git a/lib/graphql/api/groups.ex b/lib/graphql/api/groups.ex index 4ff5371c1..315853ede 100644 --- a/lib/graphql/api/groups.ex +++ b/lib/graphql/api/groups.ex @@ -8,6 +8,7 @@ defmodule Mobilizon.GraphQL.API.Groups do alias Mobilizon.Federation.ActivityPub alias Mobilizon.Federation.ActivityPub.Activity + alias Mobilizon.Service.Formatter.HTML @doc """ Create a group @@ -15,7 +16,7 @@ defmodule Mobilizon.GraphQL.API.Groups do @spec create_group(map) :: {:ok, Activity.t(), Actor.t()} | any def create_group(args) do with preferred_username <- - args |> Map.get(:preferred_username) |> HtmlSanitizeEx.strip_tags() |> String.trim(), + args |> Map.get(:preferred_username) |> HTML.strip_tags() |> String.trim(), {:existing_group, nil} <- {:existing_group, Actors.get_local_group_by_title(preferred_username)}, {:ok, %Activity{} = activity, %Actor{} = group} <- diff --git a/lib/service/export/icalendar.ex b/lib/service/export/icalendar.ex index d4ae660e5..b4ed5c17b 100644 --- a/lib/service/export/icalendar.ex +++ b/lib/service/export/icalendar.ex @@ -7,6 +7,7 @@ defmodule Mobilizon.Service.Export.ICalendar do alias Mobilizon.Actors.Actor alias Mobilizon.Addresses.Address alias Mobilizon.Events.{Event, FeedToken} + alias Mobilizon.Service.Formatter.HTML alias Mobilizon.Storage.Page alias Mobilizon.Users.User @@ -31,7 +32,7 @@ defmodule Mobilizon.Service.Export.ICalendar do dtstart: event.begins_on, dtstamp: event.publish_at || DateTime.utc_now(), dtend: event.ends_on, - description: HtmlSanitizeEx.strip_tags(event.description), + description: HTML.strip_tags(event.description), uid: event.uuid, url: event.url, geo: Address.coords(event.physical_address), diff --git a/lib/service/formatter/html.ex b/lib/service/formatter/html.ex index 532934f68..2f0857800 100644 --- a/lib/service/formatter/html.ex +++ b/lib/service/formatter/html.ex @@ -14,5 +14,15 @@ defmodule Mobilizon.Service.Formatter.HTML do def filter_tags(html), do: Sanitizer.scrub(html, DefaultScrubbler) + def strip_tags(html) do + case FastSanitize.strip_tags(html) do + {:ok, html} -> + html + + _ -> + raise "Failed to filter tags" + end + end + def filter_tags_for_oembed(html), do: Sanitizer.scrub(html, OEmbed) end diff --git a/lib/service/metadata/event.ex b/lib/service/metadata/event.ex index ac4943579..4781ab277 100644 --- a/lib/service/metadata/event.ex +++ b/lib/service/metadata/event.ex @@ -2,6 +2,7 @@ defimpl Mobilizon.Service.Metadata, for: Mobilizon.Events.Event do alias Phoenix.HTML alias Phoenix.HTML.Tag alias Mobilizon.Events.Event + alias Mobilizon.Service.Formatter.HTML, as: HTMLFormatter alias Mobilizon.Web.JsonLD.ObjectView alias Mobilizon.Web.MediaProxy import Mobilizon.Web.Gettext @@ -49,15 +50,15 @@ defimpl Mobilizon.Service.Metadata, for: Mobilizon.Events.Event do defp process_description(description, _locale) do description - |> HtmlSanitizeEx.strip_tags() + |> HTMLFormatter.strip_tags() |> String.slice(0..200) |> (&"#{&1}…").() end # Insert JSON-LD schema by hand because Tag.content_tag wants to escape it - defp json(%Event{} = event) do + defp json(%Event{title: title} = event) do "event.json" - |> ObjectView.render(%{event: event}) + |> ObjectView.render(%{event: %{event | title: HTMLFormatter.strip_tags(title)}}) |> Jason.encode!() end end diff --git a/lib/service/metadata/instance.ex b/lib/service/metadata/instance.ex index cfa15f9da..666464631 100644 --- a/lib/service/metadata/instance.ex +++ b/lib/service/metadata/instance.ex @@ -7,6 +7,7 @@ defmodule Mobilizon.Service.Metadata.Instance do alias Phoenix.HTML.Tag alias Mobilizon.Config + alias Mobilizon.Service.Formatter.HTML, as: HTMLFormatter alias Mobilizon.Web.Endpoint def build_tags do @@ -40,7 +41,7 @@ defmodule Mobilizon.Service.Metadata.Instance do defp process_description(description) do description - |> HtmlSanitizeEx.strip_tags() + |> HTMLFormatter.strip_tags() |> String.slice(0..200) |> (&"#{&1}…").() end diff --git a/lib/service/workers/build_search.ex b/lib/service/workers/build_search.ex index 184f5ad39..fc51b7a7d 100644 --- a/lib/service/workers/build_search.ex +++ b/lib/service/workers/build_search.ex @@ -7,6 +7,7 @@ defmodule Mobilizon.Service.Workers.BuildSearch do alias Mobilizon.Events alias Mobilizon.Events.Event + alias Mobilizon.Service.Formatter.HTML alias Mobilizon.Storage.Repo use Mobilizon.Service.Workers.Helper, queue: "search" @@ -44,7 +45,7 @@ defmodule Mobilizon.Service.Workers.BuildSearch do [ event.id, event.title, - HtmlSanitizeEx.strip_tags(event.description), + HTML.strip_tags(event.description), get_tags_string(event) ] ) diff --git a/lib/web/templates/email/report.html.eex b/lib/web/templates/email/report.html.eex index 227f582ba..13474c9b0 100644 --- a/lib/web/templates/email/report.html.eex +++ b/lib/web/templates/email/report.html.eex @@ -66,7 +66,7 @@

<%= gettext "Comments" %>

<%= for comment <- @report.comments do %>

- <%= HtmlSanitizeEx.strip_tags(comment.text) %> + <%= Mobilizon.Service.Formatter.HTML.strip_tags(comment.text) %>

<% end %> diff --git a/mix.exs b/mix.exs index ec47c1798..745298465 100644 --- a/mix.exs +++ b/mix.exs @@ -96,7 +96,6 @@ defmodule Mobilizon.Mixfile do {:http_signatures, git: "https://git.pleroma.social/pleroma/http_signatures.git", ref: "293d77bb6f4a67ac8bde1428735c3b42f22cbb30"}, - {:html_sanitize_ex, "~> 1.4.0"}, {:ex_cldr, "~> 2.0"}, {:ex_cldr_dates_times, "~> 2.2"}, {:ex_optimizer, "~> 0.1"}, diff --git a/test/graphql/api/report_test.exs b/test/graphql/api/report_test.exs index 645572824..5b0c85fb8 100644 --- a/test/graphql/api/report_test.exs +++ b/test/graphql/api/report_test.exs @@ -7,6 +7,7 @@ defmodule Mobilizon.GraphQL.API.ReportTest do alias Mobilizon.Conversations.Comment alias Mobilizon.Events.Event alias Mobilizon.Reports.{Note, Report} + alias Mobilizon.Service.Formatter.HTML alias Mobilizon.Users alias Mobilizon.Users.User @@ -92,7 +93,7 @@ defmodule Mobilizon.GraphQL.API.ReportTest do _comment_2 = insert(:comment, actor: reported) comment = "This is really not acceptable, remote admin I don't know" - encoded_comment = HtmlSanitizeEx.strip_tags(comment) + encoded_comment = HTML.strip_tags(comment) assert {:ok, %Activity{} = flag_activity, _} = Reports.report(%{ diff --git a/test/graphql/resolvers/event_test.exs b/test/graphql/resolvers/event_test.exs index aaba3c64a..0207b53b2 100644 --- a/test/graphql/resolvers/event_test.exs +++ b/test/graphql/resolvers/event_test.exs @@ -193,7 +193,7 @@ defmodule Mobilizon.Web.Resolvers.EventTest do ) end - test "create_event/3 creates an event and escapes title and description", %{ + test "create_event/3 creates an event and escapes title", %{ conn: conn, actor: actor, user: user @@ -214,7 +214,9 @@ defmodule Mobilizon.Web.Resolvers.EventTest do ) assert res["errors"] == nil - assert res["data"]["createEvent"]["title"] == "My Event title" + + assert res["data"]["createEvent"]["title"] == + "My Event title " assert res["data"]["createEvent"]["description"] == "My description"