[SEMVER-MAJOR] Don't send Access-Control-Allow-Credentials by default.

[STRML]

This is a very dangerous header to be sending. Given that strong-remoting/loopback encourages the use of accessToken cookies, I see no reason why we should not have secure defaults and make the user decide whether or not to open this vector.

[STRML]

Just a note - these were always undefined as the headers are lowercase.

[bajtos]

@STRML thank you for the pull request. I don't have a strong opinion on this change, therefore I am fine to go with your proposal.

@raymondfeng @ritch @fabien @strongloop/loopback-dev any objections?

I feel this is a breaking change that should not be back-ported to 2.x. However, we can change the setting value used by applications scaffolded via slc loopback in loopback-workspace here: templates/projects/empty-server/files/server/middleware.json#L10. That way any new LoopBack projects started via our code generator would immediately get the new (more secure) configuration.

[STRML]

I would label this as a security fix and release as semver-minor with 2.x.

[bajtos]

@STRML

This is a very dangerous header to be sending. Given that strong-remoting/loopback encourages the use of accessToken cookies, I see no reason why we should not have secure defaults and make the user decide whether or not to open this vector.

I am afraid this is not true, we don't encourage using accessToken cookies. The minimal loopback application relies on Authorization header:

var app = loopback();
app.use(loopback.rest());
app.listen();

Also IIUC CORS docs at MDN correctly, Access-Control-Allow-Credentials is required for requests using cookies too.

Line 7 shows the flag on XMLHttpRequest that has to be set in order to make the invocation with Cookies, namely the withCredentials boolean value.

I am closing this pull request as rejected.

@STRML I am happy to change my mind if you can provide more arguments supporting your case.

[STRML]

@bajtos

Two things:

1. There is a bug in the tests that uses req.headers instead of req.get(). Regardless of whether or not this PR is accepted, that should be fixed.
2. Other projects, such as loopback-component-passport are using cookies, token middleware searches for cookies by default, and there are many good reasons to use them (such as auth in the API explorer).

I strongly believe we should have secure defaults and Access-Control-Allow-Credentials is extremely insecure (if you use cookies, any JS on any site anywhere can access any method). I would also argue that CORS itself should be off by default. Developers should opt-in to lowsec (just like local databases should have random passwords by default), rather than rudely discover it the hard way.

[bajtos]

@STRML thanks, let's reopen the PR and the discussion.

There is a bug in the tests that uses req.headers instead of req.get(). Regardless of whether or not this PR is accepted, that should be fixed.

Good point, I agree.

Other projects, such as loopback-component-passport are using cookies, token middleware searches for cookies by default, and there are many good reasons to use them (such as auth in the API explorer).

As far as I understand Access-Control-Allow-Credentials, it covers cookie-based authentication too. So you are basically proposing a default CORS configuration that prevents any authenticated requests originating from a different domain, allowing only anonymous cross-origin requests. Unless I am missing something?

I strongly believe we should have secure defaults and Access-Control-Allow-Credentials is extremely insecure (if you use cookies, any JS on any site anywhere can access any method). I would also argue that CORS itself should be off by default. Developers should opt-in to lowsec (just like local databases should have random passwords by default), rather than rudely discover it the hard way.

I understand your point of view and I agree the default configuration should be secure.

From my point of view, loopback/strong-remoting is for building REST API servers. In my (possibly limited) experience, most APIs are accessed by cross-origin clients, therefore it makes sense to make LoopBack apps CORS-enabled by default.

BTW the current defaults were introduced by #93 in response to a mailing-list discussion where people got confused about how to configure CORS in their apps: https://groups.google.com/forum/#!topic/loopbackjs/y7vB4RFDS0E

Let me also describe the bigger picture. LoopBack applications scaffolded via yo loopback (or slc loopback or apic loopback) don't rely on strong-remoting's built-in CORS implementation. They disable it server/config.json and then explicitly configure a global CORS middleware in server/middleware-config.json.

In that light, perhaps this will be the best approach:

• Remove CORS from strong-remoting completely, i.e. reject cross-origin requests. That way people that are manually composing building blocks like strong-remoting will be forced to make an intentional decision about CORS.
• In loopback-workspace templates used to scaffold LoopBack applications, we should keep the current default CORS configuration with { origin: true, credentials: true }, because that's what most people developing API servers expect.

@raymondfeng @ritch @qpresley @rmg do you have any opinion on this topic?

[rmg]

If I understand the conversation so far, the current defaults are:

• appropriate for public facing APIs serving a variety of clients
• not appropriate API+SPA uses since the API should only be serving its own browser clients

Does that sum up the challenges with this PR?

Changing the default in the module definitely seems like a semver-major change, since anyone relying on it will be very disappointed if their app suddenly stops working from a semver-minor or semver-patch update. This seems pretty straight forward as far as impact of the change.

Updating the scaffolding to include a description of the setting and what the implications are for the different use-cases sounds like the right way to address this for loopback 2.x. Explicitly setting configurable values to their defaults is generally a good idea for scaffolding since it prevents accidental breakage from changes in default values like this.

[STRML]

Does that sum up the challenges with this PR?

Sounds right to me. Our use case is API+SPA where this default is unsafe.

[bajtos]

Closing in favour of #352.