Downloading a file after restoring the version fails with `500`

[SwikritiT]

Steps to reproduce

1. As a user upload a file with some data

curl -v -u admin:admin -XPUT http://localhost/cliCore/remote.php/dav/files/admin/test.txt -d "test file"

2. overwrite the previously created file (create a version)

curl -v -u admin:admin -XPUT http://localhost/cliCore/remote.php/dav/files/admin/test.txt -d "test file updated"

3. Download the file, it works

curl -v -u admin:admin http://localhost/cliCore/remote.php/dav/files/admin/test.txt

4. Now to restore the version, get file id

curl -v -u admin:admin http://localhost/cliCore/remote.php/dav/files/admin/test.txt -X PROPFIND -d'<?xml version="1.0"?><d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns"><d:prop><oc:fileid /></d:prop></d:propfind>'

this will give a response that include file id

<d:prop><oc:fileid>2147484342</oc:filed></d:prop>

5. Now PROPFIND to get the location of the previous version with the file id from previous step

curl -v -u admin:admin http://localhost/cliCore/remote.php/dav/meta/2147484342/v -X PROPFIND

Response

<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"><d:response><d:href>/cliCore/remote.php/dav/meta/2147484342/v/</d:href><d:propstat><d:prop><d:resourcetype><d:collection/></d:resourcetype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat></d:response><d:response><d:href>/cliCore/remote.php/dav/meta/2147484342/v/1676357084</d:href><d:propstat><d:prop><d:getlastmodified>Tue, 14 Feb 2023 06:44:44 GMT</d:getlastmodified><d:getcontentlength>9</d:getcontentlength><d:resourcetype/><d:getetag>&quot;62dd2ad72593875505630b4390d7633a&quot;</d:getetag><d:getcontenttype>text/plain</d:getcontenttype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat></d:response></d:multistatus>

You can find the location in href
6. Now restore the previous version

curl -u admin:admin -X COPY "http://localhost/cliCore/remote.php/dav/meta/2147484342/v/1676357084" -H 'Destination:http://localhost/cliCore/remote.php/dav/files/admin/test.txt' -v

This request is successful with 204
7. Now get the file

curl -u admin:admin "http://localhost/cliCore/remote.php/dav/files/admin/test.txt" -v

Expected behaviour

The content of the previous version i.e test file should be downloaded

Actual behaviour

The request gives 500 and the content is <?xml ver%

*   Trying 127.0.0.1:80...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 80 (#0)
* Server auth using Basic with user 'admin'
> GET /cliCore/remote.php/dav/files/admin/test.txt HTTP/1.1
> Host: localhost
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Date: Tue, 14 Feb 2023 07:06:43 GMT
< Server: Apache/2.4.41 (Ubuntu)
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 0
< X-Robots-Tag: none
< X-Frame-Options: SAMEORIGIN
< X-Download-Options: noopen
< X-Permitted-Cross-Domain-Policies: none
< Set-Cookie: ocxzl8xu0hvp=4cr790u214quovs59ndp0k4mbi; path=/cliCore; HttpOnly; SameSite=Strict
< Expires: Thu, 19 Nov 1981 08:52:00 GMT
< Cache-Control: no-store, no-cache, must-revalidate
< Pragma: no-cache
< Set-Cookie: oc_sessionPassphrase=c6FHVVaj0Nhp3NPyuPM95ljGVfJJo9aRRQil8MJL033GGPXXzrisdu2M0jo08aNWLHCindy%2FpB%2Fw1qk7UXIcZ6rNulTvy6uTqL51PoH8wmcS93mHg%2BHnDwnNdDEBCas0; expires=Tue, 14-Feb-2023 07:26:43 GMT; Max-Age=1200; path=/cliCore; HttpOnly; SameSite=Strict
< Content-Security-Policy: default-src 'none';
< Set-Cookie: ocxzl8xu0hvp=stgejcug3kuekvftju9m45h47i; path=/cliCore; HttpOnly; SameSite=Strict
< Set-Cookie: cookie_test=test; expires=Tue, 14-Feb-2023 08:06:43 GMT; Max-Age=3600
< Last-Modified: Tue, 14 Feb 2023 07:04:09 GMT
< ETag: "baadce7fd70f5d55962bb67728e55431"
< Content-Length: 9
< OC-ETag: "baadce7fd70f5d55962bb67728e55431"
< Content-Disposition: attachment; filename*=UTF-8''test.txt; filename="test.txt"
< X-Accel-Buffering: no
< Connection: close
< Content-Type: application/xml; charset=utf-8
< 
* Closing connection 0
<?xml ver%                                            

Screenshots of the UI

After creating the version

While restoring

Owncloud logs

{
  "reqId": "Y@sycnXc-XutX6P-UzTHpAAAAAk",
  "level": 3,
  "time": "2023-02-14T07:04:18+00:00",
  "remoteAddr": "::1",
  "user": "admin",
  "app": "no app in context",
  "method": "GET",
  "url": "/cliCore/remote.php/dav/meta/2147484342/v/1676358223?preview",
  "message": "Can not decrypt this file, probably this is a shared file. Please ask the file owner to reshare the file with you."
}
{
  "reqId": "Y@szA7t2IFH4hn@1Qw@E5AAAAAI",
  "level": 3,
  "time": "2023-02-14T07:06:43+00:00",
  "remoteAddr": "127.0.0.1",
  "user": "admin",
  "app": "no app in context",
  "method": "GET",
  "url": "/cliCore/remote.php/dav/files/admin/test.txt",
  "message": "Can not decrypt this file, probably this is a shared file. Please ask the file owner to reshare the file with you."
}
{
  "reqId": "Y@szA7t2IFH4hn@1Qw@E5AAAAAI",
  "level": 4,
  "time": "2023-02-14T07:06:43+00:00",
  "remoteAddr": "127.0.0.1",
  "user": "admin",
  "app": "webdav",
  "method": "GET",
  "url": "/cliCore/remote.php/dav/files/admin/test.txt",
  "message": "Exception: Can not decrypt this file, probably this is a shared file. Please ask the file owner to reshare the file with you.: {\"Exception\":\"OC\\\\Encryption\\\\Exceptions\\\\DecryptionFailedException\",\"Message\":\"Can not decrypt this file, probably this is a shared file. Please ask the file owner to reshare the file with you.\",\"Code\":0,\"Trace\":\"#0 \\/home\\/anu\\/www\\/cliCore\\/lib\\/private\\/Files\\/Stream\\/Encryption.php(471): OCA\\\\Encryption\\\\Crypto\\\\Encryption->decrypt(*** sensitive parameters replaced ***)\\n#1 \\/home\\/anu\\/www\\/cliCore\\/lib\\/private\\/Files\\/Stream\\/Encryption.php(293): OC\\\\Files\\\\Stream\\\\Encryption->readCache()\\n#2 [internal function]: OC\\\\Files\\\\Stream\\\\Encryption->stream_read()\\n#3 \\/home\\/anu\\/www\\/cliCore\\/apps\\/files_external\\/3rdparty\\/icewind\\/streams\\/src\\/Wrapper.php(55): fread()\\n#4 \\/home\\/anu\\/www\\/cliCore\\/lib\\/private\\/Files\\/Stream\\/Checksum.php(163): Icewind\\\\Streams\\\\Wrapper->stream_read()\\n#5 [internal function]: OC\\\\Files\\\\Stream\\\\Checksum->stream_read()\\n#6 \\/home\\/anu\\/www\\/cliCore\\/apps\\/files_external\\/3rdparty\\/icewind\\/streams\\/src\\/Wrapper.php(55): fread()\\n#7 \\/home\\/anu\\/www\\/cliCore\\/apps\\/files_external\\/3rdparty\\/icewind\\/streams\\/src\\/CallbackWrapper.php(96): Icewind\\\\Streams\\\\Wrapper->stream_read()\\n#8 [internal function]: Icewind\\\\Streams\\\\CallbackWrapper->stream_read()\\n#9 \\/home\\/anu\\/www\\/cliCore\\/apps\\/files_external\\/3rdparty\\/icewind\\/streams\\/src\\/Wrapper.php(55): fread()\\n#10 \\/home\\/anu\\/www\\/cliCore\\/apps\\/files_external\\/3rdparty\\/icewind\\/streams\\/src\\/CallbackWrapper.php(96): Icewind\\\\Streams\\\\Wrapper->stream_read()\\n#11 [internal function]: Icewind\\\\Streams\\\\CallbackWrapper->stream_read()\\n#12 \\/home\\/anu\\/www\\/cliCore\\/lib\\/composer\\/sabre\\/http\\/lib\\/Sapi.php(110): stream_copy_to_stream()\\n#13 \\/home\\/anu\\/www\\/cliCore\\/lib\\/composer\\/sabre\\/dav\\/lib\\/DAV\\/Server.php(490): Sabre\\\\HTTP\\\\Sapi::sendResponse()\\n#14 \\/home\\/anu\\/www\\/cliCore\\/lib\\/composer\\/sabre\\/dav\\/lib\\/DAV\\/Server.php(253): Sabre\\\\DAV\\\\Server->invokeMethod()\\n#15 \\/home\\/anu\\/www\\/cliCore\\/apps\\/dav\\/lib\\/Server.php(348): Sabre\\\\DAV\\\\Server->start()\\n#16 \\/home\\/anu\\/www\\/cliCore\\/apps\\/dav\\/appinfo\\/v2\\/remote.php(31): OCA\\\\DAV\\\\Server->exec()\\n#17 \\/home\\/anu\\/www\\/cliCore\\/remote.php(165): require_once('\\/home\\/anu\\/www\\/c...')\\n#18 {main}\",\"File\":\"\\/home\\/anu\\/www\\/cliCore\\/apps\\/encryption\\/lib\\/Crypto\\/Encryption.php\",\"Line\":374}"
}

Setup

Encryption enabled with master key

 sudo -u www-data ./occ encryption:enabled
sudo -u www-data ./occ encryption:select-encryption-type masterkey --yes

Owncloud 10 - master
commit id 5a381c888ed6db83b0991b0d6509a1aa1197eca9

[SwikritiT]

The recent merge of PR owncloud/core#40531 might be the reason behind this regression

[phil-davis]

@jvillafanez @mrow4a can you have a look please.
Maybe something in the core files_versions changes does not work with encryption?

[mrow4a]

hmm weird, I will check.

[jvillafanez]

Clues so far:

• It seems to be caused by the change in https://github.com/owncloud/core/pull/40531/files#diff-15bc6a3d0922d2990f622f71c6f18a14ee37052dcf2ca8e65ed03cb128c01920R513 . Moving instead of copying seems to fix the issue (not what we want though). The rest of the changes in the PR should be unrelated since they deal with the metadata.
• When the version is restored, the keys disappear. This seems to be causing the errors because it's impossible to decrypt the files

Before restoring

root@c1141f51b72d: /var/www/owncloud # find /mnt/data/files/admin/files_encryption/
/mnt/data/files/admin/files_encryption/
/mnt/data/files/admin/files_encryption/keys
/mnt/data/files/admin/files_encryption/keys/files
/mnt/data/files/admin/files_encryption/keys/files/test1.txt
/mnt/data/files/admin/files_encryption/keys/files/test1.txt/OC_DEFAULT_MODULE
/mnt/data/files/admin/files_encryption/keys/files/test1.txt/OC_DEFAULT_MODULE/master_b2c5f744.shareKey
/mnt/data/files/admin/files_encryption/keys/files/test1.txt/OC_DEFAULT_MODULE/fileKey

After restoring

root@c1141f51b72d: /var/www/owncloud # find /mnt/data/files/admin/files_encryption/
/mnt/data/files/admin/files_encryption/
/mnt/data/files/admin/files_encryption/keys
/mnt/data/files/admin/files_encryption/keys/files

[jvillafanez]

Something goes wrong with the wrappers:

[0] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Encryption.php\n            [line] => 257\n            [function] => deleteAllFileKeys\n            [class] => OC\\Encryption\\Keys\\Storage\n            [type] => ->\n        )\n\n    
[1] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Encryption.php\n            [line] => 387\n            [function] => unlink\n            [class] => OC\\Files\\Storage\\Wrapper\\Encryption\n            [type] => ->\n        )\n\n    
[2] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Wrapper.php\n            [line] => 291\n            [function] => copy\n            [class] => OC\\Files\\Storage\\Wrapper\\Encryption\n            [type] => ->\n        )\n\n    
[3] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Wrapper.php\n            [line] => 291\n            [function] => copy\n            [class] => OC\\Files\\Storage\\Wrapper\\Wrapper\n            [type] => ->\n        )\n\n    
[4] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Wrapper.php\n            [line] => 561\n            [function] => copy\n            [class] => OC\\Files\\Storage\\Wrapper\\Wrapper\n            [type] => ->\n        )\n\n    
[5] => Array\n        (\n            [file] => \/var\/www\/owncloud\/apps\/files_versions\/lib\/Storage.php\n            [line] => 584\n            [function] => copyFromStorage\n            [class] => OC\\Files\\Storage\\Wrapper\\Wrapper\n            [type] => ->\n        )\n\n    
[6] => Array\n        (\n            [file] => \/var\/www\/owncloud\/apps\/files_versions\/lib\/Storage.php\n            [line] => 535\n            [function] => copyFileContents\n            [class] => OCA\\Files_Versions\\Storage\n            [type] => ::\n        )\n\n    
[7] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Common.php\n            [line] => 773\n            [function] => restoreVersion\n            [class] => OCA\\Files_Versions\\Storage\n            [type] => ::\n        )\n\n    
[8] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Wrapper.php\n            [line] => 510\n            [function] => restoreVersion\n            [class] => OC\\Files\\Storage\\Common\n            [type] => ->\n        )\n\n    
[9] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Wrapper.php\n            [line] => 510\n            [function] => __call\n            [class] => OC\\Files\\Storage\\Wrapper\\Wrapper\n            [type] => ->\n        )\n\n    
[10] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Storage\/Wrapper\/Wrapper.php\n            [line] => 510\n            [function] => __call\n            [class] => OC\\Files\\Storage\\Wrapper\\Wrapper\n            [type] => ->\n        )\n\n    
[11] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/private\/Files\/Meta\/MetaFileVersionNode.php\n            [line] => 134\n            [function] => __call\n            [class] => OC\\Files\\Storage\\Wrapper\\Wrapper\n            [type] => ->\n        )\n\n    
[12] => Array\n        (\n            [file] => \/var\/www\/owncloud\/apps\/dav\/lib\/Meta\/MetaFile.php\n            [line] => 99\n            [function] => copy\n            [class] => OC\\Files\\Meta\\MetaFileVersionNode\n            [type] => ->\n        )\n\n    
[13] => Array\n        (\n            [file] => \/var\/www\/owncloud\/apps\/dav\/lib\/DAV\/CopyPlugin.php\n            [line] => 86\n            [function] => copy\n            [class] => OCA\\DAV\\Meta\\MetaFile\n            [type] => ->\n        )\n\n    
[14] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/composer\/sabre\/event\/lib\/WildcardEmitterTrait.php\n            [line] => 89\n            [function] => httpCopy\n            [class] => OCA\\DAV\\DAV\\CopyPlugin\n            [type] => ->\n        )\n\n    
[15] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php\n            [line] => 472\n            [function] => emit\n            [class] => Sabre\\DAV\\Server\n            [type] => ->\n        )\n\n    
[16] => Array\n        (\n            [file] => \/var\/www\/owncloud\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php\n            [line] => 253\n            [function] => invokeMethod\n            [class] => Sabre\\DAV\\Server\n            [type] => ->\n        )\n\n    
[17] => Array\n        (\n            [file] => \/var\/www\/owncloud\/apps\/dav\/lib\/Server.php\n            [line] => 348\n            [function] => start\n            [class] => Sabre\\DAV\\Server\n            [type] => ->\n        )\n\n    
[18] => Array\n        (\n            [file] => \/var\/www\/owncloud\/apps\/dav\/appinfo\/v2\/remote.php\n            [line] => 31\n            [function] => exec\n            [class] => OCA\\DAV\\Server\n            [type] => ->\n        )\n\n    

There is a copyFromStorage at 5, but then it's converted to a copy call. The copy is what reaches the encryption wrapper, what causes the keys to be deleted due to the unlink call it has in it.

[jvillafanez]

@mrow4a could you check for any side effects on removing the unlink in https://github.com/owncloud/core/blob/5a381c888ed6db83b0991b0d6509a1aa1197eca9/lib/private/Files/Storage/Wrapper/Encryption.php#L387 ?
It seems to fix the issue, but I'm not sure about side effects.

I think that the unlink there doesn't make any sense. If the storage must copy to an empty location (can't overwrite a file) it should be the storage's responsibility to ensure so, but the wrapper shouldn't need to perform an unlink action on the storage when it should copy.

[mrow4a]

@jvillafanez I think there is also other way.. more cumbersome even.. I am looking at it right now.

[mrow4a]

@jvillafanez fix is in owncloud/core#40641 . But to be frank with you.. I have no idea why we need to unlink there. For version it is plainly wrong, but for normal files maybe there is some corner case.

[jvillafanez]

I assume it's because there could be storages that don't allow overwriting files, so you need to delete the target file first. However, as said, this should be responsibility of the target storage, not any wrapper.

I think we'll need to go with that PR for now, but the whole storage system needs a big overhaul. I'm pretty sure most of the problems come from the key storage using the view, and as such, using all the wrappers again. There is specific code in place to deal with this, which is simply bad design.
The additional problem is that wrappers should forward the operation before / after doing whatever they need to do. They mustn't change the operation as they are doing right now. Again it's the storage responsibility to change the operation in order to optimize it.

[mrow4a]

@jvillafanez If this would be true, it is crazy. Because it means optimize for 1%, penalizing 99% of storage cases?

ok, so you would actually move this check into https://github.com/owncloud/core/blob/f197b69716737e20a5de3e591058643cc6a7e984/lib/private/Files/Storage/Wrapper/Encryption.php#L255-L258 ?

[mrow4a]

@jvillafanez ok, I investigated. I think the reason is different. I think that unlink in encryption is to make sure "old keys" are deleted. And then you copy a file together with new keys. So actually owncloud/core#40641 is correct, as for version this does not hold.

BTW, even if this was also to cover the case with overwriting files, most likely these storages are ok to not work with restore of versions.. as you cannot overwrite 🤡