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

Browser cache issues with user login #264

Closed
4 tasks done
NicoHood opened this issue Sep 10, 2020 · 22 comments
Closed
4 tasks done

Browser cache issues with user login #264

NicoHood opened this issue Sep 10, 2020 · 22 comments
Assignees

Comments

@NicoHood
Copy link

NicoHood commented Sep 10, 2020

I am not sure if my issue is related to some others, so I opened a new Bug.
grav 1.6.26
login 3.3.5

I am using the php command to host the website locally.

I've added the oauth2 plugin, to allow users to signin via github. I am at a very early stage, just trying things out. Logging in and logging out works, but when I change to another page, the user is still shown as logged in. The quark theme has an option to show the logged in user at the top, and he is still shown. If I press shift+reload (in firefox) the site is loaded properly. I've turned off cache for the site, and system wide.

How can that happen, and why does a browser full reload 'fix' the issue?

Edit:
I've noticed that the error happens

  • Firefox 80.0.1 (the one I am always using)
  • Developer Firefox (newly installed, no settings changed)
  • Chromium (Everything works fine)
  • Midori (Everything works fine)

I've also seen this issue before while working on some query parameters. Firefox seems to cache more aggressively, and does not invalidate websites. But is this really a firefox issue?

But how to debug this now?

Steps to reproduce:

  1. Use Grav Quark Blog Skeleton
  2. Install oauth2 plugin bin/gpm install login-oauth2
  3. Add github as authentication provider (client and secret id)
  4. Start server with php -S localhost:8000 system/router.php
  5. Go to your page http://localhost:8000/login
  6. Login via github
  7. Click the logout button
  8. You will see, that the logout does not happen. Only if you either wait a few seconds or reload the page using shift + reload.
@NicoHood
Copy link
Author

This is a screenshot of the firefox network analysis. It shows a pragma: no-cache (which is probably respected by chrome, but not firefox) and the cache-control: max-age=604800.

grafik

If I set the cache control to nocache, the site works properly. However it will always reload... and that is not the goal. Using etag also does not help. I am not sure if this is an issue of grav core or the login/oauth plugin itself. I've noticed similar issues with normal twig templates that did not change, even if I've edited them (with file cache turned off).

pages:
  cache_control: 'no-cache'

@mahagr
Copy link
Member

mahagr commented Sep 11, 2020

Try something like this:

pages:
  expires: 3600
  cache_control: 'no-store, no-cache, must-revalidate'
  etag: true

This will force the browser to send a request to the server but enables etags with 1 hour expiration time. Basically this makes Grav render the output, but if the content it identical to what browser has, it'll send 'Not Modified', thus speeding up the loading times.

@NicoHood
Copy link
Author

NicoHood commented Sep 11, 2020

Thanks for helping me out!

The issue is 'fixed', but cache is not working anymore. The browser reloads the whole page. I'd expect a 304, but I always get a 200 on a page refresh:
grafik

This happens only on firefox. Not on chromium and midori.

Correct me, if I am wrong.

The issue is very simple to reproduce with the provided steps, in case you need a minimal example to test yourself.

@NicoHood
Copy link
Author

Important update:
I've tried with Firefox 78 and the issue does not occur. Firefox 80.0.1 has the issue.(on Archlinux) But how should we proceed now?

@mahagr
Copy link
Member

mahagr commented Sep 11, 2020

I'm aware that this change was made in FF 80, but I think it's just good thing as it basically acts like you're behind caching proxy (which used to cause the same issue, but most people didn't notice it).

Try updating Grav to 1.7, it has some important improvements on 304 responses.

@NicoHood
Copy link
Author

NicoHood commented Sep 11, 2020

Could you maybe go more into detail about what changed? It would be nice, to also solve this issue with grav 1.6 and this should be added to the readme. If the is an intended behavior, you will see more confused users soon.

The upgrade did not work for me:

Would you like to upgrade now? [y|N] y

Preparing to upgrade to v1.7.0-rc.16..
  |- Downloading upgrade [6.27M]...   100%
  |- Installing upgrade...    ok                             
  '- Success!  

PHP Fatal error:  Uncaught Error: Call to undefined method Grav\Common\Page\Pages::register() in /home/user/Documents/grav-skeleton-blog-site/system/src/Grav/Common/Processors/InitializeProcessor.php:326
Stack trace:
#0 /home/user/Documents/grav-skeleton-blog-site/system/src/Grav/Common/Processors/InitializeProcessor.php(139): Grav\Common\Processors\InitializeProcessor->initializePages()
#1 /home/user/Documents/grav-skeleton-blog-site/system/src/Grav/Common/Processors/InitializeProcessor.php(46): Grav\Common\Processors\InitializeProcessor->processCli()
#2 /home/user/Documents/grav-skeleton-blog-site/system/src/Grav/Console/ConsoleCommand.php(66): Grav\Common\Processors\InitializeProcessor::initializeCli()
#3 /home/user/Documents/grav-skeleton-blog-site/system/src/Grav/Console/ConsoleCommand.php(107): Grav\Console\ConsoleCommand->initializeGrav()
#4 /home/user/Documents/ in /home/user/Documents/Sport/BikeFitting/Haendler/grav-skeleton-blog-site/system/src/Grav/Common/Processors/InitializeProcessor.php on line 326

And even when I ignore those errors, my issue is still there. I also tried downloading a clean rc1.7 release and add the skeleton user folder - same result. So no solution for 1.6 nor 1.7 :-S

@rhukster
Copy link
Member

We'll continue to investigate this...

@mahagr
Copy link
Member

mahagr commented Sep 14, 2020

I'm able to reproduce this issue.

@mahagr
Copy link
Member

mahagr commented Sep 14, 2020

I fixed part of this, because of login/profile (and admin login) pages were also cached into the browser. Those have now been fixed as they should never be cached.

What is not fixed yet are the other pages with user/login information in them and login/access protected pages.
Also CLI upgrade needs to be looked at.

@NicoHood
Copy link
Author

NicoHood commented Oct 3, 2020

Did you manage to also fix the other pages?

@mahagr
Copy link
Member

mahagr commented Oct 5, 2020

We are still working on the fixes as we're finding more cases.

@NicoHood
Copy link
Author

I just noticed, that also the messaging system is affected.

I wrote a plugin that requires the user to click an activation link via email. If he already klicked on the link, a message will pop out. But the message will not be shown unless you reload the page. I am not yet sure if this requires a fix on grav itself, or if I can manually clear the cache for the user?

@NicoHood
Copy link
Author

NicoHood commented Oct 25, 2020

It turns out, that the following setting works for me in the message scenario. It does proper caching (at least for the html file, not css and js) but also reloads if the page content changed.

  expires: 3600  
  cache_control: 'private, max-age=0, must-revalidate'
  etag: true

The setting was taken from here. I have little knowledge about this, maybe you can tell me more if that is correct. Also I am wondering if there should be a mechanism to set the cache control to private for logged in users. Also for the messaging system there must be a better public/private mechanism. It would be best to set the page/html cache to private by default?

Also how can I apply the cache headers for css and javascript files? It looks like those are not cached at all. Those could be cached public I guess.

@rhukster
Copy link
Member

@mahagr I think we should look into changing the cache_control for logged in users.. its something we could do from the login plugin.

@mahagr
Copy link
Member

mahagr commented Oct 27, 2020

Yes, I agree. Though that may not be enough as the pages which exist for the guests are still going to be an issue -- they will be cached for a week by default.

And yes, this will also be an issue with the system messages.

@NicoHood
Copy link
Author

NicoHood commented Oct 27, 2020

Nice! So I guess we found the root issue, did we? I also aggree on @mahagr that existing pages will still be cached. Since grav (and its themes) are built with messaging enabled by default it would make sense to change the default configuration to:

  expires: 0
  cache_control: 'private, max-age=0, must-revalidate'
  etag: true

This would

  • Still allow caching (etag is correctly used)
  • Fix login plugin and messages caching issue
  • Fix private information leak when a proxy is used (very important)

When this becomes a new default, static pages or static websites (without any login or message function) could use a more aggressive caching at their own risk. This should be documented for grav, the login plugin and of course in the changelog, so people will understand better.

You recent changes to the login plugin maybe could be reverted then.

What I am not yet sure is the expire setting, if it would still matter whether it is set or not. I am also (not yet) an expert about the pragma cache settings.

@NicoHood
Copy link
Author

NicoHood commented Nov 29, 2020

Is there a chance to make the cache setting suggested above the new default for 1.6/1.7? I think this is critical, especially the private setting is currently not applied, meaning proxy server will leak private information when a user login is used. It basically does not make sense to enable the session by default, but not having private cache headers (and etag support off).

@mahagr mahagr changed the title Cache issues with user login Browser cache issues with user login Dec 2, 2020
@mahagr mahagr self-assigned this Dec 22, 2020
@mahagr mahagr added the bug label Dec 22, 2020
mahagr added a commit that referenced this issue Dec 22, 2020
@mahagr
Copy link
Member

mahagr commented Dec 22, 2020

@NicoHood Now all the protected pages should have proper cache control. I made sure the pages can still be cached in the browser if ETag is enabled to speed up loading protected pages when they haven't been changed.

There is still an issue with system messages, which needs to be fixed elsewhere. Same is true with forms and their redirect pages.

@mahagr mahagr added the fixed label Dec 22, 2020
@mahagr
Copy link
Member

mahagr commented Dec 22, 2020

I'm closing this issue as the rest isn't related to login.

@mahagr mahagr closed this as completed Dec 22, 2020
@NicoHood
Copy link
Author

Where are the other issues tracked?

Wouldnt it still make sense to set some different default values? Or at least put a proper documentation/recommendation on cache strategies? I am wondering if fixing the cache works for messages etc. I am not yet sure what is a) the most common usecase and b) the best choice that does not break things

@mahagr
Copy link
Member

mahagr commented Dec 22, 2020

Session messages: getgrav/grav#3108

I didn't create issue for forms, though.

I don't think that the default values need to be changed. For protected pages I am just forcing override for the Cache-Control as there's really no other way to prevent caching. It still allows you to set expire time, though.

@NicoHood
Copy link
Author

I guess this commit is now related:
getgrav/grav@589c9e4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants