Use isMethodCacheable() instead of isMethodSafe() when checking for GET/HEAD

Reproduced with 8.2.2 on PHP 5.6.19 browsing from Chrome 54.

Escalating this to Major as it potentially breaks decoupled systems. Also, as OPTIONS requests are not cacheable (and thus should never be retrieved from cache) there is potential to conflict with the HTTP/1.1 specs: https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

From stepping through the code, it appears that the page_cache module is returning the cached response for the GET request at the same URL, instead of the OPTIONS response. The OPTIONS response is never actually cached from what I can tell, but rather the wrong response is being returned.

\Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod::check() should be returning DENY if the request will produce a response that should not be cached due to its HTTP METHOD. From that class's PHPDocs:

/**
 * Reject when running from the command line or when HTTP method is not safe.
 *
 * The policy denies caching if the request was initiated from the command line
 * interface (drush) or the request method is neither GET nor HEAD (see RFC
 * 2616, section 9.1.1 - Safe Methods).
 */

However, the code in check() uses \Symfony\Component\HttpFoundation\Request::isMethodSafe() to check the HTTP method, but that method returns true if the HTTP method is 'GET', 'HEAD', 'OPTIONS', or 'TRACE'. As a result, the check() can pass and allow the Drupal bootstrap to continue on to load the response from the page cache. The cached response for a previous GET request at the same URL is then pulled from the page cache, and erroneously returned as the response to the OPTIONS request.

It would seem the correct method to use in the check() is actually \Symfony\Component\HttpFoundation\Request::isMethodCacheable(), which only returns true if the HTTP method is "GET' or 'HEAD'.

Digging deeper, it looks like this bug cropped up as a result of my doing a recent install using the drupal-composer project, which caused Composer to use the latest version of the Symfony libraries (2.8.13) that have a different implementation of isMethodSafe() than what is included in the official tarball release (2.8.4). This library version update can also happen if a composer update is run on a Drupal codebase.

Updating issue summary based on my findings. Apologies if I'm overstepping, but this seemed like the cleanest way to tackle this issue.

A workaround for this currently is to downgrade your site's Symfony libraries to 2.8.12 or earlier (Drupal Core uses 2.8.4 out of the box).

This patch will ensure correct caching behavior for all existing 2.8.x (and 3.1.x) versions of the Symfony library. Test coverage already provided by \Drupal\Tests\Core\PageCache\CommandLineOrUnsafeMethodTest

So Symfony simply broke BC in a patch release. WTF!?

Introduced at https://github.com/symfony/symfony/commit/140460707201665bec2d3e625e93b0... — https://github.com/symfony/symfony/pull/19321 (July 9, 2016)

https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html specifically lists HEAD and GET as the only safe methods. OPTIONS and TRACE are idempotent methods. That's not the same. Therefore Symfony is clearly violating the HTTP spec. It is correct, but it's still a BC break for a patch release.

(They BTW also added Request::isMethodIdempotent(): https://github.com/symfony/symfony/commit/44df6a4677f1c36549103406cf2101... — https://github.com/symfony/symfony/pull/19322.)

Fun fact: Symfony also discovered this is a problem, and that is why they introduced Request::isMethodCacheable() shortly after:

https://github.com/symfony/symfony/commit/c43de7f21a587649bb42ca08bce9ad... — https://github.com/symfony/symfony/pull/20205 (October 13, 2016)

When pointing out that this broke BC, the answer was simply RFC conformance. That's fine for a major release. Maybe even a minor. But definitely not a patch release.

This should be reverted from the Symfony 2.8.x branch. Creating an issue for that.

Symfony issue: https://github.com/symfony/symfony/issues/20562.

Similar issue at #2770673: Prevent Composer from installing symfony/http-foundation to 2.8.8 & 2.8.9 due to a BC break.

Duplicate of this issue at #2776367: Drupal\Tests\Core\PageCache\CommandLineOrUnsafeMethodTest::testHttpMethod Failed asserting that null is identical to 'deny'..

I don't think they are going to revert this upstream in Symfony. Can we update Symfony and use Request::isMethodCacheable() instead in CommandLineOrUnsafeMethod.php?

- use Request::isMethodCacheable()
- change constraint in composer.json
- update symfony/http-foundation

Thanks, exactly what I had in mind!

definitely 8.3.x material, not sure about 8.2.x.

The 8.3.x patch did not apply, yet it's only RTBC'd for 8.3.x … that doesn't work :P Needs a reroll.

Also, this can also affect 8.2.x installations, because composer.json specifies ~2.8. What about them? How do we guarantee it doesn't break for them? That's the bigger issue.

re-roll

-        "symfony/http-foundation": "~2.8",
+        "symfony/http-foundation": ">=2.8.13 <3.0",

Why can't we do it like symfony and fix it in a patch release?

The other way is to use #9 for 8.2.x

We can do that. But Symfony violated semver. So now Drupal core and Drupal contrib modules need to be updated to require a certain patch release of Symfony HTTP Foundation and a certain patch release of Drupal core, respectively.

Symfony did not violate Semver because they did not break their API. The behavior of a method changed because it was buggy before and did not adhere to the RFC.

Semver only guarantees that function, method, class signatures etc. do not change. Of course you are allowed to change subtle behavior details, that is the point of patch releases. Otherwise you would not be able to ever fix bugs, because the fix changes behavior in some way.

Bug fixes are fine, definitely. But `Request::isMethodCacheable()` is unquestionably a new API. Semantically, for many use cases, isMethodCacheable() is the replacement for isMethodSafe(). If that happens in a patch release (like it did here), then that is a BC break.

I think they aim for maximum compatibility by providing the new method as early as possible. I guess they don't consider API additions BC breaks.

Anyway, patch looks good and is the right approach for at least 8.3.x.

API additions aren't BC breaks. But effectively renaming a method is a BC break.

Patch looks fine for 8.3.x for sure! :)

Curious what core committers think we should do for 8.2.x.

Having had this discussion here has allowed me to explain this more clearly. See https://github.com/symfony/symfony/issues/20562#issuecomment-262474462.

Verbatim copy of that comment:

From a BC POV, this means that Request::isMethodSafe() was effectively renamed to Request::isMethodCacheable().

Before #19321 + #20205:

    public function isMethodSafe()
    {
        return in_array($this->getMethod(), array('GET', 'HEAD'));
    }

After #19321 + #20205:

    public function isMethodSafe()
    {
        return in_array($this->getMethod(), array('GET', 'HEAD', 'OPTIONS', 'TRACE'));
    }

    public function isMethodCacheable()
    {
        return in_array($this->getMethod(), array('GET', 'HEAD'));
    }

I'm +100 for following RFCs closely. It means shared understanding/expectations. It's super valuable. But it's not okay to do this after it's worked this way the entire Symfony 2.x cycle, and then change this just in some patch release, not even a minor.

I'd have only made this change in a major version (Symfony 3.x). But a new minor version would be acceptable too (2.9.x). In a patch release, that simply blows my mind.

I think we should explicitly check GET/HEAD in 8.2.x and not use either method. This is the only option we would have had if isMethodSafe() had been like this in the first place. The fact that Symfony broke HttpCache which was relying on the same behaviour shows it should not have happened in a patch release. Not that we don't accidentally break things in patch releases too, but we usually own up to it.

For 8.3.x I didn't want to postpone #2712647: Update Symfony components to ~3.2 on this issue or this issue on that one, but since they're both RTBC at the same time I'd like to give the 3.1 update a chance to get committed - it explicitly fixes this bug too now. If this needs to go into 8.2.x, I'd also consider committing that quick fix to 8.3.x since there's no library change involved.

I think we should explicitly check GET/HEAD in 8.2.x and not use either method. This is the only option we would have had if isMethodSafe() had been like this in the first place.

+1. Then it doesn't matter which Symfony version you use.

The fact that Symfony broke HttpCache which was relying on the same behaviour shows it should not have happened in a patch release.

Amen.

Not that we don't accidentally break things in patch releases too, but we usually own up to it.

Exactly, that's what I argued in #26.

@catch and @wimleers the patch from #9 does just such an explicit check against GET/HEAD, and could be applied to 8.2.x.

Note Niklas Grekas opened https://github.com/symfony/symfony/pull/20602 and https://github.com/symfony/symfony/pull/20603 against Symfony to revert the bc break and deprecate instead.

Symfony has reverted the BC break:
https://github.com/symfony/symfony/pull/20602

Nice.

So given at the moment this only affects sites installed via composer, I think we can wait for a new Symfony release to get tagged rather than working around.

However we should switch to isMethodCacheable() in 8.3.x where we can. That might end up getting rolled into #2712647: Update Symfony components to ~3.2 but moving to CNW for now.

This was rolled into #2712647: Update Symfony components to ~3.2, which got committed earlier today.