[Django] #27201: Django 1.10 with STATICFILES_STORAGE breaks on absolute path in CSS with double slash
Django
double slash
-------------------------------------+--------------------
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Can you [https://docs.djangoproject.com/en/dev/internals/contributing
/triaging-tickets/#bisecting-a-regression bisect] to find where the
behavior changed?
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:1>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by edmorley):
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) `safe_join()`'s checks should perform a `normpath()` to avoid the
`SuspiciousFileOperation` and continue supporting these kind of typos
(c) the decision to fail in this case should be formalised and ideally a
clearer exception given instead of `SuspiciousFileOperation`
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:2>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: aaugustin (added)
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:3>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewbadr):
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:4>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:5>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewbadr):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:6>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
'''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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:7>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewbadr):
Thanks, aaugustin. I guess I'm surprised that Django is parsing my CSS to
begin with.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:8>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
> 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:9>
Django
double slash
Reporter: andrewbadr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewbadr):
Replying to [comment:9 charettes]:
>
> How would you suggest we generate a manifest of you static assets
without parsing your CSS files?
We're getting a bit off-topic, but 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:10>
Django
double slash
Reporter: Salkot | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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`
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:11>
Django
double slash
Reporter: Andrew Badr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Adam Zapletal):
* cc: Adam Zapletal (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:12>
Django
double slash
Reporter: Andrew Badr | Owner: nobody
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:13>
Django
double slash
Reporter: Andrew Badr | Owner: nobody
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution: fixed
Has patch: 0 | Needs documentation: 0
Changes (by Mariusz Felisiak):
* resolution: => fixed
* status: new => closed
Comment:
Agreed, the previous behavior has been restored in
53bffe8d03f01bd3214a5404998cb965fb28cd0b (intentionally or not).
Thanks Adam.
--
Ticket URL: <https://code.djangoproject.com/ticket/27201#comment:14>