Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions pontoon/base/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

from pathlib import Path

import bleach

from django import forms
from django.conf import settings
from django.core.exceptions import ValidationError
Expand All @@ -27,13 +25,7 @@ class HtmlField(forms.CharField):

def clean(self, value):
value = super().clean(value)
value = bleach.clean(
value,
strip=True,
tags=settings.ALLOWED_TAGS,
attributes=settings.ALLOWED_ATTRIBUTES,
)
return value
return utils.sanitize_html(value)


class NoTabStopCharField(forms.CharField):
Expand Down
21 changes: 11 additions & 10 deletions pontoon/base/templatetags/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

from allauth.socialaccount.adapter import get_adapter
from allauth.utils import get_request_param
from bleach.linkifier import Linker
from django_jinja import library
from justhtml import JustHTML, Linkify, SetAttrs, Unwrap

from django import template
from django.conf import settings
Expand Down Expand Up @@ -297,15 +297,16 @@ def as_plain_message(translation):
def linkify(source):
"""Render URLs in the string as links."""

def set_attrs(attrs, new=False):
attrs[(None, "target")] = "_blank"
attrs[(None, "rel")] = "noopener noreferrer"
return attrs

# Escape all tags
linker = Linker(callbacks=[set_attrs])

return linker.linkify(source)
return JustHTML(
source,
fragment=True,
sanitize=False,
transforms=[
Linkify(),
Unwrap('a[href^="mailto:"]'),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is added in a separate commit.

Unlike Bleach, JustHTML linkifies email addresses, and there is no flag to prevent that. This removes all mailto links, including those added by the user. Feels like something that we should be OK doing.

SetAttrs("a", target="_blank", rel="noopener noreferrer"),
],
).to_html(pretty=False)


@library.filter
Expand Down
37 changes: 37 additions & 0 deletions pontoon/base/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
format_datetime,
full_static,
full_url,
linkify,
metric_prefix,
nospam,
to_json,
Expand Down Expand Up @@ -100,3 +101,39 @@ def test_user_editor_theme_anonymous_resolves_to_default():
"""Anonymous users get the default editor theme."""
anon = AnonymousUser()
assert user_editor_theme(anon) == UserProfile.DEFAULT_EDITOR_THEME


@pytest.mark.parametrize(
"source,expected",
(
("no links here", "no links here"),
(
"See https://pontoon.mozilla.org/?a=1&b=2 now",
"See "
'<a href="https://pontoon.mozilla.org/?a=1&amp;b=2" target="_blank" '
'rel="noopener noreferrer">https://pontoon.mozilla.org/?a=1&amp;b=2</a>'
" now",
),
(
"Go to www.mozilla.org.",
'Go to <a href="http://www.mozilla.org" target="_blank" '
'rel="noopener noreferrer">www.mozilla.org</a>.',
),
# Existing links are left alone, but get the same attributes
(
'<a href="https://mozilla.org">Mozilla</a>',
'<a href="https://mozilla.org" target="_blank" '
'rel="noopener noreferrer">Mozilla</a>',
),
("<b>bold</b> & <i>italic</i>", "<b>bold</b> &amp; <i>italic</i>"),
# Email addresses are not linkified
("contact me@example.com", "contact me@example.com"),
(
"me@example.com or https://mozilla.org",
'me@example.com or <a href="https://mozilla.org" target="_blank" '
'rel="noopener noreferrer">https://mozilla.org</a>',
),
),
)
def test_helper_linkify(source, expected):
assert linkify(source) == expected
39 changes: 38 additions & 1 deletion pontoon/base/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.urls import reverse
from django.urls.exceptions import NoReverseMatch

from pontoon.base.utils import get_m2m_changes, get_search_phrases
from pontoon.base.utils import get_m2m_changes, get_search_phrases, sanitize_html
from pontoon.test.factories import (
LocaleCodeHistoryFactory,
LocaleFactory,
Expand Down Expand Up @@ -585,3 +585,40 @@ def test_get_m2m_mixed(user_a, user_b, user_c):
)
def test_get_search_phrases(search_query, expected_results):
assert get_search_phrases(search_query) == expected_results


@pytest.mark.parametrize(
"html,expected",
(
("", ""),
("plain text", "plain text"),
# Allowed tags and attributes are kept
(
'<p>Hello <b>bold</b> <a href="https://mozilla.org" target="_blank" '
'title="t">link</a></p>',
'<p>Hello <b>bold</b> <a href="https://mozilla.org" target="_blank" '
'title="t">link</a></p>',
),
("<ul><li>one</li></ul><br>", "<ul><li>one</li></ul><br>"),
# Disallowed tags are stripped, but their content is kept
('<div class="c">div <em>em</em></div>', "div <em>em</em>"),
("<img src=x onerror=alert(1)>text", "text"),
# Dangerous content is dropped entirely
("a<script>alert(1)</script>b", "ab"),
# Disallowed attributes and URL schemes are removed
(
'<a href="https://mozilla.org" onclick="x()">l</a>',
'<a href="https://mozilla.org">l</a>',
),
('<a href="javascript:alert(1)">l</a>', "<a>l</a>"),
(
'<a href="mailto:a@example.com">m</a>',
'<a href="mailto:a@example.com">m</a>',
),
('<p style="color:red">styled</p>', "<p>styled</p>"),
# Text is escaped
("Tom & Jerry <3", "Tom &amp; Jerry &lt;3"),
),
)
def test_sanitize_html(html, expected):
assert sanitize_html(html) == expected
2 changes: 1 addition & 1 deletion pontoon/base/tests/views/test_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def test_add_comment(member, translation_a):
def test_add_comment_sanitizes_html(member, entity_a, locale_a, project_locale_a):
url = reverse("pontoon.add_comment")

payload = "<svg><script>alert(1)</script>safe"
payload = "<svg><script>alert(1)</script></svg>safe"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that the missing </svg> was a mistake, since the test was checking that <script> is removed.

Without the closing tag, safe is considered part of svg and stripped.


response = member.client.post(
url,
Expand Down
25 changes: 25 additions & 0 deletions pontoon/base/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
from datetime import datetime
from xml.sax.saxutils import escape, quoteattr

from justhtml import JustHTML, SanitizationPolicy, UrlPolicy, UrlRule

from django.conf import settings
from django.http import HttpResponseBadRequest
from django.utils.text import slugify
from django.utils.timezone import make_aware
Expand Down Expand Up @@ -213,3 +216,25 @@ def get_search_phrases(search):

def parse_bool(value) -> bool:
return str(value).lower() in ("1", "true", "yes", "on")


@functools.cache
def _html_sanitization_policy():
return SanitizationPolicy(
allowed_tags=frozenset(settings.ALLOWED_TAGS),
allowed_attributes=settings.ALLOWED_ATTRIBUTES,
url_policy=UrlPolicy(
allow_rules={
("a", "href"): UrlRule(allowed_schemes={"http", "https", "mailto"}),
}
),
# Strip disallowed tags but keep their text content
disallowed_tag_handling="unwrap",
)


def sanitize_html(text):
"""Strip HTML tags and attributes not in ALLOWED_TAGS and ALLOWED_ATTRIBUTES."""
return JustHTML(text, fragment=True, policy=_html_sanitization_policy()).to_html(
pretty=False
)
2 changes: 1 addition & 1 deletion pontoon/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1047,7 +1047,7 @@ def _default_from_email():
# See docs/admin/deployment.rst for more information.
ENABLE_INSIGHTS = os.environ.get("ENABLE_INSIGHTS", "False") != "False"

# Bleach tags and attributes
# Tags and attributes allowed in user-submitted HTML
ALLOWED_TAGS = [
"a",
"abbr",
Expand Down
13 changes: 3 additions & 10 deletions pontoon/teams/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

from typing import cast

import bleach

from guardian.decorators import permission_required_or_403

from django.conf import settings
Expand Down Expand Up @@ -53,7 +51,7 @@
user_locale_role,
user_role,
)
from pontoon.base.utils import require_AJAX
from pontoon.base.utils import require_AJAX, sanitize_html
from pontoon.contributors.views import ContributorsMixin
from pontoon.insights.utils import get_locale_health_insights, get_locale_insights
from pontoon.teams.forms import LocaleRequestForm
Expand Down Expand Up @@ -231,12 +229,7 @@ def ajax_info(request, locale):
@transaction.atomic
def ajax_update_info(request, locale):
team_description = request.POST.get("team_info", None)
team_description = bleach.clean(
team_description,
strip=True,
tags=settings.ALLOWED_TAGS,
attributes=settings.ALLOWED_ATTRIBUTES,
)
team_description = sanitize_html(team_description)
locale = get_object_or_404(Locale, code=locale)
locale.team_description = team_description
locale.save()
Expand Down Expand Up @@ -268,7 +261,7 @@ def ajax_permissions(request, locale):
else:
errors = locale_form.errors
errors.update(project_locale_form.errors_dict)
error_msg = bleach.clean(json.dumps(errors))
error_msg = sanitize_html(json.dumps(errors))
return HttpResponseBadRequest(error_msg)

else:
Expand Down
2 changes: 1 addition & 1 deletion requirements/base.in
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
# Dependencies that do not come from pypi (eg. eggs from github) are listed at the end of the list.
# -------------------------------------------------------------------------------------------------
beautifulsoup4==4.12.3
bleach==6.4.0
celery==5.4.0
compare-locales==9.0.4
dj-database-url==3.1.2
Expand All @@ -36,6 +35,7 @@ django-pipeline==4.1.0
drf-spectacular[sidecar]==0.29.0
google-cloud-translate==3.16.0
gunicorn==23.0.0
justhtml==3.11.2
markupsafe==2.0.1
moz.l10n[xml]==0.14.1
newrelic==9.6.0
Expand Down
18 changes: 6 additions & 12 deletions requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,6 @@ billiard==4.2.4 \
# via
# -c prod.txt
# celery
bleach==6.4.0 \
--hash=sha256:4202482733d85cedd04e59fcb2f89f4e4c7c385a78d3c3c23c30446843a37452 \
--hash=sha256:4b6b6a54fff2e69a3dde9d21cc6301220bee3c3cb792187d11403fd795031081
# via
# -c prod.txt
# -r base.in
celery==5.4.0 \
--hash=sha256:369631eb580cf8c51a82721ec538684994f8277637edde2dfc0dacd73ed97f64 \
--hash=sha256:504a19140e8d3029d5acad88330c541d4c3f64c789d85f94756762d8bca7e706
Expand Down Expand Up @@ -952,6 +946,12 @@ jsonschema-specifications==2025.9.1 \
# via
# -c prod.txt
# jsonschema
justhtml==3.11.2 \
--hash=sha256:366986de83fab5f7ab643f79ffb3ad44dbaeb63f34b6361b955930575417cdd2 \
--hash=sha256:83329a7436620a79ebd0b7310b5e4d653fa0688cd315e7a92d2188febd0d6ab0
# via
# -c prod.txt
# -r base.in
kombu==5.6.2 \
--hash=sha256:8060497058066c6f5aed7c26d7cd0d3b574990b09de842a8c5aaed0b92cc5a55 \
--hash=sha256:efcfc559da324d41d61ca311b0c64965ea35b4c55cc04ee36e55386145dace93
Expand Down Expand Up @@ -2499,12 +2499,6 @@ wcwidth==0.6.0 \
# via
# -c prod.txt
# prompt-toolkit
webencodings==0.5.1 \
--hash=sha256:a0af1213f3c2226497a97e2b3aa01a7e4bee4f403f95be16fc9acd2947514a78 \
--hash=sha256:b36a1c245f2d304965eb4e0a82848379241dc04b865afcc4aab16748587e1923
# via
# -c prod.txt
# bleach
wheel==0.46.3 \
--hash=sha256:4b399d56c9d9338230118d705d9737a2a468ccca63d5e813e2a4fc7815d8bc4d \
--hash=sha256:e3e79874b07d776c40bd6033f8ddf76a7dad46a7b8aa1b2787a83083519a1803
Expand Down
12 changes: 4 additions & 8 deletions requirements/prod.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ billiard==4.2.4 \
--hash=sha256:525b42bdec68d2b983347ac312f892db930858495db601b5836ac24e6477cde5 \
--hash=sha256:55f542c371209e03cd5862299b74e52e4fbcba8250ba611ad94276b369b6a85f
# via celery
bleach==6.4.0 \
--hash=sha256:4202482733d85cedd04e59fcb2f89f4e4c7c385a78d3c3c23c30446843a37452 \
--hash=sha256:4b6b6a54fff2e69a3dde9d21cc6301220bee3c3cb792187d11403fd795031081
# via -r base.in
celery==5.4.0 \
--hash=sha256:369631eb580cf8c51a82721ec538684994f8277637edde2dfc0dacd73ed97f64 \
--hash=sha256:504a19140e8d3029d5acad88330c541d4c3f64c789d85f94756762d8bca7e706
Expand Down Expand Up @@ -749,6 +745,10 @@ jsonschema-specifications==2025.9.1 \
--hash=sha256:98802fee3a11ee76ecaca44429fda8a41bff98b00a0f2838151b113f210cc6fe \
--hash=sha256:b540987f239e745613c7a9176f3edb72b832a4ac465cf02712288397832b5e8d
# via jsonschema
justhtml==3.11.2 \
--hash=sha256:366986de83fab5f7ab643f79ffb3ad44dbaeb63f34b6361b955930575417cdd2 \
--hash=sha256:83329a7436620a79ebd0b7310b5e4d653fa0688cd315e7a92d2188febd0d6ab0
# via -r base.in
kombu==5.6.2 \
--hash=sha256:8060497058066c6f5aed7c26d7cd0d3b574990b09de842a8c5aaed0b92cc5a55 \
--hash=sha256:efcfc559da324d41d61ca311b0c64965ea35b4c55cc04ee36e55386145dace93
Expand Down Expand Up @@ -1941,10 +1941,6 @@ wcwidth==0.6.0 \
--hash=sha256:1a3a1e510b553315f8e146c54764f4fb6264ffad731b3d78088cdb1478ffbdad \
--hash=sha256:cdc4e4262d6ef9a1a57e018384cbeb1208d8abbc64176027e2c2455c81313159
# via prompt-toolkit
webencodings==0.5.1 \
--hash=sha256:a0af1213f3c2226497a97e2b3aa01a7e4bee4f403f95be16fc9acd2947514a78 \
--hash=sha256:b36a1c245f2d304965eb4e0a82848379241dc04b865afcc4aab16748587e1923
# via bleach
wheel==0.46.3 \
--hash=sha256:4b399d56c9d9338230118d705d9737a2a468ccca63d5e813e2a4fc7815d8bc4d \
--hash=sha256:e3e79874b07d776c40bd6033f8ddf76a7dad46a7b8aa1b2787a83083519a1803
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ function ProjectInfoDialog({
const ref = useRef<HTMLElement>(null);
useOnDiscard(ref, onDiscard);

// We can safely use project.info because it is validated by bleach
// We can safely use project.info because it is sanitized on the server
// before being saved into the database.
return (
<aside ref={ref} className='panel'>
Expand Down