Opened 14 months ago

Last modified 12 months ago

#27201 new Cleanup/optimization

Django 1.10 with STATICFILES_STORAGE breaks on absolute path in CSS with double slash

Reported by: Andrew Badr Owned by: nobody
Component: contrib.staticfiles Version: 1.10
Severity: Normal Keywords:
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

After upgrading Django to 1.10, my manage.py collectstatic command broke with an error like this one:

django.core.exceptions.SuspiciousFileOperation: The joined path (/fonts/crimson/CrimsonText-Bold.ttf) is located outside of the base path component (/Users/andrew/tmp/verse_collectstatic_test/_staticfiles)

Downgrading to Django 1.9 fixes the issue (collectstatic runs successfully), as does removing the line STATICFILES_STORAGE = 'django.contrib.staticfiles.storage.ManifestStaticFilesStorage' from my settings.py. The static file it is attempting to process in the above example is a .css file containing:

@font-face {
  font-family: "Crimson";
  src: url("/static//fonts/crimson/CrimsonText-Bold.ttf");
  font-weight: bold;
}

Note the double slash in the font path. This is a typo, but it is not a syntactic or semantic error, and it worked fine before. It runs fine if I fix the double-slash. This is an easy workaround, but I am filing this issue because it may be tricky for other users to diagnose, and because there may be some more dangerous underlying bug.

Change History (11)

comment:1 Changed 14 months ago by Tim Graham

Can you bisect to find where the behavior changed?

comment:2 Changed 13 months ago by Ed Morley

As part of #26249, a posixpath.normpath() was removed:
https://github.com/django/django/commit/7f6fbc906a21e9f8db36e06ace2a9b687aa26130#diff-c7242dedd7c93b857a668acec1e310feL179

...which would have previously been fixing the path like so:

>>> import posixpath
>>> posixpath.normpath('/static//fonts/crimson/CrimsonText-Bold.ttf')
'/static/fonts/crimson/CrimsonText-Bold.ttf'

The argument made in that commit was that "it's mostly an aesthetic issue and it isn't Django's job to fix it".

It would seem that either:
(a) this commit should be reverted
(b) the normpath() should be moved to safe_join(), so the SuspiciousFileOperation is prevented and these kind of typos are still supported
(c) the decision to fail in this case should be formalised and ideally a clearer exception given instead of SuspiciousFileOperation

Last edited 13 months ago by Ed Morley (previous) (diff)

comment:3 Changed 13 months ago by Tim Graham

Cc: Aymeric Augustin added
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I don't see a compelling reason to restore support for typos like this, but a more helpful error message could be nice, so I'd go with option (c) absent other arguments.

comment:4 Changed 13 months ago by Andrew Badr

@timgraham, I don't think "break on completely valid file paths" is an acceptable answer here. Something in Django is failing to handle valid paths correctly—that thing should be fixed, either by normalizing the paths so it can understand them, or making it smart enough to handle non-normalized paths.

comment:5 Changed 13 months ago by Tim Graham

I'm not aware of what the rules are for valid file paths. Could you cite a spec or something so we know what cases to support?

comment:6 Changed 13 months ago by Andrew Badr

That's a complicated question. :) The spec is at https://www.ietf.org/rfc/rfc3986.txt but Django shouldn't be in the business of trying to make sense of it. Hoping aaugustin will chime in here.

comment:7 Changed 13 months ago by Aymeric Augustin

Regarding path normalization in general

I don't care either way (which I why I haven't commented). I don't mind if sloppy coding results in exceptions. I don't mind either if we restore some form normalization -- as long as it's done properly, unlike the implementation I removed. Normalizing redundant slashes should only touch the path, not the other bits of the URL. You need to parse and reassemble the URL.

Regarding this bug report

Based on the example shown in the report, I suspect the problem only arises if the duplicate slash is found just after STATIC_URL, which Django strips at some point (if memory serve). Stripping slashes on the left of the URL just after stripping STATIC_URL could be the correct resolution here. If so, it should be fairly easy to write a patch and a test.

Last edited 13 months ago by Aymeric Augustin (previous) (diff)

comment:8 Changed 13 months ago by Andrew Badr

Thanks, aaugustin. I guess I'm surprised that Django is parsing my CSS to begin with.

comment:9 Changed 13 months ago by Simon Charette

I guess I'm surprised that Django is parsing my CSS to begin with.

How would you suggest we generate a manifest of you static assets without parsing your CSS files?

comment:10 in reply to:  9 Changed 13 months ago by Andrew Badr

Replying to charettes:

How would you suggest we generate a manifest of you static assets without parsing your CSS files?

The current implementation would work even if it didn't parse or alter the contents of any CSS files. To use this ticket as an example, the font file without a hash in its filename ("CrimsonText-Bold.ttf") would load just fine. Of course, this makes setting caching headers a little more complicated, but then again, Django can't guarantee, for example, that you aren't dynamically loading some static assets with JavaScript. Those paths in JS would not get converted in the current manifest system. What we have now is an incomplete and (to me, a little) surprising solution. There's a case to be made that the path rewriting should make it easier to distinguish altered paths from original ones in order to address this caching issue.

Last edited 12 months ago by Andrew Badr (previous) (diff)

comment:11 Changed 12 months ago by Jonatas CD

so, to make it clear, the decision in what is to be done here is this?

(c) the decision to fail in this case should be formalised and ideally a clearer exception given instead of SuspiciousFileOperation

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