diff --git a/src/pretix/base/email.py b/src/pretix/base/email.py index ab71fb45f..155b29657 100644 --- a/src/pretix/base/email.py +++ b/src/pretix/base/email.py @@ -39,7 +39,7 @@ from pretix.base.templatetags.rich_text import ( DEFAULT_CALLBACKS, EMAIL_RE, URL_RE, abslink_callback, markdown_compile_email, truelink_callback, ) -from pretix.helpers.format import SafeFormatter, format_map +from pretix.helpers.format import FormattedString, SafeFormatter, format_map from pretix.base.services.placeholders import ( # noqa get_available_placeholders, PlaceholderContext @@ -141,6 +141,7 @@ class TemplateBasedMailRenderer(BaseHTMLMailRenderer): return markdown_compile_email(plaintext, context=context) def render(self, plain_body: str, plain_signature: str, subject: str, order, position, context) -> str: + apply_format_map = not isinstance(plain_body, FormattedString) body_md = self.compile_markdown(plain_body, context) if context: linker = bleach.Linker( @@ -149,12 +150,13 @@ class TemplateBasedMailRenderer(BaseHTMLMailRenderer): callbacks=DEFAULT_CALLBACKS + [truelink_callback, abslink_callback], parse_email=True ) - body_md = format_map( - body_md, - context=context, - mode=SafeFormatter.MODE_RICH_TO_HTML, - linkifier=linker - ) + if apply_format_map: + body_md = format_map( + body_md, + context=context, + mode=SafeFormatter.MODE_RICH_TO_HTML, + linkifier=linker + ) htmlctx = { 'site': settings.PRETIX_INSTANCE_NAME, 'site_url': settings.SITE_URL, diff --git a/src/pretix/base/models/orders.py b/src/pretix/base/models/orders.py index 1b2f459d2..a2b48ed98 100644 --- a/src/pretix/base/models/orders.py +++ b/src/pretix/base/models/orders.py @@ -87,7 +87,7 @@ from pretix.base.timemachine import time_machine_now from ...helpers import OF_SELF from ...helpers.countries import CachedCountries, FastCountryField -from ...helpers.format import format_map +from ...helpers.format import FormattedString, format_map from ...helpers.names import build_name from ...testutils.middleware import debugflags_var from ._transactions import ( @@ -1178,7 +1178,8 @@ class Order(LockModel, LoggedModel): recipient = position.attendee_email email_content = render_mail(template, context) - subject = format_map(subject, context) + if not isinstance(subject, FormattedString): + subject = format_map(subject, context) mail( recipient, subject, template, context, self.event, self.locale, self, headers=headers, sender=sender, @@ -2907,7 +2908,8 @@ class OrderPosition(AbstractPosition): with language(self.order.locale, self.order.event.settings.region): recipient = self.attendee_email email_content = render_mail(template, context) - subject = format_map(subject, context) + if not isinstance(subject, FormattedString): + subject = format_map(subject, context) mail( recipient, subject, template, context, self.event, self.order.locale, order=self.order, headers=headers, sender=sender, diff --git a/src/pretix/base/services/mail.py b/src/pretix/base/services/mail.py index 2615660a4..cbc5ddd6e 100644 --- a/src/pretix/base/services/mail.py +++ b/src/pretix/base/services/mail.py @@ -81,7 +81,9 @@ from pretix.base.signals import ( ) from pretix.celery_app import app from pretix.helpers import OF_SELF -from pretix.helpers.format import SafeFormatter, format_map +from pretix.helpers.format import ( + FormattedString, PlainHtmlAlternativeString, SafeFormatter, format_map, +) from pretix.helpers.hierarkey import clean_filename from pretix.multidomain.urlreverse import build_absolute_uri from pretix.presale.ical import get_private_icals @@ -218,6 +220,9 @@ def mail(email: Union[str, Sequence[str]], subject: str, template: Union[str, La if email == INVALID_ADDRESS: return + if isinstance(template, FormattedString): + raise TypeError("Cannot pass an already formatted body template") + if no_order_links and not plain_text_only: raise ValueError('If you set no_order_links, you also need to set plain_text_only.') @@ -251,23 +256,28 @@ def mail(email: Union[str, Sequence[str]], subject: str, template: Union[str, La if event and attach_tickets and not event.settings.mail_attach_tickets: attach_tickets = False - with language(locale): + with language(locale), override(timezone): if isinstance(context, dict) and order: _autoextend_context(context, order) # Build raw content - body_plain = render_mail(template, context, placeholder_mode=SafeFormatter.MODE_RICH_TO_PLAIN) + content_plain = render_mail(template, context, placeholder_mode=None) if settings_holder: signature = str(settings_holder.settings.get('mail_text_signature')) else: signature = "" # Build full plain-text body + if not isinstance(content_plain, FormattedString): + body_plain = format_map(content_plain, context, mode=SafeFormatter.MODE_RICH_TO_PLAIN) + else: + body_plain = content_plain body_plain = _wrap_plain_body(body_plain, signature, event, order, position, no_order_links) - body_plain = format_map(body_plain, context, mode=SafeFormatter.MODE_RICH_TO_PLAIN) # Build subject - subject = str(subject).format_map(TolerantDict(context)) + if not isinstance(subject, FormattedString): + subject = format_map(subject, context) + subject = raw_subject = subject.replace('\n', ' ').replace('\r', '')[:900] if settings_holder: subject = prefix_subject(settings_holder, subject) @@ -286,26 +296,24 @@ def mail(email: Union[str, Sequence[str]], subject: str, template: Union[str, La else: renderer = ClassicMailRenderer(None, organizer) - with override(timezone): - content_plain = render_mail(template, context, placeholder_mode=None) - try: - if 'context' in inspect.signature(renderer.render).parameters: - body_html = renderer.render(content_plain, signature, raw_subject, order, position, context) - elif 'position' in inspect.signature(renderer.render).parameters: - # Backwards compatibility - warnings.warn('Email renderer called without context argument because context argument is not ' - 'supported.', - DeprecationWarning) - body_html = renderer.render(content_plain, signature, raw_subject, order, position) - else: - # Backwards compatibility - warnings.warn('Email renderer called without position argument because position argument is not ' - 'supported.', - DeprecationWarning) - body_html = renderer.render(content_plain, signature, raw_subject, order) - except: - logger.exception('Could not render HTML body') - body_html = None + try: + if 'context' in inspect.signature(renderer.render).parameters: + body_html = renderer.render(content_plain, signature, raw_subject, order, position, context) + elif 'position' in inspect.signature(renderer.render).parameters: + # Backwards compatibility + warnings.warn('Email renderer called without context argument because context argument is not ' + 'supported.', + DeprecationWarning) + body_html = renderer.render(content_plain, signature, raw_subject, order, position) + else: + # Backwards compatibility + warnings.warn('Email renderer called without position argument because position argument is not ' + 'supported.', + DeprecationWarning) + body_html = renderer.render(content_plain, signature, raw_subject, order) + except: + logger.exception('Could not render HTML body') + body_html = None m = OutgoingMail.objects.create( organizer=organizer, @@ -790,7 +798,12 @@ def render_mail(template, context, placeholder_mode: Optional[int]=SafeFormatter body = format_map(body, context, mode=placeholder_mode) else: tpl = get_template(template) - body = tpl.render(context) + context = { + # Known bug, should behave differently for plain and HTML but we'll fix after security release + k: v.html if isinstance(v, PlainHtmlAlternativeString) else v + for k, v in context.items() + } + body = FormattedString(tpl.render(context)) return body @@ -936,7 +949,7 @@ def _wrap_plain_body(content_plain, signature, event, order, position, no_order_ body_plain += "\r\n\r\n-- \r\n" if signature: - signature = signature.format(event=event.name if event else '') + signature = format_map(signature, {"event": event.name if event else ''}) body_plain += signature body_plain += "\r\n\r\n-- \r\n" @@ -948,7 +961,7 @@ def _wrap_plain_body(content_plain, signature, event, order, position, no_order_ body_plain += _( "You can view your order details at the following URL:\n{orderurl}." ).replace("\n", "\r\n").format( - event=event.name, orderurl=build_absolute_uri( + orderurl=build_absolute_uri( order.event, 'presale:event.order.position', kwargs={ 'order': order.code, 'secret': position.web_secret, diff --git a/src/pretix/helpers/format.py b/src/pretix/helpers/format.py index a79b869e5..843b22650 100644 --- a/src/pretix/helpers/format.py +++ b/src/pretix/helpers/format.py @@ -22,6 +22,7 @@ import logging from string import Formatter +from django.core.exceptions import SuspiciousOperation from django.utils.html import conditional_escape logger = logging.getLogger(__name__) @@ -37,6 +38,17 @@ class PlainHtmlAlternativeString: return f"PlainHtmlAlternativeString('{self.plain}', '{self.html}')" +class FormattedString(str): + """ + A str subclass that has been specifically marked as "already formatted" for email rendering + purposes to avoid duplicate formatting. + """ + __slots__ = () + + def __str__(self): + return self + + class SafeFormatter(Formatter): """ Customized version of ``str.format`` that (a) behaves just like ``str.format_map`` and @@ -77,8 +89,19 @@ class SafeFormatter(Formatter): # Ignore format_spec return super().format_field(self._prepare_value(value), '') + def convert_field(self, value, conversion): + # Ignore any conversions + if conversion is None: + return value + else: + return str(value) -def format_map(template, context, raise_on_missing=False, mode=SafeFormatter.MODE_RICH_TO_PLAIN, linkifier=None): + +def format_map(template, context, raise_on_missing=False, mode=SafeFormatter.MODE_RICH_TO_PLAIN, linkifier=None) -> FormattedString: + if isinstance(template, FormattedString): + raise SuspiciousOperation("Calling format_map() on an already formatted string is likely unsafe.") if not isinstance(template, str): template = str(template) - return SafeFormatter(context, raise_on_missing, mode=mode, linkifier=linkifier).format(template) + return FormattedString( + SafeFormatter(context, raise_on_missing, mode=mode, linkifier=linkifier).format(template) + ) diff --git a/src/tests/base/test_mail.py b/src/tests/base/test_mail.py index 7abfac5c8..2ba937b6d 100644 --- a/src/tests/base/test_mail.py +++ b/src/tests/base/test_mail.py @@ -32,21 +32,26 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations under the License. +import datetime import os import re +from decimal import Decimal from email.mime.text import MIMEText import pytest from django.conf import settings from django.core import mail as djmail from django.test import override_settings +from django.utils.html import escape from django.utils.timezone import now from django.utils.translation import gettext_lazy as _ -from django_scopes import scope +from django_scopes import scope, scopes_disabled from i18nfield.strings import LazyI18nString from pretix.base.email import get_email_context -from pretix.base.models import Event, Organizer, OutgoingMail, User +from pretix.base.models import ( + Event, InvoiceAddress, Order, Organizer, OutgoingMail, User, +) from pretix.base.services.mail import mail, mail_send_task @@ -68,6 +73,45 @@ def env(): yield event, user, o +@pytest.fixture +@scopes_disabled() +def item(env): + return env[0].items.create(name="Budget Ticket", default_price=23) + + +@pytest.fixture +@scopes_disabled() +def order(env, item): + event, _, _ = env + o = Order.objects.create( + code="FOO", + event=event, + email="dummy@dummy.test", + status=Order.STATUS_PENDING, + secret="k24fiuwvu8kxz3y1", + sales_channel=event.organizer.sales_channels.get(identifier="web"), + datetime=datetime.datetime(2017, 12, 1, 10, 0, 0, tzinfo=datetime.UTC), + expires=datetime.datetime(2017, 12, 10, 10, 0, 0, tzinfo=datetime.UTC), + total=23, + locale="en", + ) + o.positions.create( + order=o, + item=item, + variation=None, + price=Decimal("23"), + attendee_email="peter@example.org", + attendee_name_parts={"given_name": "Peter", "family_name": "Miller"}, + secret="z3fsn8jyufm5kpk768q69gkbyr5f4h6w", + pseudonymization_id="ABCDEFGHKL", + ) + InvoiceAddress.objects.create( + order=o, + name_parts={"given_name": "Peter", "family_name": "Miller"}, + ) + return o + + @pytest.mark.django_db def test_send_mail_with_prefix(env): djmail.outbox = [] @@ -279,7 +323,7 @@ def _extract_html(mail): def test_placeholder_html_rendering_from_template(env): djmail.outbox = [] event, user, organizer = env - event.name = "event & co. kg" + event.name = "event & co. kg {currency}" event.save() mail('dummy@dummy.dummy', '{event} Test subject', 'mailtest.txt', get_email_context( event=event, @@ -288,25 +332,26 @@ def test_placeholder_html_rendering_from_template(env): assert len(djmail.outbox) == 1 assert djmail.outbox[0].to == [user.email] - assert 'Event name: event & co. kg' in djmail.outbox[0].body - assert '**IBAN**: 123 \n**BIC**: 456' in djmail.outbox[0].body - assert '**Meta**: *Beep*' in djmail.outbox[0].body - assert 'Event website: [event & co. kg](https://example.org/dummy)' in djmail.outbox[0].body - assert 'Other website: [event & co. kg](https://example.com)' in djmail.outbox[0].body - assert '<' not in djmail.outbox[0].body - assert '&' not in djmail.outbox[0].body + # Known bug for now: These should not have HTML for the plain body, but we'll fix this safter the security release + assert escape('Event name: event & co. kg {currency}') in djmail.outbox[0].body + assert 'IBAN: 123
\nBIC: 456' in djmail.outbox[0].body + assert '**Meta**: Beep' in djmail.outbox[0].body + assert escape('Event website: [event & co. kg {currency}](https://example.org/dummy)') in djmail.outbox[0].body + # todo: assert '<' not in djmail.outbox[0].body + # todo: assert '&' not in djmail.outbox[0].body + assert 'Unevaluated placeholder: {currency}' in djmail.outbox[0].body + assert 'EUR' not in djmail.outbox[0].body html = _extract_html(djmail.outbox[0]) assert 'event' not in html - assert 'Event name: <strong>event & co. kg</strong>' in html + assert 'Event name: <strong>event & co. kg</strong> {currency}' in html assert 'IBAN: 123
\nBIC: 456' in html assert 'Meta: Beep' in html + assert 'Unevaluated placeholder: {currency}' in html + assert 'EUR' not in html assert re.search( - r'Event website: <strong>event & co. kg</strong>', - html - ) - assert re.search( - r'Other website: <strong>event & co. kg</strong>', + r'Event website: ' + r'<strong>event & co. kg</strong> {currency}', html ) @@ -324,7 +369,7 @@ def test_placeholder_html_rendering_from_string(env): }) djmail.outbox = [] event, user, organizer = env - event.name = "event & co. kg" + event.name = "event & co. kg {currency}" event.save() ctx = get_email_context( event=event, @@ -335,9 +380,9 @@ def test_placeholder_html_rendering_from_string(env): assert len(djmail.outbox) == 1 assert djmail.outbox[0].to == [user.email] - assert 'Event name: event & co. kg' in djmail.outbox[0].body - assert 'Event website: [event & co. kg](https://example.org/dummy)' in djmail.outbox[0].body - assert 'Other website: [event & co. kg](https://example.com)' in djmail.outbox[0].body + assert 'Event name: event & co. kg {currency}' in djmail.outbox[0].body + assert 'Event website: [event & co. kg {currency}](https://example.org/dummy)' in djmail.outbox[0].body + assert 'Other website: [event & co. kg {currency}](https://example.com)' in djmail.outbox[0].body assert '**IBAN**: 123 \n**BIC**: 456' in djmail.outbox[0].body assert '**Meta**: *Beep*' in djmail.outbox[0].body assert 'URL: https://google.com' in djmail.outbox[0].body @@ -352,11 +397,13 @@ def test_placeholder_html_rendering_from_string(env): assert 'IBAN: 123
\nBIC: 456' in html assert 'Meta: Beep' in html assert re.search( - r'Event website: <strong>event & co. kg</strong>', + r'Event website: ' + r'<strong>event & co. kg</strong> {currency}', html ) assert re.search( - r'Other website: <strong>event & co. kg</strong>', + r'Other website: ' + r'<strong>event & co. kg</strong> {currency}', html ) assert re.search( @@ -377,3 +424,141 @@ def test_placeholder_html_rendering_from_string(env): r'style="[^"]+" target="_blank">Link & Text', html ) + + +@pytest.mark.django_db +def test_nested_placeholder_inclusion_full_process(env, order): + # Test that it is not possible to sneak in a placeholder like {url_cancel} inside a user-controlled + # placeholder value like {invoice_company} + event, user, organizer = env + position = order.positions.get() + order.invoice_address.company = "{url_cancel} Corp" + order.invoice_address.save() + event.settings.mail_text_resend_link = LazyI18nString({"en": "Ticket for {invoice_company}"}) + event.settings.mail_subject_resend_link_attendee = LazyI18nString({"en": "Ticket for {invoice_company}"}) + + djmail.outbox = [] + position.resend_link() + assert len(djmail.outbox) == 1 + assert djmail.outbox[0].to == [position.attendee_email] + assert "Ticket for {url_cancel} Corp" == djmail.outbox[0].subject + assert "/cancel" not in djmail.outbox[0].body + assert "/order" not in djmail.outbox[0].body + html, plain = _extract_html(djmail.outbox[0]), djmail.outbox[0].body + for part in (html, plain): + assert "Ticket for {url_cancel} Corp" in part + assert "/order/" not in part + assert "/cancel" not in part + + +@pytest.mark.django_db +def test_nested_placeholder_inclusion_mail_service(env): + # test that it is not possible to have placeholders within the values of placeholders when + # the mail() function is called directly + template = LazyI18nString("Event name: {event}") + djmail.outbox = [] + event, user, organizer = env + event.name = "event & {currency} co. kg" + event.slug = "event-co-ag-slug" + event.save() + + mail( + "dummy@dummy.dummy", + "{event} Test subject", + template, + get_email_context( + event=event, + payment_info="**IBAN**: 123 \n**BIC**: 456 {event}", + ), + event, + ) + + assert len(djmail.outbox) == 1 + assert djmail.outbox[0].to == [user.email] + html, plain = _extract_html(djmail.outbox[0]), djmail.outbox[0].body + for part in (html, plain, djmail.outbox[0].subject): + assert "event & {currency} co. kg" in part or "event & {currency} co. kg" in part + assert "EUR" not in part + + +@pytest.mark.django_db +@pytest.mark.parametrize("tpl", [ + "Event: {event.__class__}", + "Event: {{event.__class__}}", + "Event: {{{event.__class__}}}", +]) +def test_variable_inclusion_from_string_full_process(env, tpl, order): + # Test that it is not possible to use placeholders that leak system information in templates + # when run through system processes + event, user, organizer = env + event.name = "event & co. kg" + event.save() + position = order.positions.get() + event.settings.mail_text_resend_link = LazyI18nString({"en": tpl}) + event.settings.mail_subject_resend_link_attendee = LazyI18nString({"en": tpl}) + + position.resend_link() + assert len(djmail.outbox) == 1 + html, plain = _extract_html(djmail.outbox[0]), djmail.outbox[0].body + for part in (html, plain, djmail.outbox[0].subject): + assert "{event.__class__}" in part + assert "LazyI18nString" not in part + + +@pytest.mark.django_db +@pytest.mark.parametrize("tpl", [ + "Event: {event.__class__}", + "Event: {{event.__class__}}", + "Event: {{{event.__class__}}}", +]) +def test_variable_inclusion_from_string_mail_service(env, tpl): + # Test that it is not possible to use placeholders that leak system information in templates + # when run through mail() directly + event, user, organizer = env + event.name = "event & co. kg" + event.save() + + djmail.outbox = [] + mail( + "dummy@dummy.dummy", + tpl, + LazyI18nString(tpl), + get_email_context( + event=event, + payment_info="**IBAN**: 123 \n**BIC**: 456\n" + tpl, + ), + event, + ) + assert len(djmail.outbox) == 1 + html, plain = _extract_html(djmail.outbox[0]), djmail.outbox[0].body + for part in (html, plain, djmail.outbox[0].subject): + assert "{event.__class__}" in part + assert "LazyI18nString" not in part + + +@pytest.mark.django_db +def test_escaped_braces_mail_services(env): + # Test that braces can be escaped by doubling + template = LazyI18nString("Event name: -{{currency}}-") + djmail.outbox = [] + event, user, organizer = env + event.name = "event & co. kg" + event.save() + + mail( + "dummy@dummy.dummy", + "-{{currency}}- Test subject", + template, + get_email_context( + event=event, + payment_info="**IBAN**: 123 \n**BIC**: 456 {event}", + ), + event, + ) + + assert len(djmail.outbox) == 1 + assert djmail.outbox[0].to == [user.email] + html, plain = _extract_html(djmail.outbox[0]), djmail.outbox[0].body + for part in (html, plain, djmail.outbox[0].subject): + assert "EUR" not in part + assert "-{currency}-" in part diff --git a/src/tests/helpers/test_format.py b/src/tests/helpers/test_format.py index 5ed3ac9b1..7c2771761 100644 --- a/src/tests/helpers/test_format.py +++ b/src/tests/helpers/test_format.py @@ -29,6 +29,8 @@ def test_format_map(): assert format_map("Foo {baz}", {"bar": 3}) == "Foo {baz}" assert format_map("Foo {bar.__module__}", {"bar": 3}) == "Foo {bar.__module__}" assert format_map("Foo {bar!s}", {"bar": 3}) == "Foo 3" + assert format_map("Foo {bar!r}", {"bar": '3'}) == "Foo 3" + assert format_map("Foo {bar!a}", {"bar": '3'}) == "Foo 3" assert format_map("Foo {bar:<20}", {"bar": 3}) == "Foo 3" diff --git a/src/tests/templates/mailtest.txt b/src/tests/templates/mailtest.txt index 1bc1487d3..a620f1ca8 100644 --- a/src/tests/templates/mailtest.txt +++ b/src/tests/templates/mailtest.txt @@ -1,13 +1,13 @@ {% load i18n %} This is a test file for sending mails. -Event name: {event} +Event name: {{ event }} +Unevaluated placeholder: {currency} {% get_current_language as LANGUAGE_CODE %} The language code used for rendering this email is {{ LANGUAGE_CODE }}. Payment info: -{payment_info} +{{ payment_info }} -**Meta**: {meta_Test} +**Meta**: {{ meta_Test }} -Event website: [{event}](https://example.org/{event_slug}) -Other website: [{event}]({meta_Website}) \ No newline at end of file +Event website: [{{event}}](https://example.org/{{event_slug}}) \ No newline at end of file