Add CORS header

[jnicklas]

When developing with Broccoli, sometimes one might want to serve up font files from the Broccoli server. However, certain browsers restrict access to font files based on CORS rules. This makes it impossible to have broccoli serve up font files to another application running on a separate port. This simply adds a CORS header which allows access to these files.

[stefanpenner]

I suspect the answer will be, this should be an option but not enabled by default. So if you would like this accepted it should likely become somehow configurable

[rwjblue]

@stefanpenner is spot on. If we pull this in, it needs to be optional and off by default...

[jonnii]

just out of interest, what's the danger of leaving this on by default? you'd only ever run this locally, right?

[stefanpenner]

@jonnii its the whole intranet exposed by your dev environment problem. Also sometimes people connect to prod data from dev.. The vector is unlikely but possible.

[timmfin]

Even if someone accidentally runs the broccoli server on production/public internet, this CORs header doesn't really change too much (IMHO). Anyone with a link directly to your broccoli server would already be see and browse your source before this change (plus link to CSS and JS files via <link> and <style> tags).

All the header change does is make it so other web pages (served on other domains) can make XHR requests to your files and hotlink assets that normally require same-origin privileges (such as web fonts, at least in Firefox and IE).

So from my perspective (note, no security expert here), I think this is worth being on by default. It will make development more convenient and request the number of support requests the project gets.

[stefanpenner]

@timmfin if this is coupled with another thing (like a proxy to your api server) the problem is real.

Ultimately making this configurable is the best of both worlds. Additionally, some people might be surprised when there local development setup works but production (which may lack the cors headers) doesn't work.

The opt-in makes it clear.

[timmfin]

Oh, ok. In that case you'd be allowing arbitrary XHR calls to your proxied API. And yeah, that is a different situation.

Maybe this makes things too complex, but the above issue could be mitigated by only making the broccoli middleware add the default CORs header when the Content-Type matches a CSS, JS, image, or web font type (and only on a GET).

[rwjblue]

I think my point is this:

• it is a potential attack vector (even if somewhat remote)
• it is not needed in a large percentage of cases

All I'm saying is that it should be opt-in (because it definitely adds value in some scenarios).

[jnicklas]

This header is only used when broccoli is serving static assets. There is no way to exploit this in any way, as far as I can see. The broccoli server is not meant to be run in production, this is stated multiple times it the broccoli documentation. Even if it were, @timmfin's analysis is spot on. The only thing the CORS header permits which wasn't permitted before are XHR requests to the static assets. Which is useless, and an equivalent action could already be performed in other ways. Also requesting certain assets which are restricted by browsers, which we want to be able to do, hence this PR ;)

The proxy server argument is not compelling either, since those would only be used if we called next(), and this header is not added in that case.

Seriously, this is just convenient and perfectly safe. There's no need for this to be behind an option flag.

[ahacking]

It might also be worth noting that someone with wget or curl doesn't need the CORS header to access static assets if broccoli is serving on interfaces other than localhost. In development not having CORS preflight header support just means you can't easily develop.

I still think it has to be configurable but for me its niether here nor there what the default should be if documentation is clear and concise I consider it lesss effort to enable than getting your CSP right.

[stefanpenner]

@jnicklas @ahacking I appreciate your feedback.

My suggestion remains the same -> exposing CORS headers as quick and easy opt in functionality.

I still think it has to be configurable but for me its niether here nor there what the default should be if documentation is clear and concise I consider it lesss effort to enable than getting your CSP right.

I suspect many scenarios were this may be a pain (modeling a multi-domain setup in development for proper alignment with production) also need these headers set in production. It seems natural to then opt-in. If development has enables CORs but production does one relies on them, it would also result in a frustrating situation.

The proxy server argument is not compelling either, since those would only be used if we called next(), and this header is not added in that case.

It is a common scenario and although the current PR may not be susceptible, the lack of regression tests to ensure this continues to work as the author intends is what gives me pause.

[jnicklas]

So dredging this up, since I am still running an npm link'ed broccoli on all machines in our office to get this fix in.

To address @stefanpenner's concerns:

1. Production and development inherently differ. Anyone who is serving assets statically in production is going to run into this sooner or later. That's what staging environments are for. "Oh, the fonts are not working, wonder why that is?", opens console, "Aha!" / "Wtf is CORS?". If this actually causes a serious problem for someone in production they have only themselves to blame.

2. Even if someone somehow broke the code to add the CORS header to requests to the hypothetical proxy server, it still wouldn't be a security issue. There are two kinds of CORS requests, simple and preflighted. Preflighted requests require an additional OPTION request before the action, and the server must provide Access-Control-Allow-Methods to specify which methods are permitted. Since we do not specify this header, a preflight request will never succeed.

Simple requests, which are basically GET and POST with certain content encodings, do not need preflight requests. It is only these requests that this PR permits. Why are the rules more relaxed for these requests? Because these can be performed without the use of XMLHttpRequest already, through cross origin form posts and cross origin resource loading respectively, which all browsers already permit. This is why we have CSRF protection in web frameworks. In short, enabling CORS on these kinds of requests doesn't expose any attack vector which can't already be exploited in other ways.

Let's not forget that CSRF is a session attack. If the app in question does not make any use of session data, and the broccoli server does not, then it cannot be exploited.

Personally I find it silly to make this optional. It just makes things needlessly frustrating for users. That being said, if after this you still feel the need to make this an option I will admit defeat and amend the PR. Your choice.

[stefanpenner]

So dredging this up, since I am still running an npm link'ed broccoli on all machines in our office to get this fix in.

Ya this is a problem, it should be configurable or some possible for you to get what you want with the official release. Having to link the project is a deficiency, and should be addressed.

@joliss your feedback would be great, as my above comments are mostly speculative assumptions of your opinion.

[joliss]

It seems that if we always add Access-Control-Allow-Origin: * as suggested by this PR, we make Broccoli users vulnerable to having all their file contents siphoned: For instance, an attacker might create an evil page http://wateringhole/, which performs lots of XHR requests to http://localhost:4200/... and sends the file contents back to http://wateringhole/.

Side note: It's true that we already allow cross-origin embedding e.g. via <script>, but this is much harder to exploit. I'd love to mitigate this, but MDN says that the only way to stop cross-origin embedding is to use a CSRF token. (I'm very surprised. Is this really true?) So pragmatically there seems to be nothing we can do about this.

Anyways, back to getting fonts working: We might add an option for a CORS header, so that users can explicitly allow access from their dev server only (e.g. http://localhost:3000, as opposed to *). However I'm really hesitant to add more options to the core Broccoli package, especially pre-1.0, just in terms of having more things to maintain. Perhaps we'll have a separately-versioned server package some time; then it's easier to add these things.

I don't suppose there is an optionless way to make CORS work for fonts without introducing security issues, is there? (Extension-sniffing doesn't count.)

In the absence of a nice optionless fix, can we revisit this in a few months?

[jnicklas]

Sorry to be taking so long to get back to this.

@joliss that's a really good point.

I've updated this PR to a more conservative option, where we're adding an access-control-allow-origin header only if the origin domain resolves to 127.0.0.1. The code isn't super pretty, but I'm not sure how to make it read better given that dns.lookup is async. Given that the attacker cannot influence the origin header, and wateringhole.com resolves to some other IP, this should prevent this attack.

Unfortunately this isn't an ideal solution since it doesn't allow fonts to be loaded from inside a VM for example, or anywhere else where we want to access them through an actual IP rather than a loopback IP.

[devongovett]

👍 on this. Came across this problem for both fonts and web worker scripts today. Any updates?

[stefanpenner]

web worker scripts today

I believe CORs headers won't help you as WebWorker spec has no awaresness of CORs. I reported this as a bug quite some time ago, but I don't believe any progress has been made.

context: http://www.w3.org/TR/workers/#dom-workerglobalscope-location
SO: http://stackoverflow.com/questions/20410119/cross-domain-web-worker

A work-around for workers is host them same origin, or consider a risky eval running worker. That accepts scripts as input. This is dubious at best but until the spec/vendors/browsers fix ^. I am uncertain of a better solution :( If someone has one feel free to share.

@devongovett but @joliss's comment: #208 (comment) is still the status of this issue.

[devongovett]

I worked around the Worker issue by using importScripts from within a worker created from a blob url, which oddly enough doesn't seem to enforce any sort of same-domain requirement.

var blob = new Blob(['importScripts("http://localhost:4200/worker.js");'], { type: 'text/javascript' });
var worker = new Worker(URL.createObjectURL(blob));

Can't seem to get around the font issue though.

[stefanpenner]

blob url,

ah yes, blob URLs.

I think when I attempted, i was just pwnd bug buggy behavior on old Android and lack of support on now "aged out" versions safari.

but i didn't realize:

importScripts from within a worker created from a blob url, which oddly enough doesn't seem to enforce any sort of same-domain requirement.

thats scary but interesting, will have to re-consider being entirely emo about WW's

[jnicklas]

@stefanpenner I have tried to address @joliss's concerns. The current iteration of this PR doesn't solve the issue completely, but it improves on the status quo. It should not be vulnerable to the sniffing method that @joliss was concerned about and it does not introduce a new option.

[joliss]

Do we need a DNS lookup here, or do you think we get away with comparing to localhost, 127.0.0.1, ::1 for performance?

[jnicklas]

The DNS lookup is convenient for people who run their dev servers on a local domain name (think /etc/hosts). Something like pow would work with this DNS lookup, whereas a simple string comparison wouldn't work. Maybe that's acceptable though, it would definitely make for prettier code.

[joliss]

Unfortunately this isn't an ideal solution since it doesn't allow fonts to be loaded from inside a VM for example, or anywhere else where we want to access them through an actual IP rather than a loopback IP.

I agree - it might be an acceptable partial solution though. I'm kind of on the fence.

Is what we're doing in this PR safe? I think it probably is, but I'm having trouble convincing myself that it's definitely safe.

@stefanpenner, @rwjblue, do you have an opinion on all this?

[stefanpenner]

If CORs must exist, my vote is for it to be an opt-in.

[stefanpenner]

I believe we should address this via @JakeDetels "enhanced customization of server options" #292

[lifeart]

related: #490