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)

comment:1 by Tim Graham, 7 years ago

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?

in reply to:  1 comment:2 by Will Gulian, 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 Tim Graham, 7 years ago

Summary: HashedFilesMixin does not properly skip protocol-relative urlsHashedFilesMixin does not properly skip protocol-relative urls when STATIC_URL='/'
Triage Stage: UnreviewedAccepted

comment:4 by Adam Zapletal, 9 months ago

Has patch: set
Owner: changed from nobody to Adam Zapletal
Status: newassigned

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 Mariusz Felisiak, 8 months ago

Patch needs improvement: set

comment:6 by Adam Zapletal, 8 months ago

Patch needs improvement: unset

comment:7 by Mariusz Felisiak, 8 months ago

Needs tests: set

comment:8 by Adam Zapletal, 8 months ago

Needs tests: unset

comment:9 by Mariusz Felisiak, 8 months ago

Triage Stage: AcceptedReady for checkin

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 8 months ago

Resolution: fixed
Status: assignedclosed

In 107aa76:

Fixed #29022 -- Fixed handling protocol-relative URLs in ManifestStaticFilesStorage when STATIC_URL is set to /.

Note: See TracTickets for help on using tickets.
Back to Top