Merge branch 'drop-html-sanitize-ex-and-fix-title-escaping' into 'master'

Drop HTMLSanitizeEx and fix title sanitizing

See merge request framasoft/mobilizon!490
This commit is contained in:
Thomas Citharel 2020-06-24 16:52:28 +02:00
commit ac623cf197
12 changed files with 40 additions and 17 deletions

View File

@ -6,9 +6,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## Unreleased ## Unreleased
### Changed
- Completely replaced HTMLSanitizeEx with FastSanitize [!490](https://framagit.org/framasoft/mobilizon/-/merge_requests/490)
### Fixed ### 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 ## [1.0.0-beta.3] - 2020-06-24

View File

@ -45,6 +45,7 @@ defmodule Mobilizon.Federation.ActivityPub do
alias Mobilizon.Federation.WebFinger alias Mobilizon.Federation.WebFinger
alias Mobilizon.GraphQL.API.Utils, as: APIUtils alias Mobilizon.GraphQL.API.Utils, as: APIUtils
alias Mobilizon.Service.Formatter.HTML
alias Mobilizon.Service.Notifications.Scheduler alias Mobilizon.Service.Notifications.Scheduler
alias Mobilizon.Service.RichMedia.Parser alias Mobilizon.Service.RichMedia.Parser
@ -499,7 +500,7 @@ defmodule Mobilizon.Federation.ActivityPub do
metadata: metadata:
additional additional
|> Map.get(:metadata, %{}) |> 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), join_data <- Convertible.model_to_as(participant),
audience <- audience <-
@ -1257,7 +1258,7 @@ defmodule Mobilizon.Federation.ActivityPub do
# If title is not set: we are not updating it # If title is not set: we are not updating it
args = args =
if Map.has_key?(args, :title) && !is_nil(args.title), 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 else: args
# If we've been given a description (we might not get one if updating) # 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 defp prepare_args_for_group(args) do
with preferred_username <- 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 <- args |> Map.get(:summary, "") |> String.trim(),
{summary, _mentions, _tags} <- {summary, _mentions, _tags} <-
summary |> String.trim() |> APIUtils.make_content_html([], "text/html") do 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)}, {:reporter, Actors.get_actor!(args.reporter_id)},
{:reported, %Actor{} = reported_actor} <- {:reported, %Actor{} = reported_actor} <-
{:reported, Actors.get_actor!(args.reported_id)}, {: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)), event <- Conversations.get_comment(Map.get(args, :event_id)),
{:get_report_comments, comments} <- {:get_report_comments, comments} <-
{:get_report_comments, {:get_report_comments,

View File

@ -8,6 +8,7 @@ defmodule Mobilizon.GraphQL.API.Groups do
alias Mobilizon.Federation.ActivityPub alias Mobilizon.Federation.ActivityPub
alias Mobilizon.Federation.ActivityPub.Activity alias Mobilizon.Federation.ActivityPub.Activity
alias Mobilizon.Service.Formatter.HTML
@doc """ @doc """
Create a group Create a group
@ -15,7 +16,7 @@ defmodule Mobilizon.GraphQL.API.Groups do
@spec create_group(map) :: {:ok, Activity.t(), Actor.t()} | any @spec create_group(map) :: {:ok, Activity.t(), Actor.t()} | any
def create_group(args) do def create_group(args) do
with preferred_username <- 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, nil} <-
{:existing_group, Actors.get_local_group_by_title(preferred_username)}, {:existing_group, Actors.get_local_group_by_title(preferred_username)},
{:ok, %Activity{} = activity, %Actor{} = group} <- {:ok, %Activity{} = activity, %Actor{} = group} <-

View File

@ -7,6 +7,7 @@ defmodule Mobilizon.Service.Export.ICalendar do
alias Mobilizon.Actors.Actor alias Mobilizon.Actors.Actor
alias Mobilizon.Addresses.Address alias Mobilizon.Addresses.Address
alias Mobilizon.Events.{Event, FeedToken} alias Mobilizon.Events.{Event, FeedToken}
alias Mobilizon.Service.Formatter.HTML
alias Mobilizon.Storage.Page alias Mobilizon.Storage.Page
alias Mobilizon.Users.User alias Mobilizon.Users.User
@ -31,7 +32,7 @@ defmodule Mobilizon.Service.Export.ICalendar do
dtstart: event.begins_on, dtstart: event.begins_on,
dtstamp: event.publish_at || DateTime.utc_now(), dtstamp: event.publish_at || DateTime.utc_now(),
dtend: event.ends_on, dtend: event.ends_on,
description: HtmlSanitizeEx.strip_tags(event.description), description: HTML.strip_tags(event.description),
uid: event.uuid, uid: event.uuid,
url: event.url, url: event.url,
geo: Address.coords(event.physical_address), geo: Address.coords(event.physical_address),

View File

@ -14,5 +14,15 @@ defmodule Mobilizon.Service.Formatter.HTML do
def filter_tags(html), do: Sanitizer.scrub(html, DefaultScrubbler) 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) def filter_tags_for_oembed(html), do: Sanitizer.scrub(html, OEmbed)
end end

View File

@ -2,6 +2,7 @@ defimpl Mobilizon.Service.Metadata, for: Mobilizon.Events.Event do
alias Phoenix.HTML alias Phoenix.HTML
alias Phoenix.HTML.Tag alias Phoenix.HTML.Tag
alias Mobilizon.Events.Event alias Mobilizon.Events.Event
alias Mobilizon.Service.Formatter.HTML, as: HTMLFormatter
alias Mobilizon.Web.JsonLD.ObjectView alias Mobilizon.Web.JsonLD.ObjectView
alias Mobilizon.Web.MediaProxy alias Mobilizon.Web.MediaProxy
import Mobilizon.Web.Gettext import Mobilizon.Web.Gettext
@ -49,15 +50,15 @@ defimpl Mobilizon.Service.Metadata, for: Mobilizon.Events.Event do
defp process_description(description, _locale) do defp process_description(description, _locale) do
description description
|> HtmlSanitizeEx.strip_tags() |> HTMLFormatter.strip_tags()
|> String.slice(0..200) |> String.slice(0..200)
|> (&"#{&1}").() |> (&"#{&1}").()
end end
# Insert JSON-LD schema by hand because Tag.content_tag wants to escape it # 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" "event.json"
|> ObjectView.render(%{event: event}) |> ObjectView.render(%{event: %{event | title: HTMLFormatter.strip_tags(title)}})
|> Jason.encode!() |> Jason.encode!()
end end
end end

View File

@ -7,6 +7,7 @@ defmodule Mobilizon.Service.Metadata.Instance do
alias Phoenix.HTML.Tag alias Phoenix.HTML.Tag
alias Mobilizon.Config alias Mobilizon.Config
alias Mobilizon.Service.Formatter.HTML, as: HTMLFormatter
alias Mobilizon.Web.Endpoint alias Mobilizon.Web.Endpoint
def build_tags do def build_tags do
@ -40,7 +41,7 @@ defmodule Mobilizon.Service.Metadata.Instance do
defp process_description(description) do defp process_description(description) do
description description
|> HtmlSanitizeEx.strip_tags() |> HTMLFormatter.strip_tags()
|> String.slice(0..200) |> String.slice(0..200)
|> (&"#{&1}").() |> (&"#{&1}").()
end end

View File

@ -7,6 +7,7 @@ defmodule Mobilizon.Service.Workers.BuildSearch do
alias Mobilizon.Events alias Mobilizon.Events
alias Mobilizon.Events.Event alias Mobilizon.Events.Event
alias Mobilizon.Service.Formatter.HTML
alias Mobilizon.Storage.Repo alias Mobilizon.Storage.Repo
use Mobilizon.Service.Workers.Helper, queue: "search" use Mobilizon.Service.Workers.Helper, queue: "search"
@ -44,7 +45,7 @@ defmodule Mobilizon.Service.Workers.BuildSearch do
[ [
event.id, event.id,
event.title, event.title,
HtmlSanitizeEx.strip_tags(event.description), HTML.strip_tags(event.description),
get_tags_string(event) get_tags_string(event)
] ]
) )

View File

@ -66,7 +66,7 @@
<h3><%= gettext "Comments" %></h3> <h3><%= gettext "Comments" %></h3>
<%= for comment <- @report.comments do %> <%= for comment <- @report.comments do %>
<p style="margin: 0;"> <p style="margin: 0;">
<%= HtmlSanitizeEx.strip_tags(comment.text) %> <%= Mobilizon.Service.Formatter.HTML.strip_tags(comment.text) %>
</p> </p>
<% end %> <% end %>
<table cellspacing="0" cellpadding="0" border="0" width="100%" style="width: 100% !important;"> <table cellspacing="0" cellpadding="0" border="0" width="100%" style="width: 100% !important;">

View File

@ -96,7 +96,6 @@ defmodule Mobilizon.Mixfile do
{:http_signatures, {:http_signatures,
git: "https://git.pleroma.social/pleroma/http_signatures.git", git: "https://git.pleroma.social/pleroma/http_signatures.git",
ref: "293d77bb6f4a67ac8bde1428735c3b42f22cbb30"}, ref: "293d77bb6f4a67ac8bde1428735c3b42f22cbb30"},
{:html_sanitize_ex, "~> 1.4.0"},
{:ex_cldr, "~> 2.0"}, {:ex_cldr, "~> 2.0"},
{:ex_cldr_dates_times, "~> 2.2"}, {:ex_cldr_dates_times, "~> 2.2"},
{:ex_optimizer, "~> 0.1"}, {:ex_optimizer, "~> 0.1"},

View File

@ -7,6 +7,7 @@ defmodule Mobilizon.GraphQL.API.ReportTest do
alias Mobilizon.Conversations.Comment alias Mobilizon.Conversations.Comment
alias Mobilizon.Events.Event alias Mobilizon.Events.Event
alias Mobilizon.Reports.{Note, Report} alias Mobilizon.Reports.{Note, Report}
alias Mobilizon.Service.Formatter.HTML
alias Mobilizon.Users alias Mobilizon.Users
alias Mobilizon.Users.User alias Mobilizon.Users.User
@ -92,7 +93,7 @@ defmodule Mobilizon.GraphQL.API.ReportTest do
_comment_2 = insert(:comment, actor: reported) _comment_2 = insert(:comment, actor: reported)
comment = "This is really not acceptable, remote admin I don't know" 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, _} = assert {:ok, %Activity{} = flag_activity, _} =
Reports.report(%{ Reports.report(%{

View File

@ -193,7 +193,7 @@ defmodule Mobilizon.Web.Resolvers.EventTest do
) )
end 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, conn: conn,
actor: actor, actor: actor,
user: user user: user
@ -214,7 +214,9 @@ defmodule Mobilizon.Web.Resolvers.EventTest do
) )
assert res["errors"] == nil assert res["errors"] == nil
assert res["data"]["createEvent"]["title"] == "My Event title"
assert res["data"]["createEvent"]["title"] ==
"My Event title <img src=\"http://placekitten.com/g/200/300\" onclick=\"alert('aaa')\" >"
assert res["data"]["createEvent"]["description"] == assert res["data"]["createEvent"]["description"] ==
"<b>My description</b> <img src=\"http://placekitten.com/g/200/300\"/>" "<b>My description</b> <img src=\"http://placekitten.com/g/200/300\"/>"