Problem/Motivation

When enabling CORS support in Drupal, cacheability is broken because this header is added to every request (of a different origin, but that is a different bug, see: #3001809: CORS breaks with cache proxies and same origin usage.):

Vary: Origin

This breaks cacheability because if a site allows numerous other origins to make requests to their site, they will get a cached version for each origin. Some CDNs will not cache the request at all if this is present as it results in too many cache objects.

Proposed resolution

This problem has been resolved upstream in asm89/stack-cors#64. The most recent release this has been included in is 2.0.0.

I would like to backport this release when it is in a stable upstream release.

Remaining tasks

  1. Write Upgrade Patch

User interface changes

None

API changes

Requests that were not cached before may become cached.

Data model changes

None.

Release notes snippet

asm89/stack-cors has been updated from version 1.3.0 to 2.0.5.

Enabling CORS now preserves cacheability whenever possible.

Previously, enabling CORS would add Vary: Origin to all requests of a different origin. With this change, enabling CORS will only add this if absolutely necessary.

CommentFileSizeAuthor
#79 3128982-d9.5.x-79.patch8.57 KBSpokje
#78 3128982-d9.x-78.patch8.57 KBSpokje
#63 3128982-fail.patch4.55 KBandypost
#35 interdiff.txt4.62 KBandypost
#35 cors-cache-3128982-35.patch9.52 KBandypost
#32 cors-cache-3128982-32.patch7.68 KBdavidwbarratt
#28 cors-cache-3128982-28.patch8.51 KBdavidwbarratt
#25 cors-cache-3128982-25.patch7.7 KBdavidwbarratt
#24 cors-cache-3128982-24.patch7.69 KBdavidwbarratt
#23 cors-cache-3128982-23.patch7.69 KBdavidwbarratt
#21 cors-cache-3128982-21.patch7.88 KBdavidwbarratt
#14 cors-cache-3128982-14.patch6.35 KBdavidwbarratt
#11 cors-cache-3128982-11.patch3.9 KBdavidwbarratt
#10 cors-cache-3128982-10.patch3.72 KBdavidwbarratt
#2 cors-cache-3128982-2.patch441 bytesdavidwbarratt

Issue fork drupal-3128982

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidwbarratt created an issue. See original summary.

davidwbarratt’s picture

Status: Active » Needs review
FileSize
441 bytes
davidwbarratt’s picture

Issue summary: View changes
davidwbarratt’s picture

Issue summary: View changes

Fix spelling. :)

davidwbarratt’s picture

Title: Enabling CORS breaks cachability » Enabling CORS breaks cacheability
davidwbarratt’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: cors-cache-3128982-2.patch, failed testing. View results

davidwbarratt’s picture

I'm not sure how to fix this....

davidwbarratt’s picture

Issue tags: +dependencies
davidwbarratt’s picture

Status: Needs work » Needs review
FileSize
3.72 KB
davidwbarratt’s picture

davidwbarratt’s picture

Issue summary: View changes

The last submitted patch, 10: cors-cache-3128982-10.patch, failed testing. View results

davidwbarratt’s picture

The last submitted patch, 11: cors-cache-3128982-11.patch, failed testing. View results

davidwbarratt’s picture

davidwbarratt’s picture

Issue summary: View changes
davidwbarratt’s picture

Issue summary: View changes
johnwebdev’s picture

+++ b/core/tests/Drupal/FunctionalTests/HttpKernel/CorsIntegrationTest.php
@@ -71,8 +71,8 @@ public function testCrossSiteRequest() {
     // Fire a request from an origin that isn't allowed.
     /** @var \Symfony\Component\HttpFoundation\Response $response */
     $this->drupalGet('/test-page', [], ['Origin' => 'http://non-valid.com']);
-    $this->assertSession()->statusCodeEquals(403);
-    $this->assertSession()->pageTextContains('Not allowed.');
+    $this->assertSession()->statusCodeEquals(200);
+    $this->assertSession()->responseHeaderEquals('Access-Control-Allow-Origin', 'http://example.com');

This doesn't seem correct?

davidwbarratt’s picture

That is actually correct. Because the config only specifies a single origin http://example.com it is safe to add that to every request. A simple request will become opaque in the browser since the Origin and the Access-Control-Allow-Origin do not match. There is no need to reject the request server-side. Preforming the unnecessary access control breaks cacheability.

davidwbarratt’s picture

FileSize
7.88 KB

Adding an additional test to ensure that multiple origins work as intended.

davidwbarratt’s picture

I have conflicting documentation on if upgrading to v2 is a bc-break or not.

On one hand, major upgrades of libraries is is a BC break:
https://www.drupal.org/core/d8-allowed-changes#minor
and (to make matters worse) the v2 branch does not have a stable version. :(

On the other hand, change(s) to an HTTP Header is not a BC break:
https://www.drupal.org/core/d8-bc-policy#http

Since I'm not sure if this breaks bc, and breaking cachability is a Major, I'll mark this as D9 release blocking for now. (Please feel free to change it!)

I can ask the maintainers to move the functionality to the 1.x branch behind a bc flag, I'm not sure if they will be willing to do so.

davidwbarratt’s picture

Issue summary: View changes
FileSize
7.69 KB
davidwbarratt’s picture

davidwbarratt’s picture

barryvdh’s picture

FYI, the usage of this library doesn't change, NO public API's have been changed and the config remains the same. The main two differences are:
- Adding CORS headers when possible, Vary correct headers otherwise (so single/wildcard allows are ALWAYS added, to ensure cacheability)
- The access checks have been removed (as noted in the tests) as CORS is not a security for your API, only for the browser. (The Origin can easily be removed/forged with CURL requests).
This does mean that GET/simple POST (non-preflighted) requests will execute, but Preflighted requests will be aborted by the browser only.

barryvdh’s picture

And to be clear, as the maintainer I'm happy to tag the final version, but I'd rather hear your feedback en make any required changes before tagging the stable 2.x release.

davidwbarratt’s picture

davidwbarratt’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 28: cors-cache-3128982-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

barryvdh’s picture

FYI, the stable version has been released.

davidwbarratt’s picture

Status: Needs work » Needs review
FileSize
7.68 KB
davidwbarratt’s picture

Component: base system » cache system
andypost’s picture

+++ b/composer.lock
@@ -8,36 +8,36 @@
-            "version": "1.3.0",
+            "version": "2.0.0",

there's 2.0.1 release https://github.com/asm89/stack-cors/releases

As I got it reverts unrelated change but would be great to use it

andypost’s picture

Here's patch that updates cors to 2.0.1 (locally it works)

@davidwbarratt thanks for pointer, #3001809: CORS breaks with cache proxies and same origin usage. could be closed as duplicate

davidwbarratt’s picture

Status: Needs review » Reviewed & tested by the community

Looks perfect to me!

btw, how do you update the composer.lock in the root? Composer was complaining to me that it couldn't find a version of drupal/core that would satisfy the contraints.

andypost’s picture

Re #36 I did change manually core/composer.json
- used COMPOSER_ROOT_VERSION=9.1.x-dev composer require asm89/stack-cors ^2.0.0
- removed it from root composer.json
- finally COMPOSER_ROOT_VERSION=9.1.x-dev composer update --lock

davidwbarratt’s picture

ooo! I learn something new everyday! thanks!

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -dependencies, -Drupal 9, -Drupal 9 Release Blocker, -Drupal 9 readiness +Needs release manager review, +Performance

Updating a major version of a dependency requires release manager sign-off.

Also, whilst @andypost updated the pinned version, he didn't write the rest of the code, so I don't feel it's appropriate to RTBC a patch you were the primary author of.

I'll ping release managers.

larowlan’s picture

I've passed on that the major version change was as a result of this change, so it may not be treated the same as other major version updates

andypost’s picture

Yes, that's uncommon that major version bump fixes issue.
Moreover cors looks broken without this change in d9 independently of caching

davidwbarratt’s picture

From what I can tell, it isn't broken, but the 1.x version of the libarary would do something like this:

Access-Control-Allow-Origin: REQUESTED ORIGIN

and does not add the header if the request is not cross-origin.

Of course, this breaks if the library doesn't also add:

Vary: Origin

which breaks cacheability.

This "works" but not in the way you might expect, which would be something like:

Access-Control-Allow-Origin: *

and also does not need to vary the request by the origin.

I wrote most of the code to fix this upstream.

davidwbarratt’s picture

Also, one of the maintainers of the upstream library affirmed in #26 that there are no public API changes and there are no config changes. I was surprised at the major version bump as it violated semver, but I think given that declaration it is acceptable to upgrade the major version.

barryvdh’s picture

Well the public API indeed has no breaking changes, but the behavior is changed on some cases; in v1 the headers we're only applied to actual CORS requests and blocking the actual requests that did not follow CORS (but this obviously being not a real protection because you can just remove the Origin header).

The v2 was explicitly changed in this behavior to fix the cacheability issue in Drupal (and other frameworks) and I think there shouldn't be a problem with the changes in behavior, for Drupal at least.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

I think it's fine to update the major version - it's a behaviour change, but the behaviour change is the bugfix, so we don't really have a choice except to live with the bug until Drupal 10.

barryvdh’s picture

I don't really see how a major release is required for Drupal. If I tag a new 1.x version it would be okay? That would be exactly the same?

catch’s picture

@Barryvdh we try not to update to major versions of dependencies in Drupal core minor releases, on the basis that a major version change in a dependency may contain API changes that would in turn require a major version bump in Drupal core.

However, we also recognise that different projects have different approaches to bc policy / thresholds for tagging a new major version, so in practice it's fine to update to the major version in this case.

The difference is that for a minor version dependency update we default to trusting the dependency (and our automated tests etc.), but for a major version, we require an explicit exception to be made and a bit more investigation than usual. So if it had been a minor version bump, we probably would have just committed the update without any discussion, but the end result is the same.

edisch’s picture

@catch so what are the next steps on getting this into core? Should we start running a patch with asm89/stack-cors@2.0.3 and report back given the "needs review" tag? @barryvdh mentioned he could create a 1.x release which would allow us to continue with Drupal semver standards.

barryvdh’s picture

To be clear, I said that releasing a 1.x version with this code would behave exactly the same, so it shouldn't matter if it's 1.x or 2.x.
I don't plan on releasing a 1.x version with these changes though.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 9.4.x-dev » 10.0.x-dev
Issue tags: +Drupal 10
Parent issue: » #3251854: [META] Requirements for tagging Drupal 10.0.0-alpha1
andypost’s picture

Title: Enabling CORS breaks cacheability » Upgrade asm89/stack-cors to ^2.0 to fix cacheability
Priority: Major » Critical

https://github.com/asm89/stack-cors/releases/tag/v2.0.2 added php8 support

Making it critical as not clear about php 8.1 support

Spokje made their first commit to this issue’s fork.

Spokje’s picture

Used patch #35 as base for the new MR against 10.0.x-dev.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

The test changes look correct to me and match the change in behaviour described in this issue.

Spokje’s picture

Hiding the patches to make clear the MR is where the party is at.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think we should we be adding testing for the origin header - so we can ensure it is set or not as we now expect.

Plus do we want to loosen the restrictions on D9 so it can be installed there too?

alexpott’s picture

Being a bit more precise in my language - we should add testing for the Vary: Origin header.

Spokje’s picture

Status: Needs work » Needs review
andypost’s picture

As this is a bug, here's fail-only patch to make sure test covers it before RTBCing

andypost’s picture

@Spokje patch in #63 is from your MR cos there's no way to create test-only runs from MR

Moreover here's 2.1.0 version which support only SF6 https://github.com/asm89/stack-cors/releases

I think we should upgrade to it

Status: Needs review » Needs work

The last submitted patch, 63: 3128982-fail.patch, failed testing. View results

Spokje’s picture

Status: Needs work » Needs review

Thanks @andypost

I agree we eventually have to get to asm89/stack-cors 2.1.0, but currently D10 is still on SF4, and if I understood correctly, will go through SF5 first, before ending up in the ever emerald-green land of SF6. So currently we can't.

I've merged the latest commits on 10.0.x-dev into the MR and upped asm89/stack-cors to the highest possible version for now (2.0.5).

barryvdh’s picture

Yes I expected it to be compatible with both Symfony 5 and 6, but unfortunately saw some issues after tagging 2.0.5
Use 2.0.x for Symfony <= 5, and 2.1.x for Symfony 6. But ^2 should select the best version for you.
I'll maintain 2.0.x on a separate branch if required.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, I think it ready for commiter's review

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3db8f62 and pushed to 10.0.x. Thanks!

  • alexpott committed 3db8f62 on 10.0.x
    Issue #3128982 by davidwbarratt, Spokje, andypost, Barryvdh, larowlan,...

Status: Fixed » Closed (fixed)

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

jlockhart’s picture

Is there a way to apply this to D9.3? Running into this issue and its blocking parts of our production site. We're also not able to upgrade to D10 right away when it comes out so would love to know how to get this patched.
I've tried to update the version required in composer but getting the version conflict error.
I also tried the steps in #37 but I'm not sure what I'm doing.
Any help would be appreciated.

xjm’s picture

Issue tags: +10.0.0 release notes
xjm’s picture

Issue summary: View changes
larowlan’s picture

Version: 10.0.x-dev » 9.4.x-dev
Status: Closed (fixed) » Needs review

In #47 @catch indicated he was ok with this being backported to Drupal 9.

Now we've got a minor update on the horizon, should we consider putting this in 9.4?

Spokje’s picture

Which would look patch-wise something like the attached.

Spokje’s picture

And this (for 9.5.x-dev)

catch’s picture

In case anyone else is confused, 2.1.0 broke compatibility with Symfony 4/5 and PHP 7.3+, but this compatibility was re-added in 2.1.1 (see https://github.com/asm89/stack-cors/releases), so we'd be OK to update to 2.1.1, but not to 2.1.0 - if we'd updated to the 2.x branch prior to 2.1.1 we'd have needed to go to 2.0.x instead.

Per #47/#49 I think it's OK to make an exception and update the major version in the minor (with a release note + change record) since we know that in practice it's only going to fix this bug and shouldn't introduce any other incompatibilities. It's another example of semver not accomplishing what many people claim it does though.

barryvdh’s picture

Yeah sorry about the confusion. But it shouldn't 'break' symfony 4/5, it just won't install them, because they're not supported for that combination. Requiring ^2 would result in the latest version usually, but don't see a reason you wouldn't just use ^2.1.1 directly.

larowlan’s picture

Version: 9.4.x-dev » 10.0.x-dev
Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

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