Fixed #25018 -- Changed simple_tag to apply conditional_escape() to its output.

This is a security hardening fix to help prevent XSS (and incorrect HTML)
for the common use case of simple_tag.

Thanks to Tim Graham for the review.

6 files changed, 110 insertions, 3 deletions

diff --git a/django/template/library.py b/django/template/library.py
index 1eaaf4c0be..23364e8024 100644
--- a/django/template/library.py
+++ b/django/template/library.py
@@ -4,6 +4,7 @@ from importlib import import_module
 
 from django.utils import six
 from django.utils.deprecation import RemovedInDjango20Warning
+from django.utils.html import conditional_escape
 from django.utils.inspect import getargspec
 from django.utils.itercompat import is_iterable
 
@@ -201,6 +202,8 @@ class SimpleNode(TagHelperNode):
         if self.target_var is not None:
             context[self.target_var] = output
             return ''
+        if context.autoescape:
+            output = conditional_escape(output)
         return output
 
 
diff --git a/docs/howto/custom-template-tags.txt b/docs/howto/custom-template-tags.txt
index c4e414cdb5..adee0f9ffd 100644
--- a/docs/howto/custom-template-tags.txt
+++ b/docs/howto/custom-template-tags.txt
@@ -441,6 +441,22 @@ A few things to note about the ``simple_tag`` helper function:
 * If the argument was a template variable, our function is passed the
   current value of the variable, not the variable itself.
 
+Unlike other tag utilities, ``simple_tag`` passes its output through
+:func:`~django.utils.html.conditional_escape` if the template context is in
+autoescape mode, to ensure correct HTML and protect you from XSS
+vulnerabilities.
+
+If additional escaping is not desired, you will need to use
+:func:`~django.utils.safestring.mark_safe` if you are absolutely sure that your
+code does not contain XSS vulnerabilities. For building small HTML snippets,
+use of :func:`~django.utils.html.format_html` instead of ``mark_safe()`` is
+strongly recommended.
+
+.. versionchanged:: 1.9
+
+   Auto-escaping for ``simple_tag`` as described in the previous two paragraphs
+   was added.
+
 If your template tag needs to access the current context, you can use the
 ``takes_context`` argument when registering your tag::
 
@@ -792,12 +808,16 @@ Ultimately, this decoupling of compilation and rendering results in an
 efficient template system, because a template can render multiple contexts
 without having to be parsed multiple times.
 
+.. _tags-auto-escaping:
+
 Auto-escaping considerations
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 The output from template tags is **not** automatically run through the
-auto-escaping filters. However, there are still a couple of things you should
-keep in mind when writing a template tag.
+auto-escaping filters (with the exception of
+:meth:`~django.template.Library.simple_tag` as described above). However, there
+are still a couple of things you should keep in mind when writing a template
+tag.
 
 If the ``render()`` function of your template stores the result in a context
 variable (rather than returning the result in a string), it should take care
diff --git a/docs/releases/1.9.txt b/docs/releases/1.9.txt
index 9b0fbb2a02..a4cbd7e46d 100644
--- a/docs/releases/1.9.txt
+++ b/docs/releases/1.9.txt
@@ -684,6 +684,49 @@ define built-in libraries via the ``'builtins'`` key of :setting:`OPTIONS
 <TEMPLATES-OPTIONS>` when defining a
 :class:`~django.template.backends.django.DjangoTemplates` backend.
 
+.. _simple-tag-conditional-escape-fix:
+
+``simple_tag`` now wraps tag output in ``conditional_escape``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In general, template tags do not autoescape their contents, and this behavior is
+:ref:`documented <tags-auto-escaping>`. For tags like
+:class:`~django.template.Library.inclusion_tag`, this is not a problem because
+the included template will perform autoescaping. For
+:class:`~django.template.Library.assignment_tag`, the output will be escaped
+when it is used as a variable in the template.
+
+For the intended use cases of :class:`~django.template.Library.simple_tag`,
+however, it is very easy to end up with incorrect HTML and possibly an XSS
+exploit. For example::
+
+    @register.simple_tag(takes_context=True)
+    def greeting(context):
+        return "Hello {0}!".format(context['request'].user.first_name)
+
+In older versions of Django, this will be an XSS issue because
+``user.first_name`` is not escaped.
+
+In Django 1.9, this is fixed: if the template context has ``autoescape=True``
+set (the default), then ``simple_tag`` will wrap the output of the tag function
+with :func:`~django.utils.html.conditional_escape`.
+
+To fix your ``simple_tag``\s, it is best to apply the following practices:
+
+* Any code that generates HTML should use either the template system or
+  :func:`~django.utils.html.format_html`.
+
+* If the output of a ``simple_tag`` needs escaping, use
+  :func:`~django.utils.html.escape` or
+  :func:`~django.utils.html.conditional_escape`.
+
+* If you are absolutely certain that you are outputting HTML from a trusted
+  source (e.g. a CMS field that stores HTML entered by admins), you can mark it
+  as such using :func:`~django.utils.safestring.mark_safe`.
+
+Tags that follow these rules will be correct and safe whether they are run on
+Django 1.9+ or earlier.
+
 Miscellaneous
 ~~~~~~~~~~~~~
 
diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py
index b0c74b31fa..43dd92e7f9 100644
--- a/tests/admin_views/tests.py
+++ b/tests/admin_views/tests.py
@@ -2434,7 +2434,7 @@ class AdminViewStringPrimaryKeyTest(TestCase):
         expected_link = reverse('admin:%s_modelwithstringprimarykey_history' %
             ModelWithStringPrimaryKey._meta.app_label,
             args=(quote(self.pk),))
-        self.assertContains(response, '<a href="%s" class="historylink"' % expected_link)
+        self.assertContains(response, '<a href="%s" class="historylink"' % escape(expected_link))
 
     def test_redirect_on_add_view_continue_button(self):
         """As soon as an object is added using "Save and continue editing"
diff --git a/tests/template_tests/templatetags/custom.py b/tests/template_tests/templatetags/custom.py
index 50ebd353ec..9d47ca4201 100644
--- a/tests/template_tests/templatetags/custom.py
+++ b/tests/template_tests/templatetags/custom.py
@@ -4,6 +4,7 @@ import warnings
 from django import template
 from django.template.defaultfilters import stringfilter
 from django.utils import six
+from django.utils.html import escape, format_html
 
 register = template.Library()
 
@@ -110,6 +111,24 @@ simple_tag_without_context_parameter.anything = "Expected simple_tag_without_con
 
 
 @register.simple_tag(takes_context=True)
+def escape_naive(context):
+    """A tag that doesn't even think about escaping issues"""
+    return "Hello {0}!".format(context['name'])
+
+
+@register.simple_tag(takes_context=True)
+def escape_explicit(context):
+    """A tag that uses escape explicitly"""
+    return escape("Hello {0}!".format(context['name']))
+
+
+@register.simple_tag(takes_context=True)
+def escape_format_html(context):
+    """A tag that uses format_html"""
+    return format_html("Hello {0}!", context['name'])
+
+
+@register.simple_tag(takes_context=True)
 def current_app(context):
     return "%s" % context.current_app
 
diff --git a/tests/template_tests/test_custom.py b/tests/template_tests/test_custom.py
index 5d2907074d..dec97589d6 100644
--- a/tests/template_tests/test_custom.py
+++ b/tests/template_tests/test_custom.py
@@ -104,6 +104,28 @@ class SimpleTagTests(TagTestCase):
             with self.assertRaisesMessage(TemplateSyntaxError, entry[0]):
                 self.engine.from_string("%s as var %%}" % entry[1][0:-2])
 
+    def test_simple_tag_escaping_autoescape_off(self):
+        c = Context({'name': "Jack & Jill"}, autoescape=False)
+        t = self.engine.from_string("{% load custom %}{% escape_naive %}")
+        self.assertEqual(t.render(c), "Hello Jack & Jill!")
+
+    def test_simple_tag_naive_escaping(self):
+        c = Context({'name': "Jack & Jill"})
+        t = self.engine.from_string("{% load custom %}{% escape_naive %}")
+        self.assertEqual(t.render(c), "Hello Jack &amp; Jill!")
+
+    def test_simple_tag_explicit_escaping(self):
+        # Check we don't double escape
+        c = Context({'name': "Jack & Jill"})
+        t = self.engine.from_string("{% load custom %}{% escape_explicit %}")
+        self.assertEqual(t.render(c), "Hello Jack &amp; Jill!")
+
+    def test_simple_tag_format_html_escaping(self):
+        # Check we don't double escape
+        c = Context({'name': "Jack & Jill"})
+        t = self.engine.from_string("{% load custom %}{% escape_format_html %}")
+        self.assertEqual(t.render(c), "Hello Jack &amp; Jill!")
+
     def test_simple_tag_registration(self):
         # Test that the decorators preserve the decorated function's docstring, name and attributes.
         self.verify_tag(custom.no_params, 'no_params')