[Django] #26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't
-------------------------------+--------------------
Reporter: SeanHayes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/26329>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+------------------------------------

Reporter: SeanHayes | Owner: nobody
Type: Bug | Status: new

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
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
* stage: Unreviewed => Accepted
* component: Uncategorized => contrib.staticfiles
* needs_tests: => 0
* needs_docs: => 0

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:1>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+-------------------------------------
Reporter: SeanHayes | Owner: Deacalion
Type: Bug | Status: assigned

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Deacalion):

* status: new => assigned
* cc: matt@… (added)
* owner: nobody => Deacalion

--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:2>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+-------------------------------------
Reporter: SeanHayes | Owner: Deacalion
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

Comment (by dsanders11):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:3>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+------------------------------------
Reporter: Seán Hayes | Owner: (none)
Type: Bug | Status: new

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Changes (by Matt Deacalion Stevens):

* owner: Matt Deacalion Stevens => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:4>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+------------------------------------
Reporter: Seán Hayes | Owner: (none)

Type: Bug | Status: new

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Changes (by Adam Zapletal):

* cc: Adam Zapletal (added)

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:5>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+------------------------------------
Reporter: Seán Hayes | Owner: (none)

Type: Bug | Status: new

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Comment (by Mariusz Felisiak):

Adam, Can you submit PR with a regression test to prove it's fixed?
--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:6>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+------------------------------------
Reporter: Seán Hayes | Owner: (none)

Type: Bug | Status: new

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Comment (by Adam Zapletal):

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:7>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+-------------------------------------
Reporter: Seán Hayes | Owner: Jake
| Howard
Type: Bug | Status: assigned

Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jake Howard):

* has_patch: 0 => 1
* owner: (none) => Jake Howard

* status: new => assigned

Comment:

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:
[https://github.com/django/django/pull/#### PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:8>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+-------------------------------------
Reporter: Seán Hayes | Owner: Jake
| Howard
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:9>

[Django]

#26329: StaticFilesStorage permits leading slash, CachedStaticFilesStorage doesn't

-------------------------------------+------------------------------------
Reporter: Seán Hayes | Owner: blighj

Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Changes (by blighj):

* needs_better_patch: 1 => 0
* owner: Jake Howard => blighj

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26329#comment:10>