Notifications: small fixes found after reviewer (#10996)

* Notifications: small fixes found after reviewer

Use single `{}` since we are using `.format()` to format these strings.

* Handle missing `.format()` key

Avoid internal error and log the exception so we can figure it out how to solve
it. This happens when the `Notification` does not have _all_ the required `format_values`.

* Protection agasint XSS when rendering notifications

See #10922 (comment)

* Test for missing key in format values and XSS protection

* Update common/

* Lint

* Document we only support `str` and `int` for now in `format_values`

We don't support nested dictionaries in `format_values` or random objects.
Only `str` and `int`. That should be enough for now.

Skip all the values that are not `str` or `int` from the format values to render
the messages.

* Typo

Author: humitos

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.

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": "<p>xss</p>",
+                "body": "<span>xss</span>",
+            }
+        )
+
+        assert message.get_rendered_header() == "XSS: &lt;p&gt;xss&lt;/p&gt;"
+        assert message.get_rendered_body() == "XSS: &lt;span&gt;xss&lt;/span&gt;"
+
+    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: "

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(

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 <a href="{{ subscription_url }}">renew your subscription</a> to keep using Read the Docs
+            The organization "{instance.name}" is currently disabled. You need to <a href="{subscription_url}">renew your subscription</a> to keep using Read the Docs
             """
             ).strip(),
         ),