[WIP] Add INodeByPath to Directory #54441

salmart-dev · 2025-08-15T14:14:40Z

Summary

Sabre has an interface INodeByPath that allows nodes to fetch a specific node by its path. This was not implemented in our Directory class, leading to any file query to fetch all parent nodes until the file node was reached. This PR makes Directory implement that interface, extends makes the NC Node class extend the Node class from Sabre (required step).

Before:

After:

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

salmart-dev · 2025-08-15T16:26:06Z

apps/dav/lib/Connector/Sabre/Node.php

 use OCP\Share\IManager;

-abstract class Node implements \Sabre\DAV\INode {
+abstract class Node extends \Sabre\DAV\Node implements \Sabre\DAV\INode {


Comment: unfortunately this is needed, as there is a check in Tree that checks for Node and not INode 😢 without this, the whole thing would not work.

This is not ideal. I this an overview in sabre code? Can you point to the code?

The problematic check is here:
https://github.com/sabre-io/dav/blob/58be83aae10a244372f113b63624c48034378094/lib/DAV/Tree.php#L87

but it's open source, so easy to change :) sabre-io/dav#1595

come-nc · 2025-08-18T07:38:30Z

apps/dav/lib/Connector/Sabre/Node.php

 use OCP\Share\IManager;

-abstract class Node implements \Sabre\DAV\INode {
+abstract class Node extends \Sabre\DAV\Node implements \Sabre\DAV\INode {


This is not ideal. I this an overview in sabre code? Can you point to the code?

apps/dav/lib/Connector/Sabre/Directory.php

Signed-off-by: Salvatore Martire <[email protected]>

github-actions · 2025-09-10T11:03:32Z

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/work/server/server/occ(33): require_once('...')
#9 {main}

github-actions · 2025-09-10T13:32:03Z

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/work/server/server/occ(33): require_once('...')
#9 {main}

salmart-dev · 2025-09-10T13:46:12Z

Performance regression warning is unrelated. The output of the check contains text which makes the json_decode part of the check fail and return null.

github-actions · 2025-09-10T13:47:04Z

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/actions-runner/_work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/actions-runner/_work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/actions-runner/_work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/actions-runner/_work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/actions-runner/_work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/actions-runner/_work/server/server/occ(33): require_once('...')
#9 {main}

github-actions · 2025-09-10T13:54:42Z

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/work/server/server/occ(33): require_once('...')
#9 {main}

come-nc · 2025-09-11T14:25:03Z

apps/dav/lib/Connector/Sabre/Directory.php

+			// In case reading a directory was allowed but it turns out the node was a not a directory, reject it now.
+			if (!$this->info->isReadable()) {
+				throw new NotFound();
+			}


Why is $this->info used here and not $info?
Also I do not understand the comment, is it related to this if?

@come-nc $this->info refers to the node on which getNodeForPath gets called.

Regarding the comment: it is a leftover of the checks for the files drop checks in getChild which I had dropped for this function. But of course dropping the checks was not OK, since now I can skip ACLs from the files_accesscontrol app. I tested this after discovering the files_accesscontrol app and I am able to do things I shouldn't be able to.

I will add a comment to describe the problem and see if anyone has ideas on what else can break and if there are other solutions other than abandoning the PR.

console.php

CarlSchwan · 2025-09-11T15:24:57Z

apps/dav/lib/Connector/Sabre/Directory.php

 		return $this->node;
 	}
+
+	public function getNodeForPath($path) {


Suggested change

public function getNodeForPath($path) {

public function getNodeForPath($path): INode {

or whatever is the correct node ;)

console.php

salmart-dev · 2025-09-15T17:26:51Z

I converted the PR back to a draft because I ran into an issue with access control rules.

Problem statement

Apps that rely on path navigation to perform checks that block an action could be broken with this PR.

Example

The files_accesscontrol app relies on each node's name to match for names that have been blocked. With this PR, the intermediate nodes in a path may never be navigated to, creating an issue where a file deep in the tree can be accessed, although one of the skipped nodes is marked as not readable through the app.

In the current implementation, the app relies on the fact that isReadable gets called in every Node, which in turn triggers the check for the file name in the app.

How to reproduce

Create a public share
Add a folder inside the public share
Block access to the directory through a flow with the files_accesscontrol app
Try uploading inside the new folder

Uploading should fail but succeeds instead

Possible solution?

Alter files_accesscontrol

For the case in the example, a fix could be to change files_accesscontrol to stop relying on the assumption that all nodes in a path will be navigated to and compare each part of the exploded path with the given value to block, but I fear this would break other flows and checks.

Can someone come up with ideas on how to address this or other things that may break, so that if we try a new solution I can test it still works?

icewind1991 · 2025-09-17T12:35:59Z

stop relying on the assumption that all nodes in a path will be navigated to and compare each part of the exploded path

This is probably the best option imo.

Another alternative would be to have a dav plugin in files_accesscontrol to do extra checking without effecting any of the other rules or non-dav code paths.

salmart-dev self-assigned this Aug 15, 2025

salmart-dev force-pushed the fix/directoryAsINodeByPath branch from cad0ea3 to 87d9b88 Compare August 15, 2025 16:12

salmart-dev commented Aug 15, 2025

View reviewed changes

salmart-dev added 2. developing Work in progress performance 🚀 labels Aug 15, 2025

come-nc reviewed Aug 18, 2025

View reviewed changes

salmart-dev force-pushed the fix/directoryAsINodeByPath branch from f67982e to c54b5c9 Compare August 21, 2025 16:28

come-nc requested a review from icewind1991 August 25, 2025 09:05

salmart-dev added 7 commits September 10, 2025 12:12

fix: add INodeByPath to Directory [wip]

b261be6

Signed-off-by: Salvatore Martire <[email protected]>

fixup! fix: add INodeByPath to Directory [wip]

22fffd4

Signed-off-by: Salvatore Martire <[email protected]>

fixup! fix: add INodeByPath to Directory [wip]

c5e6469

Signed-off-by: Salvatore Martire <[email protected]>

fixup! fix: add INodeByPath to Directory [wip]

731f4cd

Signed-off-by: Salvatore Martire <[email protected]>

fixup! fix: add INodeByPath to Directory [wip]

2e6416a

Signed-off-by: Salvatore Martire <[email protected]>

fixup! fixup! fix: add INodeByPath to Directory [wip]

9ea300d

Signed-off-by: Salvatore Martire <[email protected]>

fixup! fix: add INodeByPath to Directory [wip]

cf952ce

Signed-off-by: Salvatore Martire <[email protected]>

salmart-dev force-pushed the fix/directoryAsINodeByPath branch from c54b5c9 to cf952ce Compare September 10, 2025 11:02

salmart-dev added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 10, 2025

salmart-dev marked this pull request as ready for review September 10, 2025 14:04

salmart-dev requested a review from a team as a code owner September 10, 2025 14:04

salmart-dev requested review from Altahrim and sorbaugh and removed request for a team September 10, 2025 14:04

salmart-dev changed the title ~~Add INodeByPath to Directory [wip]~~ Add INodeByPath to Directory Sep 10, 2025

icewind1991 approved these changes Sep 11, 2025

View reviewed changes

come-nc reviewed Sep 11, 2025

View reviewed changes

console.php Outdated Show resolved Hide resolved

CarlSchwan reviewed Sep 11, 2025

View reviewed changes

salmart-dev marked this pull request as draft September 15, 2025 16:38

salmart-dev changed the title ~~Add INodeByPath to Directory~~ [WIP] Add INodeByPath to Directory Sep 15, 2025

salmart-dev force-pushed the fix/directoryAsINodeByPath branch from a1a6278 to cf952ce Compare September 16, 2025 08:08

	public function getNodeForPath($path) {
	public function getNodeForPath($path): INode {

Uh oh!

[WIP] Add INodeByPath to Directory #54441

Are you sure you want to change the base?

[WIP] Add INodeByPath to Directory #54441

Conversation

salmart-dev commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Sep 10, 2025

Uh oh!

github-actions bot commented Sep 10, 2025

Uh oh!

salmart-dev commented Sep 10, 2025

Uh oh!

github-actions bot commented Sep 10, 2025

Uh oh!

github-actions bot commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

salmart-dev commented Sep 15, 2025

Problem statement

Example

How to reproduce

Possible solution?

Alter files_accesscontrol

Uh oh!

icewind1991 commented Sep 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

salmart-dev commented Aug 15, 2025 •

edited

Loading