CORS allow-origin '*' not possible because of cached headers

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?

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...

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.

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?

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 :(

@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.

+++ 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.

The new test certainly makes more sense!

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.