Bug: User-Specific Cache Expiration w/ _wpnonce

[jaswrks]

Overview

There are some links generated in markup—by WP core, or by plugins like bbPress, and the markup may contain <a> links that have _wpnonce query string variables in them. It is possible for the _wpnonce value to expire before the cache does, and before a user performs an action on the site that would force the cache to clear automatically.

In short, there are certain portions of WP core (and in plugins like bbPress) that output dynamic content without also setting define('DONOTCACHEPAGE', true), because the assumption is that the user is logged-in, and that cache plugins will not cache logged-in users anyway. However, ZenCache does! (Edit: Note that this can also be a problem when users are NOT logged in; see next comment below.)

Steps to Reproduce

• Enable bbPress
• Enable logged-in user caching.
• While logged in as the Admin, load a page that contains a link which has ?_wpnonce in it; e.g., a [stick] or [unstick] topic link.
• Let the _wpnonce expire while still in the cache.
• Click the link to stick or unstick a topic.

Expected Behavior

That sticking or unsticking a topic (or performing some other action) that requires _wpnonce would be performed as expected.

Observed Behavior

There are times whenever the _wpnonce expires before the cache does, and the action fails.

[jaswrks]

A similar issue may also exist in comment forms for users who are logged in (or NOT logged in), whenever you also run the Akismet plugin.

<input type="hidden" id="akismet_comment_nonce" name="akismet_comment_nonce" value="2340846c5f" />

[jaswrks]

This bug was reported by @clavaque at s2member.net.

[jaswrks]

Next Actions (Step 1 of 2)

• New feature branch in the websharks/zencache-pro repo.

• After this line add the following:

  /**
  * No-cache because the current user is logged into the site and the current page contains an `nonce`.
  *
  * @since 15xxxx Enhancing logged-in user caching support.
  *
  * @type string A unique string identifier in the set of `NC_DEBUG_` constants.
  */
  const NC_DEBUG_IS_LOGGED_IN_USER_NONCE = 'nc_debug_is_logged_in_user_nonce';
  
  /**
  * No-cache because the current page contains an `nonce`.
  *
  * @since 15xxxx Enhancing `nonce` detection.
  *
  * @type string A unique string identifier in the set of `NC_DEBUG_` constants.
  */
  const NC_DEBUG_PAGE_CONTAINS_NONCE = 'nc_debug_page_contains_nonce';

• After this line add the following:

  if(preg_match('/\b(?:_wpnonce|akismet_comment_nonce)\b/', $cache)) {
      if (IS_PRO && ZENCACHE_WHEN_LOGGED_IN && $self->isLikeUserLoggedIn()) {
          return (boolean) $self->maybeSetDebugInfo(NC_DEBUG_IS_LOGGED_IN_USER_NONCE);
      } else {
          return (boolean) $self->maybeSetDebugInfo(NC_DEBUG_PAGE_CONTAINS_NONCE);
      }
  }

• After this line add the following:

  case NC_DEBUG_IS_LOGGED_IN_USER_NONCE:
      $reason = __('because the current page contains `_wpnonce` or `akismet_comment_nonce`. While your current configuration states that pages SHOULD be cache for logged-in visitors, `*nonce*` values in the markup (because these expire automatically) are not cache-compatible.', SLUG_TD);
      break; // Break switch handler.
  
  case NC_DEBUG_PAGE_CONTAINS_NONCE:
      $reason = __('because the current page contains `_wpnonce` or `akismet_comment_nonce`. Note that `*nonce*` values in the markup (because these expire automatically) are not cache-compatible.', SLUG_TD);
      break; // Break switch handler.

• Submit PR.

---

On Deck (Step 2 of 2)

Take a closer look at the impact that this new auto-exclusion will have on comment forms whenever Akismet is running. While caching nonce values is definitely problematic, so is excluding every post that contains a comment form! We need to take a closer look and determine what Step 2 should look like here.

[jaswrks]

Assigning this to @kristineds :-)

[jaswrks]

@raamdev Thoughts on Step 2 of 2, and on this issue in general?

[raamdev]

@jaswsinc Hmm, my first reaction after reviewing the details of this issue is that we should treat this the same way we treat issues caching dynamic content generated by any other WordPress plugins: If the plugin doesn't set DONOTCACHEPAGE, then your only other option is to exclude any pages with the dynamic content from being cached using the URI Exclusion patterns.

However, I hesitate to say that's my recommendation as we're talking about certain portions of WP core, Akismet, and bbPress, all of which deserve more consideration IMO.

How do other WordPress caching plugins handle this issue (if they do at all)? Initially, it sounded like this issue was specific to logged-in users (which would explain how other caching plugins, which don't do logged-in user caching, are able to avoid this issue), but you mentioned this would be a problem for non-logged in users replying to a comment form on a site running Akismet. I imagine WP Super Cache (maintained by the same company that makes Aksimet) must have a way around this?

[jaswrks]

A quick search of the codebase turns up nothing for "akismet", and very little for "nonce". So nothing jumping out at me right away that would deal with this issue in WP Super Cache.

Referencing:

• https://github.com/Automattic/wp-super-cache/search?utf8=%E2%9C%93&q=akismet
• https://github.com/Automattic/wp-super-cache/search?utf8=%E2%9C%93&q=nonce

[raamdev]

I'm wondering why this hasn't been reported as an issue with ZenCache yet. Do you suppose that's because this only affects Logged-In User Caching and that option is disabled by default?

Or maybe when Logged-In User Caching is enabled, we're still disable caching for users who post a comment (i.e., the comment cookies are still disabling ZenCache)? I seem to recall working on something related to Logged-In User Caching + Comment Cookies in the recent past...

[jaswrks]

I'm wondering why this hasn't been reported as an issue with ZenCache yet. Do you suppose that's because this only affects Logged-In User Caching and that option is disabled by default?

I had the same thought. It does seem like an edge case though. It may not impact a site owner, only commenters, and only if the cache expiration time is lower than the _wpnonce timeout, and only if they are: a) caching users, or b) using Akismet, or c) using another plugin that requires a _wpnonce to be submitted by a form that is cached.

I keep looking at this issue hoping that I'm missing something, and that maybe this is actually a non-issue, but each time I think about it, I keep coming back to cached _wpnonce values being a problem. The most problematic use of these that I can think of is with comment forms; e.g., akismet_comment_nonce seems like a concern when it comes to caching.

In the case of logged-in users, we can simply disable caching on pages that contain a _wpnonce or akisment_comment_nonce. It's a problem, but nothing that we can't work around. However, when it comes to akisment_comment_nonce, that looks to me like it is simply not cache-compatible. That's dynamic content like any other dynamic content. The akisment_comment_nonce value may expire before the cache does.

[raamdev]

Is there any way to figure out when an akisment_comment_nonce value would expire and then dynamically set the expiration time for a cache file containing that akisment_comment_nonce to something lower?

Or would really fixing this require solving #222?

[jaswrks]

Is there any way to figure out when an akisment_comment_nonce value would expire and then dynamically set the expiration time for a cache file containing that akisment_comment_nonce to something lower?

There isn't at the moment, but that seems possible to do if we wanted to. However, I don't see that being bulletproof either, because an nonce can also be associated with a specific context or action; e.g., a specific IP address or a specific type of action that is being performed. While that might work OK in the case of Akismet (I suspect it will), it may not cover all use cases in other plugins that use nonce values.

See: https://codex.wordpress.org/Function_Reference/wp_create_nonce

I like where you're going with this though. Maybe a short-term solution would be to force an overall cache expiration time that is lower than the nonce timeout. We should research if that's even safe to do though. It seems like a potential security issue to me.

---

Or would really fixing this require solving #222?

That would help us, but I think the discussion in that issue is not going anywhere at the moment. Others have tried that in the past and failed, and each time I look at that I cringe. I can't see us actually implementing something like that. It's just asking for security problems and other headaches IMO.

[raamdev]

Then maybe the best way to handle this would be to detect when certain well-known plugins that use nonce's are active on a site and then make specific recommendations about how the Cache Expiration Time should be adjusted (e.g., if Akismet is installed, suggest a Cache Expiration Time of 24 hours)?

It's not a bulletproof solution, as you said, but it seems to me like the default Cache Expiration Time of 7 days might be the thing that causes the most trouble with this issue.

[jaswrks]

Taken from this post...
https://make.wordpress.org/core/2010/12/06/as-was-discussed-in-the-last-dev-chat-i/

Passive comment nonce
The plugin adds a nonce field to the comment form. The result of the nonce check is then passed to Akismet to help look for abuse.

So it's not a show-stopper, but it could be adding to the overall spam score whenever it is a cached value that is being submitted. That explains why we haven't seen this become a major issue.

There was also a conversation about how this might impact caching plugins. See: https://make.wordpress.org/core/2010/12/06/as-was-discussed-in-the-last-dev-chat-i/#comment-4077

I'm also seeing that there is a filter for this in the Akismet plugin. We might consider resolving this problem by disabling this particular spam check whenever the page will be cached. Referencing: https://github.com/git-mirror/wordpress-akismet/blob/7e392b11d991be602020dc6c92f98934bb2f172e/akismet.php#L333

Removing that check would remove the nonce from the comment score equation.

[raamdev]

We might consider resolving this problem by disabling this particular spam check whenever the page will be cached.

Interesting. Great find with those links! Yep, I'd say disabling that particular spam check might be the best route to go here. I'd couple a new configurable option with a Dashboard message when Akismet is active, and a link to a KB Article that explains things in detail.

[raamdev]

Next Pro Release Changelog:

• bbPress Compatibility: This release improves compatibility with bbPress when ZenCache Logged-In User caching is enabled. In this scenario, bbPress may generate links for Admins that contain time-sensitive _wpnonce values which could expire if cached and result in certain admin-related actions failing. ZenCache no longer caches pages that contain _wpnonce in the markup. This ensures that pages containing time-sensitive nonce values are not cached. Props @kristineds, @jaswsinc, and @clavaque. See Issue #601.
• Akismet Compatibility: ZenCache no longer caches pages that contain akismet_comment_nonce in the markup. This ensures that a page that contains a time-sensitive nonce value does not get cached. Props @kristineds and @jaswsinc. See Issue #601.
• Akismet Compatibility: ZenCache now automatically disables the Akismet Comment Nonce using the akismet_comment_nonce filter, which improves compatibility between Akismet and the page caching functionality provided by ZenCache. This ensures that pages do not contain time-sensitive nonce values that should not be cached. If you'd like to revert this behavior, please see this article. Props @kristineds and @jaswsinc. See Issue #601.
• Bug Fix: ZenCache no longer caches any pages that contain _wpnonce in the markup. This ensures that pages containing time-sensitive nonce values are not cached. Props @kristineds and @jaswsinc. See Issue #601.

[raamdev]

ZenCache Pro v151220 has been released and includes changes worked on as part of this GitHub Issue. See the release announcement for further details.

---

This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#601).