mark decision notes as safe so jinja doesn't escape them (#22454)

* mark decision notes as safe so jinja doesn't escape them

* add test to prove unsafe content is included unescaped

* rm pointless no_jinja_autoescape (it's not jinja); update comments

src/olympia/abuse/tests/test_utils.py

@@ -43,7 +43,7 @@ class BaseTestCinderAction:
         self.decision = CinderDecision.objects.create(
             cinder_id='ab89',
             action=DECISION_ACTIONS.AMO_APPROVE,
-            notes='extra notes',
+            notes="extra note's",
             addon=addon,
         )
         self.cinder_job = CinderJob.objects.create(
@@ -167,6 +167,7 @@ class BaseTestCinderAction:
         assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[0].body
         assert '"' not in mail.outbox[0].body
         assert '<b>' not in mail.outbox[0].body
+        assert ''' not in mail.outbox[0].body
         assert self.decision.notes in mail.outbox[0].body
 
     def _check_owner_email(self, mail_item, subject, snippet):
@@ -177,6 +178,7 @@ class BaseTestCinderAction:
         assert '[ref:ab89]' in mail_item.body
         assert '"' not in mail_item.body
         assert '<b>' not in mail_item.body
+        assert ''' not in mail_item.body
         assert self.decision.notes in mail_item.body
 
     def _test_owner_takedown_email(self, subject, snippet):
@@ -198,6 +200,7 @@ class BaseTestCinderAction:
         )
         assert '"' not in mail_item.body
         assert '<b>' not in mail_item.body
+        assert ''' not in mail_item.body
         assert self.decision.notes in mail_item.body
 
     def _test_owner_affirmation_email(self, subject):
@@ -206,6 +209,7 @@ class BaseTestCinderAction:
         assert 'right to appeal' not in mail_item.body
         notes = f'{self.decision.notes}. ' if self.decision.notes else ''
         assert f' was correct. {notes}Based on that determination' in (mail_item.body)
+        assert ''' not in mail_item.body
         if isinstance(self.decision.target, Addon):
             # Verify we used activity mail for Addon related target emails
             log_token = ActivityLogToken.objects.get()
@@ -216,6 +220,7 @@ class BaseTestCinderAction:
         assert len(mail.outbox) == 1
         self._check_owner_email(mail_item, subject, 'we have restored')
         assert 'right to appeal' not in mail_item.body
+        assert ''' not in mail_item.body
         assert self.decision.notes in mail_item.body
 
     def _test_approve_appeal_or_override(CinderActionClass):
@@ -307,6 +312,20 @@ class BaseTestCinderAction:
             assert 'Bad policy' not in mail.outbox[idx].body  # policy name
             assert 'Parent' not in mail.outbox[idx].body  # parent policy text
 
+    def test_email_content_not_escaped(self):
+        unsafe_str = '<script>jar=window.triggerExploit();"</script>'
+        self.decision.update(notes=unsafe_str)
+        action = self.ActionClass(self.decision)
+        action.notify_owners()
+        assert unsafe_str in mail.outbox[0].body
+
+        action = CinderActionApproveNoAction(self.decision)
+        mail.outbox.clear()
+        action.notify_reporters(
+            reporter_abuse_reports=[self.abuse_report_auth], is_appeal=True
+        )
+        assert unsafe_str in mail.outbox[0].body
+
 
 class TestCinderActionUser(BaseTestCinderAction, TestCase):
     ActionClass = CinderActionBanUser

src/olympia/abuse/utils.py

@@ -15,7 +15,7 @@ from olympia import activity, amo
 from olympia.activity import log_create
 from olympia.addons.models import Addon
 from olympia.amo.templatetags.jinja_helpers import absolutify
-from olympia.amo.utils import no_jinja_autoescape, send_mail
+from olympia.amo.utils import send_mail
 from olympia.bandwagon.models import Collection
 from olympia.ratings.models import Rating
 from olympia.users.models import UserProfile
@@ -90,16 +90,15 @@ class CinderAction:
         owners = self.get_owners()
         if not owners:
             return
-        with no_jinja_autoescape():
         template = loader.get_template(self.owner_template_path)
         target_name = self.get_target_name()
         reference_id = f'ref:{self.decision.get_reference_id()}'
         context_dict = {
             'is_third_party_initiated': self.decision.is_third_party_initiated,
-            'manual_reasoning_text': self.decision.notes or '',
-            # Auto-escaping is already disabled above as we're dealing with an
-            # email but the target name could have triggered lazy escaping when
-            # it was generated so it needs special treatment to avoid it.
+            # It's a plain-text email so we're safe to include comments without escaping
+            # them - we don't want ', etc, rendered as html entities.
+            'manual_reasoning_text': mark_safe(self.decision.notes or ''),
+            # It's a plain-text email so we're safe to include the name without escaping
             'name': mark_safe(target_name),
             'policy_document_url': POLICY_DOCUMENT_URL,
             'reference_id': reference_id,
@@ -148,7 +147,6 @@ class CinderAction:
         )
         if not template or not reporter_abuse_reports:
             return
-        with no_jinja_autoescape():
         template = loader.get_template(template)
         for abuse_report in reporter_abuse_reports:
             email_address = (
@@ -169,10 +167,8 @@ class CinderAction:
                     target_name, reference_id
                 )
                 context_dict = {
-                    # Auto-escaping is already disabled above as we're dealing
-                    # with an email but the target name could have triggered
-                    # lazy escaping when it was generated so it needs special
-                    # treatment to avoid it.
+                    # It's a plain-text email so we're safe to include the name without
+                    # escaping it.
                     'name': mark_safe(target_name),
                     'policies': self.decision.policies.all(),
                     'policy_document_url': POLICY_DOCUMENT_URL,
@@ -181,8 +177,12 @@ class CinderAction:
                     'type': self.get_target_type(),
                     'SITE_URL': settings.SITE_URL,
                 }
-                if self.decision.cinder_job.is_appeal:
-                    context_dict['manual_reasoning_text'] = self.decision.notes or ''
+                if is_appeal:
+                    # It's a plain-text email so we're safe to include comments without
+                    # escaping them - we don't want ', etc, rendered as html entities.
+                    context_dict['manual_reasoning_text'] = mark_safe(
+                        self.decision.notes or ''
+                    )
                 if self.decision.can_be_appealed(
                     is_reporter=True, abuse_report=abuse_report
                 ):