Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#9 | cors_allow_origin-2850034-9.patch | 6.74 KB | hampercm |
#9 | cors_allow_origin-2850034-9-test-only.patch | 6.27 KB | hampercm |
#6 | 2850034-6.patch | 5.94 KB | dawehner |
#6 | 2850034-6-test.patch | 2.55 KB | dawehner |
#5 | cors_allow_origin-2850034-5.patch | 484 bytes | hampercm |
Comments
Comment #2
yobottehg CreditAttribution: yobottehg at WONDROUS commentedI created a pull request on the upstream for this:
https://github.com/asm89/stack-cors/pull/33
Comment #3
dawehnerI'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?
Comment #4
Wim LeersComment #5
hampercm CreditAttribution: hampercm at Acquia commentedI 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...
Comment #6
dawehnerHere 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.
Comment #7
Wim Leers@dawehner wrote:
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?
Comment #8
dawehnerWell, 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.
Yeah, for me the bugfix is as expected.
Sadly the test only patch doesn't fail :(
Comment #9
hampercm CreditAttribution: hampercm at Acquia commented@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.
Comment #11
Wim Leerss/CrossSite/CrossOrigin/
Leaving at NR so @dawehner can review this too.
Comment #12
dawehnerThe new test certainly makes more sense!
Comment #13
alexpottre #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.