Opened 3 months ago
Last modified 2 months ago
#35607 assigned Cleanup/optimization
Improve Storage base backend API flexibility to allow filename validation to be overridden safely
Reported by: | Natalia Bidart | Owned by: | Natalia Bidart |
---|---|---|---|
Component: | File uploads/storage | Version: | |
Severity: | Normal | Keywords: | storages |
Cc: | Josh Schneier, Shai Berger | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Currently, Django's Storage base backend API provides limited flexibility with regards to customization, particularly concerning filename validation. This rigidity has historically led to challenges and security concerns. See for example CVE-2024-39330, CVE-2021-45452, and CVE-2021-31542.
To address this, I'm proposing revisiting and enhancing the public API of storage backends to support customizable validation methods. This was also discussed internally in the Django Security mailing list. This public validation method would provide a default implementation very similar to the current validations, and should be used to replace the ad-hoc validations being done in save, generate_filename, and get_available_name.
Change History (6)
comment:1 by , 3 months ago
comment:2 by , 3 months ago
Cc: | added |
---|---|
Component: | Core (Other) → File uploads/storage |
Summary: | Improve Storage base backend API Flexibility → Improve Storage base backend API flexibility to allow filename validation to be overridden safely |
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 2 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 2 months ago
Replying to Natalia Bidart:
I want to write these tests for the functions get_available_name, generate_filename, get_valid_name, and validate_file_name:
1- Where there is double dots in filename-> "../file.ext"
2_ If there is an unusual character in the filename, include one of the following characters: @ # $ % & * ( ) + = { } | \ / : ; ' " < > ? for exapmle ->
"file@…"
3_ Where there is long filename -> "a_very_very_very_long_filename_that_exceeds_the_usual_limits_of_a_filesystem.ext"
4_ Where there is filename without extension -> "filename"
5_ Where there is filename with hidden extension -> "filename."
Is this ok?
comment:6 by , 2 months ago
Owner: | changed from | to
---|
Hello Mohammad Salehi! Thank you for your interest in helping with this ticket!
I have been working with and thinking about the Storages
API for a while, and I have a few branches already with half way of a refactor. My advice would be to pause a bit on this work until I can clean up my work and push it. I will assign this ticket to myself to better signal the fact that I have started this work some time ago. Thanks again!
To add some extra context and details, below some extracts of conversations held while discussing the security reports around
Storage
API:From Shai:
From Josh Schneier:
From me:
Outcome up to this point: we did evaluate adding a public and overridable method to do validation in the recent security release but we decided not to, to keep the patch small and self contained, with the counterpart of having to duplicate file name validation in various places.
I'll be proposing a PR (no logic change) to land in
main
to extend test coverage in preparation for future refactoring.