fix(dav): Do not respond version/trashbin download requests with 404 due to ChunkingV2Plugin

[DerDreschner]

Summary.

This PR fixes an issue in the ChunkingV2Plugin that was introduced with #59780 . It's hard to spot, as it's being hidden in Nextcloud 30 and newer by using a different way of registering to the beforeMethod:GET sabre event.

Nextcloud 30 and newer uses the following way: $server->on('beforeMethod:GET', $this->beforeGet(...));
Nextcloud 29 and older uses the following way: $server->on('beforeMethod:GET', [$this, 'beforeGet']);

This slight change has an effect on the order of the plugins being executed. If the ChunkingV2Plugin is being run too soon - which is the case on Nextcloud 29 and older -, the file versions and trash bin items can't be downloaded due to a 404 being returned. This isn't the case once the PR is merged, as the ChunkingV2Plugin handles that case gracefully now.

This is a summary of the situation by AI:

ChunkingV2Plugin::beforeGet eagerly resolves the request path during beforeMethod:GET to block reading intermediate chunked uploads. App-provided DAV collections (versions, trashbin) are attached to the root lazily in a beforeMethod:* closure in Server.php, while uploads is registered eagerly. When beforeGet runs before that closure, getNodeForPath() throws NotFound and aborts the whole request, turning every GET under /dav/versions/ and /dav/trashbin/ into "404 File not found: versions in 'root'". PROPFIND is unaffected, since nothing resolves the path that early for it.

This does not currently surface on master only by accident of listener ordering: beforeGet and the collection closure share the default priority, and because beforeGet is registered as a first-class callable (a Closure), the wildcard closure happens to sort before it, so the collections are already attached by the time beforeGet resolves the path. That ordering is not guaranteed -- it depends on how equal-priority listeners are tie-broken -- so the unguarded resolution is a latent fault that any reordering can expose. On stable 28 it is already broken, because the backport registered the handler as an array callable, which tie-breaks the other way and runs beforeGet first.

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)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

• The content of this PR was partly or fully generated using AI (ClaudeCode:claude-opus-4-8[1m])

[DerDreschner]

/backport to stable34

[DerDreschner]

/backport to stable33

[DerDreschner]

/backport to stable32

[DerDreschner]

/backport to stable31

[DerDreschner]

/backport to stable30

[DerDreschner]

/backport to stable29

[DerDreschner]

/backport to stable28

[DerDreschner]

/backport to stable27

[DerDreschner]

/backport to stable26

[DerDreschner]

Failing Cypress run is unrelated to this change

[provokateurin]

This slight change has an effect on the order of the plugins being executed

How so? I don't understand.

[DerDreschner]

How so? I don't understand.

It's PHP internal stuff about what wins when an array and a closure have the same priority.

This is an easy description of the issue made by Claude, the array_multisort is what Sabre uses to make the sorting:

Why the registration style changes the order

array_multisort($listenersPriority, SORT_NUMERIC, $listeners) sorts by priority first. But priorities usually tie (everyone defaults to 100), so array_multisort breaks the tie by comparing the second array — the callables themselves. And PHP compares a Closure differently than an [$obj, 'method'] array:

Comparison	<=> result
[$obj,'m'] vs Closure	-1 → array always sorts before
Closure vs [$obj,'m']	+1 → object always sorts after

That type rule is deterministic: an array callable is always "less than" a closure. So the interesting case is the realistic one — in NC30 only ChunkingV2Plugin switched to $this->beforeGet(...) (a Closure) while every sibling GET listener stayed an array. The demo output:

=== MIXED  ->  only ChunkingV2Plugin is a Closure, the rest are arrays ===
after sort:
   #1  ->  Versions/Core GET handler   (serves the file)
   #2  ->  SomeOtherPlugin::beforeGet
   #3  ->  ChunkingV2Plugin::beforeGet (throws MethodNotAllowed for uploads)

vs. the all-array (NC29) form, where the same listeners keep their intended registration order:

=== Nextcloud 29-  ->  [$this,'beforeGet']  (array callables) ===
after sort:
   #1  ->  ChunkingV2Plugin::beforeGet
   #2  ->  Versions/Core GET handler
   #3  ->  SomeOtherPlugin::beforeGet

The single closure gets shoved to a different slot purely because object always sorts after array. Since Sabre's sort isn't a proper stable/consistent ordering when the keys tie, the "shape" of the callable silently decides who runs first — and that reorders the GET pipeline, which is what produces the DAV-versions 404.

[DerDreschner]

Here is a demo that Claude made to illustrate the issue: https://onlinephp.io/c/3690dc

=== Nextcloud 29-  ->  [$this, 'beforeGet']  (array callables) ===
before sort:
   prio=100  type=array   
   prio=100  type=array   
   prio=100  type=array   
after sort (this is the order Sabre will actually call them):
   #1  ->  ChunkingV2Plugin::beforeGet (throws MethodNotAllowed for uploads)
   #2  ->  Versions/Core GET handler (serves the file -> the one we must reach)
   #3  ->  SomeOtherPlugin::beforeGet

=== Nextcloud 30+  ->  $this->beforeGet(...)  (Closure objects) ===
before sort:
   prio=100  type=Closure 
   prio=100  type=Closure 
   prio=100  type=Closure 
after sort (this is the order Sabre will actually call them):
   #1  ->  ChunkingV2Plugin::beforeGet (throws MethodNotAllowed for uploads)
   #2  ->  Versions/Core GET handler (serves the file -> the one we must reach)
   #3  ->  SomeOtherPlugin::beforeGet

=== MIXED  ->  only ChunkingV2Plugin is a Closure, the rest are arrays ===
before sort:
   prio=100  type=array   
   prio=100  type=Closure 
   prio=100  type=array   
after sort (this is the order Sabre will actually call them):
   #1  ->  Versions/Core GET handler (serves the file -> the one we must reach)
   #2  ->  SomeOtherPlugin::beforeGet
   #3  ->  ChunkingV2Plugin::beforeGet (throws MethodNotAllowed for uploads)

=== Why: how PHP compares a Closure vs an array callable ===
   [obj, "method"]  <=>  Closure   =>   -1  (array sorts before object)
   Closure          <=>  [obj,...]  =>   1  (object sorts after array)

   => An [$obj,'method'] callable and a $obj->method(...) Closure are NOT interchangeable
      as far as array_multisort's tie-break is concerned, so equal-priority GET
      listeners end up in a different order -> the wrong handler answers -> 404.

[come-nc]

I checked and this is true about closures being sorted differently than arrays, what a mess.
I would argue that this is a bug in Sabre, it should not use array_multisort in this way because of this side-effect.

For me the correct fix for this is to add a priority for these plugins, as clearly the order is important.
So I suppose in Server.php we should give a priority <100 so like 50 for the $this->server->on('beforeMethod:*', and for ChunkingV2Plugin, maybe use 120 or something?
@DerDreschner What do you think?

[come-nc]

I opened sabre-io/event#162 on sabre side. We should still fix it on our side with proper priorities though.

[provokateurin]

@DerDreschner I just released the fix for the ordering issue: https://github.com/sabre-io/event/releases/tag/6.1.1

[DerDreschner]

I checked and this is true about closures being sorted differently than arrays, what a mess. I would argue that this is a bug in Sabre, it should not use array_multisort in this way because of this side-effect.

I agree with that and I'm happy that's being fixed.

For me the correct fix for this is to add a priority for these plugins, as clearly the order is important.

I have mixed feelings about relying exclusively on that. I try to explain what's in my head. Please also keep in mind that I have the information being unlocked by this PR.

1. We're currently often relying on the default priority in our code base. Which is honestly fine, if all methods expect to be executed that way. This isn't the case here as the chunking plugin is throwing a NotFoundException where it shouldn't. The other methods in the chunking plugin catch that exception and handle it the same way I do with this PR. So, I think it's right from an algorithmic point of view to catch that exception instead of throwing it, as this case should clearly not being handled by the chunking plugin. It's just not their responsibility.

2. If number one is being ruled out as solution, we could change the priority for the chunking plugin and I was thinking about that as well. But I'm really hesitant to do that exclusively, besides the previously mentioned things. We might keep side-effects we're unaware of right now. As far as I understand the documentation and behavior of Sabre (see the initially mentioned PR as well as the brewHandler example ), throwing an exception in a beforeMethod listener causes all method, afterMethod and afterResponse listeners to skip. That might have implications we don't see right now. File locking would be such an example and we're only save on that front because of a safety net (that might be unnecessary if we don't skip the afterMethod listeners sometimes, I've not looked into the development history to see if that's the case).

For that reason, I would prefer to change the behavior of the chunking plugin, which would make the priority change unnecessary. The fix in sabre itself is very well welcomed tho.

On a side-pick: The documentation of sabre states:

All default PHP callbacks are supported, so closures are not required.

That honestly sounds like the original developer of sabre discourage the use of closures for registering external plugins? I'm not sure what are the pros and cons of using a closure vs. an array here to be honest. PhpStorm is able to detect the usage of the method when it's referenced inside the array, so...?