Problem/Motivation
If you use ajax requests from the same origin, CORS support is omitted (for obvious reasons) and no `Origin` key is added to the `Vary` header and naturally the Access-Control-Allow-Origin header is not emitted. However, the request does cache and if a request from another origin is made, it receives the cached item without the CORS data.
Proposed resolution
Technically, every route in Drupal is a CORS route since CORS will activate if an Origin header is passed in the request. So shouldn't the Origin key be added to the Vary response for every Drupal request? That way, upstream caches will variate their cache and miss if the origin header is present or different?
Comment | File | Size | Author |
---|---|---|---|
#66 | drupal-n3001809-66-100x.patch | 4.87 KB | DamienMcKenna |
#56 | interdiff_52-56.txt | 3.51 KB | ravi.shankar |
#56 | 3001809-56.patch | 4.58 KB | ravi.shankar |
#52 | interdiff_42-52.txt | 3.53 KB | ravi.shankar |
#52 | 3001809-52.patch | 4.51 KB | ravi.shankar |
Issue fork drupal-3001809
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:
Comments
Comment #2
znerol CreditAttribution: znerol commentedYes. The
Vary
response header must be identical on every response from the same route. Otherwise this will lead to problems with caching proxies (and even the browser cache). This is whyVary: Cookie
header is added unconditionally to every response - even on anonymous requests.Looks like the affected code is located in asm89/stack-cors, thus this needs to be fixed over there.
Comment #3
Wim LeersGreat find!
Interestingly,
asm89/stack-cors
does have test coverage for doing exactly this: https://github.com/asm89/stack-cors/blob/c163e2b614550aedcf71165db2473d9.... The problem is that it does so conditionally: it only does this for request that are:Origin
headerAnd that conditionality is exactly what is the problem here of course: the
Vary
header must always be set. Nobody reported this yet at https://github.com/asm89/stack-cors/issues?utf8=%E2%9C%93&q=vary.I guess the assumption that
asm89/stack-cors
makes is that any request that contains aOrigin
header is not allowed to be served a response from a reverse proxy (such as Drupal's built-inpage_cache
anddynamic_page_cache
). I wonder what Varnish does? Based on https://stackoverflow.com/questions/43998474/varnish-caching-options-cor..., it also doesn't handle this automatically?P.S.: all AJAX requests that Drupal makes are POST requests, which are never cached.
Comment #4
Josh Waihi CreditAttribution: Josh Waihi as a volunteer commentedBut Drupal makes the decision that all routes could be CORS responses, so isn't it Drupal's job to provide the Origin key in the Vary header? As I'd imagine not all applications that use asm89/stack-cors require all responses in there application to be CORS ready?
Ugh. Unfortunately true.
Comment #5
Wim LeersThe
laravel-cors
middleware (which also reusesasm89/stack-cors
) had a similar discussion >4 years ago in https://github.com/barryvdh/laravel-cors/issues/20. https://github.com/barryvdh/laravel-cors/issues/20#issuecomment-50369259 says exactly what you say in #4. That led to https://github.com/asm89/stack-cors/pull/11, which we (Drupal) helped land.Right — Drupal doesn't have this luxury because it needs routing to happen before it can even know whether a particular URL will need to vary by
Origin
.Which brings me to the same conclusion as you:
I think you're right.
But this is a pretty big change. In principle, certain responses may also be available to anonymous users. Which means that Drupal core's Page Cache would also need to vary by
Origin
(because certain origins may need to get a 403). Which would make it far less effective.Before we make any changes to core, let's add a failing test that demonstrates the problem.
Comment #6
Wim LeersNote: if this is correct:
Then we'll need changes like this:
Comment #7
Wim Leers#5 passes for me locally. The existing test coverage in HEAD proves that CORS runs after Page Cache:
Note how both are a cache hit as far as Page Cache is concerned, yet they have different response headers.
This means that also a 403 due to CORS reasons should still be able to happen. Let's expand the test coverage to prove this.
Comment #8
Wim LeersD'oh, ignore #7, that test coverage already existed a bit further down 😅
Comment #10
Wim Leers#5 set the foundation in that it adds a request to the same URL without specifying the
Origin
request header.This patch then adds assertions for the
Vary
header that match the current behavior.Comment #11
Wim LeersThis is how it works today, but this is wrong. A single request without an
Origin
header would prime the reverse proxy's cache (and Drupal core's Page Cache, which is just a simple built-in reverse proxy) and result in incorrect responses.This will fail. 🔥
Comment #12
Wim LeersMy analysis in #6 is wrong, because of the reasons stated in #7: the CORS middleware runs after the Page Cache middleware.
This is also why Drupal core is not affected by this bug: because the CORS middleware (priority 250) wraps the Page Cache middleware (priority 200). This was done in #2850034: CORS allow-origin '*' not possible because of cached headers without thinking through the consequences for reverse proxies.
If we're to follow this pattern, then the recommendation would be for reverse proxies wrapping Drupal to do CORS themselves, and not use Drupal's native support for this.
That would of course require infrastructure-level configurability that doesn't exist everywhere.
So the simplest alternative would be to indeed add the
Vary: Origin
response header to all responses, until https://github.com/asm89/stack-cors/issues/49 (just opened that) gets solved.Comment #13
Wim LeersWithout this, #12 will still fail because
\Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
ends up calling\Drupal\Core\EventSubscriber\FinishResponseSubscriber::setResponseNotCacheable()
, which does$response->setVary(NULL)
.Why is that called? Because by default,
cache.page.max_age === 0
, which means that no responses from Drupal are allowed to be cached by any intermediary (such as a reverse proxy), and hence it's okay to throw awayVary
.So this allows intermediary caching, and in doing so it also allows us to test
Vary
sanely.Comment #17
Wim LeersI forgot that DrupalCI's test env somehow always adds
Accept-Encoding
but local test runner don't 🙃Comment #18
sebas5384 CreditAttribution: sebas5384 at Taller commentedHi,
I can confirm this issue because on my case I'm having multiple clients requesting the same (API) so when te request is from the server which doesn't has any
Origin
header then when the browser requests it's missing theAccess-Control-Allow-Origin
.Btw, thanks for this issue! what's missing for this patch to be RTBC ?
Comment #19
sebas5384 CreditAttribution: sebas5384 at Taller commentedHey @Wim Leers ! after I applied the #12 patch the
Cookie
value was removed from theOrigin
header, do you have any ideia of why?thanks!!
Comment #20
Wim Leers#19: no, no idea :( But
PageCacheTest
is failing for a similar reason, so that problem you're observing must also be causing at least that test to fail. It'd be great if you could debug this and update the patch… 🙏 😊Comment #21
sebas5384 CreditAttribution: sebas5384 at Taller commentedHey @Wim Leers, I just changed the order since this code at the
setResponseCacheable
method:is not adding the
Cookie
in case there's already content in theVary
header, but since I don't know what's the reason of this I just changed the order to add theOrigin
after the method execution, attached there's a rerolled patch and also I added thecookie
value to our tests.I hope this make's sense :P
Comment #22
sebas5384 CreditAttribution: sebas5384 at Taller commentedTrying to pass the tests now.
Comment #23
sebas5384 CreditAttribution: sebas5384 at Taller commentedThis patch it's for the 8.6.x
Comment #24
tjwelde CreditAttribution: tjwelde commentedWe port a site to headless currently and are using GraphQL requests, with APQ (which optimises the queries), which means that we can send those requests as GET and cache them.
We also do server-side-rendering, so the GraphQL requests come either from a server, or a client.
A big issue was, that Acquias Varnish proxy threw away access-control-allow-origin headers, when being requested without origin header. We could circumvent it temporarily, by setting an origin header on our server request, but the underlying issue remained.
Anyone knowing the issue could potentially easily exploit it and make the site unusable for everyone (since without cors header, no data could be requested by clients).
This patch fixed this issue.
Thanks for the effort!
Comment #26
oknateReroll for 8.7, dropping tests, as I don't have time to investigate them right now.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedAfter applying the patch in #26, I'm still getting an issue with 403 errors on AJAX'd request with an explicit `allowedOrigin` set.
Comment #29
mr.baileysI created a pull request against asm89/stack-cors to (optionally) always output the Vary: Origin header.
Comment #30
mr.baileysAttempt to reroll #23 against 8.9.x, no other changes (although there were some conflicts during rebase).
Comment #32
mr.baileysSeems the only test failure is caused by the order in which the headers in the vary-header are listed. Rather than checking the full vary-header string, we should just check for the presence of the 'origin' header in it (since we want to verify that 'Origin' is added to the 'Vary'-header), but for now let's see if we can get the tests to pass by just changing the order.
Comment #33
mr.baileysTook me way too long to figure out that running drush core tests with nginx is not the best idea.
Switched the test to just verifying the presence of Origin in the Vary-header. Interdiff is against #30, not #32. Passes locally.
Comment #35
Wim Leers@oknate Want to review this? :)
Comment #36
Lukas von BlarerThe patch in #33 worked for me to resolve caching issues with varnish.
Comment #37
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedThis has been fixed upstream. Updated the library in #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability
Comment #39
andypostThe issue fixed
need void type to return
Comment #40
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedandypost, can you take a look at #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability? That fixes this issue.
Comment #41
andypostThank you, upgrade to 2.0 helps, guess it could be closed as duplicate
Comment #42
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have rerolled patch #33 and addressed comment #39.
Comment #44
davidwbarratt CreditAttribution: davidwbarratt as a volunteer commentedIf #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability is resovled, I'm not sure why this patch would be needed any longer?
Comment #48
andypostThe related #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability is mostly ready to be merged to 10.0.x
Comment #49
erin.rasmussen CreditAttribution: erin.rasmussen at Acquia commentedIt makes sense to me that the fix in #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability will be released as a part of d10. We don't want to introduce big changes in a minor release. Is there a way to update Drupal 9 to use asm89/stack-cors to ^2.0 ?
What would be the best way to apply to Drupal 9 for testing?
Comment #50
DamienMcKennaTagging this "Pantheon" because it seems to help CORS errors that are logged when running a site on Pantheon.
Comment #51
mstrelan CreditAttribution: mstrelan at PreviousNext commented@erin.rasmussen you can alias the package version.
composer require "asm89/stack-cors:2.0.5 as 1.3.0"
Comment #52
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed test of patch #42.
Comment #53
andypostProbably #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability could be backported to 9.x
Comment #55
useernamee CreditAttribution: useernamee at drunomics commentedI've ran into this issue only recently on drupal 9.3.11 and varnish-5.2.1 (on amazeeio hosting). After applying #52 patch the issue was solved. If I add some more context we are having a decoupled website and the request to rest menu items was failing due to missing
access-control-allow-origin
header. So thanks to everyone working on this issue. I'd say it can go into RTBC after tests are passingComment #56
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed tests of patch #52.
Comment #57
useernamee CreditAttribution: useernamee at drunomics commentedGreat @ravi.shankar, I'm moving the ticket to RTBC.
Comment #60
catchMarking this duplicate of #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability. Sites can alias the cors version for Drupal 9. If that's not enough, then we could look at backporting the cors update after all but I don't think we should workaround it.
Comment #61
Dave Reid#3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability is likely not going to be backported to D9.5 though, given this is a bug, we should probably still have this open for resolving here?
Comment #62
Dave ReidComment #65
dpagini CreditAttribution: dpagini as a volunteer commentedWhen reading through this issue, I think this should ONLY be tagged for branch 9.5.x, which is about to (3 months) be marked unsupported, potentially killing this issue. But yeah, I don't think this is relevant to 10.x or 11.x at all, since they are using the new 2.x asm89/stack-cors library. Does anyone disagree with that?
Comment #66
DamienMcKennaRerolled for 10.0.x that also applies to 11.x.
Comment #67
smustgrave CreditAttribution: smustgrave at Mobomo commentedHave not reviewed but seems to have a test failure.