[Security] Due to the default caching of POST requests personal information can be leaked

[craigpearson]

We've just had a GDPR issue with WooCommerce and Trellis, based on the trellis docs here: https://docs.roots.io/trellis/master/fastcgi-caching/#example-cache-configurations

Specifically for WooCommerce you assume that by adding the skip_cache_uri parameters for WooCommerce is enough, however, POST data can still be stored on another URL, and recalled on an uncached page.

As a result although our checkout page was uncached, the way that WooCommerce outputs data on the checkout page, is to check for POST data containing customer information, if that data exists it outputs it. This is only visible as an issue if you are making requests within the cache window specified.

That POST data can be set on other WooCommerce pages not included in the skip_cache_uri, such as checkout completion.

This could also be a problem for other plugins taking personal data via POST such as Gravity Forms etc. Although Trellis can't account for a users specific configuration, should someone not set a URI where POST data is set in the skip_cache_uri then that data is cached by default and potentially leaked to the world

The caveat to this is, that setting a default of not caching any POST data could result as a bypass to any DDoS protection via POST requests, however this seems like a safer default given the data POST can contain. To get around DDoS concerns we could potentially add an option to enable caching of POST data, but with a warning / disclaimer

[swalkinshaw]

Wow I'm surprised this has never come up, thank you.

I agree this should be the default.

The caveat to this is, that setting a default of not caching any POST data could result as a bypass to any DDoS protection via POST requests, however this seems like a safer default given the data POST can contain. To get around DDoS concerns we could potentially add an option to enable caching of POST data, but with a warning / disclaimer

I'm not too worried about this since DDoS protection isn't really the purpose of the cache (though I guess a nice bonus if it helps). Plus they could already target non-cached pages. This cache is designed to be conservative and safe by default, which should include these new condition.

[tangrufus]

How about a allow-list approach?

  set $skip_cache 1;

  # Only allow GET data being held in cache
  if ($request_method = GET) {
      set $skip_cache 0;
  }

Otherwise, we need to handle DELETE as well.

[swalkinshaw]

@TangRufus I do think that's a better refactor. We can look at that separately after this?

[craigpearson]

I think switching the $skip_cache behaviour might be more risky to push through upstream with minimal impact on any existing installs (?) but open to this approach. My thought path is should someone have extra rules in place and we unwittingly reverse those rules due to the new default (although that is probably slim pickings)

I could also tweak the commit to something like so:

# Prevent POST data being held in cache
if ($request_method = POST) {
    set $skip_cache 1;
}

# Prevent common API POST like requests being cached
if ( $request_method = ^(PUT|PATCH|DELETE)$ ) {
   set $skip_cache 1;
}

Untested

[swalkinshaw]

The grouping + comment is nice 👍

[swalkinshaw]

Thanks @craigpearson 🚀

[PDowney]

Adding OPTIONS to that might be a good idea too.

You really only want to be caching GET & HEAD. So that means everything below should skip the cache:

CONNECT DEBUG DELETE MOVE OPTIONS PATCH PUT TRACE TRACK