Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(settings): Add setup check for too much caching #40960

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Oct 18, 2023

Summary

Warn admins if their server caching is caching responses that should not be cached.

The approach is simple:

  • Two requests with the same URL
  • Make sure there is a Set-Cookie response header
  • Return something unique, so uniqueness of the responses can be checked

If the two responses are identical, then the server didn't process the requests individually.

TODO

  • Code
  • Testing

Checklist

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst added 2. developing Work in progress feature: settings feature: caching Related to our caching system: scssCacher, jsCombiner... labels Oct 18, 2023
@ChristophWurst ChristophWurst self-assigned this Oct 18, 2023
@@ -94,6 +95,8 @@
use OCP\Security\Bruteforce\IThrottler;
use OCP\Security\ISecureRandom;
use Psr\Log\LoggerInterface;
use function setcookie;
use function time;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timefactory? :D

Copy link
Member Author

@ChristophWurst ChristophWurst Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constructor has like 37 injected services and I didn't want to touch that beast

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Oct 18, 2023

I can't get my local nginx to cause the issues reported in #40863. When I turn on asset caching, all resources give an HTTP404.
@joshtrichards were you able to reproduce the issue yourself?

Edit: set up nginx proxy manager for my dev env. Even with asset caching I'm not able to trigger a cached response that has set-cookie headers.

@solracsf solracsf added this to the Nextcloud 28 milestone Oct 27, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@ChristophWurst ChristophWurst added 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 2. developing Work in progress labels Nov 2, 2023
@ChristophWurst ChristophWurst removed this from the Nextcloud 28 milestone Nov 2, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Nov 21, 2023
@blizzz blizzz mentioned this pull request Nov 22, 2023
5 tasks
@ChristophWurst ChristophWurst removed this from the Nextcloud 28 milestone Nov 22, 2023
@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@joshtrichards
Copy link
Member

joshtrichards commented Feb 5, 2024

Edit: set up nginx proxy manager for my dev env. Even with asset caching I'm not able to trigger a cached response that has set-cookie headers.

I haven't managed an account takeover yet, but with NPM + their Cache Assets enabled there are behavior changes that are red flags:

  • if I use Web Files and trigger a WebDAV GET (e.g. /remote.php/dav/files/ncadmin/Photos/Nextcloud%20community.jpg) by using Download it'll download the first time in the browser then subsequent additional download requests of the same image will trigger a basic auth popup (session cookie rotation on the Nextcloud side and the cache ignoring it?)
  • if I cURL that same WebDAV URL after the browser download, I get the same cookie as appears in the browser (!) and the cookies don't change ever again (!) for subsequent cURL requests of that URL [doesn't matter I'm not providing any authentication information to cURL hah]
  • random in-browser log-outs out of nowhere occur (might be the source of some of the mysterious recurring session management and log-in/log-out problems that get reported or pop up on the forum)
  • I can replicate issues like [Bug]: TypeError: OCA\UserStatus\Controller\UserStatusController::__construct() Argument #3 ($userId) must be of type string, null given #42156

As soon as I turn off NPM's caching things go back to normal and I can't replicate any problems. Some of these behaviors seem to be timing specific and/or depend on which session connected first (which makes sense intuitively).

I think a strong generic inappropriate caching check is a good idea for us.

This type of caching contradicts our recommended config and randomly appears to breaks stuff. That's enough for me to justify a check.

Asides:

  • Probably also introduces security problems (account takeovers like [Bug]: A user accesses the administrator account without doing anything special. #40863 and/or inappropriate protected asset access). I have yet to demo that firsthand, but given another cup of coffee or two the evidence suggests that being possible (though we may have enough countermeasures on our side these days that it's more challenging than it appears; not sure it even matters give the breakage introduced already).
  • I can't figure out the intended use case for NPM's Cache Assets option. It is too aggressive for both public and private caching IMO. And it's not documented anywhere I can find (and no original PR; it was brought in through the original initial commit). I suspect most people just activate it thinking "Caching = better performance!"

@skjnldsv skjnldsv added 2. developing Work in progress and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Feb 21, 2024
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress feature: caching Related to our caching system: scssCacher, jsCombiner... feature: settings
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

[Bug]: A user accesses the administrator account without doing anything special.
6 participants