Problem If I set allowedOrigins

So the opposite of <a class="issue-link js-issue-link" data-error-text="Failed to load

So the opposite of <a class="issue-link js-issue-link" data-error-text="F

Lastly, we should deal with this: <div

Only Vary the request when necessary about stack-cors HOT 19 CLOSED

asm89 commented on August 23, 2024

Only Vary the request when necessary

from stack-cors.

Comments (19)

barryvdh commented on August 23, 2024

So the opposite of #49 ?

I understand the case, if you don't use credentials, I think * is probably fine and better for caching indeed.

from stack-cors.

davidbarratt commented on August 23, 2024

So the opposite of #49 ?

Uhh... kind of. From reading #49 it seems a little misguided. If the the request needs to be varried by the Origin then it should always add it (even if it's not necessarily a cors request, as it could become one). If it doesn't need to be, it shouldn't be. I think adding this as configuration is more confusing.

In other words, this logic doesn't make any sense:

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

Line 65 in 82ede75

return $request->headers->has('Origin') && !$this->isSameHost($request);

because if the request is missing the Origin header (for wahtever reason) or comes from the same Origin it gets cached as a non-cors request, which would break all subsequent cross-origin requests. :(

I think it should follow this logic:

if ( $config['supportsCredentials'] === true || $config['allowedOrigins'] !== ['*']) {
  // Set or append `Origin` to `Vary` header on all requests.
}

from stack-cors.

davidbarratt commented on August 23, 2024

FYI: This is still an issue because of:

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

Lines 48 to 50 in 561a5ee

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

 return $this->app->handle($request, $type, $catch); 

 }

from stack-cors.

davidbarratt commented on August 23, 2024

Lastly, we should deal with this:

stack-cors/src/Cors.php

Lines 48 to 50 in 456655a

 if ($this->cors->isPreflightRequest($request)) { 

 return $this->cors->handlePreflightRequest($request); 

 }

at a minimum, it should check that the method is OPTIONS first:

stack-cors/src/CorsService.php

Lines 70 to 75 in 456655a

 public function isPreflightRequest(Request $request) 

 { 

 return $this->isCorsRequest($request) 

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

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

 }

from stack-cors.

barryvdh commented on August 23, 2024

Can we close this now?

from stack-cors.

davidbarratt commented on August 23, 2024

Can we close this now?

Yep! It looks so beautiful! :) Thank you so much!

from stack-cors.

barryvdh commented on August 23, 2024

Okay I've tagged a 2.0.0-beta1 so please test it out :)

from stack-cors.

davidbarratt commented on August 23, 2024

Okay I've tagged a 2.0.0-beta1 so please test it out :)

Oh where was the breaking change?

from stack-cors.

barryvdh commented on August 23, 2024

Well, the behavior has changed a bit imho, not sure if that qualifies as actually breaking. Is that a problem?

from stack-cors.

davidbarratt commented on August 23, 2024

Well, the behavior has changed a bit imho, not sure if that qualifies as actually breaking. Is that a problem?

I have no idea, I can submit the Drupal patch. :)

from stack-cors.

davidbarratt commented on August 23, 2024

Looking at https://semver.org/ it seems like this would be a minor since we didn't change the public API at all. The breaking change would be removing the methods we no longer use. :) but your call. :)

from stack-cors.

davidbarratt commented on August 23, 2024

I guess the question is: Does this change break backwards compatability? It doesn't seem like it does. I could argue that it's actually a bug fix and not even a new feature.

from stack-cors.

davidbarratt commented on August 23, 2024

Seems like it would be wise to release a beta version of the point release (i.e. 1.4.0-beta1) though.

from stack-cors.

GrahamCampbell commented on August 23, 2024

A breaking change to semantics is also a breaking change, even if there's no breaking changes to the signatures.

from stack-cors.

GrahamCampbell commented on August 23, 2024

2.0.0-beta1 was the right choice.

from stack-cors.

davidbarratt commented on August 23, 2024

A breaking change to semantics is also a breaking change, even if there's no breaking changes to the signatures.

https://semver.org/ defines a breaking change as a change in the "Public API" not to the semantics, but true you may define the Public API however you want. :)

from stack-cors.

GrahamCampbell commented on August 23, 2024

A change to the semantics IS a breaking change to the public API...

from stack-cors.

davidbarratt commented on August 23, 2024

A change to the semantics IS a breaking change to the public API...

well I'm not sure how anyone could argue with that level of circular reasoning.

from stack-cors.

davidbarratt commented on August 23, 2024

Here's the issue in Drupal's Queue
https://www.drupal.org/project/drupal/issues/3128982

from stack-cors.

Only Vary the request when necessary about stack-cors HOT 19 CLOSED

Comments (19)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	if (!$this->cors->isCorsRequest($request)) {
	return $this->app->handle($request, $type, $catch);
	}

	if ($this->cors->isPreflightRequest($request)) {
	return $this->cors->handlePreflightRequest($request);
	}

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