PROPFIND on (password protected) public link returns invalid XML

[dschmidt]

Steps to reproduce

1. Create a password-protected public link to a file
2. Do a prop find for that public link:

e.g.
curl 'http://host.docker.internal:8080/remote.php/dav/public-files/cm4440L2nLUxyXs' \
  -X 'PROPFIND' \
  -H 'Connection: keep-alive' \
  -H 'Pragma: no-cache' \
  -H 'Cache-Control: no-cache' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Depth: 0' \
  -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.54 Safari/537.36' \
  -H 'OCS-APIREQUEST: true' \
  -H 'Content-Type: application/xml; charset=UTF-8' \
  -H 'Origin: http://host.docker.internal:8080' \
  -H 'Referer: http://host.docker.internal:8080/index.php/apps/web/index.html' \
  -H 'Accept-Language: en,en-US;q=0.9,de;q=0.8,nl;q=0.7' \
  -H 'Cookie: oc_sessionPassphrase=....' \
  --data-raw $'<?xml version="1.0"?>\n<d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">\n  <d:prop>\n    <oc:permissions />\n    <oc:favorite />\n    <oc:fileid />\n    <oc:owner-id />\n    <oc:owner-display-name />\n    <oc:share-types />\n    <oc:privatelink />\n    <d:getcontentlength />\n    <oc:size />\n    <d:getlastmodified />\n    <d:getetag />\n    <d:getcontenttype />\n    <d:resourcetype />\n    <oc:downloadURL />\n    <oc:public-link-item-type />\n    <oc:public-link-permission />\n    <oc:public-link-expiration />\n    <oc:public-link-share-datetime />\n    <oc:public-link-share-owner />\n  </d:prop>\n</d:propfind>' \
  --compressed \
  --insecure

Expected behaviour

A non password protected link gives me something like this:

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"><d:response><d:href>/remote.php/dav/public-files/SjxLd54N1k4utk3/</d:href><d:propstat><d:prop><oc:permissions></oc:permissions><d:resourcetype><d:collection/></d:resourcetype><oc:public-link-item-type>file</oc:public-link-item-type><oc:public-link-permission>1</oc:public-link-permission><oc:public-link-share-datetime>Fri, 21 Jan 2022 20:38:09 GMT</oc:public-link-share-datetime><oc:public-link-share-owner>admin</oc:public-link-share-owner></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat><d:propstat><d:prop><oc:favorite/><oc:fileid/><oc:owner-id/><oc:owner-display-name/><oc:share-types/><oc:privatelink/><d:getcontentlength/><oc:size/><d:getlastmodified/><d:getetag/><d:getcontenttype/><oc:downloadURL/><oc:public-link-expiration/></d:prop><d:status>HTTP/1.1 404 Not Found</d:status></d:propstat></d:response><d:response><d:href>/remote.php/dav/public-files/SjxLd54N1k4utk3/Lake-Constance.jpg</d:href><d:propstat><d:prop><oc:permissions></oc:permissions><oc:fileid>10</oc:fileid><oc:owner-id>admin</oc:owner-id><oc:owner-display-name>admin</oc:owner-display-name><d:getcontentlength>538942</d:getcontentlength><oc:size>538942</oc:size><d:getlastmodified>Tue, 18 Jan 2022 14:14:48 GMT</d:getlastmodified><d:getetag>&quot;fec2941b4f10299fb0472a9ad65785c1&quot;</d:getetag><d:getcontenttype>image/jpeg</d:getcontenttype><d:resourcetype/><oc:downloadURL>http://host.docker.internal:8080/remote.php/dav/public-files/SjxLd54N1k4utk3/Lake-Constance.jpg</oc:downloadURL></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat><d:propstat><d:prop><oc:favorite/><oc:share-types/><oc:privatelink/><oc:public-link-item-type/><oc:public-link-permission/><oc:public-link-expiration/><oc:public-link-share-datetime/><oc:public-link-share-owner/></d:prop><d:status>HTTP/1.1 404 Not Found</d:status></d:propstat></d:response></d:multistatus>

Actual behaviour

... but if a password is required (and missing in the request?) I get something like this:

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAVACL\Exception\NeedPrivileges</s:exception>
  <s:message>User did not have the required privileges ({DAV:}read) for path "public-files/cm4440L2nLUxyXs"</s:message>
  <d:need-privileges>
    <d:resource>
      <d:href>/remote.php/dav/public-files/cm4440L2nLUxyXs</d:href>
      <d:privilege>
        <d:read/>
      </d:privilege>
    </d:resource>
  </d:need-privileges>
</d:error>

Notice especially <d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"<?xml version="1.0" encoding="utf-8"?> which seems utterly broken.

Server configuration

I'm using owncloud/server:latest docker image

[phil-davis]

Added QA:team and assigned myself to find where we have a test like this (if at all) and make a test that demonstrates the issue. Maybe in doing that I will also discover the cause and it will be an easy fix. Otherwise I will hand it to the devs.

[jvillafanez]

I'm not fully sure if this was implemented in the "remote.php/dav/" endpoint.

A partial patch below. Note that it's incomplete:

diff --git a/apps/dav/lib/Files/PublicFiles/PublicSharedRootNode.php b/apps/dav/lib/Files/PublicFiles/PublicSharedRootNode.php
index 20e202318e..a19c233d61 100644
--- a/apps/dav/lib/Files/PublicFiles/PublicSharedRootNode.php
+++ b/apps/dav/lib/Files/PublicFiles/PublicSharedRootNode.php
@@ -190,7 +190,17 @@ class PublicSharedRootNode extends Collection implements IACL {
                                'privilege' => '{DAV:}read',
                                'principal' => 'principals/system/public',
                                'protected' => true,
-                       ]
+                       ],
+                       [
+                               'privilege' => '{DAV:}read',
+                               'principal' => '{DAV:}authenticated',
+                               'protected' => true,
+                       ],
+                       [
+                               'privilege' => '{DAV:}read',
+                               'principal' => '{DAV:}unauthenticated',
+                               'protected' => true,
+                       ],
                ];
 
                if ($this->checkPermissions(Constants::PERMISSION_UPDATE)) {
diff --git a/apps/dav/lib/Files/PublicFiles/SharedFolder.php b/apps/dav/lib/Files/PublicFiles/SharedFolder.php
index 7a33f1a2a7..4911cbf233 100644
--- a/apps/dav/lib/Files/PublicFiles/SharedFolder.php
+++ b/apps/dav/lib/Files/PublicFiles/SharedFolder.php
@@ -97,7 +97,17 @@ class SharedFolder extends Collection implements IACL, IPublicSharedNode {
                                'privilege' => '{DAV:}read',
                                'principal' => 'principals/system/public',
                                'protected' => true,
-                       ]
+                       ],
+                       [
+                               'privilege' => '{DAV:}read',
+                               'principal' => '{DAV:}authenticated',
+                               'protected' => true,
+                       ],
+                       [
+                               'privilege' => '{DAV:}read',
+                               'principal' => '{DAV:}unauthenticated',
+                               'protected' => true,
+                       ],
                ];
 
                if ($this->checkSharePermissions(Constants::PERMISSION_DELETE)) {
diff --git a/apps/dav/lib/Files/PublicFiles/SharedNodeTrait.php b/apps/dav/lib/Files/PublicFiles/SharedNodeTrait.php
index 362ac0378a..2667fadafd 100644
--- a/apps/dav/lib/Files/PublicFiles/SharedNodeTrait.php
+++ b/apps/dav/lib/Files/PublicFiles/SharedNodeTrait.php
@@ -118,7 +118,17 @@ trait SharedNodeTrait {
                                'privilege' => '{DAV:}read',
                                'principal' => 'principals/system/public',
                                'protected' => true,
-                       ]
+                       ],
+                       [
+                               'privilege' => '{DAV:}read',
+                               'principal' => '{DAV:}authenticated',
+                               'protected' => true,
+                       ],
+                       [
+                               'privilege' => '{DAV:}read',
+                               'principal' => '{DAV:}unauthenticated',
+                               'protected' => true,
+                       ],
                ];
 
                if (($this->node instanceof File) && $this->checkSharePermissions(Constants::PERMISSION_UPDATE)) {

That would work partially but there are still some problems:

• Authenticated users are able to access without providing a password. Taking into account that it's a propfind request, it should be fine.
  • Downloads are expected to use the "downloadUrl" property (?), but using that url doesn't require a password
• Unauthenticated users can't access

  <?xml version="1.0" encoding="utf-8"?>
  <d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
    <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
    <s:message>No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, No public access to this resource., No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured</s:message>
  </d:error>
  

[jvillafanez]

Should non-authenticated users be able to make a propfind? It seems this will break password-protected links.

Assuming you have the token / id for the link:

1. unauthenticated user could do a propfind on the link to get the downloadURL (which is expected to be signed)
2. using that downloadURL, the user can get the file without providing a password.

Even if the link lasts for 30 minutes, since anyone can regenerate the link, anyone can download the file anytime.

By limiting the propfind to authenticated users, at least you need to be logged in to generate the downloadURL.

Note that password won't be asked for in any of these workflows. The signed url takes care of the password, but this seems more like a link with a timed expiration than a password-protected link.

So, things to clarify:

• Should authenticated users be able to perform a propfind on the public link?
• Should unauthenticated users be able to perform a propfind on the public link?

[C0rby]

Should non-authenticated users be able to make a propfind? It seems this will break password-protected links.

No, IMO this should not be possible for password protected links.

Should authenticated users be able to perform a propfind on the public link?

In which way authenticated? Using the link password or using their ownCloud account? I'd say using the link password yes, using the ownCloud account, no.

[C0rby]

But as far as I understand this issue it's not about permissions. The issue is about the invalid xml: <d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"<?xml version="1.0" encoding="utf-8"?>

[jvillafanez]

Ok, so other than fixing the broken XML format, there isn't anything else to do.

[C0rby]

At least this is what I understood.

[dschmidt]

My original ticket was about that - yes.

I saw it again in @kulmann 's report here: https://github.com/owncloud/web/issues/6248#issuecomment-1035545942 Possibly the response is syntactically and semantically wrong ... to me it looks like it should be possible to do a PROPFIND when authenticated by password, basically what @C0rby said here: https://github.com/owncloud/core/issues/39707#issuecomment-1041261944

@kulmann happy to hear your feedback

[jvillafanez]

It seems the bad formatting is partially caused by the sabre-dav library in combination with the "streamMultistatus" property (https://github.com/owncloud/core/blob/54b1aabb8febad49a1967d127a43e7afee197dbb/apps/dav/lib/Server.php#L228)

What happens is that the XML and namepaces are written, and then, trying to get the properties of the node throws an exception. That exception starts a new XML document without taking into account what has been written before.

Turning off the "streamMultistatus" property (which is currently hardcoded) solves the problem. The initial document isn't written, so the exception can write the XML document without problem.

The other option is to set sabre not to throw exceptions (https://github.com/owncloud/core/blob/54b1aabb8febad49a1967d127a43e7afee197dbb/apps/dav/lib/Connector/Sabre/DavAclPlugin.php#L48), but it seems a bad UX. It's a barely empty document without any useful information.

curl -k -u admin:admin -X PROPFIND 'https://server.prv/remote.php/dav/public-files/WEDFiC3juS1y5Wa'
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"/>

---

On our side, the options we have are:

• Do not stream multistatus for public links
• Allow authenticated users (checked with username+password, but it sould work with cookies and other) to perform the propfind

[jvillafanez]

https://github.com/owncloud/core/pull/39797 should fix the problem with the broken XML format.

What confused me is that you have to use public:<password> as username and password to be able to perform the propfind. For me, that means that you have to authenticate with a fake account, but an account nevertheless. That also means that I can't access with my personal account because I have to use that fake account, not sure if it could be relevant for auditing.

Anyway, as said, other than fixing the format, there is no other relevant change in the behavior.

[phil-davis]

I removed my assignment. Some other QA-team person can be assigned. Please read the comments above, then sort out adding some tests that cover doing PROPFIND on public links in various combinations (with/without password).

Some test scenarios will probably be failing - we can discuss in the PR what to do about that (do we write bug-demo scenarios, or is this a case where we should create an expected-failures file for core, or...)

[jnweiger]

Confirmed fixed in 10.10.0 RC2

The error response when password is needed, but not provided now looks like this:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, No public access to this resource., No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, No 'Authorization: Bearer' header found. Either the client didn't send one, or the server is mis-configured, No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured</s:message>
</d:error>

The message contains No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, three times, that seems redundant, but hey, fine with me. Its an error message after all.