Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifications: small fixes found after reviewer #10996

Merged
merged 9 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion common
31 changes: 28 additions & 3 deletions readthedocs/notifications/messages.py
Original file line number Diff line number Diff line change
@@ -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 (
Expand All @@ -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):
Expand All @@ -29,8 +34,18 @@ 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.
"""
return {key: escape(value) for key, value in format_values.items()}
humitos marked this conversation as resolved.
Show resolved Hide resolved

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:
Expand All @@ -55,10 +70,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))
Copy link
Member

Choose a reason for hiding this comment

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

Since we have control over the notification, I think this should fail hard, our tests should catch these instead of silently failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't catch this by tests since this rendering happens at runtime. This could happen because a Notification was created with invalid format values, or because we have change the message over time and started required more format values.

Copy link
Member

Choose a reason for hiding this comment

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

The idea here would be to test for each notification we generate on each part of the code base. If we generate a notification when the user does something, we should have a test for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine, we can test the current state, but it could fail still in the future:

  1. we have a notification with body="Go to {url} and put {code}." and format_values={"url": "https://..", "code": 1234}
  2. in the future, we change the message to body="Go to {url} and put {code}. Otherwise, contact support at {email}

At this point, all the old notifications with only url and code as format values will start to fail because they are missing the email key. This check here is to protect ourselves against this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will find some edge cases as we go that we can improve and write test cases for them and protections like this one against them.


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.
Expand Down
32 changes: 32 additions & 0 deletions readthedocs/notifications/tests/test_messages.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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_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: "
2 changes: 1 addition & 1 deletion readthedocs/subscriptions/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
),
Expand Down