keycdn / cache-enabler

A lightweight caching plugin for WordPress that makes your website faster by generating static HTML files.
https://wordpress.org/plugins/cache-enabler/
123 stars 46 forks source link

add default query string exclusion #154

Closed coreykn closed 4 years ago

coreykn commented 4 years ago

Add default regular expression to query string cache exclusion setting. All query strings will be excluded except what is matched in the following regular expression:

/^(?!(fbclid|ref|mc_(cid|eid)|utm_(source|medium|campaign|term|content|expid)|gclid|fb_(action_ids|action_types|source)|age-verified|ao_noptimize|usqp|cn-reloaded|_ga|_ke)).+$/

Closes #148

futtta commented 4 years ago

why ao_noptimize @coreykn ? This means that parameter cannot be used for debugging purposes any more (as the cache and autoptimize page will be served instead)?

svenba commented 4 years ago

This was proposed by @centminmod

Is it better to remove it from the list?

futtta commented 4 years ago

I think so, as per my understanding this would prevent the use of ?ao_noptimize=1 as a way to see the unoptimized page as the cache autoptimize page would be returned instead? @centinmod any objections?

On Wed, Oct 14, 2020 at 9:02 PM Sven notifications@github.com wrote:

This was proposed by @centminmod https://github.com/centminmod

Is it better to remove it from the list?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keycdn/cache-enabler/pull/154#issuecomment-708600034, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMIMMJB33Y5D3X2V2TMH3SKXYUTANCNFSM4SQ7PPFQ .

coreykn commented 4 years ago

When reviewing it I had thought it would be used in the sense that Autoptimize would not optimize the page but Cache Enabler could still cache the page (just without the optimization for troubleshooting). (This would require the cache be cleared for that page.) It looks like I'm mistaken what it's used for, though.

centminmod commented 4 years ago

I think so, as per my understanding this would prevent the use of ?ao_noptimize=1 as a way to see the unoptimized page as the cache autoptimize page would be returned instead? @centinmod any objections?

No objections but the query string list I proposed is for cache inclusion in 1.4.9 not exclusion from cache. This update does the opposite of what I'd want to cache only just those query strings and not all query strings?

Or wait did I read that wrongly?

The idea for cache inclusion is to prevent cache bypass in app level DDOS attacks so ?ao_noptimize=1 is cached but owners can easily use that query string by clearing cache when needed to. Just for automated attacks it isn't a vector to run from. Attacker says hey look this wordpress site uses Autoptimize lets attack it from that query string to bypass the cache

I mentioned in #148 about maybe differentiating where the query string request is coming from or who initiates it

Maybe if there's a way to group query strings on front end user requested versus query strings backend/wp-admin requested so that the latter is excluded from cache for backend/wp-admin operations ?

So ?ao_noptimize=1 can be excluded from cache if called by user with a specific cookie only

futtta commented 4 years ago

Ideally requests with ao_noptimize are neither cached nor served from cache to ensure cache free trouble shooting.

On Wed, 14 Oct 2020, 22:49 George Liu (eva2000), notifications@github.com wrote:

I think so, as per my understanding this would prevent the use of ?aonoptimize=1 as a way to see the unoptimized page as the cache autoptimize page would be returned instead? @centinmod any objections? … <#m-4564734409483331926_> On Wed, Oct 14, 2020 at 9:02 PM Sven @.***> wrote: This was proposed by @centminmod https://github.com/centminmod https://github.com/centminmod Is it better to remove it from the list? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#154 (comment) https://github.com/keycdn/cache-enabler/pull/154#issuecomment-708600034>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMIMMJB33Y5D3X2V2TMH3SKXYUTANCNFSM4SQ7PPFQ .

No objections but the query string list I proposed is for cache inclusion in 1.4.9 not exclusion from cache

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keycdn/cache-enabler/pull/154#issuecomment-708652186, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMIMNWOTO7VUVNQK67CWTSKYFHBANCNFSM4SQ7PPFQ .

coreykn commented 4 years ago

@futtta We can remove ao_noptimize if you prefer. (I don't have a preference on this.) Other parameters could be appended though too, like ?ao_noptimize=1&nocache or something like that.

@centminmod I believe you read it wrong. 🙂 The new default query string exclusion in this PR does what you wanted from what I can tell. It excludes everything it matches (e.g. ?utm_source=example does not match so the cache is not bypassed but ?foo=bar matches so it would).

centminmod commented 4 years ago

I see thanks for clarification. Should be labeled as query string cache persistent/specific inclusion rather than query string exclusion in the context of a caching plugin. I have no objection in removing ao_noptimize query string or can we get a admin setting option to predefined this regex rather than have it hard coded? Like old 1.4.9 query string inclusion setting