search

jdesrosiers / silex-cors-provider

A silex service provider that adds CORS services to silex
MIT License
78 stars 25 forks source link

Add conditional check when cors.allowOrigin is * #8

Closed jrschumacher closed 10 years ago

jrschumacher commented 10 years ago

When cors.allowOrigin is set to * (any) it currently defaults to null -- this fixes that.

jdesrosiers commented 10 years ago

Thanks for your contribution.

Although the current implementation handles the allow-all case by simply not setting cors.allowOrigin, it makes sense that people would expect to be able to use * for the allow-all case.

However, this project follows the CORS security best practice of never using * or an origin-list as the value of the Access-Control-Allow-Origin response header. If the Origin is allowed, it should echo the Origin request header in the Access-Control-Allow-Origin response header.

So, it can accept * for the cors.allowOrigin parameter, but it can not use * for the Access-Control-Allow-Origin response header. Also, please add a unit test that demonstrates your change and be sure to follow the PSR-2 code style standard.

jrschumacher commented 10 years ago

please add a unit test that demonstrates your change

Sure

Although the current implementation handles the allow-all case

Right, so it just returns the same Origin set?

jdesrosiers commented 10 years ago

Correct. The default functionality is to simply echo the Origin request header to the Access-Control-Allow-Origin response header.

jrschumacher commented 10 years ago

So wouldn't default functionality and wildcard essentially be the same?

So I guess my suggestion would be to just set it to null if wildcard is set to revert back to default.

jdesrosiers commented 10 years ago

Yep. It should be a pretty small change.

jrschumacher commented 10 years ago

Also since we are discussing this. A big problem for me is that the master branch is currently requiring unstable branch of silex. Are you familiar how to add this patch to 0.1.* so it works with Silex 1.0 and Silex 2.0?

jdesrosiers commented 10 years ago

Yes. This change will be tagged and will only have support for Silex 1.x. I will also fix the master branch to have support for both Silex 1.x and Silex 2.x. When Silex 2.x is officially released, I will tag a new version with support for both Silex 1.x and Silex 2.x.

jrschumacher commented 10 years ago

Awesome!

jdesrosiers commented 10 years ago

Manually merged to the v0 branch.