[Django] #21581: collecstatic --clear can potentially wipe clean a user's system.
Django
-------------------------------------+--------------------
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: master
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+--------------------
`STATIC_ROOT` is not set in the `settings.py` that ships with the default
project template, so `STATIC_ROOT = ''` from `global_settings.py` is used
instead.
`''` is a valid relative path, which means "from the current directory",
so common case, you would wipe your whole project, worse case you can wipe
your system (assuming you have sufficient privileges).
This is made worse by another bug: we don't display the affected
directory.
The `isinstance(self.storage, FileSystemStorage)` (1) check fails because
`self.storage` is not yet evaluated and still resolves to
`ConfiguredStorage` (2) which is not a `FileSystemStorage` subclass.
(1)
https://github.com/django/django/blob/master/django/contrib/staticfiles/management/commands/collectstatic.py#L141
(2)
https://github.com/django/django/blob/master/django/contrib/staticfiles/storage.py#L304
Finally I think `--dry-run` should print a message that confirms that the
command is really run in dry-run mode. Currently, when you do `--dry-run
--clear`, you get a scary warning that you will delete files and you even
have to confirm by typing "yes" just like the real command, that's enough
to make you doubt that the `--dry-run` is effective.
I suggest the following:
- Set `global_settings.STATIC_ROOT` to `None`.
- Add `STATIC_ROOT = os.path.join(BASE_DIR, 'static')` to the default
template.
- Have management commands refuse to run when `settings.STATIC_ROOT ==
None`.
- Evaluate `Command.storage` one way or another.
- Add a warning when the command is run with `--dry-run` mode.
--
Ticket URL: <https://code.djangoproject.com/ticket/21581>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* cc: eromijn@… (added)
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Yes, this does seem quite serious. I think your suggestions are a good way
to go, although I don't know this area of Django very well.
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:1>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jezdez):
Sounds like a plan, loic84
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:2>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* status: new => assigned
* owner: nobody => loic84
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:3>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
PR https://github.com/django/django/pull/2057
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:4>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Component: contrib.staticfiles | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4befb3015c26810a68cfcf57e0cd8b062f56f1c5"]:
{{{
#!CommitTicketReference repository=""
revision="4befb3015c26810a68cfcf57e0cd8b062f56f1c5"
Fixed #21581 -- Fixed a number of issues with collectstatic.
When STATIC_ROOT wasn't set, collectstatic --clear would delete
every files within the current directory and its descendants.
This patch makes the following changes:
Prevent collectstatic from running if STATIC_ROOT isn't set.
Fixed an issue that prevented collectstatic from displaying the
destination directory.
Changed the warning header to notify when the command is run
in dry-run mode.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:5>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3fd16e6261f9c19a68ba69f1d97027a6eaf3a22b"]:
{{{
#!CommitTicketReference repository=""
revision="3fd16e6261f9c19a68ba69f1d97027a6eaf3a22b"
[1.6.x] Fixed #21581 -- Fixed a number of issues with collectstatic.
When STATIC_ROOT wasn't set, collectstatic --clear would delete
every files within the current directory and its descendants.
This patch makes the following changes:
Prevent collectstatic from running if STATIC_ROOT isn't set.
Fixed an issue that prevented collectstatic from displaying the
destination directory.
Changed the warning header to notify when the command is run
in dry-run mode.
Backport of 4befb3015c from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:6>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Loic, could you please take a look at #21750? I think we should probably
make a change here to avoid that issue (perhaps move the validation of
`STATIC_ROOT` to `collectstatic`).
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:7>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"1e9e7351f88a59261da6e616934c8283a6e3e565"]:
{{{
#!CommitTicketReference repository=""
revision="1e9e7351f88a59261da6e616934c8283a6e3e565"
Fixed #21750 -- Fixed regression introduced by 4befb30.
Validating STATIC_ROOT in StaticFilesStorage.__init__ turned out to be
problematic - especially with tests - because the storage refuses to work
even
if there are no actual interactions with the file system, which is
backward
incompatible.
Originally the validation happened in the StaticFilesStorage.path method,
but
that didn't work as expected because the call to
FileSystemStorage.__init__
replaced the empty value by a valid path. The new approach is to move back
the
check to the StaticFilesStorage.path method, but ensure that the location
attribute remains None after the call to super.
Refs #21581.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:8>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6728f159f060e536f1543b4afecb602b6693babb"]:
{{{
#!CommitTicketReference repository=""
revision="6728f159f060e536f1543b4afecb602b6693babb"
[1.6.x] Fixed #21750 -- Fixed regression introduced by 4befb30.
Validating STATIC_ROOT in StaticFilesStorage.__init__ turned out to be
problematic - especially with tests - because the storage refuses to work
even
if there are no actual interactions with the file system, which is
backward
incompatible.
Originally the validation happened in the StaticFilesStorage.path method,
but
that didn't work as expected because the call to
FileSystemStorage.__init__
replaced the empty value by a valid path. The new approach is to move back
the
check to the StaticFilesStorage.path method, but ensure that the location
attribute remains None after the call to super.
Refs #21581.
Backport of 1e9e7351f8 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:9>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by marfire):
FYI, I'm embarrassed to report that I just did this exact thing in 1.6.1
and wiped out my entire project! Thanks to VCS I was able to recover
easily, but, yikes. Thanks Loic for finding and fixing this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:10>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
@marfire I can relate to your story since this happened to me on a staging
server. I was quite grateful that my fabric task performed a `cd` to the
project folder and didn't execute `/srv/project/manage.py` from my user
directory!
The tricky part is that since [3f1c7b70537330435e2ec2fca9550f7b7fa4372e],
`STATIC_ROOT` isn't present in the `settings.py` shipped by the default
template, so it's a lot easier to forget setting it to a meaningful value,
which in turn exposes to this bug.
Hopefully we'll be able to release 1.6.2 soon enough so we can forget
about this bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:11>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"7e27885c6e7588471fd94a4def16b7081577bdfc"]:
{{{
#!CommitTicketReference repository=""
revision="7e27885c6e7588471fd94a4def16b7081577bdfc"
Reworked the detection of local storages for the collectstatic command.
Before 4befb30 the detection was broken because we used isinstance
against a LazyObject rather than against a Storage class. That commit
fixed it by looking directly at the object wrapped by LazyObject.
This could however be a problem to anyone who subclasses the
collectstatic management Command and directly supplies a Storage class.
Refs #21581.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:12>
Django
-------------------------------------+------------------------------------
Reporter: loic84 | Owner: loic84
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d6db48e5f6320ec3d76419b67c6a3819f078be5c"]:
{{{
#!CommitTicketReference repository=""
revision="d6db48e5f6320ec3d76419b67c6a3819f078be5c"
[1.6.x] Reworked the detection of local storages for the collectstatic
command.
Before 4befb30 the detection was broken because we used isinstance
against a LazyObject rather than against a Storage class. That commit
fixed it by looking directly at the object wrapped by LazyObject.
This could however be a problem to anyone who subclasses the
collectstatic management Command and directly supplies a Storage class.
Refs #21581.
Backport of 7e27885c6e7588471fd94a4def16b7081577bdfc from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21581#comment:13>