Opened 9 years ago

Closed 7 years ago

#25484 closed Bug (fixed)

static template tag outputs invalid HTML if storage class's url() returns a URI with '&' characters.

Reported by: João Miguel Neves Owned by: alix-
Component: contrib.staticfiles Version: dev
Severity: Normal Keywords:
Cc: olivier.tabone@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using a storage class like 'storages.backend.s3.S3BotoStorage' from django-storages/django-storages-redux the following template code:

<link href="{% static 'css/styles.css' %}" rel="stylesheet" type="text/css" />

is rendered as:

<link href="https://bucket.region.amazonaws.com:443/css/styles.css?Signature=sm8uyGp12rTST59hSWyTtZz8A&Expires=1443476662&AWSAccessKeyId=AN_AWS_KEY" rel="stylesheet" type="text/css" />

when the valid html should be:

<link href="https://bucket.region.amazonaws.com:443/css/styles.css?Signature=sm8uyGp12rTST59hSWyTtZz8A&amp;Expires=1443476662&amp;AWSAccessKeyId=AN_AWS_KEY" rel="stylesheet" type="text/css" />

This issue is only visible when using a storage that returns URLs with '&' characters in the query string.

Work-around: You get the correct result if you render the above as:

{% static 'css/styles.css' as this_url %}
<link href="{{ this_url }}" rel="stylesheet" type="text/css" />

Attachments (2)

static_escape.patch (662 bytes ) - added by João Miguel Neves 9 years ago.
Patch to fix the static lack of escape issue
static_escape.2.patch (1.4 KB ) - added by João Miguel Neves 9 years ago.
Patch to fix the static lack of escape issue (now also covering staticfiles)

Download all attachments as: .zip

Change History (13)

by João Miguel Neves, 9 years ago

Attachment: static_escape.patch added

Patch to fix the static lack of escape issue

by João Miguel Neves, 9 years ago

Attachment: static_escape.2.patch added

Patch to fix the static lack of escape issue (now also covering staticfiles)

comment:1 by Tim Graham, 9 years ago

Component: Uncategorizedcontrib.staticfiles
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

I think something like this is more correct:

if context.autoescape:
    url = conditional_escape(url)

Could you also add some tests?

comment:2 by alix-, 7 years ago

Owner: changed from nobody to alix-
Status: newassigned

comment:3 by alix-, 7 years ago

PR https://github.com/django/django/pull/7494 including:

  • Changes suggested by Tim
  • Test

comment:4 by alix-, 7 years ago

Has patch: set

comment:5 by Olivier Tabone, 7 years ago

Cc: olivier.tabone@… added

comment:6 by Simon Charette, 7 years ago

Patch needs improvement: set
Version: 1.8master

The patch is looking but I left some comments for improvement.

comment:7 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 374e623:

Fixed #25484 -- Made {% static %} render escaped URLs.

comment:8 by Tim Graham, 7 years ago

Patch needs improvement: unset
Resolution: fixed
Status: closednew

As discussed on the original PR, the added unquote() likely isn't correct. Here's Florian's suggestion:

Given that staticfiles operates on files and in general as such has no concept of query params, I think it is safe to assume and expect that the static functions are passed filenames/paths. Filenames (or rather file paths) literally allow basically every character aside from a null byte (depending on your system). If we agree that this assumption is correct the following steps seem logical:

  • To generate a URL, we are forced to run the file through urlquote, since it might contain characters not suitable in this form as part of the URL (most notably a "?" since that would mark the start of the query params).
  • Since URL generation can add API access keys, the final URL can indeed contain unquoted "&" and "?", which is completely valid and should be allowed.
  • When it comes to HTML though, we still have to run it through HTML escape since it might contain otherwise invalid or (thanks to browser parsing of & in that context) ambiguous characters.

Follow up PR.

comment:9 by Tim Graham <timograham@…>, 7 years ago

In e233f357:

Refs #25484 -- Removed incorrect unquoting in {% static %}.

Regression in 374e6230ca9f9bb84cc9dd760dfb6395fbb5ff0f.

Thanks Florian Apolloner for the report and analysis.

comment:10 by Tim Graham <timograham@…>, 7 years ago

In 1a04b17:

Refs #25484 -- Made non-staticfiles {% static %} tag quote its output.

comment:11 by Tim Graham, 7 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top