Fix missing collection type in paginated requests #53725

salmart-dev · 2025-06-30T11:53:15Z

Resolves: [Bug]: DAV missing collection type in paginated PROPFIND requests #53674

Summary

Problem

When using the custom pagination some attributes are missing or have invalid values.

json_encode and json_decode are used to store and retrieve entries to/from the cache. This involuntarily removes classes that are not extending JsonSerializable or anyways produce wrong XML output.

Solution

To prevent this from happening in the future, since we cannot get the guarantee that properties are json-serializable, we must make sure that they are ourselves. This is done by providing a custom implementation of Sabre\XML\Writer which produces an array with a structure that can be re-serialised by a Sabre\XML\Writer instance.

Once this writer is used, cached file properties are then rendered correctly:

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)

icewind1991

using serialize/deserialize is a bad idea for a number of reasons.

It would be much better to just ensure that all properties are json-serializable

salmart-dev · 2025-07-15T10:22:56Z

apps/dav/lib/Paginate/ArrayWriter.php

+		} elseif (is_array($value)) {
+			$this->decomposeArray($value);
+		} elseif (is_callable($value)) {
+			$value($this);


Note to reviewers: could you please have a look at this and verify my understanding that this is a false alert is incorrect?

This is scary, do you really need to support callables in there? What is the source of the data, can it not be set by the user?

This would be set by a sabre plugin, not the user.

We can probably get away with just not allowing this though.

I'll see if this can be removed, but this is already part of SabreDAV's serializer, so I don't think we are introducing anything new here.

@icewind1991 it's gone, now an InvalidArgumentException is thrown instead. I can't guarantee though that this won't ever happen 🤔 is there any way to test this in a more real-world scenario?

come-nc

This feels overengineered to me, writing a custom serializer is scary.
But I’m not sure I understand the whole context.

salmart-dev · 2025-07-28T08:43:25Z

This feels overengineered to me, writing a custom serializer is scary. But I’m not sure I understand the whole context.

I had a solution with serialize/deserialize but it would have acted only on the properties that we know break and introduced issues due to serializing/deserializing objects into the cache, plus the possible security issues it could've introduced.

With the custom serializer any property will be made serializable using the property of the Writer that it can serialize arrays with a certain structure. I agree that it was a lot of work for a fix of an issue that seems simple, but I couldn't come up with a fourth solution that addressed as many concerns 👀

I also considered the option of posting a PR for SabreDAV to make properties JsonSerializable, but even that is no guarantee that every property we handle will implement that interface, plus the properties still would need to be "JsonDeserialized" before handing them over back to SabreDAV which makes it impossible, considering that there is no "JsonDeserializable" format.

On the positive side, I tried to add a couple tests to make sure that if anything changes from SabreDAV's side, tests will fail.

provokateurin

Didn't review the entire thing, but I have to agree that this is really overengineered. I would rather like to find a way to force developers to make their props JsonSerializable, as this is just simpler for everyone.

provokateurin · 2025-07-28T14:03:20Z

apps/dav/lib/Paginate/MakePropsSerializableIterator.php

+			}
+
+			foreach ($value as &$property) {
+				if (is_object($property)) {


Suggested change

if (is_object($property)) {

if (!is_array($property)) {

Maybe I don't understand enough of the context, but does this make more sense?

No those are the values of the actual properties. So $property would be 'files/admin' for href and an instance of GetLastModified for the last-modified property.

apps/dav/lib/Paginate/PaginatePlugin.php

apps/dav/lib/Paginate/PaginateCache.php

salmart-dev · 2025-07-28T15:38:55Z

Didn't review the entire thing, but I have to agree that this is really overengineered. I would rather like to find a way to force developers to make their props JsonSerializable, as this is just simpler for everyone.

Yay, now we're three 😄

Here's what I tried or considered, in conjunction with Robin in some parts:

Patch the property manually: ruled out because this would need to be done for every object property
Serialize object properties using serialize/deserialize: ruled out because of security concerns
Adding JsonSerializable to all classed representing properties in the upstream, ruled out because:
- the object needs to be serialized into clark-notation, to be reusable in a normal writer serializer, this already seems very specific to the use we want to have
- if any format other than clark/array-notation is used, custom de-serialization logic would be needed
- often to serialize objects a reference to the NS map or other data is needed, which is not available in the objects themselves, requiring injection data into the original classes or the toJson function
- the logic for serialization can be complex depending on the class and may need to involve children objects
Writing a serializer that can be used to serialize properties using a similar process as the original one

Regarding the fact that adding JsonSerializable would be easier for everyone I disagree: this would need everyone from the Sabre devs, us, to the contributors of NC apps to have to deal with Sabre's serialization mechanism to make sure their properties can be encoded into json and then serialized directly to XML from that by Sabre's writer. This will need to be done in every new plugin using object properties. On the other hand, the writer solution requires the writer logic to be consistent with what Sabre expects, which could also be a challenge, but would probably be easier to adapt, than having to do the same in tens or more of plugins.

come-nc · 2025-07-29T08:26:17Z

So, the point is to cache sabre answer when we have a paginated request to be able to return other pages later, right?
Is it not possible to serialize to xml and cache that as it will get serialized to xml later anyway?

salmart-dev · 2025-07-29T15:51:58Z

So, the point is to cache sabre answer when we have a paginated request to be able to return other pages later, right? Is it not possible to serialize to xml and cache that as it will get serialized to xml later anyway?

It is probably possible to just render the "pages" and store those in cache as single items, provided huge strings are not a problem to store (not sure TBH), but that requires the count of items per page to be stable and due to the way the pagination is currently implemented, that is not guaranteed.

If the pagination parameters were stable, that would've been a viable option. Since the pagination has been out in the public for a while, I'm not sure how acceptable it would be to freeze the pagination parameters after the first request and then store XML directly and return it in subsequent pagination calls.

The pagination plugin for DAV stores file properties in a cache using json_encode. This works only if all proprierties are serializable, but the properties represented by ResourceType and GetLastModified are not. Moreover, apps can add other properties and those also needs to be serializable. Since the json-serializability is not guaranteed, this commit introduces an ArrayWriter class that can be passed to xmlSerialize to create a serializable array structure that is usable by \Sabre\Xml\Writer during the actual XML serialization process. Signed-off-by: Salvatore Martire <[email protected]>

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

salmart-dev · 2025-10-28T09:50:50Z

closing this PR in favor of #56035

salmart-dev force-pushed the fix/53674-webdav-paginate-missing-collection-type branch from c4da152 to 55bc091 Compare June 30, 2025 11:54

salmart-dev self-assigned this Jun 30, 2025

salmart-dev added bug feature: dav labels Jun 30, 2025

salmart-dev marked this pull request as ready for review June 30, 2025 16:55

salmart-dev requested a review from a team as a code owner June 30, 2025 16:55

salmart-dev requested review from Altahrim, come-nc and icewind1991 and removed request for a team June 30, 2025 16:55

icewind1991 requested changes Jun 30, 2025

View reviewed changes

salmart-dev force-pushed the fix/53674-webdav-paginate-missing-collection-type branch from 55bc091 to 06b0b35 Compare July 3, 2025 15:56

salmart-dev force-pushed the fix/53674-webdav-paginate-missing-collection-type branch from 1610dbf to e29211c Compare July 14, 2025 17:02

github-advanced-security bot found potential problems Jul 14, 2025

View reviewed changes

sorbaugh requested a review from icewind1991 July 17, 2025 07:28

salmart-dev added the 3. to review Waiting for reviews label Jul 21, 2025

come-nc reviewed Jul 28, 2025

View reviewed changes

provokateurin reviewed Jul 28, 2025

View reviewed changes

icewind1991 approved these changes Aug 1, 2025

View reviewed changes

salmart-dev added 7 commits August 27, 2025 18:28

fixup! fix(dav): make file-properties serializable

2af4c46

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

fixup: fix warnings and failures on php versions <84

eec930f

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

fixup: new static instead of new self

9c87a70

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

fixup: avoid reshaping all properties if they are not "complex" ones

b49e9d9

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

fixup: remove callable support in ArrayWriter

aa5620c

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

fixup: use qualified reference for built-in functions

18282e0

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

salmart-dev added 3 commits August 27, 2025 18:28

fixup: remove unnecessary var annotation

757d176

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

fixup! fixup: use qualified reference for built-in functions

f4e0c78

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

fixup! fixup! fix(dav): make file-properties serializable

eed3248

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

salmart-dev force-pushed the fix/53674-webdav-paginate-missing-collection-type branch from 47e84fe to eed3248 Compare August 27, 2025 16:29

salmart-dev mentioned this pull request Oct 27, 2025

fix(pagination): render multistatus to XML before caching #56035

Merged

7 tasks

salmart-dev closed this Oct 28, 2025

Uh oh!

Fix missing collection type in paginated requests #53725

Fix missing collection type in paginated requests #53725

Uh oh!

Conversation

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

Uh oh!

Summary

Problem

Solution

Checklist

Uh oh!

icewind1991 left a comment

Choose a reason for hiding this comment

Uh oh!

Check failure

salmart-dev Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

come-nc Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

icewind1991 Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

salmart-dev Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

salmart-dev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

salmart-dev commented Jul 28, 2025

Uh oh!

provokateurin left a comment

Choose a reason for hiding this comment

Uh oh!

provokateurin Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

salmart-dev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

salmart-dev commented Jul 28, 2025

Uh oh!

come-nc commented Jul 29, 2025

Uh oh!

salmart-dev commented Jul 29, 2025

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

salmart-dev commented Jun 30, 2025 •

edited

Loading

salmart-dev commented Oct 28, 2025 •

edited

Loading