Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30745 closed New feature (needsinfo)

Allow serving a default file for FileField from static URL.

Reported by: bhch Owned by: nobody
Component: File uploads/storage Version: 2.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Prior to version 1.9, it was possible to serve a default file for a FileFIeld from an absolute url just by setting the default value on the field like:

ImageField(default='/static/placeholder.png' ...)

But due to the security concerns raised in #25905, the commit fdf5cd34 strips off all the leading slashes thereby making the path relative to which urljoin later prepends a base url. This removes the possibility of serving a default file from the static url.

I think a better solution would be to remove more than 1 leading slashes, but not one.

Current implementation: url.lstrip('/').

Proposed: re.sub(r'/{2,}', '/', url) or re.sub(r'/{2,}', '', url).

This will keep the absolute urls as intended and also convert external urls to internal.

Change History (2)

comment:1 by Carlton Gibson, 5 years ago

Resolution: needsinfo
Status: newclosed

I suspect the best approach here is to just use an known default relative to STATIC_URL — so instead of /static/placeholder.png, just placeholder.png, allowing urljoin() to do it's thing? (Does that not work for you? If not, I'd try overriding the url property on the field to handle the default case before thinking about adjusting the storage...)

Given the sensitive nature of this, any change here will need to go via a full proposal to the DevelopersMailingList. I'm guessing this isn't something we'd be keen to change but we'd need to see exactly what's being proposed, and exactly how we can be sure we're not introducing a security regression before moving forwards. (Test cases showing that the re is correct for all manner of cases being at least a starting point there.) I'll mark it needsinfo on that basis.

comment:2 by bhch, 5 years ago

The current implementation doesn't return the path relative to STATIC_URL, but to MEDIA_URL. Using placeholder.png will return /media/placeholder.png -- makes it a little difficult for apps to distribute a default image.

Overriding methods is certainly an option; but this seems to be a nice little shortcut feature.

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