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

[Andrew Badr]

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.

[Tim Graham]

Can you ​bisect to find where the behavior changed?

[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

[Tim Graham]

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.

[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.

[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?

[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.

[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 it 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.

[Andrew Badr]

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

[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?

[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.

[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

[Adam Zapletal]

I'm unable to reproduce this issue on Django v5.0.2, so I wonder if this ticket can be closed. ManifestStaticFilesStorage is able to alter a file path containing // to the hashed version just fine. The // is retained in the hashed file's content.

The problem may have been fixed in 53bffe8d03f01bd3214a5404998cb965fb28cd0b, where a call to posixpath.normpath was added back. The former removal of the call is referenced in the second comment above.

[Mariusz Felisiak]

Agreed, the previous behavior has been restored in 53bffe8d03f01bd3214a5404998cb965fb28cd0b (intentionally or not).

Thanks Adam.