From d186dd1e20a5d0f1b7d295d410485bcfb7807ea4 Mon Sep 17 00:00:00 2001
From: John Whitlock <jwhitlock@mozilla.com>
Date: Thu, 19 May 2016 08:42:29 -0500
Subject: [PATCH] bug 1272791 - Encode attachment data for JS

Data about attachments are assigned to a JavaScript variable so that
they are available to CKEditor. The JSON encoder does not escape
characters for this context, allowing user-entered data to be injected
into the page in an unsafe way, allowing XSS attacks. This code switches
to explicit escaping of strings using escapejs, removing the XSS vector.

r=jgmize
---
 .../attachments/includes/attachment_list.html       | 11 ++++++++++-
 kuma/attachments/templatetags/jinja_helpers.py      |  3 +--
 kuma/attachments/tests/test_templates.py            |  3 +++
 kuma/attachments/utils.py                           | 13 -------------
 kuma/core/templatetags/jinja_helpers.py             |  1 +
 5 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/kuma/attachments/jinja2/attachments/includes/attachment_list.html b/kuma/attachments/jinja2/attachments/includes/attachment_list.html
index 7dcc3face61..cf71e6735d3 100644
--- a/kuma/attachments/jinja2/attachments/includes/attachment_list.html
+++ b/kuma/attachments/jinja2/attachments/includes/attachment_list.html
@@ -23,7 +23,16 @@ <h3><i aria-hidden="true" class="icon-paperclip"></i>{{ _('Attachments') }} <a h
   </p>
 
   <script>
-    mdn.wiki.attachments = {{ attachments_payload(document_attachments)|jsonencode }};
+    mdn.wiki.attachments = [
+        {% for attachment in document_attachments -%}
+        {
+          "title": "{{ attachment.file.title | escapejs }}",
+          "description": "{{ attachment.file.current_revision.description | escapejs }}",
+          "mime": "{{ attachment.file.current_revision.mime_type | escapejs }}",
+          "url": "{{ attachment.file.get_file_url() | escapejs }}"
+        }{% if not loop.last %},{% endif %}
+        {%- endfor %}
+    ];
     mdn.wiki.attachments_enabled = {{ show_attach_button|jsonencode }};
   </script>
 
diff --git a/kuma/attachments/templatetags/jinja_helpers.py b/kuma/attachments/templatetags/jinja_helpers.py
index 80c40a08056..46ee33af90f 100644
--- a/kuma/attachments/templatetags/jinja_helpers.py
+++ b/kuma/attachments/templatetags/jinja_helpers.py
@@ -1,7 +1,6 @@
 from django_jinja import library
 
-from ..utils import allow_add_attachment_by, attachments_payload
+from ..utils import allow_add_attachment_by
 
 
 library.global_function(allow_add_attachment_by)
-library.global_function(attachments_payload)
diff --git a/kuma/attachments/tests/test_templates.py b/kuma/attachments/tests/test_templates.py
index 09e720d168e..5f128783960 100644
--- a/kuma/attachments/tests/test_templates.py
+++ b/kuma/attachments/tests/test_templates.py
@@ -59,3 +59,6 @@ def test_xss_file_attachment_title(self):
             '&gt;&lt;img src=x onerror=prompt(navigator.userAgent);&gt;',
             doc('.page-attachments-table .attachment-name-cell').html()
         )
+        # security bug 1272791
+        for script in doc('script'):
+            self.assertNotIn(title, script.text_content())
diff --git a/kuma/attachments/utils.py b/kuma/attachments/utils.py
index c3673748ece..5097529339a 100644
--- a/kuma/attachments/utils.py
+++ b/kuma/attachments/utils.py
@@ -72,16 +72,3 @@ def attachment_upload_to(instance, filename):
         'md5': hashlib.md5(str(now)).hexdigest(),
         'filename': filename
     }
-
-
-def attachments_payload(document_attachments):
-    """
-    Given a list of document attachments make some output that can be used
-    by the CKeditor plugins.
-    """
-    return [{
-        'title': attachment.file.title,
-        'description': attachment.file.current_revision.description,
-        'mime': attachment.file.current_revision.mime_type,
-        'url': attachment.file.get_file_url()
-    } for attachment in document_attachments]
diff --git a/kuma/core/templatetags/jinja_helpers.py b/kuma/core/templatetags/jinja_helpers.py
index e0a5c51f486..ea8346850a6 100644
--- a/kuma/core/templatetags/jinja_helpers.py
+++ b/kuma/core/templatetags/jinja_helpers.py
@@ -27,6 +27,7 @@
 
 
 # Yanking filters from Django.
+library.filter(defaultfilters.escapejs)
 library.filter(defaultfilters.linebreaksbr)
 library.filter(strip_tags)
 library.filter(defaultfilters.truncatewords)