v160514-RC: Admin Toolbar disabled on front-end for Admin Users

[raamdev]

This GitHub issue started with the title Admin Toolbar disabled on front-end for Admin Users to discuss whether or not the Admin Toolbar should be disabled for Administrators on the front-end, however the discussion forked into one about Nonce caching and whether or not we should allow Nonce's to be cached.

It was then decided that Nonces could be cached safely (with a few caveats) and a plan was formulated to work towards that goal. That decision obviates the need to discuss whether or not the Admin Toolbar should be disabled for Administrators on the front-end, as allowing Nonce caching would mean that the Admin Toolbar could also be cached, i.e., there's no need to disable the Admin Toolbar at all.

This issue has been closed and further discussion about Allowing Nonce caching has been moved to #793.

---

Original Issue

When Logged-In Users caching is enabled, the Admin Toolbar is disabled by default on the front-end of the site. This applies to Administrator users as well, i.e., the Admin Toolbar is disabled for them on the front-end as well.

The reason for this is that the Logged-In Users caching feature itself applies to Administrators as well, so if it's expected that Comet Cache will cache the front-end of the site for Administrators, then it makes sense that the Admin Toolbar would be disabled on the front-end of the site for Administrators, since the reason we're disabling the Admin Toolbar in the first place is to prevent Nonce values from appearing in the page source and thereby disabling caching (which defeats the purpose of enabling Logged-In Users caching).

So, should the Admin Toolbar be disabled for Administrators on the front-end? Or maybe we should have three options?

• No, leave the Admin Bar disabled for logged-in users EXCEPT Administrators (recommended option).
• No, leave the Admin Bar disabled for ALL logged-in users
• Yes, enable the Admin Bar for logged-in users

I'd love to hear what others think here.

---

Forked from #756 (comment).

[KTS915]

@raamdev My first preference is that this just shouldn't be handled by Comet Cache at all. It seems to me that this is an issue that goes to heart of different user roles, and that is something that is beyond the scope of a caching plugin.

If it were to be included in a WebSharks plugin, then I'd have thought that s2Member would be more appropriate. But I'm doubtful even then. There's a very good plugin called Admin Bar Disabler that already handles all this very well (and there are others too -- see below), and I like the idea of keeping different functionality in different plugins.

If, nevertheless, you choose to keep this in Comet Cache, then I would definitely want to have the option of having the adminbar enabled for admins while disabled for others, so your menu of three options looks appropriate.

Which takes me to the issue that has manifested with my testing of the RC. In fact, it turns out that the adminbar option is causing a conflict with a setting in the plugin Clean Login. I use this for all my logging-in and out and password reset functions, and it comes with an option to "Hide admin bar for non-admin users," which is what I have set.

If I turn this option off in Clean Login, then the Comet Cache Pro options page functions as expected.

Again, I'd prefer that Comet Cache just doesn't include this option. But, if it does, then I'd certainly want to get back the functionality that Clean Login already gives me.

[raamdev]

The problem with not including the ability to disable the toolbar is that the Logged-In Users caching feature becomes useless, unless you install another plugin (that can disable the toolbar bar) or unless you manually disable the toolbar on each of your users profiles.

So while I'm all for keeping unrelated functionality out of Comet Cache, my feeling is that if we're going to support Logged-In Users caching, we have to make the feature actually work (i.e., actually cache logged in users). Without disabling the toolbar, Logged-In Users caching does not work, i.e., Comet Cache just reports that it can't cache the page because of nonce values present in the source, which makes Comet Cache look broken.

If we did remove anything that controls the toolbar, then we'd need to do a lot more messaging that informs site owners of the need to disable the Toolbar if they want users to be cached. Then we could point them to another plugin, like Admin Bar Disabler. I'm not opposed to this taking this route, but I'd love to hear @jaswsinc's thoughts.

[KTS915]

@raamdev I see. Yes, that is a problem.

Another suggestion, then, for you and @jaswsinc perhaps to consider, is keeping this as it is, but defaulting to leaving the adminbar enabled. You could then add an explanatory message as to why the setting might need to be changed to disable the adminbar.

As it is, if someone, like me, already has something already activated to deal with this, then it comes as a bit of a shock to find things going haywire! But if Comet Cache defaults to leaving the adminbar on, then there is no conflict with any other plugins that might already be activated to handle this.

[raamdev]

@KTS915 I agree. If defaulting the Admin Bar to disabled in Comet Cache is conflicting with other plugins like Clean Login, then I would rather err on the safe side and leave it enabled, with a new message that appears in the HTML Notes when User Caching is enabled but an Admin Bar is preventing it from being cached.

[jaswrks]

I was just giving Nonce ticks some more thought. I also reviewed this article again, because the article came about as a result of our last conversation about Nonces and caching in WordPress.

It seems to me that this part of the article is at the heart of what this issue addresses, because the reason we are disabling the admin bar to begin with (I think we'd all like to avoid doing that in a caching plugin) is to prevent the conflict with Nonces that are present in the admin bar.

In this article, it was determined that caching Nonce values globally is dangerous, but user-specific caching not so much, because the Nonce values are cached for each user; i.e., not cached in a way that would ever expose them to others. Therefore, the only reason to not cache Nonces for logged-in users is to avoid a situation where an Nonce would go stale and not work as expected for the user it was created for. A WordPress Nonce has a minimum lifespan of 12 hours.

With that in mind, we know that a site owner could allow Nonce values to be cached, and thus, allow the admin bar to be shown (or anything else that uses Nonces), so long as those user-specific cache files are never allowed to live longer than 12 hours.

I didn't always feel this way, but at this point (after having seen the problems that our current Nonce stance creates), it seems to me that it might be easier for site owners if we simply enforce this 12 hour maximum automatically whenever user-specific caching is enabled; i.e., force the Cache Expiration Time to no longer than 12 hours when user-specific caching is enabled and a user-specific cache is generated.

That way things like the Meta widget in WordPress (it uses an Nonce too), the admin bar, bbPress, BuddyPress, etc... will not prevent user-specific caching from working as originally intended; i.e., they won't require any additional consideration in this regard.

So long as Comet Cache is smart enough not to serve a user-specific cache file that is older than 12 hours, Nonces are not a problem in a user-specific context, and therefore the admin bar is no longer an issue that needs to be dealt with in Comet Cache—no matter which Role you have.

[jaswrks]

... thoughts continued ...

If we go that route, we should also do another full security review of the Comet Cache plugin and the architecture behind user-specific caching in general. From a security standpoint, I don't like putting Nonce values into a cache file and storing them. However, this is a case where it does seem more practical for us to do that. So if we do decide to take this approach, we should be more and more careful about how secure the cache directory is. That's already something we consider, but when we knowingly cache a user-specific security token (e.g., Nonce) we must also assume responsibility for protecting it every way that we can; i.e., take some additional precautions.

[KTS915]

@jaswsinc If that would mean, as I think it would, that user-specific cache files could not be older than 12 hours, then I'd find that very disappointing. I have sites where members will revisit posts regularly over several months. The posts don't change at all over that time unless a comment is made, in which case the cache would be flushed automatically anyway.

The way that caching of posts with nonces now work is causing no problems for me, and is very useful.

[jaswrks]

user-specific cache files could not be older than 12 hours, then I'd find that very disappointing.

I see. Hmm, well one thing we could do, if that route becomes something Raam wants to explore, is to enforce that 12-hour maximum only on cache files that are known to contain Nonce values. That way it won't impact user-specific caching in such a broad way.

The unfortunate circumstance is that any cache file that contains an Nonce must either: a) not be cached to begin with; or b) expire after 12 hours. Otherwise there is a risk of the Nonce values becoming stale, and this can lead to extremely mysterious bugs that most site owners would not even realize existed, and yet functionality on the site would fail for users at random.

---

Noting also that I found a few similar discussions out there like this one. This one in particular does a good job of explaining this problem greater detail: http://myatus.com/p/wordpress-caching-and-nonce-lifespan/

[KTS915]

one thing we could do ... is to enforce that 12-hour maximum only on cache files that are known to contain Nonce values.

I am not sure that would help much because if a site has a logout link on every post and page (as mine does) then there will also be a nonce there.

Thanks for posting to the article. But what I don't understand from that is why it matters if a nonce is cached in a cache created for that specific user. That means, after all, that there's no chance of getting users mixed up.

[raamdev]

I agree this needs some more thought. If we can cache Nonce values safely, that sounds like the best route.

We're in the middle of a Release Candidate, so I'd rather not try to change things too drastically at this point.

Here's what I'm going to do to get the next release out: I'm going to change the default for this new setting so that it doesn't touch the Admin Toolbar by default. If a site owner wants to disable the Admin Bar using Comet Cache, they can flip the setting in the Logged-In Users panel, but for now I'm going to make this disabled by default so that if a site owner is already using a plugin to disable the Admin Bar, Comet Cache won't introduce a conflict (as @KTS915 experienced with Clean Login).

Once we're through this release cycle, we can revisit this issue and decide if and how we're going to change the Nonce caching behavior.

[jaswrks]

Once we're through this release cycle, we can revisit this issue and decide if and how we're going to change the Nonce caching behavior.

Sounds like a plan. Thanks for reviewing.

[raamdev]

Relevant changes coming to the Admin Toolbar on the front-end in WP v4.6: https://make.wordpress.org/core/?p=18245

I think this means we'll need to be even more careful if we decide to allow caching the Toolbar.

[raamdev]

@KTS915 writes...

what I don't understand from that is why it matters if a nonce is cached in a cache created for that specific user. That means, after all, that there's no chance of getting users mixed up.

The problem is that if a Nonce is cached for longer than its lifetime, the user ends up being presented with a cached page that contains an invalid (expired) Nonce value. What does that mean for the user? It means that when they click a link or submit a form that contains that (expired) Nonce value, whatever they were trying to do won't work.

See the following from the WordPress Codex:

The invalid nonce causes WordPress to send a "403 Forbidden" response to the browser, with the error message: "Are you sure you want to do this?"

Caching Nonce values safely

From what I've gathered, the only real way to cache Nonce values safely is to do the following:

1. Ensure that the cache file is being served to a user for whom the Nonce value was created (which means only Logged-In User caching in the context of Comet Cache). The only way Comet Cache can know that the cache file being served belongs to the visitor it's being served to is via Logged-In User caching.
2. Ensure that the user-specific cache files that contain Nonce values are not publicly accessible, otherwise attackers could find Nonce values in the cache files and attempt to exploit them. We need to take into consideration sites running Nginx (where .htaccess rules are not possible) and sites where we may be unable to write an .htaccess file to protect files in the cache from being publicly accessible. We may not need to worry about this; see more on this below.
3. Ensure the cache file that contains a Nonce value does not stay around for longer than the minimum lifetime of a WordPress Nonce, which is 12 hours. If it stays around for longer than 12 hours, then the user who loads the cache file will find themselves clicking buttons and submitting forms that result in an error due to the expired Nonce value.

Points 1 and 3 we can control: We can only Nonce caching only for Logged-In Users and we can set an expiration date on cache files that contain Nonce values to 12 hours. Point 2 is somewhat out of our control, however that may not be a problem.

@jaswsinc writes...

if we do decide to take this approach, we should be more and more careful about how secure the cache directory is. That's already something we consider, but when we knowingly cache a user-specific security token (e.g., Nonce) we must also assume responsibility for protecting it every way that we can; i.e., take some additional precautions.

The WordPress Codex says to "always assume Nonces can be compromised" and that "Nonces should never be relied on for authentication or authorization, access control." If we go by that, then we don't need to worry about making sure that cache files are not publicly accessible just because they may contain a Nonce value. (I agree, however, that we should still try to make sure user-specific cache files are not publicly accessible, as those will probably contain user-specific information that a site owner would not want to expose.)

Improving the way Comet Cache handles Nonces

Here's what I propose we do to improve the way Comet Cache handles WordPress Nonces:

• Allow caching of Nonce values by default for Logged-In Users when Logged-In Users caching is enabled; a new option should be added to the Logged-In Users option panel to enable/disable this functionality if desired. Caching will remain disabled for users who are not logged in (as it is now).
• When Nonces are cached, the cache files that contain Nonces get an expiration time of 12 hours (other cache files get whatever expiration date is set in the Options). A new filter should be added that allows a site owner to override this 12-hour cache file expiration if they choose to do so.
• A note should be added to the Comet Cache notes for that cache file indicating that a Nonce value was detected and hence the cache expiration time has been set to 12 hours (or whatever overridden value was set using the filter).

[jaswrks]

@raamdev writes...

Here's what I propose we do to improve the way Comet Cache handles WordPress Nonces:

I went over this carefully and it sounds like an excellent plan to me. With that in place we can stop removing the admin bar for logged-in users.

---

@raamdev writes...

A new filter should be added that allows a site owner to override this 12-hour cache file expiration if they choose to do so.

I suggest not allowing this to be overridden, even with a filter, because anyone changing this will be going down the wrong path, without question. Of course, that's their right, but if they really want to do that they can alter the source code. Exposing the filter suggests it's OK to do it, when it's not.

[jaswrks]

I guess it would be OK to reduce the time, as long as it's not allowed to be longer than 12 hours.

[raamdev]

Exposing the filter suggests it's OK to do it, when it's not.

Yep, I agree.

I guess it would be OK to reduce the time, as long as it's not allowed to be longer than 12 hours.

Right. That sounds fine to me as well.

Related to this: If the Cache Expiration Time in the Comet Cache settings is lower than 12 hours, then the Nonce cache expiration should follow that. E.g., if we set the Cache Expiration Time to 6 hours (in the Comet Cache options), then the cache files that contain Nonces should expire in 6 hours as well, otherwise we will be creating cause for confusion.

[jaswrks]

then the Nonce cache expiration should follow that.

I see. Yep.

[raamdev]

Closing Summary

This GitHub issue started with the title Admin Toolbar disabled on front-end for Admin Users to discuss whether or not the Admin Toolbar should be disabled for Administrators on the front-end, however the discussion forked into one about Nonce caching and whether or not we should allow Nonce's to be cached.

It was then decided that Nonces could be cached safely (with a few caveats) and a plan was formulated to work towards that goal. That decision obviates the need to discuss whether or not the Admin Toolbar should be disabled for Administrators on the front-end, as allowing Nonce caching would mean that the Admin Toolbar could also be cached, i.e., there's no need to disable the Admin Toolbar at all.

This issue has been closed and further discussion about Allowing Nonce caching has been moved to #793.

[KTS915]

@raamdev, @jaswsinc: I'd just like to record my thanks to you both on this issue! Thanks very much!

[raamdev]

@KTS915 Thanks for all of the feedback! :-)