moleculer-web

moleculer-web copied to clipboard

Problem

When setting-up cors, if one would pass:

cors: {
  origin: "*"
}

...it would end up passing here:

https://github.com/moleculerjs/moleculer-web/blob/ad0686a76c8b0e60b1c9a03b9a35f5c4fd31eaf4/src/index.js#L1000-L1001

producing a header such as Access-Control-Allow-Origin: *

but just wrapping up in an array such as:

cors: {
  origin: ["*"],
}

...woud end up passing in a different condition:

https://github.com/moleculerjs/moleculer-web/blob/ad0686a76c8b0e60b1c9a03b9a35f5c4fd31eaf4/src/index.js#L1002-L1004

and further

https://github.com/moleculerjs/moleculer-web/blob/ad0686a76c8b0e60b1c9a03b9a35f5c4fd31eaf4/src/index.js#L962-L966

producing a header such as Access-Control-Allow-Origin: ${origin}

which i find personally very confusing api.

Proposal

Remove the condition which is here: https://github.com/moleculerjs/moleculer-web/blob/ad0686a76c8b0e60b1c9a03b9a35f5c4fd31eaf4/src/index.js#L1000-L1001 in favor of the later one since the result would be similar if not identical.

Oooo, could you explain in more detail?

Hi @icebob :) Thanks for answering!

So, as i explained, imho it's confusing that a signature of cors: { origin: "*" } would end up with a very different result than cors: { origin: ["*"] } (only thing changing is the array)

This is due to the 1st if case catching cors.origin === '*' (here) and responding res.setHeader("Access-Control-Allow-Origin", "*") (value of header being *).

If the value is an array though, it won't go in the 1st if, rather in the 2nd one else if (this.checkOrigin()) where a value of * is now considered a regexp wildcard ; for which the response will be res.setHeader("Access-Control-Allow-Origin", origin); reflecting req's origin.

My recommendation would be:

cors.origin value	expected result
false | undefined	no cors header (default should be super secure, not super open)
true	cors header reflects origin
string	cors header uses string blindly
(string | regexp)[]	if item is string, use ===, if item is regexp, use .test() → cors header reflects origin

if you want i can implement this as part of my current PR #215 instead, and add test on top. Let me know.

@y-nk great, thanks! Please implement it in the PR.