diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 2f0d01a5bbe..6fef2648e9f 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -1,5 +1,8 @@ import textwrap +from collections import defaultdict +import structlog +from django.utils.html import escape from django.utils.translation import gettext_noop as _ from readthedocs.doc_builder.exceptions import ( @@ -13,6 +16,8 @@ from .constants import ERROR, INFO, NOTE, TIP, WARNING +log = structlog.get_logger(__name__) + class Message: def __init__(self, id, header, body, type, icon_classes=None): @@ -29,8 +34,26 @@ def __repr__(self): def __str__(self): return f"Message: {self.id} | {self.header}" + def _escape_format_values(self, format_values): + """ + Escape all potential HTML tags included in format values. + + This is a protection against rendering potential values defined by the user. + It uses the Django's util function ``escape`` (similar to ``|escape`` template tag filter) + to convert HTML characters into regular characters. + + NOTE: currently, we don't support values that are not ``str`` or ``int``. + If we want to support other types or nested dictionaries, + we will need to iterate recursively to apply the ``escape`` function. + """ + return { + key: escape(value) + for key, value in format_values.items() + if isinstance(value, (str, int)) + } + def set_format_values(self, format_values): - self.format_values = format_values or {} + self.format_values = self._escape_format_values(format_values) def get_display_icon_classes(self): if self.icon_classes: @@ -55,10 +78,20 @@ def get_display_icon_classes(self): return " ".join(classes) def get_rendered_header(self): - return self.header.format(**self.format_values) + try: + return self.header.format(**self.format_values) + except KeyError: + # There was a key missing + log.exception("There was a missing key when formating a header's Message.") + return self.header.format_map(defaultdict(str, **self.format_values)) def get_rendered_body(self): - return self.body.format(**self.format_values) + try: + return self.body.format(**self.format_values) + except KeyError: + # There was a key missing + log.exception("There was a missing key when formating a body's Message.") + return self.body.format_map(defaultdict(str, **self.format_values)) # TODO: review the copy of these notifications/messages on PR review and adapt them. diff --git a/readthedocs/notifications/tests/test_messages.py b/readthedocs/notifications/tests/test_messages.py new file mode 100644 index 00000000000..edc7f9dd250 --- /dev/null +++ b/readthedocs/notifications/tests/test_messages.py @@ -0,0 +1,51 @@ +from readthedocs.notifications.constants import INFO +from readthedocs.notifications.messages import Message + + +class TestMessage: + def test_xss_protection(self): + message = Message( + id="test", + header="XSS: {header}", + body="XSS: {body}", + type=INFO, + ) + message.set_format_values( + { + "header": "

xss

", + "body": "xss", + } + ) + + assert message.get_rendered_header() == "XSS: <p>xss</p>" + assert message.get_rendered_body() == "XSS: <span>xss</span>" + + def test_invalid_format_values_type(self): + message = Message( + id="test", + header="Header: {dict}", + body="Body: {dict}", + type=INFO, + ) + message.set_format_values( + { + "dict": { + "key": "value", + }, + } + ) + + # The rendered version skips the ``dict`` because it's not supported + assert message.get_rendered_header() == "Header: " + assert message.get_rendered_body() == "Body: " + + def test_missing_key_format_values(self): + message = Message( + id="test", + header="Missing format value: {header}", + body="Missing format value: {body}", + type=INFO, + ) + + assert message.get_rendered_header() == "Missing format value: " + assert message.get_rendered_body() == "Missing format value: " diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 257c8cfd39f..dc48beb465d 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -11,7 +11,9 @@ MESSAGE_OAUTH_WEBHOOK_NO_ACCOUNT = "oauth:webhook:no-account" MESSAGE_OAUTH_WEBHOOK_INVALID = "oauth:webhook:invalid" MESSAGE_OAUTH_BUILD_STATUS_FAILURE = "oauth:status:send-failed" -MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_SUCCESSFULY = "oauth:deploy-key:attached-successfully" +MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_SUCCESSFULLY = ( + "oauth:deploy-key:attached-successfully" +) MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_FAILED = "oauth:deploy-key:attached-failed" messages = [ @@ -71,7 +73,7 @@ type=ERROR, ), Message( - id=MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_SUCCESSFULY, + id=MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_SUCCESSFULLY, header=_("Deploy key added successfully"), body=_( textwrap.dedent( diff --git a/readthedocs/subscriptions/notifications.py b/readthedocs/subscriptions/notifications.py index 3ec27f094ff..f15ed53eb3c 100644 --- a/readthedocs/subscriptions/notifications.py +++ b/readthedocs/subscriptions/notifications.py @@ -93,7 +93,7 @@ def for_organizations(cls): body=_( textwrap.dedent( """ - The organization "{instance.name}" is currently disabled. You need to renew your subscription to keep using Read the Docs + The organization "{instance.name}" is currently disabled. You need to renew your subscription to keep using Read the Docs """ ).strip(), ),