CORS breaks with cache proxies and same origin usage.

So shouldn't the Origin key be added to the Vary response for every Drupal request? That way, upstream caches will variate their cache and miss if the origin header is present or different?

Yes. The Vary response header must be identical on every response from the same route. Otherwise this will lead to problems with caching proxies (and even the browser cache). This is why Vary: Cookie header is added unconditionally to every response - even on anonymous requests.

Looks like the affected code is located in asm89/stack-cors, thus this needs to be fixed over there.

Great find!

Interestingly, asm89/stack-cors does have test coverage for doing exactly this: https://github.com/asm89/stack-cors/blob/c163e2b614550aedcf71165db2473d9.... The problem is that it does so conditionally: it only does this for request that are:

• CORS requests
• not preflight requests
• for requests that have an allowed Origin header

And that conditionality is exactly what is the problem here of course: the Vary header must always be set. Nobody reported this yet at https://github.com/asm89/stack-cors/issues?utf8=%E2%9C%93&q=vary.

I guess the assumption that asm89/stack-cors makes is that any request that contains a Origin header is not allowed to be served a response from a reverse proxy (such as Drupal's built-in page_cache and dynamic_page_cache). I wonder what Varnish does? Based on https://stackoverflow.com/questions/43998474/varnish-caching-options-cor..., it also doesn't handle this automatically?

P.S.: all AJAX requests that Drupal makes are POST requests, which are never cached.

The laravel-cors middleware (which also reuses asm89/stack-cors) had a similar discussion >4 years ago in https://github.com/barryvdh/laravel-cors/issues/20. https://github.com/barryvdh/laravel-cors/issues/20#issuecomment-50369259 says exactly what you say in #4. That led to https://github.com/asm89/stack-cors/pull/11, which we (Drupal) helped land.

As I'd imagine not all applications that use asm89/stack-cors require all responses in there application to be CORS ready?

Right — Drupal doesn't have this luxury because it needs routing to happen before it can even know whether a particular URL will need to vary by Origin.

Which brings me to the same conclusion as you:

But Drupal makes the decision that all routes could be CORS responses, so isn't it Drupal's job to provide the Origin key in the Vary header?

I think you're right.

But this is a pretty big change. In principle, certain responses may also be available to anonymous users. Which means that Drupal core's Page Cache would also need to vary by Origin (because certain origins may need to get a 403). Which would make it far less effective.

Before we make any changes to core, let's add a failing test that demonstrates the problem.

Note: if this is correct:

But this is a pretty big change. In principle, certain responses may also be available to anonymous users. Which means that Drupal core's Page Cache would also need to vary by Origin (because certain origins may need to get a 403). Which would make it far less effective.

Then we'll need changes like this:

diff --git a/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
index e99caa1..e82f929 100644
--- a/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
+++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
@@ -84,6 +84,9 @@ class DynamicPageCacheSubscriber implements EventSubscriberInterface {
         // requested that the subscriber does not support.
         // @see \Drupal\Core\EventSubscriber\RenderArrayNonHtmlSubscriber::onResponse()
         'request_format',
+        // Vary by CORS response in case that is turned on.
+        // @todo only do this if the `cors.config` container parameter has `enabled` set to `true`.
+        'headers:Origin',
       ],
       'bin' => 'dynamic_page_cache',
     ],
diff --git a/core/modules/page_cache/src/StackMiddleware/PageCache.php b/core/modules/page_cache/src/StackMiddleware/PageCache.php
index ca9a733..9bb5905 100644
--- a/core/modules/page_cache/src/StackMiddleware/PageCache.php
+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -342,6 +342,9 @@ protected function getCacheId(Request $request) {
     $cid_parts = [
       $request->getSchemeAndHttpHost() . $request->getRequestUri(),
       $request->getRequestFormat(),
+      // Vary by CORS response in case that is turned on.
+      // @todo only do this if the `cors.config` container parameter has `enabled` set to `true`.
+      $request->headers->get('Origin'),
     ];
     return implode(':', $cid_parts);
   }

#5 passes for me locally. The existing test coverage in HEAD proves that CORS runs after Page Cache:

    // Fire the same exact request. This time it should be cached.
    $this->drupalGet('/test-page', [], ['Origin' => 'http://example.com']);
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
    $this->assertSession()->responseHeaderEquals('Access-Control-Allow-Origin', 'http://example.com');

    // Fire a request for a different origin. Verify the CORS header.
    $this->drupalGet('/test-page', [], ['Origin' => 'http://example.org']);
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
    $this->assertSession()->responseHeaderEquals('Access-Control-Allow-Origin', 'http://example.org');

Note how both are a cache hit as far as Page Cache is concerned, yet they have different response headers.

This means that also a 403 due to CORS reasons should still be able to happen. Let's expand the test coverage to prove this.

D'oh, ignore #7, that test coverage already existed a bit further down 😅

#5 set the foundation in that it adds a request to the same URL without specifying the Origin request header.

This patch then adds assertions for the Vary header that match the current behavior.

+++ b/core/tests/Drupal/FunctionalTests/HttpKernel/CorsIntegrationTest.php
@@ -39,28 +39,31 @@ public function testCrossSiteRequest() {
     // Fire off a request without 'Origin' request header.
     $this->drupalGet('/test-page');
     $this->assertSession()->statusCodeEquals(200);
+    $this->assertSession()->responseHeaderEquals('Vary', '');

This is how it works today, but this is wrong. A single request without an Origin header would prime the reverse proxy's cache (and Drupal core's Page Cache, which is just a simple built-in reverse proxy) and result in incorrect responses.

This will fail. 🔥

My analysis in #6 is wrong, because of the reasons stated in #7: the CORS middleware runs after the Page Cache middleware.

This is also why Drupal core is not affected by this bug: because the CORS middleware (priority 250) wraps the Page Cache middleware (priority 200). This was done in #2850034: CORS allow-origin '*' not possible because of cached headers without thinking through the consequences for reverse proxies.

If we're to follow this pattern, then the recommendation would be for reverse proxies wrapping Drupal to do CORS themselves, and not use Drupal's native support for this.

That would of course require infrastructure-level configurability that doesn't exist everywhere.

So the simplest alternative would be to indeed add the Vary: Origin response header to all responses, until https://github.com/asm89/stack-cors/issues/49 (just opened that) gets solved.

+++ b/core/tests/Drupal/FunctionalTests/HttpKernel/CorsIntegrationTest.php
@@ -21,6 +21,18 @@ class CorsIntegrationTest extends BrowserTestBase {
+  protected function setUp() {
+    parent::setUp();
+
+    // Enable page caching.
+    $config = $this->config('system.performance');
+    $config->set('cache.page.max_age', 3600);
+    $config->save();
+  }

Without this, #12 will still fail because \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond() ends up calling \Drupal\Core\EventSubscriber\FinishResponseSubscriber::setResponseNotCacheable(), which does $response->setVary(NULL).

Why is that called? Because by default, cache.page.max_age === 0, which means that no responses from Drupal are allowed to be cached by any intermediary (such as a reverse proxy), and hence it's okay to throw away Vary.

So this allows intermediary caching, and in doing so it also allows us to test Vary sanely.

Behat\Mink\Exception\ExpectationException: Current response header "Vary" is "Origin,Accept-Encoding", but "Origin" expected.

I forgot that DrupalCI's test env somehow always adds Accept-Encoding but local test runner don't 🙃

Hi,

I can confirm this issue because on my case I'm having multiple clients requesting the same (API) so when te request is from the server which doesn't has any Origin header then when the browser requests it's missing the Access-Control-Allow-Origin .

Btw, thanks for this issue! what's missing for this patch to be RTBC ?

Hey @Wim Leers ! after I applied the #12 patch the Cookie value was removed from the Origin header, do you have any ideia of why?

thanks!!

#19: no, no idea :( But PageCacheTest is failing for a similar reason, so that problem you're observing must also be causing at least that test to fail. It'd be great if you could debug this and update the patch… 🙏 😊

Hey @Wim Leers, I just changed the order since this code at the setResponseCacheable method:

if (!$response->hasVary() && !Settings::get('omit_vary_cookie')) {
  $response->setVary('Cookie', FALSE);
}

is not adding the Cookie in case there's already content in the Vary header, but since I don't know what's the reason of this I just changed the order to add the Origin after the method execution, attached there's a rerolled patch and also I added the cookie value to our tests.

I hope this make's sense :P

Trying to pass the tests now.

This patch it's for the 8.6.x

We port a site to headless currently and are using GraphQL requests, with APQ (which optimises the queries), which means that we can send those requests as GET and cache them.
We also do server-side-rendering, so the GraphQL requests come either from a server, or a client.

A big issue was, that Acquias Varnish proxy threw away access-control-allow-origin headers, when being requested without origin header. We could circumvent it temporarily, by setting an origin header on our server request, but the underlying issue remained.
Anyone knowing the issue could potentially easily exploit it and make the site unusable for everyone (since without cors header, no data could be requested by clients).

This patch fixed this issue.
Thanks for the effort!

Reroll for 8.7, dropping tests, as I don't have time to investigate them right now.

I created a pull request against asm89/stack-cors to (optionally) always output the Vary: Origin header.

Attempt to reroll #23 against 8.9.x, no other changes (although there were some conflicts during rebase).

Seems the only test failure is caused by the order in which the headers in the vary-header are listed. Rather than checking the full vary-header string, we should just check for the presence of the 'origin' header in it (since we want to verify that 'Origin' is added to the 'Vary'-header), but for now let's see if we can get the tests to pass by just changing the order.

Took me way too long to figure out that running drush core tests with nginx is not the best idea.

Switched the test to just verifying the presence of Origin in the Vary-header. Interdiff is against #30, not #32. Passes locally.

@oknate Want to review this? :)

The patch in #33 worked for me to resolve caching issues with varnish.

This has been fixed upstream. Updated the library in #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -175,6 +175,11 @@ public function onRespond(FilterResponseEvent $event) {
   +    // @todo Remove this when https://github.com/asm89/stack-cors/issues/49 lands and Drupal core updates to a new release.
   

   The issue fixed

2. +++ b/core/tests/Drupal/FunctionalTests/HttpKernel/CorsIntegrationTest.php
   @@ -21,6 +21,18 @@ class CorsIntegrationTest extends BrowserTestBase {
   +  protected function setUp() {
   

   need void type to return

andypost, can you take a look at #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability? That fixes this issue.

Thank you, upgrade to 2.0 helps, guess it could be closed as duplicate

I have rerolled patch #33 and addressed comment #39.

If #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability is resovled, I'm not sure why this patch would be needed any longer?

The related #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability is mostly ready to be merged to 10.0.x

It makes sense to me that the fix in #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability will be released as a part of d10. We don't want to introduce big changes in a minor release. Is there a way to update Drupal 9 to use asm89/stack-cors to ^2.0 ?
What would be the best way to apply to Drupal 9 for testing?

Tagging this "Pantheon" because it seems to help CORS errors that are logged when running a site on Pantheon.

Is there a way to update Drupal 9 to use asm89/stack-cors to ^2.0 ?

@erin.rasmussen you can alias the package version.

composer require "asm89/stack-cors:2.0.5 as 1.3.0"

Fixed failed test of patch #42.

Probably #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability could be backported to 9.x

I've ran into this issue only recently on drupal 9.3.11 and varnish-5.2.1 (on amazeeio hosting). After applying #52 patch the issue was solved. If I add some more context we are having a decoupled website and the request to rest menu items was failing due to missing access-control-allow-origin header. So thanks to everyone working on this issue. I'd say it can go into RTBC after tests are passing

Fixed failed tests of patch #52.

Great @ravi.shankar, I'm moving the ticket to RTBC.

Marking this duplicate of #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability. Sites can alias the cors version for Drupal 9. If that's not enough, then we could look at backporting the cors update after all but I don't think we should workaround it.

#3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability is likely not going to be backported to D9.5 though, given this is a bug, we should probably still have this open for resolving here?

Given folks can use a composer alias, the consensus was just to leave this as 10.x only

When reading through this issue, I think this should ONLY be tagged for branch 9.5.x, which is about to (3 months) be marked unsupported, potentially killing this issue. But yeah, I don't think this is relevant to 10.x or 11.x at all, since they are using the new 2.x asm89/stack-cors library. Does anyone disagree with that?

Rerolled for 10.0.x that also applies to 11.x.

Have not reviewed but seems to have a test failure.