search

nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
26.45k stars 3.98k forks source link

[Bug]: Restoring files from trashbin looses path information #31200

Open despens opened 2 years ago

despens commented 2 years ago

⚠️ This issue respects the following points: ⚠️

Bug description

When restoring files from the trashbin, they might not go to the path they originated from, but to the user's home directory. This is not discernible before restoring a file and can lead to data loss.

Steps to reproduce

  1. Delete all files in a folder.
  2. Delete the empty folder.
  3. In the trashbin, restore a file deleted in Step 1.
  4. An unspecified error message will appear ("Error while restoring files from trash bin")
  5. The file will appear in the user's home directory.

Expected behavior

The file should appear again at the path it was deleted from. If that path doesn't exist anymore, the user should be given the choice to re-create that path upon restoration.

Path information for a file is data, especially within formalized directory structures. For instance, it is highly important where a README.md file is located not only its content and file name.

Installation method

Official Docker image

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.0

Web server

No response

Database engine version

PostgreSQL

Is this bug present after an update or on a fresh install?

Updated to a major version (ex. 22.2.3 to 23.0.1)

Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

What user-backends are you using?

Configuration report

No response

List of activated Apps

Enabled:
  - accessibility: 1.9.0
  - activity: 2.15.0
  - admin_audit: 1.13.0
  - apporder: 0.14.0
  - bruteforcesettings: 2.3.0
  - calendar: 3.0.5
  - checksum: 1.1.3
  - circles: 23.0.0
  - cloud_federation_api: 1.6.0
  - comments: 1.13.0
  - contacts: 4.0.7
  - contactsinteraction: 1.4.0
  - dav: 1.21.0
  - deck: 1.6.0
  - event_update_notification: 1.4.0
  - federatedfilesharing: 1.13.0
  - federation: 1.13.0
  - files: 1.18.0
  - files_accesscontrol: 1.13.0
  - files_automatedtagging: 1.13.0
  - files_downloadactivity: 1.12.0
  - files_external: 1.15.0
  - files_sharing: 1.15.0
  - files_trashbin: 1.13.0
  - files_versions: 1.16.0
  - firstrunwizard: 2.12.0
  - groupfolders: 11.1.2
  - integration_google: 1.0.6
  - logreader: 2.8.0
  - lookup_server_connector: 1.11.0
  - maps: 0.1.10
  - metadata: 0.15.0
  - nextcloud_announcements: 1.12.0
  - notes: 4.3.0
  - notifications: 2.11.1
  - notify_push: 0.3.0
  - oauth2: 1.11.0
  - onlyoffice: 7.3.0
  - photos: 1.5.0
  - previewgenerator: 3.4.1
  - privacy: 1.7.0
  - provisioning_api: 1.13.0
  - serverinfo: 1.13.0
  - settings: 1.5.0
  - sharebymail: 1.13.0
  - survey_client: 1.11.0
  - systemtags: 1.13.0
  - tasks: 0.14.2
  - text: 3.4.0
  - theming: 1.14.0
  - twofactor_backupcodes: 1.12.0
  - updatenotification: 1.13.0
  - user_status: 1.3.1
  - viewer: 1.7.0
  - weather_status: 1.3.0
  - workflowengine: 2.5.0
Disabled:
  - dashboard
  - encryption
  - files_pdfviewer
  - files_rightclick
  - files_videoplayer
  - password_policy
  - recommendations
  - support
  - user_ldap

Nextcloud Signing status

Technical information
=====================
The following list covers which files have failed the integrity check. Please read
the previous linked documentation to learn more about the errors and how to fix
them.

Results
=======
- core
    - INVALID_HASH
        - core/js/mimetypelist.js

Raw output
==========
Array
(
    [core] => Array
        (
            [INVALID_HASH] => Array
                (
                    [core/js/mimetypelist.js] => Array
                        (
                            [expected] => 94195a260a005dac543c3f6aa504f1b28e0078297fe94a4f52f012c16c109f0323eecc9f767d6949f860dfe454625fcaf1dc56f87bb8350975d8f006bbbdf14a
                            [current] => 1b07fb272efa65a10011ed52a6e51260343c5de2a256e1ae49f180173e2b6684ccf90d1af3c19fa97c31d42914866db46e3216883ec0d6a82cec0ad5529e78b1
                        )

                )

        )

)

Nextcloud Logs

No response

Additional info

No response

PVince81 commented 2 years ago

The problem

This is a known issue but only in specific conditions. Let's say we have a tree like this:

a
\a1
\a1\b1
\a2
\a2\b2

1) if you delete "a" directly, the whole tree is preserved in trashbin, so you only see "a" as a deleted entry. Restoring that one restores the tree

2) if you delete "b1", "a1", "b2", "a2" in that order, you'll see those entries also appearing in the list. if you restore them manually in the reverse order: "a2", "b2", "a1", "b1", the entries will be back at their original location, because the parent is always restored before

3) if at scenario 2) you restore in a different order, for example "b1", "b2", "a1", "b2", the code restoring "b1" will not find the former parent "a1" as it did not exist before, so it will put it into "a" (or in the root, I don't remember)

Scenario 3 can often happen if restoring in the web UI using "restore all" while sorted alphabetically and if it results in the restoration order described in scenario 3.

Solution

for the backend side, search for the string getTrashFiles() and make it so that the order is reversed where it makes sense

Workaround

When restoring in the web UI, don't use "restore all" but manually click the deletion time column to sort it by reversed deletion time, then manually select all the entries minus one to avoid triggering the "restore all" logic. This way the web UI will send the restore command individually for each selected entry, in the order they appear from top to bottom in the list instead of triggering the "bulk restore all" command.

PVince81 commented 2 years ago

I've tagged it as "good first issue" as it should be rather easy to tackle. I've added some pointers for searching the code in the task list: https://github.com/nextcloud/server/issues/31200#issuecomment-1130358549

raimund-schluessler commented 2 years ago

I don't know much about how restoring files works, honestly, but the proposed fix by sorting appropriately sounds a bit weird to me. Wouldn't it be possible to re-create the missing path only, instead of restoring deleted parent folders (including their content at the time of deletion)?

Given a file tree like

\a1
\a1\b1
\a1\b2

from which you delete b1 and a1 in that order and then restore b1. This would bring back the whole file tree including b2 with the proposed solution, no? Although one would probably expect this as a result

\a1
\a1\b1

(If I misunderstood something, just ignore my comment :slightly_smiling_face:)

PVince81 commented 2 years ago

@raimund-schluessler it depends on the use case I guess.

in some cases a user might not necessarily want to recreate the whole tree structure, especially if very deep, for just a single entry

in the use cases I heard of it was mostly about users where a folder tree got accidentally deleted by the desktop client and they wanted to restore the whole tree with its contents, but due to the fact that the desktop client sometimes deletes individual entries instead of the parent one (technical limitation due to change detection on client side), so the trashbin will contain a flat list

also to note: restoring would also keep the metadata and file ids (like tags, shares, etc), while recreating wouldn't as it would be a brand new folder entry

despens commented 2 years ago

Thanks for the workaounrd, @PVince81, and I'm glad to read the problem is already fully understood! However, when users will search for this problem and are made aware of the workaround, the damage will already have been done and they might have appear files in the wrong place, never being able to figure out again where it originally was supposed to be. That's data loss. Tagging an issue that can lead to data loss as "good first issue" does not seem appropriate. Restoring deleted files should not be something that users have to be careful about, it is for them to be forgiven their mistakes. Restoring deleted data needs to be the most boring, predictable process.

Regarding thinking about what "use case" users could have in mind when restoring accidentally deleted files, I would always err on restoring as much information as is available. This includes the whole path for files. Every file manager I know would restore the original path of deleted single files or folders, on Mac, Windows, and the few Linux desktops I used. If users are not interested in the path, they can move that single file in your example to a different place afterwards.

So given the structure

a/
a/b/
a/b/c.txt
a/b/d.txt

And first just a/b/d.txt being deleted and then the a/, restoring d.txt should restore a/b/d.txt. If a/ would be restored after, the missing c.txt should also appear again at a/b/c.txt.

PVince81 commented 2 years ago

when thinking about your proposed approach of restoring "a/" automatically, which would then keep the fileid and metadata, there's a catch: it is possible to have multiple folders called "a" in the trashbin if you deleted them, even from the same source.

Steps to reproduce:

  1. create a folder "x/a"
  2. delete "x/a"
  3. recreate "x/a"
  4. delete "x/a" again
  5. create "x/a/b"
  6. check the trashbin

you'll now have two entries "a" which both would be restored to "x/a" so restoring "b" would not know which one of the "a" entries to delete first. except maybe if it always picks the one most recently deleted :thinking:

I've tested something with Dolphin locally and it seems it behaves even more differently:

  1. create a folder "delete"
  2. create a subfolder "delete/this"
  3. create a file "delete/this/delete_this.txt"
  4. delete "delete_this.txt" alone
  5. delete "delete" folder
  6. go to trashbin
  7. try restoring "delete_this.txt"
  8. popup appears
image

I'm a bit surprised because this is even less usable than having the file restored to the root.

Now for the implementation: Solution 1) fixing the restoration order is the most low hanging fruit and is unlikely to have any consequences, and also doesn't have any behavior change Solution 2) change behavior and recreate the folder structure: this is more involved and is likely to have side effects in other code paths that rely on restoration

PVince81 commented 2 years ago

adding @jancborchardt @schiessle for design/behavior discussion

despens commented 2 years ago

Whoops, Dolphin really takes the trophy here 😅

I quickly recorded this "nested deleting and restoring in a different order" on a Windows 10 vm:

In the trash can the source path is always displayed and this the item is going to be restored to that path in any case.

https://user-images.githubusercontent.com/571494/169310573-aa3ff5b0-4d6a-4aaa-bba3-7697f89a64c4.mp4

Here is how a naming conflict is handled on restoring:

https://user-images.githubusercontent.com/571494/169315159-12ce5e13-95c5-49f5-b92c-44dad9e52d30.mp4

The whole process is designed very well, except comparing the conflicting files. The file in the trash can is listed as being zero size, which is not true and could lead to the user discarding the "empty" file. (My guess this is one of the many confusing side effects of OneDrive.)

despens commented 1 year ago

Related: #22474

szaimen commented 1 year ago

Hi, please update to 24.0.9 or better 25.0.3 and report back if it fixes the issue. Thank you!

My goal is to add a label like e.g. 25-feedback to this ticket of an up-to-date major Nextcloud version where the bug could be reproduced. However this is not going to work without your help. So thanks for all your effort!

If you don't manage to reproduce the issue in time and the issue gets closed but you can reproduce the issue afterwards, feel free to create a new bug report with up-to-date information by following this link: https://github.com/nextcloud/server/issues/new?assignees=&labels=bug%2C0.+Needs+triage&template=BUG_REPORT.yml&title=%5BBug%5D%3A+

despens commented 1 year ago

Hey @szaimen — I understand the desire for the initiative to make the bug reports more manageable, and appreciate the large amount of requests and issues the developers are managing. But I wonder if asking everyone to update their nextcloud deployment and try to reproduce the issue with the latest version is a good tactic overall. Especially when the issue was observed on a production instance, or at a place where system administrator capacity is limited, or test systems are not available.

This issue in particular, which can easily cause data loss, affects very basic functionality, restoration of files and folders from the trash bin. Unless a developer took on the issue and changed the logic of this functionality, it will not magically have healed by upgrading some library elsewhere. That means asking users to upgrade before triaging the issue is asking them to do a lot of work when there is zero chance that the bug will have disappeared.

I would really like this bug to be fixed because it continues to cause grief. But I won't be able to spend a full day updating and testing my nextcloud instances before end of February 2023.

norcon commented 1 year ago

Tested it today with Version 25.0.3. Same problem as reported above. Restored file and folders out of recycle bin. They were not restored to original location. Restored faulty under / Root Path. Please fix it. I have 400 GB Files in Recycle bin to recover. But without correct restore location this will be a mess! (Deleted my archive folder in / root accidently. So no timely problem. But in future it would be great to get all the files and folders back to its original place!)

sorbaugh commented 1 year ago

Hello! I would like to contribute and saw that this issue is still open. I set up the repository on my machine and while familiarizing myself with the codebase, I noticed that the "restore all" proposed solution by Vincent (sorting by most recently deleted first) has already been implemented, both in apps\files_trashbin\src\filelist.js

grafik

as well as for the command apps\files_trashbin\lib\Command\RestoreAllFiles.php

grafik

Furthermore, I can't seem to reproduce the Error Message "Error while restoring files from trash bin".

To avoid a duplication of efforts, is this issue still relevant? Maybe I'm missing something. Thanks in advance!

norcon commented 1 year ago

For me, this problem is still existing. Tested in 25.0.3 and the restore of my trash bin creates every file and folder flat in the / root path.

davidjanke commented 1 year ago

Hey, The error message does not appear using the web UI with Nextcloud Hub 4 (27.0.0dev). However if you try to restore a file without restoring the original directory, that still gets restored into the root (All Files). I also checked google drive, which has the same behaviour as NextCloud. I would understand if this the feature parity wants to be more like Google Cloud than a file manager.

Maybe this is not a bug? The system asking if the missing path needs to be restored would be nice though.

norcon commented 1 year ago

Tried with 25.0.5 today again. No luck. If i restore a file of the trashbin, where the original Path does not exist anymore, the file will be saved to / Root Path. It would be greate to have an Option to set, that the original path/folders should be created, if it does not exist anymore.

norcon commented 1 year ago

Any news? I am still sitting on many hundreds GB Data in trash bin waiting to get restored in the right folder structure.

awesomekosm commented 1 year ago

The whole trash/restore functionality seems broken. Deleted 6gb file then tried to permanently delete it, entire nextcloud instance froze (100% cpu, 1gb full overflowing to swap 512mb). It's a small instance but usually works just fine. Tried restoring if after rebooting the server, it froze again. Basically stuck in a limbo and it shows that the space is still being used. No good error either.

norcon commented 1 year ago

Any news? Is there a chance to get this problem fixed?

norcon commented 10 months ago

Any news? Still not getting my data back out of the trashbin 🙁

Jaxom99 commented 10 months ago

From what I tested, you can restore files in the right place with the occ command : https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/occ_command.html#trashbin-label Worth a try ? It's not a satisfying solution however, so this issue should remain open...

norcon commented 9 months ago

occ command is not an option. Same problematic here! Still waiting for a fix. Still hundreds of gigabyte waiting to be moved to the original location.... or does anybody have a script to move the files to the original location?

norcon commented 9 months ago

I need my data back, so I decided to create an simple and awful script to download my files and safe it in the original structure an my HDD: https://github.com/norcon/nextcloud-trash-restore-original-location/

For future versions it would be nice, if this bug will be fixed. Nextcloud is not reliable with such a big bug wich will not be fixed in a reasonable period of time, for me.

But I still thank you for the work on nextcloud!