Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#21581 closed Bug (fixed)

collecstatic --clear is too lax about warning users

Reported by: loic84 Owned by: loic84
Component: contrib.staticfiles Version: dev
Severity: Release blocker Keywords:
Cc: eromijn@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (13)

comment:1 by Sasha Romijn, 11 years ago

Cc: eromijn@… added
Triage Stage: UnreviewedAccepted

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.

comment:2 by Jannis Leidel, 11 years ago

Summary: collecstatic --clear can potentially wipe clean a user's system.collecstatic --clear is too lax about warning users

Sounds like a plan, loic84

comment:3 by loic84, 11 years ago

Owner: changed from nobody to loic84
Status: newassigned

comment:5 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 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.

comment:6 by Tim Graham <timograham@…>, 11 years ago

In 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

comment:7 by Tim Graham, 11 years ago

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).

comment:8 by Tim Graham <timograham@…>, 11 years ago

In 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.

comment:9 by Tim Graham <timograham@…>, 11 years ago

In 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

comment:10 by Kevin Christopher Henry, 11 years ago

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.

comment:11 by loic84, 11 years ago

@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.

comment:12 by Tim Graham <timograham@…>, 11 years ago

In 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.

comment:13 by Tim Graham <timograham@…>, 11 years ago

In 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.

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