caciobanu / improved-magento-layered-navigation

This project has reached its end-of-life (EOL).
MIT License
138 stars 54 forks source link

Fix FPC-related issues #99

Closed dborishansky closed 7 years ago

dborishansky commented 7 years ago

There are two issues this is intended to address:

1) Avoid incorrect content-type headers from being cached in full page cache 2) Avoid incorrect response data from being cached in FPC

Worth noting that this change will likely break things on sites that utilize request URI's with an isLayerAjax querystring parameter.

caciobanu commented 7 years ago

Yes, this for sure will break the implementation for community edition. Can you elaborate what is the issue maybe I can come with a better solution.

toddbc commented 7 years ago

I would propose using $this->getRequest()->isAjax() if CE then, inside the helper, i.e.:

    public function isRequestAjax()
    {
        if (Mage::getEdition() == Mage::EDITION_ENTERPRISE) {
            // On Enterprise, FPC caches headers based on request-path (sans query string.)
            // This means, request-headers or query string values cannot affect the Content-Type.
            // Otherwise, an attacker can cause cached category pages to be served as application/json.
            $requestString = Mage::app()->getRequest()->getRequestString();
            return strpos($requestString, '/isLayerAjax/1') !== false;
        }

        return $this->getRequest()->isAjax();
    }

For a bit of background, we're seeing two separate intermittent problems:

18 greatly improved these cases (which were previously happening frequently), but it's still possible for archived pages to cause these problems, and it's still possible for a malicious attacker to trivially DoS a site by poisoning the cache with JSON response headers.

David's change ensures that an attacker could not prevent category pages from being usable, by requiring the request-path differ for any and all JSON responses (to match what Magento EE FPC uses as a cache key.)

I don't know if this may be an issue in other CE FPC implementations, but I wouldn't be surprised if they didn't use X-Requested-With as part of the cache key either.

caciobanu commented 7 years ago

@dborishansky @toddbc Can you check if #102 is a good solution ?

caciobanu commented 7 years ago

Merged #102 instead.