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.

CommentFileSizeAuthor
#9 cors_allow_origin-2850034-9.patch6.74 KBhampercm
#9 cors_allow_origin-2850034-9-test-only.patch6.27 KBhampercm
#6 2850034-6.patch5.94 KBdawehner
#6 2850034-6-test.patch2.55 KBdawehner
#5 cors_allow_origin-2850034-5.patch484 byteshampercm

Comments

yobottehg created an issue. See original summary.

yobottehg’s picture

yobottehg commented
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

dawehner
Primary language German
commented

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

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +API-First Initiative
hampercm’s picture

hampercm commented
StatusFileSize
new cors_allow_origin-2850034-5.patch484 bytes

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

dawehner
Primary language German
commented
StatusFileSize
new 2850034-6-test.patch2.55 KB
new 2850034-6.patch5.94 KB

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

wim leers
Location Ghent 🇧🇪🇪🇺
commented

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

dawehner
Primary language German
commented

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

hampercm commented
StatusFileSize
new cors_allow_origin-2850034-9-test-only.patch6.27 KB
new cors_allow_origin-2850034-9.patch6.74 KB

@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

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ 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

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

The new test certainly makes more sense!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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.