[7.x] Add HandleCors middleware

[barryvdh]

Based on a discussion with Taylor, I've added a HandleCors middleware similar to TrustProxies. This is using https://github.com/fruitcake/laravel-cors (formerly barryvdh/laravel-cors), which is updated with a few breaking changes to v1 (still in dev, awaiting possible feedback for this integration).

This would make it easy to implement CORS handling in Laravel, based on path matching.
An alternative approach would be route middleware, but the biggest issue is that it wouldn't match 404-routes, which would in turn lead to difficult debugging issues.

An approach could be to add a fallback route for the API middleware though, if you prefer route middleware, instead of the paths option.

[GrahamCampbell]

Thanks for the PR. asm89/stack-cors#57 is kinda concerning. The original author killed the tests on PHP 5, but left the PHP version constraint as "at least 5.5.9" in the composer.json file.

[barryvdh]

Thanks, I've merged your PR.

[taylorotwell]

Would it make sense to either add the configuration file to this PR or stub out any properties on the middleware that people might use to configure this?

[barryvdh]

What would you prefer?
What do you think about global middleware vs route group?

Then I'll see how to best change both things.

[taylorotwell]

I'm open to suggestions on route group vs. global. Would it make more sense in the "api" route group? I'm not sure how people typically use the package. It's easy to move a package from being global to a route group or vice versa so it's not a huge problem either way.

Maybe go ahead and add the configuration file to this PR.

[barryvdh]

Group middleware doesnt' really properly match the OPTIONS requests in all cases, so for stability I would prefer global middleware.
I've added the options, but then you would just keep everything in the config instead of middleware? Should I still add the middleware in the App namespace?

I think most cases would be to just enable CORS for a single path, not very much difference in the other options (except maybe the origins and allowing of credentials).

Adding it to the route groups would perhaps also be confusing if we still match on the request URL. And we wouldn't want to add it by default to just everything because not everyone needs in on their API. That's why I chose the global middleware + route matching.

[driesvints]

I don't really know if adding the package by default is a good idea. It's a small thing to do it manually. If we're going to do this it might be better if we include everything into the core. Just my 2 cents.

[GrahamCampbell]

That's fair. I suppose we'd just move the Laravel side of things into the core, but not the asm89/stack-cors package.

[GrahamCampbell]

I also wonder if collision should be moved into the core?

[barryvdh]

Taylor asked me to look into integrating it, similar to Trusted Proxy.
I think adding it to laravel/laravel would make it more lightweight, if you don't want it, you can easily remove it. With laravel/framework that would be harder.
collision is more difficult because that is a dev-dependency, right?

[GrahamCampbell]

It doesn't stop the code from being in the core though?

[barryvdh]

Not sure what you mean exactly. If you put collision in the core, you will get it on both dev and non-dev installed.

[GrahamCampbell]

Not sure what you mean exactly. If you put collision in the core, you will get it on both dev and non-dev installed.

Sure, the code will exist, but it doesn't mean you have to load it.

[GrahamCampbell]

Laravel's src has test code in, but it doesn't mean you have to run it in the browser.

[barryvdh]

I feel like this is getting a bit off-topic. Let's focus on CORS:

• Do we want to add this to laravel/laravel by default
• If so, should we add the config,, the middleware or both?

[taylorotwell]

I've removed the middleware override and just used configuration file.

[LukeTowers]

@barryvdh @GrahamCampbell why wouldn't Laravel just absorb this package into the core rather than include it as an external dependency?

[driesvints]

@LukeTowers it already has a maintainer. Doesn't makes sense for us to take upon the burden ourselves.

[LukeTowers]

@driesvints fair enough, although personally it seems to be approaching left-pad territory in my mind. If we implement it in @octobercms we'll probably just absorb into our own core.

Please let me know if I'm missing any hidden complexity that tips the scale in favour of keeping it an external dependency, right now it seems like it's just a middleware and a config file.

[driesvints]

@LukeTowers it's more than just that. For example, Barry's package depends on asm89/stack-cors: https://github.com/fruitcake/laravel-cors/blob/master/composer.json#L22

We'd have to require that anyway. So it doesn't matters if we require the fruitcake one or that one. We'll always have an extra dependency.

[LukeTowers]

Ah, I see. Thanks for the additional information!

[barryvdh]

Currently I'm maintaing both asm89/stack-cors and the Laravel version fruitcake/laravel-cors. It is indeed a relative simple wrapper, but as a seperate package that I both maintain, it's easy to keep in sync/upgrade.

[barryvdh]

However, there might be some advantages to having it inside the core, mainly to have a different layer above the Middleware, which could make it easier to catch all the edge-cases (eg. when another Middleware returns its own response etc).

[driesvints]

@barryvdh would that mean both the fruitcake one and the asm89 one? Or just the middleware?

[barryvdh]

The fruitcake one, if you want to create your own Middleware (eg. https://github.com/fruitcake/laravel-cors/blob/master/src/HandleCors.php ) and service provider. You would still want to use stack-cors probably for the logic.

But looking at the code in https://github.com/laravel/framework/blob/0c93bf343dd54496b026e6fa900061c60421e8d5/src/Illuminate/Foundation/Http/Kernel.php#L104-L121, that could also probably be fixed with not using a middleware, but just the RequestHandled event (eg like in https://github.com/fruitcake/laravel-cors/pull/469/files) But removing the Middleware and adding it always would perhaps be a bit of breaking changes as CORS headers are then added when the middleware is not setup). But easier to set-up, eg just require fruitcake/laravel-cors to have cors, no need to both require the package + add the middleware.

[barryvdh]

But removing the fruitcake package and re-implementing it in the core would make it less easy to upgrade/change things (eg. for me when updating stack-cors), so not really sure what the benefit would be.

[driesvints]

@barryvdh yeah I agree. Think it's best that it stays a separate package.

[LukeTowers]

I would agree with having it a separate package as well, didn't realize this was the laravel/laravel repository and not the laravel/framework repository at first. Logic for absorbing packages would be different between the two :)