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.

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.

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

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

The grouping + comment is nice 👍

Thanks @craigpearson 🚀

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