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
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.
Comment | File | Size | Author |
---|---|---|---|
#79 | 3128982-d9.5.x-79.patch | 8.57 KB | Spokje |
#78 | 3128982-d9.x-78.patch | 8.57 KB | Spokje |
#63 | 3128982-fail.patch | 4.55 KB | andypost |
Issue fork drupal-3128982
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:
- 3128982-upgrade-asm89stack-cors-to changes, plain diff MR !1477
Comments
Comment #2
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #3
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #4
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedFix spelling. :)
Comment #5
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #6
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #8
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedI'm not sure how to fix this....
Comment #9
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #10
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #11
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #12
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #14
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #16
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #17
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #18
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #19
johnwebdev CreditAttribution: johnwebdev commentedThis doesn't seem correct?
Comment #20
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedThat 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 theOrigin
and theAccess-Control-Allow-Origin
do not match. There is no need to reject the request server-side. Preforming the unnecessary access control breaks cacheability.Comment #21
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedAdding an additional test to ensure that multiple origins work as intended.
Comment #22
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedI 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.
Comment #23
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #24
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #25
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #26
barryvdh CreditAttribution: barryvdh commentedFYI, 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.
Comment #27
barryvdh CreditAttribution: barryvdh commentedAnd 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.
Comment #28
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #29
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #31
barryvdh CreditAttribution: barryvdh commentedFYI, the stable version has been released.
Comment #32
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #33
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedComment #34
andypostthere'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
Comment #35
andypostHere'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
Comment #36
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedLooks 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 ofdrupal/core
that would satisfy the contraints.Comment #37
andypostRe #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
Comment #38
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedooo! I learn something new everyday! thanks!
Comment #39
larowlanUpdating 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.
Comment #40
larowlanI'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
Comment #41
andypostYes, that's uncommon that major version bump fixes issue.
Moreover cors looks broken without this change in d9 independently of caching
Comment #42
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedFrom what I can tell, it isn't broken, but the 1.x version of the libarary would do something like this:
and does not add the header if the request is not cross-origin.
Of course, this breaks if the library doesn't also add:
which breaks cacheability.
This "works" but not in the way you might expect, which would be something like:
and also does not need to vary the request by the origin.
I wrote most of the code to fix this upstream.
Comment #43
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedAlso, 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.
Comment #44
barryvdh CreditAttribution: barryvdh commentedWell 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.
Comment #47
catchI 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.
Comment #48
barryvdh CreditAttribution: barryvdh commentedI 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?
Comment #49
catch@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.
Comment #50
edisch@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.
Comment #51
barryvdh CreditAttribution: barryvdh commentedTo 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.
Comment #53
andypostComment #54
andyposthttps://github.com/asm89/stack-cors/releases/tag/v2.0.2 added php8 support
Making it critical as not clear about php 8.1 support
Comment #56
SpokjeUsed patch #35 as base for the new MR against
10.0.x-dev
.Comment #58
longwaveThe test changes look correct to me and match the change in behaviour described in this issue.
Comment #59
SpokjeHiding the patches to make clear the MR is where the party is at.
Comment #60
alexpottI 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?
Comment #61
alexpottBeing a bit more precise in my language - we should add testing for the
Vary: Origin
header.Comment #62
SpokjeComment #63
andypostAs this is a bug, here's fail-only patch to make sure test covers it before RTBCing
Comment #64
andypost@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
Comment #66
SpokjeThanks @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 uppedasm89/stack-cors
to the highest possible version for now (2.0.5).Comment #67
barryvdh CreditAttribution: barryvdh commentedYes 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.
Comment #68
andypostThank you, I think it ready for commiter's review
Comment #69
alexpottCommitted 3db8f62 and pushed to 10.0.x. Thanks!
Comment #73
cilefen CreditAttribution: cilefen commented#3262071: Uncaught exception 'TypeError' with message 'implode(): Argument #2 ($array) must be of type ?array, bool given'
Comment #74
jlockhartIs 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.
Comment #75
xjmComment #76
xjmComment #77
larowlanIn #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?
Comment #78
SpokjeWhich would look patch-wise something like the attached.
Comment #79
SpokjeAnd this (for
9.5.x-dev
)Comment #80
catchIn 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.
Comment #81
barryvdh CreditAttribution: barryvdh commentedYeah 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.
Comment #82
larowlanGiven folks can use a composer alias, the consensus was just to leave this as 10.x only