[Bug]: Files/Cache/Cache.php->get() returns partial array, leads to errors

[mejo-]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• I agree to follow Nextcloud's Code of Conduct.

Bug description

Files/Cache/Cache.php->get() returns an array (from $partial) when it doesn't find the file in filecache.

This leads to errors as other functions expect get() to return either ICacheEntry or false. E.g. remove() expects to get an ICacheEntry and calls getId() on the result.

The bug seems to happen e.g. when deleting files from a Collectives app mountpoint with the Encryption app enabled. The Collectives app implements its own mountpoints, but no custom trash. So deleting a file moves it from the (unencrypted) Collectives storage to the default Nextcloud trash.

Steps to reproduce

1. Enable Encryption app
2. Enable Collectives app
3. Create a collective and a collective page in the Collectives app
4. Delete the page in the Collectives app
5. See the resulting Exception:

{
  "Exception": "Error",
  "Message": "Call to a member function getId() on array",
  "Code": 0,
  "Trace": [
     {},
  ],
  "File": "/var/www/html/lib/private/Files/Cache/Cache.php",
  "Line": 546,
  "CustomMessage": "Collectives App Error: Call to a member function getId() on array"
}

Expected behavior

First, either Cache->get() should not return an array, or it should mention this in the type hints in PHP annotations.

Second, Cache->remove() should probably check whether the returned $entry is of type ICacheEntry.

The following patch solves the concrete problem with deleting pages in Collectives and enabled Encryption app, but I'm unsure whether it's the correct fix:

diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php
index dc6ab6dae2..5dded8c32d 100644
--- a/lib/private/Files/Cache/Cache.php
+++ b/lib/private/Files/Cache/Cache.php
@@ -540,7 +540,7 @@ class Cache implements ICache {
        public function remove($file) {
                $entry = $this->get($file);
 
-               if ($entry) {
+               if ($entry instanceof ICacheEntry) {
                        $query = $this->getQueryBuilder();
                        $query->delete('filecache')
                                ->whereFileId($entry->getId());

Installation method

No response

Operating system

No response

PHP engine version

No response

Web server

No response

Database engine version

No response

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

No response

Are you using the Nextcloud Server Encryption module?

No response

What user-backends are you using?

• Default user-backend (database)
• LDAP/ Active Directory
• SSO - SAML
• Other

Configuration report

No response

List of activated Apps

Enabled:
  - accessibility: 1.10.0
  - activity: 2.17.0
  - circles: 24.0.0
  - cloud_federation_api: 1.7.0
  - collectives: 1.3.0
  - comments: 1.14.0
  - contacts: 4.1.1
  - contactsinteraction: 1.5.0
  - dashboard: 7.4.0
  - dav: 1.22.0
  - encryption: 2.12.0
  - federatedfilesharing: 1.14.0
  - federation: 1.14.0
  - files: 1.19.0
  - files_sharing: 1.16.2
  - files_trashbin: 1.14.0
  - files_versions: 1.17.0
  - lookup_server_connector: 1.12.0
  - notifications: 2.13.0
  - oauth2: 1.12.0
  - provisioning_api: 1.14.0
  - settings: 1.6.0
  - sharebymail: 1.14.0
  - systemtags: 1.14.0
  - text: 3.5.1
  - theming: 1.15.0
  - twofactor_backupcodes: 1.13.0
  - updatenotification: 1.14.0
  - user_oidc: 1.2.0
  - user_status: 1.4.0
  - viewer: 1.8.0
  - weather_status: 1.4.0
  - workflowengine: 2.6.0
Disabled:
  - admin_audit
  - announcementcenter
  - deck
  - files_external
  - groupfolders
  - profiler
  - testing
  - user_ldap

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

[kesselb]

Hi, reminds me at: #22598

[mejo-]

@kesselb, @icewind1991, @PVince81: what do you think about my proposed patch? Shall I open a PR?

[icewind1991]

The proposed change looks good.

In the long term, the whole "partial cache entry" is a wart that should probably be removed (and I don't remember the original motivation for it)