From aef2a0ec59301022354c043744a6a2fa13583aa1 Mon Sep 17 00:00:00 2001 From: Luke Plant Date: Mon, 15 Jun 2015 11:17:09 +0100 Subject: [PATCH] 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. --- django/template/library.py | 3 ++ docs/howto/custom-template-tags.txt | 24 +++++++++++- docs/releases/1.9.txt | 43 +++++++++++++++++++++ tests/admin_views/tests.py | 2 +- tests/template_tests/templatetags/custom.py | 19 +++++++++ tests/template_tests/test_custom.py | 22 +++++++++++ 6 files changed, 110 insertions(+), 3 deletions(-) diff --git a/django/template/library.py b/django/template/library.py index 1eaaf4c0be1..23364e80246 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 c4e414cdb5e..adee0f9ffd7 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 9b0fbb2a022..a4cbd7e46d8 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 ` 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 `. 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 b0c74b31fa0..43dd92e7f98 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, '