Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17737 closed Bug (fixed)

collectstatic copies wrong files to STATIC_ROOT

Reported by: pigletto Owned by: jezdez
Component: contrib.staticfiles Version: 1.3
Severity: Normal Keywords:
Cc: pigletto@…, slinkp@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In specific situation, in Django 1.3.1, second run of collecstatic command might copy wrong files to STATIC_ROOT. This is dangerous situation in production environment.

Attached test and patch against tags/1.3.1

This issue doesn't exist in current trunk (Django 1.4) but seems to me that it might been fixed 'accidentally' so I also attach (passing) test for current trunk.

To reproduce the issue:

  1. INSTALLED_APPS = ['appA', 'appB']
  2. static files in apps:
    appA/
       static/
            file.js
    
    appB/
       static/
            file.js
    
  1. Modification date of file.js in appB is newer than modification date of file.js in appA
  1. run collectstatic twice

At first run everything is OK. File from appA is copied to STATIC_ROOT (as documentation says duplicate files are taken in order of apps in INSTALLED_APPS), but in second run file.js is replaced by wrong one - from appB.

This is caused by use of:

 shutil.copy2(source_path, full_path)

in copy_file method at collectstatic.py.

shutil.copy2 preservers original file modification date what causes that file.js from appB is considered newer than file in STATIC_ROOT.

Attachments (2)

django_trunk_patch.diff (4.7 KB) - added by pigletto 3 years ago.
Tests and patch against current trunk
django_1.3.1_patch.diff (4.8 KB) - added by pigletto 3 years ago.
Tests and patch against 1.3.1 tag

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by jezdez

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

This is a release blocker for 1.3.X as it's a potential data loss bug.

In trunk it was indeed fixed by using the staticfiles storage's save method.

comment:2 Changed 3 years ago by jezdez

  • Owner changed from nobody to jezdez

comment:3 follow-up: Changed 3 years ago by slinkp

  • Cc slinkp@… added

I think I just discovered a different way to trigger the same problem - different file being collected on second run.
What I had was duplicate paths found via different Finders.

To reproduce w/ Django 1.3.1:

  • Use default value of settings.STATICFILES_FINDERS
  • Put ['appA', 'appB'] in settings.INSTALLED_APPS. Order does not matter.
  • Static files in apps:
     appA/
       static/
         file.js   # MORE RECENT
    
     appB/
       something_else/
         file.js   # OLDER
    
  • settings.STATICFILES_DIRS = [ '/path/to/appB/something_else' ]

Now, the first time you run, the file.js in appB/something_else will be collected first because FileSystemFinder runs first. Then the AppDirectoriesFinder will find the one in appA/static, but it's skipped because "file.js" is already in self.copied_files.
So appB wins.

The second time you run, the file.js in appB/something_else is found first again, but skipped because it's not modified. Then the AppDirectoriesFinder runs, and the file.js in appA/static is found, but this time "file.js" is not in self.copied_files, and self.delete_file() finds that it's more recent than the one we collected on the previous run.
So appA wins.

If I understand the original bug report correctly, this variant should be fixed on trunk too.

comment:4 in reply to: ↑ 3 Changed 3 years ago by pigletto

Replying to slinkp:

The second time you run, the file.js in appB/something_else is found first again, but skipped because it's not modified. Then the AppDirectoriesFinder runs, and the file.js in appA/static is found, but this time "file.js" is not in self.copied_files, and self.delete_file() finds that it's more recent than the one we collected on the previous run.
So appA wins.

If I understand the original bug report correctly, this variant should be fixed on trunk too.

I think that you're right there but I just started to wonder about slightly different way to get wrong results with collectstatic, even with trunk version(!).

According to your example structure: if appA/static/file.js will be modified after collectstatic was run, then, even in trunk version, it will be copied to STATIC_ROOT, because it's modification date will be newer than modification date of STATIC_ROOT/file.js.

Changed 3 years ago by pigletto

Tests and patch against current trunk

Changed 3 years ago by pigletto

Tests and patch against 1.3.1 tag

comment:5 Changed 3 years ago by pigletto

Added tests and patch for both 1.3 and trunk. Interesting thing is that after the patch I had to slightly modify another test (at trunk) that used cached storage. This was caused by use of wrong file name in that test.

comment:6 Changed 3 years ago by jezdez

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

In [17612]:

Fixed #17737 -- Stopped the collectstatic management command from copying the wrong file in repeated runs. Thanks, pigletto.

comment:7 Changed 3 years ago by jezdez

In [17613]:

[1.3.X] Fixed #17737 -- Stopped the collectstatic management command from copying the wrong file in repeated runs. Thanks, pigletto.

Backport from trunk (r17612).

comment:8 Changed 3 years ago by slinkp

Works for my variant, thanks. Jezdez, will this get backported to the standalone django-staticfiles? Thanks!

comment:9 Changed 3 years ago by jezdez

Yeah, I'll backport the change asap to the django-staticfiles apps.

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