[upstream] CORS breaks form submission unless allowed origins includes site's own host

Hm, is this actually a bug? I guess we could whitelist all trusted hosts automatically (\Symfony\Component\HttpFoundation\Request::getTrustedHosts()).

This almost seems more like a documentation problem?

CORS is only supposed to be relevant to cross-origin requests, hence the name, so it seems like a bad idea to maintain the current behavior.

That is a very good point! :)

However, I don't understand how this is possible, because \Asm89\Stack\Cors::handle() does this:

        if ( ! $this->cors->isCorsRequest($request)) {
            return $this->app->handle($request, $type, $catch);
        }

… and isCorsRequest() does this:

    public function isCorsRequest(Request $request)
    {
        return $request->headers->has('Origin');
    }

Aha!

I just submitted a form with Chrome, and sure enough, there was an Origin request header.

So guess, what? This has been broken since July 2014: https://github.com/asm89/stack-cors/issues/10 + https://github.com/asm89/stack-cors/pull/11.

Just in case someone is looking for using the github patches, you can update your composer.json with the following:

  "extra": {
        "installer-paths": {
            "web/core": ["type:drupal-core"],
            "web/libraries/{$name}": ["type:drupal-library"],
            "web/modules/contrib/{$name}": ["type:drupal-module"],
            "web/profiles/contrib/{$name}": ["type:drupal-profile"],
            "web/themes/contrib/{$name}": ["type:drupal-theme"],
            "drush/contrib/{$name}": ["type:drupal-drush"]
        },
        "patches": {
            "asm89/stack-cors": {
                "Add check for same origin while identifying a CORS request" : "https://github.com/asm89/stack-cors/pull/11.patch"
            }
        }
    }

I'm wondering whether core should point to the fork containing the fix for now.

The alternative would be to actually override the classes. Sadly they are using private functions which makes things harder than it would have to be.

Yeah! finally, it got merged. https://github.com/asm89/stack-cors/pull/11

I am wondering whether we should have some form of regression / basic functional test coverage for that.

#12 I wonder the same, but then again: shouldn't that be up to the upstream? We shouldn't have test coverage for every upstream bugfix, should we?

Difficult line to walk.

Although I guess in this case thanks to CorsIntegrationTest being added in #2850034: CORS allow-origin '*' not possible because of cached headers, it's now trivial to add this test coverage? So, let's do this.

Well I agree there should be upstream unit testing ... but I think from the Drupal REST product point of view ensuring that enabling cors doesn't break the Drupal site makes sense, and as such having test coverage would be useful. Some simple form would be totally enough.

You're absolutely right. Thanks for insisting on that!

+++ b/core/tests/Drupal/FunctionalTests/HttpKernel/CorsIntegrationTest.php
@@ -16,10 +16,26 @@
+   * @var \GuzzleHttp\ClientInterface
+   */
+  protected $httpClient;
+
+  /**
    * {@inheritdoc}
    */
   public static $modules = ['system', 'test_page_test', 'page_cache'];
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    // Set up a HTTP client that accepts relative URLs.
+    $this->httpClient = $this->container->get('http_client_factory')
+      ->fromOptions(['base_uri' => $this->baseUrl]);
+  }

@@ -72,6 +88,18 @@ public function testCrossSiteRequest() {
+    $response = $this->httpClient->request('POST', '/test-page', [
+      'headers' => [
+        'Origin' => $origin,
+      ]

You should get the client from mink directly: $client = $this->getSession()->getDriver()->getClient()->getClient();, see \Drupal\Tests\rest\Functional\ResourceTestBase::request

Here is a small improvement for the test, but all in all, this looks great.

Committed 453d552 and pushed to 8.4.x. Thanks!

Unfortunately we can only bump minors in our own minor release so that makes this allegedly not patch safe. However there is something interesting here and worth thinking about.

+++ b/core/composer.json
@@ -32,7 +32,7 @@
-        "asm89/stack-cors": "~1.0"
+        "asm89/stack-cors": "~1.1"

By our current definitions 1.1 is compatible with ~1.0 - so what we're really changing here is the minimum version. So the question is can we change our minimum version in a patch release. Looking at https://github.com/asm89/stack-cors/compare/1.0.0...1.1.0 it drops PHP 5.3 and 5.4 support but non of the other changes would not be considered for a Drupal patch release.

I guess a mitigation is that work around is pretty simple.

Tagging with 8.4.0 release notes as I think library upgrades should be mentioned.

We could ask the author to release a 1.0.1 which has the fix applied.

@dawehner yep that'd allow use to fix this in a patch release imo. Do you think it is worth it given the mitigation?

@dawehner yep that'd allow use to fix this in a patch release imo. Do you think it is worth it given the mitigation?

Practically it makes it impossible to use CORS for real sites, I would argue.

It is never not worth to try: https://github.com/asm89/stack-cors/issues/44

Both contentacms and reserverouir ran into this specific problem. It makes it impossible for them to use cors. I think the theoretical possibility of a BC problem, vs. people running into a broken site is a bit ridiculous.

Today was the final normal bugfix release for 8.3.x. Aside from criticals and security fixes, no more commits will be made to 8.3.x, so marking this issue fixed again. Thanks!