collecstatic --clear is too lax about warning users

[loic84]

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.

[Sasha Romijn]

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.

[Jannis Leidel]

Sounds like a plan, loic84

[loic84]

PR ​https://github.com/django/django/pull/2057

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

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

[Tim Graham]

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

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

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

[Kevin Christopher Henry]

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.

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

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

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.