Core: Wrong PROPFIND answer with depth infinity requests

[blancjerome]

Hi,

I found a strange behavior with infinity depth propfind requests : under subdirectories, files are reported as directories. This is due to a Collection resource type returned in the propfind response. It occurs only for level 2 subdirectories and above.

This behavior appeared with 10.0.x version of owncloud, but it maybe was there in the 9.1.x as I discovered it after upgrading from a 9.0.x version.

I didn't found any report of this bug, except with nextcloud, but no answer yet (https://github.com/nextcloud/server/issues/5543).
I don't know what's the relationship between owncloud and nextcloud except the fact that they obviously share some code.

Because my client (Documents by Readdle) couldn't sync properly, I took a look at the code and found where the problem is.

In /var/www/html/owncloud/lib/composer/sabre/dav/lib/DAV/Server.php, the "generatePathNodes" method clones "ProFind" objects. But as these objects have been created for the containing directory, they already contain the "Collection" value for the "ResourceType" property. So the workaround is creating a brand new "PropFind" object instead of cloning it.

I hope the correction will be included in the future version of owncloud, I don't think it has an impact on memory footprint, maybe a bit on performance because Propfind objects have to be filled again. But I haven't seen any significant change.

Here is the new code for the method :

 * Small helper to support PROPFIND with DEPTH_INFINITY.
 *
 *
 * @param PropFind $propFind
 * @param array $yieldFirst
 * @return \Iterator
 */
private function generatePathNodes(PropFind $propFind, array $yieldFirst = null) {
    if ($yieldFirst !== null) {
        yield $yieldFirst;
    }
    $newDepth = $propFind->getDepth();
    $path = $propFind->getPath();

    if ($newDepth !== self::DEPTH_INFINITY) {
        $newDepth--;
    }

//FIX in order to create new PropFind objects and not clone them
$propertyNames = $propFind->getRequestedProperties();
$propFindType = $propertyNames ? PropFind::NORMAL : PropFind::ALLPROPS;

    foreach ($this->tree->getChildren($path) as $childNode) {
     //FIX : no cloning, creating new PropFind objects
        //$subPropFind = clone $propFind;
        //$subPropFind->setDepth($newDepth);
        if ($path !== '') {
            $subPath = $path . '/' . $childNode->getName();
        } else {
            $subPath = $childNode->getName();
        }
        //$subPropFind->setPath($subPath);

     //FIX : create a new PropFind object with the right parameters
     $subPropFind = new PropFind($subPath, $propertyNames, $newDepth, $propFindType);

        yield [
            $subPropFind,
            $childNode
        ];

        if (($newDepth === self::DEPTH_INFINITY || $newDepth >= 1) && $childNode instanceof ICollection) {
            foreach ($this->generatePathNodes($subPropFind) as $subItem) {
                yield $subItem;
            }
        }

    }
}

[PVince81]

From what I see this is Sabre code and needs to be submit upstream, and there is already a PR for that: https://github.com/fruux/sabre-dav/pull/982/

So once the PR is merged, will need to upgrade the Sabre library to get this.

[blancjerome]

Yes. Nextcloud team submitted the fix to Sabre few days ago. So it will be available soon.

[welljsjs]

Version 10.0.3.3 is out, still not fixed. Same problem here using Documents by Readdle.

[PVince81]

The Sabre project hasn't merged the fix https://github.com/fruux/sabre-dav/pull/982 😦

[PVince81]

It also worries me to see that there hasn't been any new release of the Sabre lib since 3.3. Last commit is from February (https://github.com/fruux/sabre-dav/tree/3.3). And on master last commit is from May 17th https://github.com/fruux/sabre-dav/tree/master.

@DeepDiver1975 ^

[welljsjs]

That is pretty sad. Do you have any idea why they did not accept it?
@PVince81
__EDIT:__ May have to do something with the _owncloud Desktop client_. Running it on my Mac, it does not work after changing the _Server.php_ script as described above.

[ghost]

@PVince81 The following links should explain why you don't see much activity at the Sabre-Project:

https://github.com/owncloud/core/issues/27422
https://evertpot.com/sabredav-eol/
https://evertpot.com/sabredav-maintenance-update/

[phil-davis]

Hmmmm - I remember reading those a few months ago and then forgot all about it. So, what resources can be put in to maintain those Sabre repos?

[PVince81]

Yeah I'm aware of these, but I was hoping at least some minimal occasional activity.

At least from today on, more people will be able to merge and make releases: https://github.com/sabre-io/dav/issues/1007

[ownclouders]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

[PVince81]

We need that Sabre release

[ownclouders]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

[michaelstingl]

@PVince81 What is the status of the Sabre release?

[PVince81]

@DeepDiver1975 what is the status of the Sabre release ?

[PVince81]

@DeepDiver1975 I heard there was a new Sabre release with the fix ? Can you update the libs on master and stable10 ?

[michaelstingl]

We've got feedback, the patch is running fine in production for 2 months now and they did not get any complain so far.

@DeepDiver1975 When will the patch be merged?

[PVince81]

the patch was merged already but Sabre DAV needs a new upstream release first, then we can update our composer.json to use the new version

@DeepDiver1975 can we move this forward?

[PaulAnnekov]

@PVince81 as I understand you don't want to depend on commit id, right? This would be a solution.

[PVince81]

@DeepDiver1975 please make a decision for the timeline for upgrading and releasing Sabre. If no release, should we switch to commit id ?

[patrickjahns]

Might also be related:
https://github.com/owncloud/core/issues/32009#issuecomment-415169998

[voroyam]

Related issue from a central user.

https://central.owncloud.org/t/webdav-folder-sync-ios-readdle-documents-6-issue/10789/23

[PVince81]

as far as I can see we've updated Sabre on stable10 here https://github.com/owncloud/core/pull/33276 and it contains the mentioned fix from https://github.com/owncloud/core/issues/28341#issuecomment-335196188