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

Wrong PROPFIND answer with depth infinity requests #28341

Closed
blancjerome opened this Issue Jul 8, 2017 · 22 comments

Comments

Projects
None yet
@blancjerome
Copy link

blancjerome commented Jul 8, 2017

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 (nextcloud/server#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;
            }
        }

    }
}

@blancjerome blancjerome changed the title Wrong PROPFIND answer with depth infinity Wrong PROPFIND answer with depth infinity requests Jul 8, 2017

@PVince81 PVince81 added the bug label Jul 10, 2017

@PVince81 PVince81 added this to the triage milestone Jul 10, 2017

@PVince81 PVince81 added the p2-high label Jul 11, 2017

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Jul 14, 2017

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.

@guruz guruz modified the milestones: 10.1, triage Jul 14, 2017

@blancjerome

This comment has been minimized.

Copy link

blancjerome commented Jul 14, 2017

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

@welljsjs

This comment has been minimized.

Copy link

welljsjs commented Oct 9, 2017

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

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Oct 9, 2017

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

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Oct 9, 2017

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

This comment has been minimized.

Copy link

welljsjs commented Oct 9, 2017

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

This comment has been minimized.

Copy link

ghost commented Oct 10, 2017

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

#27422
https://evertpot.com/sabredav-eol/
https://evertpot.com/sabredav-maintenance-update/

@phil-davis

This comment has been minimized.

Copy link
Contributor

phil-davis commented Oct 10, 2017

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

This comment has been minimized.

Copy link
Member

PVince81 commented Oct 10, 2017

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: sabre-io/dav#1007

@ownclouders

This comment has been minimized.

Copy link

ownclouders commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Member

PVince81 commented Dec 19, 2017

We need that Sabre release

@ownclouders

This comment has been minimized.

Copy link

ownclouders commented Jan 27, 2018

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

This comment has been minimized.

Copy link

michaelstingl commented Jan 27, 2018

@PVince81 What is the status of the Sabre release?

@michaelstingl michaelstingl reopened this Jan 27, 2018

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Jan 29, 2018

@DeepDiver1975 what is the status of the Sabre release ?

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Feb 23, 2018

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

@michaelstingl

This comment has been minimized.

Copy link

michaelstingl commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

PVince81 commented Mar 13, 2018

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

This comment has been minimized.

Copy link

PaulAnnekov commented Apr 22, 2018

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

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Apr 23, 2018

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

@patrickjahns

This comment has been minimized.

Copy link
Member

patrickjahns commented Aug 30, 2018

Might also be related:
#32009 (comment)

@voroyam

This comment has been minimized.

Copy link
Contributor

voroyam commented Oct 2, 2018

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Nov 5, 2018

as far as I can see we've updated Sabre on stable10 here #33276 and it contains the mentioned fix from #28341 (comment)

@PVince81 PVince81 closed this Nov 5, 2018

@davitol davitol referenced this issue Nov 22, 2018

Open

10.1 - Changelog Test Results #33624

26 of 37 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment