Fix viewer in public albums

[starypatyk]

This is an attempt to fix #1452 (and related duplicates). It consists of two related changes.
Also fix #1440

Part 1 - commit 1d1f011

If I understand correctly, the WebDAV requests to /remote.php/dav/photospublic/album-token/id-filename are supposed to be available anonymously - for everyone who has a link to the album, without a need to authenticate first.

The Sabre\DAV\Auth\Backend\AbstractBasic class expects the Authorization: Basic header, and returns 401 error code if it is not present:
https://github.com/sabre-io/dav/blob/master/lib/DAV/Auth/Backend/AbstractBasic.php#L101

I think that in this case we should just allow anonymous access - by implementing the Sabre\DAV\Auth\Backend\BackendInterface interface directly. The PublicAlbumAuthBackend class is registered only for DAV URLs starting with photospublic, so it seems safe to always report success in the check method.

See: https://github.com/nextcloud/photos/blob/master/lib/Listener/SabrePluginAuthInitListener.php#L47

Part 2 - commit 166c635

I propose two changes in PublicAlbumContent.vue:

• Set hasPreview to false for all files in the album. This will force current implementation in the Viewer app to use the source URL, rather than the default preview URL that does not work in this case. These changes alone should suffice to fix the issue - at least temporarily.
• The second change - setting the appPreviewPath - is a proposal how the app can provide a custom preview URL to the Viewer app. This is obviously not yet handled - I plan to submit a complementary PR to the Viewer app.

Comments/suggestions?

[simonspa]

@starypatyk thanks a lot, I'll test in a bit!

I noticed that also in "regularly" shared albums there is an issue whenever trying to open a photo from another user in the viewer, also seems related to a wrong endpoint. #1440

...and one more: the deletion in Viewer has a wrong path: #1591

Any chances these are also related to the same auth mixup?

[starypatyk]

@simonspa

@starypatyk thanks a lot, I'll test in a bit!

Thanks! 👍

I noticed that also in "regularly" shared albums there is an issue whenever trying to open a photo from another user in the viewer, also seems related to a wrong endpoint.

...and one more: the deletion in Viewer has a wrong path: #1591

Any chances these are also related to the same auth mixup?

Most likely these are unrelated. My changes are relevant only for public albums.

[simonspa]

Works like a charm for me, now clicking on thumbnails in a publicly shared album correctly opens the image in the viewer. Also downloading the original from there works.

When removing the public share and accessing the file DAV endpoint, i.e. http://localhost:8210/remote.php/dav//photospublic/oMtN1RGlMrJvNdCheowrZ9XNuVqg4obD/413-IMG_8480.jpg I get a full page returned with:

Shouldn't this just be a HTTP error code?

[simonspa]

Right now there is no bruteforce checking anymore, right? So even if an album link does not lead to a correct album returned this is not registered.

Could we add this back?

[starypatyk]

@simonspa

When removing the public share and accessing the file DAV endpoint, i.e. http://localhost:8210/remote.php/dav//photospublic/oMtN1RGlMrJvNdCheowrZ9XNuVqg4obD/413-IMG_8480.jpg I get a full page returned with:

Shouldn't this just be a HTTP error code?

I think that this is the content that gets returned together with 404 error code. Apparently the output depends on the request - most likely based on the Accept header. When invoking an invalid URL with Curl I get (simplified for clarity):

HTTP/1.1 404 Not Found
Server: nginx
Date: Sun, 22 Jan 2023 18:07:07 GMT
Content-Type: application/xml; charset=utf-8
Content-Length: 219

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>Unable to find public album</s:message>
</d:error>

[starypatyk]

@simonspa

Right now there is no bruteforce checking anymore, right? So even if an album link does not lead to a correct album returned this is not registered.

Could we add this back?

Indeed - I have removed it. Some thoughts:

• Given that the token seems to be 192 bits, brute force attack is quite unlikely to be successful. 😉
• I think we should not include it in the PublicAlbumAuthBackend (to avoid wrong error codes like 401), maybe rather in PublicRootCollection - e.g. here:

  photos/lib/Sabre/PublicRootCollection.php

  Line 82 in 167b532

  throw new NotFound("Unable to find public album");

[simonspa]

I think that this is the content that gets returned together with 404 error code.

Alright then. :) I now realize I get the same return also for regularly shared files. 👍

brute force attack is quite unlikely to be successful

That's right, just not sure if it might reduce resource usage if we throttle early instead of querying the DB for every supposed album. But I really have no clue 😄

[starypatyk]

brute force attack is quite unlikely to be successful

That's right, just not sure if it might reduce resource usage if we throttle early instead of querying the DB for every supposed album. But I really have no clue smile

Me neither... 😉 I have not yet figured out how the throttling works. Can try to add it back if suggested.

[marcelklehr]

I think having brute force protection on share endpoints would be good. here's the docs on that https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html#brute-force-protection

[starypatyk]

I have aligned the changes in PublicAlbumContent.vue to the changes done in the Viewer app discussed in the related PR: nextcloud/viewer#1525

I had to revert setting the hasPreview property to false as this would break the proposed solution in the Viewer app.

This would need to be discussed further, if we would like to have this workaround available for currently released and older versions of the Viewer app.

[starypatyk]

I had to revert setting the hasPreview property to false as this would break the proposed solution in the Viewer app.

This would need to be discussed further, if we would like to have this workaround available for currently released and older versions of the Viewer app.

After nextcloud/viewer@4a080e4 the previewUrl has precedence over hasPreview. So it has been possible to once again disable default preview for public albums: d72624d

[szaimen]

fix in viewer was merged and backported

[starypatyk]

@marcelklehr

I think having brute force protection on share endpoints would be good. here's the docs on that https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html#brute-force-protection

In 8d5572b I have put brute force throttling back. As this needs to protect WebDAV requests, this seemed to me like a reasonable place.

This is very similar to the implementation here:
https://github.com/nextcloud/server/blob/41148acf833d401aa6c8bd23617ae8639b6aaae6/apps/dav/lib/Connector/PublicAuth.php#L79

But I am not satisfied with the result. 😞

I have tried to open an invalid URL a few times and the throttling turned on very quickly - I started to get significant delays (10 seconds or more). Unfortunately, this continued even when I finally used a correct URL.

I imagine this might easily happen for regular users - in situations like missing last character when copying a link, someone enabling/disabling sharing of the same album, etc.

Might be reasonable to change the throttling parameters to make it less intrusive. Or maybe resetting throttling when the user provides a correct link.

Thoughts/suggestions?

[artonge]

@starypatyk can you check whether you can implement throttling as suggested by the doc ? https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html#rate-limiting
Then you can configure it to something like (limit=10, period=60) which would probably be enough.
But maybe it only works for controller.

[starypatyk]

@artonge @marcelklehr

I have looked at the code in OC\Security\Bruteforce\Throttler and OC\Security\RateLimiting\Limiter that implement the underlying brute force throttling and rate limiting functions.

TL;DR - I would suggest not to implement either of these mechanisms for public albums.

Here is my reasoning:

1. First of all - these links are intended to be public, so general throttling when the user provides a correct address (i.e. token) should not be applied.
2. Since the tokens are long (192 bits apparently) brute-forcing is highly unlikely to succeed.
3. Current implementation of brute force throttling does not allow to configure throttling parameters. With current parameters the mechanism might be quite annoying when a user makes some simple mistake.
4. The rate limiting mechanism is completely separate from the brute force throttling and does not provide any delay. When the rate is exceeded it just raises an exception.
5. The rate limiting mechanism logs additional information in the database to determine the rate of incoming requests.
6. Due to item #1 above - we must verify the token before even trying to involve the rate limiting mechanism.
7. So - if we add the rate limiting mechanism after verifying the token, we will add additional database load, and the outcome will just be a different kind of exception if the rate is exceeded. We already raise an exception and terminate processing of the request when the token is invalid.
8. Conclusion Leaving brute force throttling might be annoying. Adding rate limiting would increase load on the server (processing and database) without giving any benefits.

If you agree with this, I will revert the last commit: 8d5572b

It is possible to implement a more advanced rate limiter that would also introduce a delay and hopefully reduce the server load. But this seems to be a topic for another PR - likely in the core server code. 😉

[artonge]

Rebased

[szaimen]

/backport to stable25

[szaimen]

@artonge is cypress failure related?

[artonge]

I don't think so. Let's rerun, we'll see

[szaimen]

Thanks a lot @starypatyk for fixing this and @simonspa for the help and @artonge for reviews! :)

[starypatyk]

@szaimen @artonge @marcelklehr

Just one thing - I have not reverted the brute force throttling introduced in commit 7922c69 as discussed in the comment above: #1602 (comment), as I was not sure if it is OK to do so.

Should I submit a separate PR to discuss this?

[artonge]

Should I submit a separate PR to discuss this?

I think it is better to keep it for now.