The new corsService sets the allowed origin to the request origin in line 72:

$response->headers->set('Access-Control-Allow-Origin', $request->headers->get('Origin'));

Drupal page cache caches the response and even if the configuration says allowed-origin: '*' only the origin of the first request will be allowed.

Possibly needs and upstream fix.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yobottehg created an issue. See original summary.

yobottehg’s picture

Status: Active » Needs review

I created a pull request on the upstream for this:
https://github.com/asm89/stack-cors/pull/33

dawehner’s picture

I'm wondering whether one solution could be to ensure that the cors middleware runs after the page cache one? This would ensure that code is executed all the time?

Wim Leers’s picture

Issue tags: +API-First Initiative
hampercm’s picture

I don't think an upstream change is what's needed for fixing this.

Here's a patch based on @dawehner's suggestion from #3. This seems to solve the issue. Let's see if automated tests pass...

dawehner’s picture

Here is a test. It is adapting the kernel test in a true functional tests, given CORS is all about CORS.

Just a random sidenote: CORS doesn't allow empty header values.

Wim Leers’s picture

Here's a patch based on @dawehner's suggestion from #3.

@dawehner wrote:

I'm wondering whether one solution could be to ensure that the cors middleware runs after the page cache one?

The patch is making CORS' middleware have priority 250, PageCache's middleware has priority 200. This means CORS runs before PageCache.

So this is not implementing what #3 suggested. But at the same time, I think this is what @dawehner actually meant, I think he just wrote it incorrectly?

dawehner’s picture

The patch is making CORS' middleware have priority 250, PageCache's middleware has priority 200. This means CORS runs before PageCache.

Well, I think I should have written that the CORS middleware should wrap the pagecache one. A more outer middleware can do things before and after the more inner middleware.

So this is not implementing what #3 suggested. But at the same time, I think this is what @dawehner actually meant, I think he just wrote it incorrectly?

Yeah, for me the bugfix is as expected.

Sadly the test only patch doesn't fail :(

hampercm’s picture

@dawehner the tests in #8 look OK, but they don't test what's currently not working: a request from a different origin from the original request (whose response is cached). That's why they're passing on the test-only patch.

Added a test case to do just that. It uses allowed-origin of '*', which is what the bug report states.

The last submitted patch, 9: cors_allow_origin-2850034-9-test-only.patch, failed testing.

Wim Leers’s picture

+++ b/core/tests/Drupal/FunctionalTests/HttpKernel/CorsIntegrationTest.php
@@ -0,0 +1,77 @@
+  public function testCrossSiteRequest() {

s/CrossSite/CrossOrigin/

Leaving at NR so @dawehner can review this too.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The new test certainly makes more sense!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

re #11 the test was called CrossSite before so lets fix that in a followup if we want.

Committed and pushed cc77425 to 8.4.x and 54b5b2e to 8.3.x. Thanks!

As this is a bug fix backporting to 8.3.x.

  • alexpott committed cc77425 on 8.4.x
    Issue #2850034 by hampercm, dawehner, Wim Leers: CORS allow-origin '*'...

  • alexpott committed 54b5b2e on 8.3.x
    Issue #2850034 by hampercm, dawehner, Wim Leers: CORS allow-origin '*'...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.