Skip to content

Commit 52f2b3b

Browse files
committed
fix: escape user input in notification content templates
Discussion thread titles (and other user-controlled context fields — replier_name, author_name, username) were interpolated raw into notification.content via `str.format(**context)`. That output is rendered with Django's `|safe` filter in digest_content.html, which is included by both the email_digest and batched_email body templates, so a `<style>` block in a thread title survived into recipient inboxes as executable CSS on email open — enabling open-tracking, content spoofing, and phishing. Escape at the source: in `get_notification_content`, wrap every context value with `django.utils.html.escape` before `template.format(**context)`, exempting the two structural keys (`p`, `strong`) that content_templates use as HTML tag names. This defends every renderer of `notification.content` in one place. This is the incomplete-patch companion of GHSA-4xv3-5j4x-q8g4 (CVE-2026-42857), which sanitized the post body via `clean_thread_html_body()` but did not cover the title path. Fixes GHSA-rv5w-f4r5-h77g. (cherry picked from commit 08b719ce41bb369fa0cabbe8d0547124e64c8566)
1 parent f7f9508 commit 52f2b3b

2 files changed

Lines changed: 57 additions & 2 deletions

File tree

openedx/core/djangoapps/notifications/base_notification.py

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
"""
22
Base setup for Notification Apps and Types.
33
"""
4+
from django.utils.html import escape
45
from django.utils.translation import gettext_lazy as _
56

67
from .email_notifications import EmailCadence
@@ -11,6 +12,13 @@
1112

1213
FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE = 'filter_audit_expired_users_with_no_role'
1314

15+
# Context keys whose values are used as HTML tag names by content_templates
16+
# (e.g. `<{p}>...<{strong}>{post_title}</{strong}></{p}>`). These must NOT be
17+
# HTML-escaped before `template.format(**context)`; every other context value
18+
# must be, since it typically comes from user input (thread title, username,
19+
# etc.). See get_notification_content below.
20+
_STRUCTURAL_CONTEXT_KEYS = frozenset({'p', 'strong'})
21+
1422
COURSE_NOTIFICATION_TYPES = {
1523
'new_comment_on_response': {
1624
'notification_app': 'discussion',
@@ -538,8 +546,16 @@ def get_notification_content(notification_type, context):
538546
context = context_function(context)
539547

540548
if template:
541-
# Handle grouped templates differently by modifying the context using a different function.
542-
return template.format(**context)
549+
# HTML-escape every context value except the structural tag-name
550+
# keys, so that user-controlled input (post_title, replier_name,
551+
# etc.) cannot inject `<style>` / `<script>` / other HTML into
552+
# notification.content — which is rendered with `|safe` in the
553+
# digest and batched email templates.
554+
safe_context = {
555+
key: value if key in _STRUCTURAL_CONTEXT_KEYS else escape(value)
556+
for key, value in context.items()
557+
}
558+
return template.format(**safe_context)
543559

544560
return ''
545561

openedx/core/djangoapps/notifications/tests/test_base_notification.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
"""
22
Tests for base_notification
33
"""
4+
import pytest
5+
46
from common.djangoapps.student.tests.factories import UserFactory
57
from openedx.core.djangoapps.notifications import base_notification, models
68
from openedx.core.djangoapps.notifications.models import (
@@ -262,3 +264,40 @@ def test_validate_non_core_notification_types(self):
262264
assert isinstance(notification_type[key], str)
263265
for key in bool_keys:
264266
assert isinstance(notification_type[key], bool)
267+
268+
269+
@pytest.mark.parametrize(
270+
('user_input', 'escaped'),
271+
[
272+
('<style>body{background:red}</style>evil', '&lt;style&gt;body{background:red}&lt;/style&gt;evil'),
273+
('<script>alert(1)</script>', '&lt;script&gt;alert(1)&lt;/script&gt;'),
274+
('AT&T "quoted"', 'AT&amp;T &quot;quoted&quot;'),
275+
],
276+
)
277+
def test_get_notification_content_escapes_user_input(user_input, escaped):
278+
"""
279+
Regression test for GHSA-rv5w-f4r5-h77g: user-controlled context values
280+
must be HTML-escaped before being interpolated into a content_template
281+
via `str.format`. Structural context keys (`p`, `strong`) are exempt so
282+
the template can still emit real <p>/<strong> tags.
283+
"""
284+
context = {'replier_name': 'alice', 'post_title': user_input}
285+
content = base_notification.get_notification_content('new_response', context)
286+
assert '<style>' not in content
287+
assert '<script>' not in content
288+
assert escaped in content
289+
290+
291+
def test_get_notification_content_preserves_structural_tags():
292+
"""
293+
Companion to test_get_notification_content_escapes_user_input: verify
294+
that the structural `p` and `strong` keys still produce real HTML tags
295+
after the escape pass, and that innocuous user input renders as plain
296+
text alongside them.
297+
"""
298+
context = {'replier_name': 'alice', 'post_title': 'Hello world'}
299+
content = base_notification.get_notification_content('new_response', context)
300+
assert '<p>' in content
301+
assert '</p>' in content
302+
assert '<strong>alice</strong>' in content
303+
assert '<strong>Hello world</strong>' in content

0 commit comments

Comments
 (0)