#35607: Improve Storage base backend API Flexibility
-------------------------------------+-------------------------------------
Reporter: Natalia Bidart | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Core (Other) | Version:
Severity: Normal | Resolution:
Keywords: storages | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
To add some extra context and details, below some extracts of
conversations held while discussing the security reports around `Storage`
API:
From Shai:
> [...] I think the validation itself should be overridable, but I feel
weary about adding an API in a security release; can we add it as
"internal" in the security release, and document it (making it public)
later?
From Josh Schneier:
> Untangling the various ways that validation was peppered through the
code base in a couple of competing ways led me to drafting a local change
that unified them all. I’m happy to hear you’re taking a stab at that,
shows my head was in the right place.
> I think you are already on top of `get_available_name` and
`generate_filename` (which uses `validate_file_name`) but want to also
call your attention to `get_valid_name` which is also part of the publicly
overridable API. Currently it calls into `get_valid_filename` which has
yet another implementation of some of the protection logic, it also
appears to be its only used place in the code base.
From me:
> At a high level, I believe the core challenges to resolve are:
> 1. The entanglement and tight coupling of file name and file path
validations in the code.
> 2. The scattering of validations across multiple methods and files.
> To fix 1, I think we should take path validation out of
`validate_file_name` and put it in a dedicated `validate_file_path`. This
makes the `allow_relative_path` switch disappear. Backward compatibility
is an issue with this plan though.
> There is one more change that I considered but postponed, which is
potentially removing the call to `validate_file_name` in
FileField.generate_filename[1] since this method calls it's storage's
`generate_filename` which would *also* call `validate_file_name` (at least
in its default implementation). My main concern here is how often/likely
is to have custom storages overriding `generate_filename` without
validating the given name
> [1]
https://github.com/django/django/blob/main/django/db/models/fields/files.py#L336
Outcome up to this point: we did evaluate adding a public and overridable
method to do validation in the
[https://www.djangoproject.com/weblog/2024/jul/09/security-releases/
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35607#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/01070190c69b7883-b454c7dc-3bae-4ee6-a15c-d208ac9a6055-000000%40eu-central-1.amazonses.com.
