Opened 7 years ago
Closed 8 months ago
#29022 closed Bug (fixed)
HashedFilesMixin does not properly skip protocol-relative urls when STATIC_URL='/'
Reported by: | Will Gulian | Owned by: | Adam Zapletal |
---|---|---|---|
Component: | contrib.staticfiles | Version: | 2.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
While protocol-relative urls have been deprecated it would be nice for Django staticfiles to support it since a lot of code still uses it or explicitly not support it. Right now the relevant snippet implies that the code does filter out protocol-relative urls but it currently does not:
# django/contrib/staticfiles/storage.py # Ignore absolute/protocol-relative and data-uri URLs. if re.match(r'^[a-z]+:', url): return matched
I've included an example snippet that uses a protocol-relative url but is not filtered:
@import url("//fonts.googleapis.com/css?family=Source+Sans+Pro:400,700|Raleway:400,800,900");
Change History (10)
follow-up: 2 comment:1 by , 7 years ago
comment:2 by , 7 years ago
Replying to Tim Graham:
The code changed in 08ed3cc6d160d0d864ff687db9a62959a86e7372 so the comment is outdated but as far as I see, a URL starting with
//
would likely be filtered out in the next block:if url.startswith('/') and not url.startswith(settings.STATIC_URL):
. Anyway, there's still a test assertion for//foobar
remaining unchanged and I don't see a change to the URL you provided if I add that to the test. Can you find the difference between that test and your situation that reproduces the problem?
Sorry I should have looked at that function more closely. It's not being caught in my case because my STATIC_URL
is /
so the line that should exit doesn't because the protocol-relative url actually starts with my STATIC_URL
.
comment:3 by , 7 years ago
Summary: | HashedFilesMixin does not properly skip protocol-relative urls → HashedFilesMixin does not properly skip protocol-relative urls when STATIC_URL='/' |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 9 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
It looks like adding back a simple check for protocol-relative URLs before the STATIC_URL
check will fix this. That seems reasonable to me if Django is going to support setting STATIC_URL
to /
.
I opened a PR with this change and a regression test for discussion.
comment:5 by , 8 months ago
Patch needs improvement: | set |
---|
comment:6 by , 8 months ago
Patch needs improvement: | unset |
---|
comment:7 by , 8 months ago
Needs tests: | set |
---|
comment:8 by , 8 months ago
Needs tests: | unset |
---|
comment:9 by , 8 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
The code changed in 08ed3cc6d160d0d864ff687db9a62959a86e7372 so the comment is outdated but as far as I see, a URL starting with
//
would likely be filtered out in the next block:if url.startswith('/') and not url.startswith(settings.STATIC_URL):
. Anyway, there's still a test assertion for//foobar
remaining unchanged and I don't see a change to the URL you provided if I add that to the test. Can you find the difference between that test and your situation that reproduces the problem?