Make responses cacheable by only using dynamic origins when required #70

barryvdh · 2020-04-05T14:52:40Z

If a simple origin check works, leave it to the browser, instead of changing the Origin ourselves.

Fixes #65

davidbarratt

This still checks the request's Origin header here:

stack-cors/src/Asm89/Stack/Cors.php

Line 48 in b1bcef5

if (!$this->cors->isCorsRequest($request)) {

By accessing the Origin header, all requests need to varied by it.

src/Asm89/Stack/CorsService.php

davidbarratt · 2020-04-05T16:41:39Z

src/Asm89/Stack/CorsService.php

        } else {
-            $response->headers->set('Vary', $response->headers->get('Vary') . ', Origin');
+            $response->headers->set('Access-Control-Allow-Origin', implode(', ', $this->options['allowedOrigins']));


This could be empty.

You are right, so what would expected behavior be then? If both the patterns (case above) and the origins array is empty, no Origin will be considered valid. So don't add the header?

Also, I missed this, but Access-Control-Allow-Origin only supports a single value. It can either be * or a single origin.

If allowedOrigins and allowedOriginsPatterns are empty, this header should not be added.
if allowedOriginsPatterns is empty, then we know there is a possability of a static value, but the header can only be static if there is either a single value in allowedOrigins and that value is not * OR the value is * and supportsCredentials is false

I've updated it:

Wildcard + no credentials -> just '*', no Vary

Just 1 origin allowed, no wildcard/patterns -> Just the static value, no Vary

Always add Vary + check if Origin is allowed. Add origin if allowed, skip otherwise.

barryvdh · 2020-04-05T16:52:43Z

This still checks the request's Origin header here:

stack-cors/src/Asm89/Stack/Cors.php

Line 48 in b1bcef5

if (!$this->cors->isCorsRequest($request)) {

By accessing the Origin header, all requests need to varied by it.

You are right, I've removed that check.

barryvdh · 2020-04-05T17:57:50Z

I've updated it based on your comments.

davidbarratt

One small problem with all requests.

Do you want to handle the cachability of preflight requests in a seperate PR?

src/Asm89/Stack/Cors.php

barryvdh · 2020-04-06T07:52:01Z

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers it should respond with an error if the method is not supported. Does that mean we should add a Vary header? (Like https://stackoverflow.com/a/45081016/444215)

barryvdh · 2020-04-06T07:55:17Z

Also, this would mean we would need to intercept ALL Options requests and add the headers. Not sure what the impact would be there.

barryvdh · 2020-04-06T10:05:13Z

src/Asm89/Stack/CorsService.php

+            if ($this->options['supportsCredentials']) {
+                $allowMethods = strtoupper($request->headers->get('Access-Control-Request-Method'));
+                $this->varyHeader('Access-Control-Request-Method');
+                $maxAge = -1;


Not sure if either of these are required, or that maxAge is implied to be different on different headers.

It should follow the same logic as the main request I think. Basically make it static if if it can be, otherwise Vary it by the header being accessed.

Yes, in this case I think it's unnecessary to change the maxAge and add a Vary. The browser will know if the request header has changed (according to the Vary) and ignore the cache.

davidbarratt · 2020-04-06T13:34:50Z

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers it should respond with an error if the method is not supported. Does that mean we should add a Vary header? (Like https://stackoverflow.com/a/45081016/444215)

That's a rather odd line, I looked at the fetch spec, and it says:

If request’s method is not in methods, request’s method is not a CORS-safelisted method, and request’s credentials mode is "include" or methods does not contain *, then return a network error.
https://fetch.spec.whatwg.org/#cors-preflight-fetch

So it wont make the request if the method is not included in the list or it's a * (if I'm reading that correctly). It seems like a mistake in Mozilla's documentation, but to be fair, most of the time the serrver is going to respond with an error if it hasn't implemented that method at that route anyways. But yes, if we access the header, we need to Vary by that header on all requests.

Also, this would mean we would need to intercept ALL Options requests and add the headers. Not sure what the impact would be there.

I thought about that, and I implemented that in the PR I made. I think it's safe to do that because it would intercept all uncached Options requests. In the same way that we are now (with these changes) intercepting all uncached main requests. Though I am not certain how things happen when the method isn't impelemented in the app, I assume it throws...

barryvdh · 2020-04-10T13:29:03Z

I looked at https://github.com/expressjs/cors/blob/master/lib/index.js for a bit and think it's fine to skip the 403/405 errors and just add the headers for the preflight. I skipped max-age to -1 because I'm not really sure if thats good or bad. But easy enough for anyone to configure if they need to.

I've made allowed-methods always static by just a list of methods instead of * when credentials are used.

Also refactored a bit to avoid duplication and more logical names. But it's quite a bit more changes than I'd hoped..

barryvdh · 2020-04-10T14:01:34Z

@asm89 Do you have an opinion about removing the validity checks?

davidbarratt · 2020-04-10T22:31:48Z

src/Asm89/Stack/CorsService.php

@@ -72,134 +67,159 @@ public function isPreflightRequest(Request $request)
            && $request->headers->has('Access-Control-Request-Method');


Calling this method requires adding Vary: Origin, Access-Control-Request-Method, but doing so will add it to every request.

To avoid that, maybe refactor to something like this:

// Cache is already varied by method. if ($request->getMethod() !== 'OPTIONS) { return false; } // @TODO Get the response if we are **not** taking it over. $this->addVary($response, 'Origin, Access-Control-Request-Method'); return $this->isCorsRequest($request) && $request->headers->has('Access-Control-Request-Method');

which will only Vary the OPTIONS requests (which is way better, imho).

or even better would be to change this in Cors::handle() like this:

if ($request->getMethod() === 'OPTIONS') { if ($this->cors->isPreflightRequest($request)) { $response = $this->cors->handlePreflightRequest($request); } else { $response = $this->app->handle($request, $type, $catch); $this->cors->addActualRequestHeaders($response, $request); } // @TODO May need to dedupe the headers. $this->cors->addVary($response, 'Origin, Access-Control-Request-Method'); return $response; }

This way it's either taken over, or passed, but regardless, get the Vary header added.

Basically we would either need to Vary origin for ALL responses, or remove the cors check everywhere?
Maybe we could just add Vary origin in the middleware always, and remove the check if the methods header is set?

Basically we would either need to Vary origin for ALL responses, or remove the cors check everywhere?

So what happens if a request gets sent like this:

OPTIONS https://example.com/

And then later:

OPTIONS https://example.com/ Origin: https://example.org/

And then later:

OPTIONS https://example.com/ Origin: https://example.org/ Access-Control-Request-Method: PATCH

Without adding Vary: Origin, Access-Control-Request-Method to all of the OPTIONS requests, the first would be cached, the second and third responses would result in the same response as the first and the last would be missing the Preflight headers. :(

Maybe we could just add Vary origin in the middleware always, and remove the check if the methods header is set?

I think you may have that backwards, whatever we are checking for first needs to be in the Vary for all requests. If the thing we fail on is Access-Control-Request-Headers instead of Origin then we would need to add:
Vary: Access-Control-Request-Method to all OPTIONS requests (which would make the above example pass). We could use that header as a way to determine if it's a CORS request or not. This would result in fewer cache variants than varying by Origin (which isn't always necessary anyways). If we then proceed to the origin check, we would need to append Vary: Origin to all OPTIONS requests where Vary: Access-Control-Request-Method is set.

The reason I would put Vary: Origin, Access-Control-Request-Method with the current Origin check is because of the way the condition is ordered:

stack-cors/src/Asm89/Stack/CorsService.php

Lines 70 to 72 in df153ce

return $this->isCorsRequest($request)

&& $request->getMethod() === 'OPTIONS'

&& $request->headers->has('Access-Control-Request-Method');

The first thing that is checked is the Origin rather than Access-Control-Request-Method. If we were to first check for Access-Control-Request-Method then we could simply add: Vary: Access-Control-Request-Method to all OPTIONS requests (and only add the Vary: Origin depending on the config like the main requests).

Though I don't think varying by the Access-Control-Request-Method is always necessary (as we've seen from the main request).

Like the primary requests, there isn't technically a need to check that it's a CORS request at all, as I've shown here:
https://github.com/davidbarratt/stack-cors/blob/22e70f5d51903f731e4608b7061e208d56f83db4/src/Asm89/Stack/Cors.php#L48-L61

That removes the CORS check completely, but the downside is, is that the application may not handle the request, if it doesn't we should provide a (static, if possible) response anyways.

The upside is that it increases cachability of the preflight because (depending on the config) you could get a completely static response that could be cached for a long time.

The only downside is if the OPTIONS request throws an error (rather than an simply not implemented) it would be hard to see the error. Though I suppose we could append a custom header with the error message or something so it's not completely opaque.

src/Asm89/Stack/CorsService.php

barryvdh · 2020-04-18T12:11:07Z

Sorry for taking so long at this. I'm just not really sure about adding the CORS headers on each request (eg overhead). On the one hand I kind of understand the issue with caching, but also it could be a bit of and edge case.
Maybe we can make it configurable? Something like alwaysAddHeaders which can be used if you use Varnish or something. If you cache in the php layer, you could just run CORS middleware over your cached response.

davidbarratt · 2020-04-18T12:56:34Z

Sorry for taking so long at this. I'm just not really sure about adding the CORS headers on each request (eg overhead). On the one hand I kind of understand the issue with caching, but also it could be a bit of and edge case.
Maybe we can make it configurable? Something like alwaysAddHeaders which can be used if you use Varnish or something. If you cache in the php layer, you could just run CORS middleware over your cached response.

I think the overhead only exists on unchached requests. Ironically, by increasing the cachability, you're decreasing the overhead, not increasing it. If the site/app doesn't cache it's requests that's a completely different problem and I think is outside the scope of this library.

barryvdh · 2020-04-18T13:00:20Z

Yeah but probably most API's are dynamic, right? At least for the Preflighted requests (not simple GET requests)

davidbarratt · 2020-04-18T13:02:36Z

Yeah but probably most API's are dynamic, right? At least for the Preflighted requests (not simple GET requests)

What makes you think it's being used on an API? In Drupal it's used on all requests (including HTML).

davidbarratt · 2020-04-18T13:03:07Z

And I would say most REST APIs are cached, which is why REST was chosen over GraphQL (or some other query based API)

davidbarratt · 2020-04-18T13:04:14Z

Regardless, I have a feeling adding the headers takes nanoseconds to do so we're talking about a nano optomization on uncached requests.

Making it more cachable skips the operation of the entire request more often.

davidbarratt · 2020-04-18T13:20:12Z

Maybe we can make it configurable? Something like alwaysAddHeaders which can be used if you use Varnish or something. If you cache in the php layer, you could just run CORS middleware over your cached response.

I think in that scenario, the config should be modifyVary to either modify the vary header, or not at all. But it's really weird to assume that Varnish, nor a CDN, nor the browser itself isn't going to at least try to cache it, so maybe the config could be the other way around? neverModifyVary ?

Again, I think the "overhead" is effectively non-existant, but if you're really concerned about it, I would either always modify it, or never modify it, there isn't really an inbetween where you sometimes modify it since that is useless in the caching case and also useless in the non-caching case.

barryvdh · 2020-04-18T13:25:08Z

I've removed it in #73

Only thing I'm not 100% sure about, it the OPTIONS request. Should we hijack the options request? Can we rely on all frameworks to handle OPTIONS properly? Some library explicitly require to send 204 + no content to avoid problems.

davidbarratt · 2020-04-18T13:26:03Z

Using this library at scale, I think it would be more helpful to have a config like preventOriginVary that would throw an exception if I had the config in such a way that would result in varying by the Origin (which could bring down the origin servers if you didn't catch it before deployment).

davidbarratt · 2020-04-18T13:29:35Z

Only thing I'm not 100% sure about, it the OPTIONS request. Should we hijack the options request? Can we rely on all frameworks to handle OPTIONS properly? Some library explicitly require to send 204 + no content to avoid problems.

I mean if their app is implementing OPTIONS with content (why would anyone do that?) I don't know why we would take it over just because the browser made the same request?

But, if that is a concern, we could Vary all OPTIONS requests with Vary: Access-Control-Request-Method which would resolve the problem and not break cachability that badly.

barryvdh · 2020-04-18T13:45:12Z

So like this? #74

davidbarratt · 2020-04-18T13:58:21Z

So like this? #74

Exactly like that. :)

barryvdh added 2 commits April 5, 2020 16:22

Remove CORS check in actual request, fixes #65

ba138d1

Only vary origin when actually required

3109cf2

This was referenced Apr 5, 2020

Access control is unnecessary #65

Closed

Add caching support and remove access control. Fixes #65 #64 #49 #17 #68

Closed

Fix cs

b1bcef5

davidbarratt suggested changes Apr 5, 2020

View reviewed changes

Remove check for CORS headers, always add them for caching

ea18782

Add static Origins when possible

e01da06

davidbarratt suggested changes Apr 5, 2020

View reviewed changes

src/Asm89/Stack/Cors.php Outdated Show resolved Hide resolved

barryvdh added 3 commits April 6, 2020 10:11

Set wildcard for headers

92ef0fc

Merge branch 'master' into feat-cacheableresponses

a5215b0

Force refresh for 'dynamic' preflight

fa36f3d

barryvdh commented Apr 6, 2020

View reviewed changes

barryvdh added 5 commits April 10, 2020 14:44

Set all headers for supportsCredentials

44a7a82

Tweak max-age

00f689a

Move methods to own function

956266a

Remove checks, make preflight extendible

be8632f

Clarify origin

c0be478

CS

b38ed5e

davidbarratt suggested changes Apr 10, 2020

View reviewed changes

Update vary headers, add tests

61dab97

Add check back for now

c29e79a

Update test

2f53586

barryvdh merged commit 561a5ee into master Apr 18, 2020

barryvdh mentioned this pull request Apr 18, 2020

Handle OPTIONS cacheable #74

Merged

barryvdh deleted the feat-cacheableresponses branch April 18, 2020 13:45

This was referenced Jun 6, 2021

Update dependency fruitcake/laravel-cors to v2 - autoclosed ResponseAMS/response-install#11

Closed

fix(deps): update dependency fruitcake/laravel-cors to v2 - autoclosed alouini333/DocDoc_api#34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make responses cacheable by only using dynamic origins when required #70

Make responses cacheable by only using dynamic origins when required #70

barryvdh commented Apr 5, 2020

davidbarratt left a comment

davidbarratt Apr 5, 2020

barryvdh Apr 5, 2020

davidbarratt Apr 5, 2020

barryvdh Apr 5, 2020 •

edited

barryvdh commented Apr 5, 2020

barryvdh commented Apr 5, 2020

davidbarratt left a comment

barryvdh commented Apr 6, 2020

barryvdh commented Apr 6, 2020

barryvdh Apr 6, 2020

davidbarratt Apr 6, 2020

davidbarratt commented Apr 6, 2020

barryvdh commented Apr 10, 2020

barryvdh commented Apr 10, 2020

davidbarratt Apr 10, 2020 •

edited

davidbarratt Apr 10, 2020 •

edited

barryvdh Apr 11, 2020

davidbarratt Apr 11, 2020 •

edited

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

davidbarratt commented Apr 18, 2020 •

edited

davidbarratt commented Apr 18, 2020

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

		@@ -72,134 +67,159 @@ public function isPreflightRequest(Request $request)
		&& $request->headers->has('Access-Control-Request-Method');

	return $this->isCorsRequest($request)
	&& $request->getMethod() === 'OPTIONS'
	&& $request->headers->has('Access-Control-Request-Method');

Make responses cacheable by only using dynamic origins when required #70

Make responses cacheable by only using dynamic origins when required #70

Conversation

barryvdh commented Apr 5, 2020

davidbarratt left a comment

Choose a reason for hiding this comment

davidbarratt Apr 5, 2020

Choose a reason for hiding this comment

barryvdh Apr 5, 2020

Choose a reason for hiding this comment

davidbarratt Apr 5, 2020

Choose a reason for hiding this comment

barryvdh Apr 5, 2020 • edited

Choose a reason for hiding this comment

barryvdh commented Apr 5, 2020

barryvdh commented Apr 5, 2020

davidbarratt left a comment

Choose a reason for hiding this comment

barryvdh commented Apr 6, 2020

barryvdh commented Apr 6, 2020

barryvdh Apr 6, 2020

Choose a reason for hiding this comment

davidbarratt Apr 6, 2020

Choose a reason for hiding this comment

davidbarratt commented Apr 6, 2020

barryvdh commented Apr 10, 2020

barryvdh commented Apr 10, 2020

davidbarratt Apr 10, 2020 • edited

Choose a reason for hiding this comment

davidbarratt Apr 10, 2020 • edited

Choose a reason for hiding this comment

barryvdh Apr 11, 2020

Choose a reason for hiding this comment

davidbarratt Apr 11, 2020 • edited

Choose a reason for hiding this comment

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

davidbarratt commented Apr 18, 2020 • edited

davidbarratt commented Apr 18, 2020

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

barryvdh commented Apr 18, 2020

davidbarratt commented Apr 18, 2020

barryvdh Apr 5, 2020 •

edited

davidbarratt Apr 10, 2020 •

edited

davidbarratt Apr 10, 2020 •

edited

davidbarratt Apr 11, 2020 •

edited

davidbarratt commented Apr 18, 2020 •

edited