Opened 19 months ago

Closed 18 months ago

Last modified 17 months ago

#21581 closed Bug (fixed)

collecstatic --clear is too lax about warning users

Reported by: loic84 Owned by: loic84
Component: contrib.staticfiles Version: master
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 Changed 19 months ago by erikr

  • Cc eromijn@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 19 months ago by jezdez

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

Sounds like a plan, loic84

comment:3 Changed 19 months ago by loic84

  • Owner changed from nobody to loic84
  • Status changed from new to assigned

comment:5 Changed 18 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 18 months ago by 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

comment:7 Changed 18 months ago 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).

comment:8 Changed 18 months ago by 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.

comment:9 Changed 18 months ago by 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

comment:10 Changed 17 months ago 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.

comment:11 Changed 17 months ago 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.

comment:12 Changed 17 months ago by 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.

comment:13 Changed 17 months ago by 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.

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