Opened 10 years ago

Last modified 3 weeks ago

#26329 assigned Bug

StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

Reported by: Seán Hayes Owned by: blighj
Component: contrib.staticfiles Version: 1.8
Severity: Normal Keywords:
Cc: matt@…, Adam Zapletal Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I got the following email from our staging server:

The joined path (/images/no-image.jpg) is located outside of the base path component (/full-path/collected-static)

Someone was using the following template tag:

{% static "/images/no-image.jpg" as no_image_url %}

I checked to see why our tests didn't raise the same error, and it turns out it only happens with CachedStaticFilesStorage (and likely the other manifest storages), StaticFilesStorage and FileSystemStorage seem to just ignore this error.

Since CachedStaticFilesStorage shouldn't be used during testing, I think the parent classes should raise the same error.

Change History (10)

comment:1 by Tim Graham, 10 years ago

Component: Uncategorizedcontrib.staticfiles
Triage Stage: UnreviewedAccepted

As a note to anyone tackling this, it only happens with DEBUG = False.

comment:2 by Matt Deacalion Stevens, 10 years ago

Cc: matt@… added
Owner: changed from nobody to Matt Deacalion Stevens
Status: newassigned

comment:3 by David Sanders, 9 years ago

If no one knows of a legitimate use case for absolute paths being passed to the static templatetag, it seems like this could be fixed fairly easily by raising a ValueError in the StaticFileStorage url method if the path has a leading slash.

However, while we're on the topic there are some other inconsistencies such as for CachedFileStorage leading spaces (but no initial slash) in DEBUG returns the path with a leading space, where as not in DEBUG chomps the leading space. For StaticFileStorage the leading space is always URL encoded.

Seems like a general 'clean_url' method for StaticFileStorage would be useful, that strips spaces and raises a ValueError for a leading slash.

comment:4 by Matt Deacalion Stevens, 8 years ago

Owner: Matt Deacalion Stevens removed
Status: assignednew

comment:5 by Adam Zapletal, 19 months ago

Cc: Adam Zapletal added

I tested this in Django v5.0.2, and the error message now takes this form: ValueError: Missing staticfiles manifest entry for '/test.css'.

The error message seems plenty clear to me, so I wonder if a change is even needed here. Also, it makes sense to me that this only happens with ManifestStaticFilesStorage since StaticFilesStorage doesn't have a manifest. The inconsistency between the two storage classes is expected.

comment:6 by Mariusz Felisiak, 19 months ago

Adam, Can you submit PR with a regression test to prove it's fixed?

comment:7 by Adam Zapletal, 19 months ago

I'm not sure what that would look like since the current behavior seems to be expected. Do you want a test showing that the leading slash typo gives the expected error message in the static templatetag tests? I'm happy to help, but I need some guidance.

comment:8 by Jake Howard, 6 months ago

Has patch: set
Owner: set to Jake Howard
Status: newassigned

I just got caught out by this, thanks to a simple typo. I think regardless of which way around, the behaviour should be the same. I've opened a PR to add support for leading slashes: PR

comment:9 by Sarah Boyce, 4 months ago

Patch needs improvement: set

comment:10 by blighj, 3 weeks ago

Owner: changed from Jake Howard to blighj
Patch needs improvement: unset

The last patch on this has stalled, so I'm providing a new patch.

I think there is a more general point that could be another ticket, the behaviour of ManifestStaticFilesStorage can be different on development vs production. The leading slash is one case. The other obvious case you see in forms/stackoverflow is that when a file is missing this won't error in development but will in production. I've also been caught out by the backslash as path separtor and url in the wrong case. Both of these will work in development, but fail on production with ManifestStaticFilesStorage.

The patch will solve two of these issues. The package I put together has a potential solution to the other two (missing file or wrong case). It could be incorporated in a follow up ticket or as part of this patch if that was deemed better. https://github.com/blighj/django-manifeststaticfiles-enhanced

Last edited 3 weeks ago by blighj (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top