Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26249 closed Bug (fixed)

ManifestStaticFilesStorage crashes on absolute URLs

Reported by: Aymeric Augustin Owned by: nobody
Component: contrib.staticfiles Version: 1.9
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aymeric Augustin)

To reproduce, enable ManifestStaticFilesStorage:

STATIC_ROOT= '...'
STATIC_URL = '/static/'
STATICFILES_DIRS = ['...']
STATICFILES_STORAGE = 'django.contrib.staticfiles.storage.ManifestStaticFilesStorage'

Create a.css in one of STATICFILES_DIRS with this content:

@font-face{font-family:A;src:url(/static/a.woff) format("woff");}

Create a a.woff file next to a.css.

Then collectstatic crashes with this stack trace:

Post-processing 'a.css' failed!

Traceback (most recent call last):
  File "/Users/myk/.virtualenvs/project/bin/django-admin", line 11, in <module>
    sys.exit(execute_from_command_line())
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/core/management/__init__.py", line 353, in execute_from_command_line
    utility.execute()
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/core/management/__init__.py", line 345, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/core/management/base.py", line 348, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/core/management/base.py", line 399, in execute
    output = self.handle(*args, **options)
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 176, in handle
    collected = self.collect()
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 128, in collect
    raise processed
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/storage.py", line 245, in post_process
    content = pattern.sub(converter, content)
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/storage.py", line 184, in converter
    hashed_url = self.url(unquote(joined_result), force=True)
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/storage.py", line 131, in url
    hashed_name = self.stored_name(clean_name)
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/storage.py", line 280, in stored_name
    cache_name = self.clean_name(self.hashed_name(name))
  File "/Users/myk/.virtualenvs/project/lib/python3.5/site-packages/django/contrib/staticfiles/storage.py", line 94, in hashed_name
    (clean_name, self))
ValueError: The file 'a.css/a.woff' could not be found with <django.contrib.staticfiles.storage.ManifestStaticFilesStorage object at 0x105a34240>.

Using url(./a.woff) instead of url(/static/a.woff) avoids the issue.

It looks like HashedFilesMixin.url_converter doesn't handle that case gracefully, so we end up with a.css/a.woff instead of just a.woff. When a URL starts with STATIC_URL, perhaps it should strip it?

(I ended up with the /static/ prefix because I set webpack's output.publicPath to /static/. I don't remember why I need it, but it doesn't make the bug less valid.)

(EDIT: I need to set output.publicPath to /static/ because webpack's code splitting doesn't work without this -- chunks get loaded from /current/url/chunk.js instead of /static/chuck.js, which obviously doesn't work. I don't want to include the hostname in the URL because I want to use the same build in staging and production.).

Change History (6)

comment:1 Changed 4 years ago by Aymeric Augustin

Description: modified (diff)

comment:2 Changed 4 years ago by Aymeric Augustin

TestHashedFiles.test_template_tag_absolute seems to test this case, but the bug only manifests when files are directly in STATIC_ROOT, not when they're in a subdirectory.

comment:3 Changed 4 years ago by Aymeric Augustin

Has patch: set
Triage Stage: UnreviewedAccepted

comment:4 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:5 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: newclosed

In 706b33fe:

Fixed #26249 -- Fixed collectstatic crash for files in STATIC_ROOT referenced by absolute URL.

collectstatic crashed when:

  • a hashing static file storage backend was used
  • a static file referenced another static file located directly in STATIC_ROOT (not a subdirectory) with an absolute URL (which must start with STATIC_URL, which cannot be empty)

It seems to me that the current code reimplements relative path joining
and doesn't handle edge cases correctly. I suspect it assumes that
STATIC_URL is of the form r'/[/]+/'.

Throwing out that code in favor of the posixpath module makes the logic
easier to follow. Handling absolute paths correctly also becomes easier.

comment:6 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In 7f6fbc9:

Prevented static file corruption when URL fragment contains '..'.

When running collectstatic with a hashing static file storage backend,
URLs referencing other files were normalized with posixpath.normpath.
This could corrupt URLs: for example 'a.css#b/../c' became just 'c'.

Normalization seems to be an artifact of the historical implementation.
It contained a home-grown implementation of posixpath.join which relied
on counting occurrences of .. and /, so multiple / had to be collapsed.

The new implementation introduced in the previous commit doesn't suffer
from this issue. So it seems safe to remove the normalization.

There was a test for this normalization behavior but I don't think it's
a good test. Django shouldn't modify CSS that way. If a developer has
rendundant /s, it's mostly an aesthetic issue and it isn't Django's job
to fix it. Conversely, if the user wants a series of /s, perhaps in the
URL fragment, Django shouldn't destroy it.

Refs #26249.

Note: See TracTickets for help on using tickets.
Back to Top