Closed (fixed)
Project:
Flysystem - S3
Version:
2.0.0-rc1
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
9 Nov 2021 at 13:41 UTC
Updated:
1 Mar 2022 at 10:34 UTC
Jump to comment: Most recent
Problem/Motivation
Marking this issue as major as it leads to data loss.
When a file is uploaded through CORS, a file entity (marked as temporary) is created before the upload to S3 is started. When this file entity is created, there is no check for whether there already is a file entity with the same path. The only check is for whether the file already exists on S3.
This means that if the upload fails, the file stays in the file_managed table (with status = 0). If you upload the same file again (with the same name), it will not find the file in S3, and a duplicate entity will be created. Both entities will share the same filename and uri. If the second one is successful, and the content is saved, you will have one file entity with status = 0, and one with status = 1.
This can lead to data loss. When cron runs its file cleanup job, it looks for inactive files (in file_cron()). This finds the ones the failed to upload, and deletes them. Because these share the same filename, the successfully uploaded file is also deleted.
This issue only effects files uploaded through CORS, as when uploading via Drupal, the file entity is only created after the upload is successful.
Steps to reproduce
1. Enable CORS on a Drupal site.
2. Upload a file, and do something to make the upload fail before completing, e.g. refreshing the page before it completes.
3. Upload a file with the same filename again, this time allow it to pass.
4. Look in the file_managed table, notice that there are two entires for the same filename.
5. Wait for 6 hours (or however long you have set system.file.temporary_maximum_age) and run cron.
6. View the node that you successfully uploaded the file to, notice that an AccessDenied error is returned from S3 when viewing the file. The file has been removed from S3, although Drupal still thinks it's there (as there is still the status = 1 entry in the file_managed table).
Proposed resolution
Fix it so that a unique filename is given when the file object is first created.
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork flysystem_s3-3248466
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
leon kessler commentedUpdating issue summary with more precise details and replication steps.
We have been struggling with this bug for several months, and have been seeing countless files go missing (we enabled S3 versioning so that we had a way to retrieve theme).
Comment #4
leon kessler commentedHaving looked at this again, I think the original approach taken (of creating a file entity before the file has been uploaded to S3) is incorrect.
Drupal has an assumption that everything in its file_managed table, also exists as a file. If it did not have this assumption, then it would also check for file entities in
Drupal\Core\File\FileSystem::createFilename(). But as you can see only a check forfile_existsis performed.Looking at the s3fs_cors module, their implementation is to only create the file object after the upload to S3 was successful. I think this is the correct approach.
Will be submitting a new MR with this approach.
Comment #8
leon kessler commented