cache clear conditional appears malformed

[nosilver4u]

https://github.com/keycdn/cache-enabler/blob/1115ffc39c7ec4f38e6cd8f9ad96b0965fb9ca37/inc/cache_enabler.class.php#L863

The conditional on that line seems a bit off. If I'm reading it right, currently goes like so: If the _cache query param is empty OR the _action query param is empty OR the _cache query param isn't what it should be AND ( _action isn't equal to 'clear' OR _action isn't equal to 'clearurl' ) --then bail out of the function.

But I believe what you really want is to bail out of the function in this case: If the _cache query param is empty OR the _action query param is empty OR the _cache query param isn't what it should be OR ( _action isn't equal to 'clear' AND _action isn't equal to 'clearurl' ).

[coreykn]

I believe that's the exact same if statement just written differently, right? Both are saying only _cache=cache-enabler&_action=clear and _cache=cache-enabler&_action=clearurl are valid. The current statement is checking the values of both _cache and _action at the same time whereas the proposed statement checks _cache and then _action.

[nosilver4u]

They are slightly different, as the proposed version fails if either the _cache param OR the _action param is invalid. The existing statement does not fail if _action is non-empty but invalid and I don't think that's what you want. This part is always true, because _action can't be equal to both 'clear' and 'clearurl': ( $_GET['_action'] !== 'clear' || $_GET['_action'] !== 'clearurl' )

But because that is joined to $_GET['_cache'] !== 'cache-enabler' with an AND/&&, that section only fails if _cache is invalid.

So the correct statement should be this: if ( empty( $_GET['_cache'] ) || empty( $_GET['_action'] ) || $_GET['_cache'] !== 'cache-enabler' || ( $_GET['_action'] !== 'clear' && $_GET['_action'] !== 'clearurl' ) ) {

A clearer way of looking at the existing statement is like so: If the _cache query param is empty OR the _action query param is empty OR the _cache query param isn't what it should be AND (true)

Thus if _cache is 'cache_enabler' and _action is 'something-else', the condition is false and the function continues when it shouldn't.

[coreykn]

Ah, you're right! That was my mistake. Thanks for the additional details, I really appreciate it, as that helped clear that up.

[nosilver4u]

You're welcome!