Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

moving a file and providing the fileID of the destination destroys data #6739

Closed
Tracked by #8440
individual-it opened this issue Jul 7, 2023 · 17 comments · Fixed by #8396
Closed
Tracked by #8440

moving a file and providing the fileID of the destination destroys data #6739

individual-it opened this issue Jul 7, 2023 · 17 comments · Fixed by #8396
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug
Milestone

Comments

@individual-it
Copy link
Member

Describe the bug

When making a WebDAV MOVE request and giving only the fileID of the destination, all the data of the destination folder or space will be destroyed.

Steps to reproduce

Steps to reproduce the behavior:

folder

  1. upload a file called file-to-be-moved.txt to the personal space
  2. in the personal space create a folder called dest
  3. make a PROPFIND to the personal space to get the full path of file-to-be-moved.txt and the fileId of dest
  4. make a MOVE request to the file-to-be-moved.txt with Destination header pointing to the fileId of dest (make sure to add remote.php to the destination URL, that seems to trigger it)
    curl -uadmin:admin -k -XMOVE https://localhost:9200/remote.php/dav/spaces/136ffc9b-d317-4219-a0c3-084d5e070c60/file-to-be-moved.txt -H'Destination: https://localhost:9200/remote.php/dav/spaces/33c9baa8-450f-4873-a049-7beb8227face$136ffc9b-d317-4219-a0c3-084d5e070c60!9c495ede-bd11-439f-8496-85c649ef6121' -v

Expected behavior

the folder dest should be overwritten with the content of the file file-to-be-moved.txt as it happens when using the path of the dest folder
OR
return Error 400, same as it happens when not using remote.php in the Destionation URL

Actual behavior

  • Error 500 returned
  • path of the ocis data storage on the server exposed
    <?xml version="1.0" encoding="UTF-8"?>
    <d:error xmlns:d="DAV" xmlns:s="http://sabredav.org/ns"><s:exception></s:exception><s:message>move:Decomposedfs: could not move child: rename /home/artur/.ocis/storage/users/spaces/13/6ffc9b-d317-4219-a0c3-084d5e070c60/nodes/13/6f/fc/9b/-d317-4219-a0c3-084d5e070c60/file-to-be-moved.txt /home/artur/.ocis/storage/users/spaces/13/6ffc9b-d317-4219-a0c3-084d5e070c60/nodes: file exists</s:message></d:error>
    
  • destand all its content are gone
  • no log output

space

  1. upload a file called file-to-be-moved.txt to the personal space
  2. create an other space
  3. query /graph/v1.0/me/drives to get the id of the new space
  4. make a MOVE request to the file-to-be-moved.txt with Destination header pointing to: <ocis-url>/remote.php/dav/spaces/<spaceId> using remote.php seems to trigger it

Expected behavior

  • this should not be possible
  • Error 400 returned, same as it happens when not using remote.php in the Destionation URL

Actual behavior

  • Error 500 returned
  • logs {"level":"error","service":"ocdav","name":"com.owncloud.web.ocdav","traceid":"00000000000000000000000000000000","request-id":"deepthought3/0nFVzCuvCk-000764","spaceid":"136ffc9b-d317-4219-a0c3-084d5e070c60","path":"/file-to-be-moved.txt","status":{"code":15,"message":"delete:remove /home/artur/.ocis/storage/users/spaces/9f/429b6f-50a8-431d-9e72-ed59b0c52efc/nodes/Neuer Space: no such file or directory","trace":"00000000000000000000000000000000"},"code":500,"time":"2023-07-07T12:52:20.718783329+05:45","message":"Internal Server Error"}
  • space is destroyed

Setup

Please describe how you started the server and provide a list of relevant environment variables or configuration files.

PROXY_ENABLE_BASIC_AUTH=true IDM_ADMIN_PASSWORD=admin bin/ocis server

Additional context

Add any other context about the problem here.

@kobergj
Copy link
Collaborator

kobergj commented Jul 7, 2023

I tried to reproduce. There are slight differences but the issue is the same.

When moving to a folder

  • The folder dest will be deleted just as you said but I can still find it in my trashbin and can restore it from there.
  • using remote.php or not has no impact on the behaviour.

When moving to a space

  • The space and all its contents cannot be accessed by the webUI any more
  • The data of the space is still there. Only the drive listing doesn't work any more
  • using remote.php or not has no impact on the behaviour

This ticket will probably need a raise in priority as it can lead to data loss. (not really - but it will look like this) @micbar

@micbar micbar added the Priority:p2-high Escalation, on top of current planning, release blocker label Jul 7, 2023
@2403905 2403905 self-assigned this Jul 7, 2023
@2403905
Copy link
Contributor

2403905 commented Jul 10, 2023

@individual-it
It happens in the case of a folder because the destination in the request is the destination parent folder. And the overwrite is true by default.
If we add the file name as a destination it works properly.
The same for the spaces.

curl -uadmin:admin -k -XMOVE https://localhost:9200/remote.php/dav/spaces/136ffc9b-d317-4219-a0c3-084d5e070c60/file-to-be-moved.txt -H'Destination: https://localhost:9200/remote.php/dav/spaces/33c9baa8-450f-4873-a049-7beb8227face$136ffc9b-d317-4219-a0c3-084d5e070c60!9c495ede-bd11-439f-8496-85c649ef6121/file-to-be-moved.txt' -v

Definitely, it is a defect and we have to prevent data loss.

@micbar @kobergj FYI

@amrita-shrestha
Copy link
Contributor

amrita-shrestha commented Aug 3, 2023

COPYmethod faces a similar case

curl -uadmin:admin -k -XCOPY 'https://localhost:9200/remote.php/dav/spaces/b11e098c-f29b-4fb2-816d-bc07bdb7fb18$a86cb5dd-9a94-41d2-87be-58953658f8f0!1da3b1a7-430b-4ee4-981a-26d078650cd6' \
-H'Destination:: https://localhost:9200/remote.php/dav/spaces/b11e098c-f29b-4fb2-816d-bc07bdb7fb18$a86cb5dd-9a94-41d2-87be-58953658f8f0!480d53df-3927-4ad1-9b34-3d3308b3ff18'

Response:
HTTP status code 500 and dest moved to trashbin

@saw-jan
Copy link
Member

saw-jan commented Sep 7, 2023

Tested with Infinite Scale 4.0.0+82428024f8 Community

Personal - Personal Space (Destination uses ID)

Command:

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/f1291c6b-0076-4ad2-8a6c-5da9640d50ca\$099e189e-fe3f-439d-a20a-f71192946b2f/afile.txt" \              
-H"Destination: https://localhost:9200/remote.php/dav/spaces/f1291c6b-0076-4ad2-8a6c-5da9640d50ca\$099e189e-fe3f-439d-a20a-f71192946b2f\!1c127019-f497-43bc-93e9-4caa30beccba" \
-uadmin:admin -vk

Response: ❗ issue persists

< HTTP/1.1 500 Internal Server Error
...
<?xml version="1.0" encoding="UTF-8"?>
<d:error
	xmlns:d="DAV"
	xmlns:s="http://sabredav.org/ns">
	<s:exception></s:exception>
	<s:message>move:Decomposedfs: could not move child: rename /home/sawjan/.ocis/storage/users/spaces/09/9e189e-fe3f-439d-a20a-f71192946b2f/nodes/09/9e/18/9e/-fe3f-439d-a20a-f71192946b2f/afile.txt /home/sawjan/.ocis/storage/users/spaces/09/9e189e-fe3f-439d-a20a-f71192946b2f/nodes: file exists</s:message>
</d:error>

Personal - Project Space (Destination uses ID)

Command:

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/f1291c6b-0076-4ad2-8a6c-5da9640d50ca\$099e189e-fe3f-439d-a20a-f71192946b2f/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/f1291c6b-0076-4ad2-8a6c-5da9640d50ca\$a3d1da3e-281c-4b13-a06d-748f1d255e7a" \
-uadmin:admin -vk

Response: ❗ issue persists

< HTTP/1.1 500 Internal Server Error

Space gets deleted

NOTE: fix PR was https://github.com/cs3org/reva/pull/4056/files

@2403905
Copy link
Contributor

2403905 commented Oct 4, 2023

@saw-jan Merget cs3org/reva#4229 to the reva that disallows using the Personal and Project spaces root as a source/destination while move/copy.
Will be better to consider other cases apart.

@saw-jan
Copy link
Member

saw-jan commented Oct 10, 2023

We fixed a bug that caused destroying the Personal and Project spaces data when providing as a destination while move/copy file.
Disallow use the Personal and Project spaces root as a source while move/copy file.

@2403905 I see that the PR changelog describes above but I think the issue here is not related to space root but it is about using folder-id as the destination header.

@saw-jan
Copy link
Member

saw-jan commented Oct 10, 2023

Steps to reproduce:

  1. create a file afile.txt
  2. create a folder dest
  3. Find:
    • personal-space-id: fee02af4-bdc5-4ec6-863d-465f5eb4257d$0126f1a4-01aa-441b-9d9d-e0e808058ff1
    • dest folder-id: fee02af4-bdc5-4ec6-863d-465f5eb4257d$0126f1a4-01aa-441b-9d9d-e0e808058ff1!df03d210-6381-4ce2-89a9-5d519ff7c971
  4. try to MOVE afile.txt using id of folder dest as destination

Command:

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/<personal-space-id>/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/<dest-folder-id>" \
-uadmin:admin -vk

Example:

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/fee02af4-bdc5-4ec6-863d-465f5eb4257d\$0126f1a4-01aa-441b-9d9d-e0e808058ff1/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/fee02af4-bdc5-4ec6-863d-465f5eb4257d\$0126f1a4-01aa-441b-9d9d-e0e808058ff1\!df03d210-6381-4ce2-89a9-5d519ff7c971" \
-uadmin:admin -vk

@2403905
Copy link
Contributor

2403905 commented Oct 17, 2023

We fixed a bug that caused destroying the Personal and Project spaces data when providing as a destination while move/copy file.
Disallow use the Personal and Project spaces root as a source while move/copy file.

@2403905 I see that the PR changelog describes above but I think the issue here is not related to space root but it is about using folder-id as the destination header.

@saw-jan Why the issue is not related to space root?
The bug description consists of two cases folder and spase. And the space part's actual behavior said space is destroyed
The fix covers only the case when the Personal or Project spaces root is a destination while move/copy a file.

The folder case that about using folder-id as the destination header is still opened. We need to fix the decomposefs.

@saw-jan
Copy link
Member

saw-jan commented Oct 17, 2023

The bug description consists of two cases folder and spase. And the space part's actual behavior said space is destroyed

Yeah, you are right. Sorry for the confusion. I thought the fix was for folder-id realted.

Trying to MOVE to space-id as destination is confirmed to be fixed 👍

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/storage-users-1%24some-admin-user-id-0000-000000000000/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/storage-users-1%24a73a43f5-260a-4a1d-92ac-42a98248c3b8" \
-uadmin:admin -vk
< HTTP/1.1 400 Bad Request

@saw-jan
Copy link
Member

saw-jan commented Oct 17, 2023

The folder case that about using folder-id as the destination header is still opened. We need to fix the decomposefs.

so now we are waiting for this fix

@ScharfViktor
Copy link
Contributor

can we close it?

@saw-jan
Copy link
Member

saw-jan commented Dec 15, 2023

can we close it?

Was there any fix for?

The folder case that about using folder-id as the destination header is still opened. We need to fix the decomposefs.

@micbar micbar closed this as completed Dec 27, 2023
@saw-jan
Copy link
Member

saw-jan commented Dec 28, 2023

Steps to reproduce:

  1. create a file afile.txt

  2. create a folder dest

  3. Find:

    • personal-space-id: fee02af4-bdc5-4ec6-863d-465f5eb4257d$0126f1a4-01aa-441b-9d9d-e0e808058ff1
    • dest folder-id: fee02af4-bdc5-4ec6-863d-465f5eb4257d$0126f1a4-01aa-441b-9d9d-e0e808058ff1!df03d210-6381-4ce2-89a9-5d519ff7c971
  4. try to MOVE afile.txt using id of folder dest as destination

Command:

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/<personal-space-id>/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/<dest-folder-id>" \
-uadmin:admin -vk

Example:

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/fee02af4-bdc5-4ec6-863d-465f5eb4257d\$0126f1a4-01aa-441b-9d9d-e0e808058ff1/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/fee02af4-bdc5-4ec6-863d-465f5eb4257d\$0126f1a4-01aa-441b-9d9d-e0e808058ff1\!df03d210-6381-4ce2-89a9-5d519ff7c971" \
-uadmin:admin -vk

There's still an issue with trying to move file to a folder-id as destination

HTTP/1.1 500 Internal Server Error

<?xml version="1.0" encoding="UTF-8"?>
<d:error
	xmlns:d="DAV"
	xmlns:s="http://sabredav.org/ns">
	<s:exception></s:exception>
	<s:message>move:Decomposedfs: could not move child: rename /home/sawjan/.ocis/storage/users/spaces/df/92eaf2-a807-42cf-a8d4-cb9219b8dc4c/nodes/df/92/ea/f2/-a807-42cf-a8d4-cb9219b8dc4c/move-me.txt /home/sawjan/.ocis/storage/users/spaces/df/92eaf2-a807-42cf-a8d4-cb9219b8dc4c/nodes: file exists</s:message>
</d:error>%

CC @micbar

@2403905
Copy link
Contributor

2403905 commented Jan 23, 2024

@micbar The issue is partially solved. We disallowed to use the Personal and Project spaces root as a source. It prevents the data loss.
We could also disallow overwrite folder by file or file by folder and sounds to me like this doesn't contradict webDAV spec. I tried but such changes affected a lot of tests.
We need to discuss what we want to achieve.

@2403905
Copy link
Contributor

2403905 commented Jan 26, 2024

@dragotin Could we discuss what we can do with the overwriting folder by file or file by folder? It could affect the clients.

@butonic
Copy link
Member

butonic commented Feb 5, 2024

What is left unclear when reading the rfc? At leas whon tdhe overwrite header is set, the outcome should be clear:

9.9.3. MOVE and the Overwrite Header

If a resource exists at the destination and the Overwrite header is
"T", then prior to performing the move, the server MUST perform a
DELETE with "Depth: infinity" on the destination resource. If the
Overwrite header is set to "F", then the operation will fail.

Edit: ah, if the destination is just a node it would be deleted. The old name should be reused, and the fileid of the destination should be changed to the one of the source.

@2403905 2403905 self-assigned this Feb 6, 2024
@saw-jan
Copy link
Member

saw-jan commented Feb 9, 2024

Tested and confirmed the behavior.

curl -XMOVE "https://localhost:9200/remote.php/dav/spaces/<space-id>/afile.txt" \
-H"Destination: https://localhost:9200/remote.php/dav/spaces/<folder-id>" \
-uadmin:admin -vk
  • 204 ✔️
    < HTTP/1.1 204 No Content
    
  • dest folder is deleted and moved to trashbin ✔️
  • afile.txt is renamed to dest file ✔️
  • dest file has the file-id of previous filename afile.txt ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants