[SEMVER-MAJOR] Remove built-in CORS middleware #352
Conversation
bajtos
force-pushed
the
feature/drop-cors
branch
from
9f836d0 to
9169bc3
Compare
| .end(function(err, res) { | ||
| assert(res.get('Access-Control-Allow-Origin') === undefined); | ||
| assert(res.get('Access-Control-Allow-Credentials') === undefined); | ||
| .expect(200, function(err, res) { |
There was a problem hiding this comment.
When you make a cross origin request, you wouldn't get a 200 would you?
There was a problem hiding this comment.
200 may not be the right http status code, test should check for the appropriate error code
There was a problem hiding this comment.
In my understanding of CORS, it's the responsibility of the browser (the client) to enforce cross-origin policies. The server is required to describe which origins are allowed to access it from the browser.
See https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS (emphasis is mine)
The Cross-Origin Resource Sharing standard works by adding new HTTP headers that allow servers to describe the set of origins that are permitted to read that information using a web browser. Additionally, for HTTP request methods that can cause side-effects on user data (in particular, for HTTP methods other than GET, or for POST usage with certain MIME types), the specification mandates that browsers "preflight" the request, soliciting supported methods from the server with an HTTP OPTIONS request method, and then, upon "approval" from the server, sending the actual request with the actual HTTP request method. Servers can also notify clients whether "credentials" (including Cookies and HTTP Authentication data) should be sent with requests.
In our tests here, we are not acting like a browser (the supertest library is an HTTP agent only, not a browser-like client) and therefore the status code 200 is expected.
|
Was the affect on API Connect taken into consideration? I know that CORS support is configured at the gateway level there, just something to consider if it hasn't been already. |
|
Thank you @richardpringle and @deepakrkris for the review.
Yes. There are two parts to the integration with API Connect. As I explained in the pull request description, applications scaffolded by our generator ( When the LoopBack app is run behind APIC gateway, the gateway will provide CORS headers as specified in Gateway's config. As a result, any application-provided CORS headers should be ignored. I was pondering the idea of disabling CORS in the LoopBack application that are expected to run in APIC environment, but @raymondfeng mentioned good reasons why we should keep CORS in the LoopBack app too:
|
I have enabled GitHub checks to require each pull request to get an approval before it can be merged. @richardpringle @deepakrkris when the code LGTY, please use the new pull request UI (docs) for approving the patch. |
Push the responsibility of enabling/configuring CORS back to the application developer.
bajtos
force-pushed
the
feature/drop-cors
branch
from
9169bc3 to
b0761b1
Compare
|
The build is failing on |
|
LGTM |
Following the discussion in #293, I would like to revisit the way how CORS is configured in LoopBack applications.
There are two types of applications that are commonly built using LoopBack, and each type has different requirements when it comes to CORS.
So far, we were enabling CORS by default to make it easy to build public facing API servers. Unfortunately, this was also making API+SPA sites powered by strong-remoting unsecure by default. What's even worse, this insecure setting was not immediately obvious.
In this pull request, I am removing CORS from strong-remoting, to push the responsibility of enabling/configuring CORS back to the application developer.
Note that this change does not affect LoopBack applications scaffolded by a recent version of
slc loopbackorapic loopback, as these applications don't rely on the built-in CORS middleware - they configure CORS explicitly inserver/middleware.json. (See the project template in loopback-workspace, it is disabling the built-in CORS middleware here and explicitly configuring CORS for the entire server here.@richardpringle @STRML please review
cc @gunjpan @deepakrkris @raymondfeng @ritch