Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

es6-shim breaks Chrome App CSP #301

Closed
stewart opened this issue Nov 7, 2014 · 32 comments
Closed

es6-shim breaks Chrome App CSP #301

stewart opened this issue Nov 7, 2014 · 32 comments

Comments

@stewart
Copy link

stewart commented Nov 7, 2014

Unless sandboxed, es6-shim cannot run inside of Chrome Apps right now due to this line, which triggers the CSP, and is functionally an eval() statement.

I'm unsure if the best fix is reverting ceeb51c, or if a better solution can be found.

@ljharb
Copy link
Collaborator

ljharb commented Nov 8, 2014

It's definitely an eval statement, and it's the only way to reliably get the global object in any environment.

Are there no CSP directives that can allow for it?

I'm definitely open to another alternative to reliably get the global object if you can suggest one.

@stewart
Copy link
Author

stewart commented Nov 8, 2014

There are CSP directives to allow for eval() usage, but they're not permitted in packaged apps, only extensions.

If there's not a clear fix, for now would it make sense to add a caveat to the README indicating that the module will conflict with Content Security Policy in Chrome extensions/apps, and suggest the appropriate fixes (CSP directive or sandboxing)?

@rwaldron
Copy link
Contributor

rwaldron commented Nov 8, 2014

@ljharb What are the target platforms (general) for es6-shim? Just browser and node, right? (I'm looking for ways around new Function('return this;');

@AndersDJohnson
Copy link

+1 Ran into this problem today.

@ljharb
Copy link
Collaborator

ljharb commented Nov 17, 2014

The target platforms for es6-shim are currently all web browsers, desktop or mobile (ie, run as a normal web page through a normal protocol); and every released version of node (but practically, 0.6 and higher).

Additional platforms are always ideally supported but the decision what to support or not is generally on a case by case basis. #259 specifically was to support node-webkit (#258) which was broken by a fix to make this work in web workers (4e4dc69).

@anthonyryan1
Copy link

Skimming the previous issues, this is a summary of my understanding:

Browsers Node Node Webkit Web Workers
self yes yes
window yes yes broken
global yes broken

Unless I'm missing something on that table, it sounds like checking self, then window, then global and using the first one found would cover all these environments and avoid eval.

@ljharb
Copy link
Collaborator

ljharb commented Nov 30, 2014

That definitely seems like it would work :-) In addition, the es*-shims always assume they're the first to run, so shadowing/tampering with global objects isn't much of a concern. However, we don't have any node-webkit nor great web worker code coverage, and I'm worried about adding untested code paths.

It's really a shame that Function('return this')() is so simple, bulletproof, and unfakeable, and that there's no similar non-eval alternative.

If someone were able to prepare a PR that could add test coverage for node-webkit and ensure this path is covered for Web Workers as well, I'd be happy to then modify the getGlobal function to return self || window || global;.

(one idea that pops in my head to ensure correctness is testing that global.Array === [].constructor && global.RegExp === /a/g.constructor && global.Number === 42..constructor && global.String === ''.constructor && global.Function === function () {}.constructor && global.Object === ({}).constructor and throwing an exception if not)

@rwaldron
Copy link
Contributor

rwaldron commented Dec 1, 2014

However, we don't have any node-webkit

IIRC, you'll get both window and global.

@Yaffle
Copy link
Contributor

Yaffle commented Dec 1, 2014

can somebody explain me, why this cannot be used ?

(using factory(root) - https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L24 or define(function () { factory(root); }))

@ljharb
Copy link
Collaborator

ljharb commented Dec 1, 2014

When es6-shim is used in a global context and first-run, perhaps we could. But, whatever method we use needs to work in a CommonJS/AMD context as well, where "this" can not be guaranteed to be the global object.

@Yaffle
Copy link
Contributor

Yaffle commented Dec 1, 2014

@ljharb , not loader context, but the context of the es6-shim.js, because you are controlling factory call.
And if you are loading es5-shim.js with <script src=..> or with new Worker(...) or with node es5-shim.js (altough, I do not know about new versions of node), this points to the global object for me

@ljharb
Copy link
Collaborator

ljharb commented Dec 1, 2014

Hmm. It appears that since the initial commit (https://github.com/paulmillr/es6-shim/blame/a57577fdd71cd6726fec6354a1ab296bdd885209/es6-shim.js#L12) the shim's been using "window || global", and over time, evolved to check "self", then to use the current eval approach.

@Yaffle If the es6-shim code happens to be included in a strict mode context, then wouldn't "this" be undefined?

@Yaffle
Copy link
Contributor

Yaffle commented Dec 1, 2014

<script>
"use strict";
console.log(this.isFinite);
(function (root) {
  "use strict";
  console.log(root.isFinite);
  console.log(this == undefined);
}(this));
</script>

@timmarinin
Copy link

This also breaks Firefox OS CSP for privileged apps.

@sandstrom
Copy link

CSP is quite handy and — in addition to being used in Chrome apps and Firefox OS — many web applications also use it. We're one of them and just ran into this.

Would anthonyryan1's method work? [https://github.com//issues/301#issuecomment-64966395]

@ljharb
Copy link
Collaborator

ljharb commented Mar 7, 2015

@sandstrom See my response right below it: #301 (comment)

@stewart
Copy link
Author

stewart commented Jul 3, 2015

I think I've got a possible fix here - https://gist.github.com/stewart/321560330b152dfde649.

Didn't cause any issues in the environments I tested in, but maybe I'm missing something?

@ljharb
Copy link
Collaborator

ljharb commented Jul 3, 2015

@stewart What did you test it on? It would need to work back to IE 6, in node 0.6 and later, all io.js versions, node-webkit/nw.js, web workers, the node vm module's sandbox, and probably in all of a CJS, AMD, and no-module environment for each.

@stewart
Copy link
Author

stewart commented Jul 3, 2015

The one major drawback to my approach that the CSP-violating version doesn't have is that if es6-shim gets wrapped up in an IIFE w/ strict mode enabled (say by a minifier or project bundler), it'll stop working.

On the other hand, a situation like that is easier to work around for developers than a CSP-violating dependency, and relatively easy for the library to detect (check if globals === undefined).

Travis indicates at least Node/iojs support about on par with master - https://travis-ci.org/stewart/es6-shim/builds/69499282, I'll see about testing other platforms later (don't have access to IEs older than 10 though, I'm afraid). I did test in Chrome/Safari/Firefox and they all seemed to like it.

@ljharb
Copy link
Collaborator

ljharb commented Jul 4, 2015

I'm less concerned about browsers and node (where window and global are reliable enough), than I am about web workers and node-webkit, both of which we can't currently test on travis, nor do I have any way to test locally.

@stewart
Copy link
Author

stewart commented Jul 4, 2015 via email

@ljharb
Copy link
Collaborator

ljharb commented Jul 4, 2015

Thanks!

@ljharb
Copy link
Collaborator

ljharb commented Jul 30, 2015

I've worked on this with @rwaldron and @jugglinmike in person, and we've found a non-eval approach that seems to work in browsers, workers, node-webkit, and node.

In order to unblock Chrome apps from having CSP issues, I'm going to go ahead with this change. Please let me know if this change has broken anything.

@ljharb ljharb closed this as completed in 2367e09 Jul 30, 2015
@Yaffle
Copy link
Contributor

Yaffle commented Jul 31, 2015

@ljharb this throws an error when running using WSH.
Altough, es6-shim does not work in WSH anyway...

@ljharb
Copy link
Collaborator

ljharb commented Jul 31, 2015

Thanks, that's good to know - what is the thrown error?

If you can provide me with various exceptions that the shim throws (as separate issues, please) I'll do my best to fix them!

@Yaffle
Copy link
Contributor

Yaffle commented Jul 31, 2015

throw new Error('unable to locate global object');

@ljharb
Copy link
Collaborator

ljharb commented Jul 31, 2015

Perfect, that's exactly what it's supposed to do :-) It appears that in WSH, the only option is to get this in the global context. http://stackoverflow.com/questions/14450424/equivalent-to-window-in-jscript-runtime

I'd suggest then that in wsh, users need to do something like var global = this; in the global context prior to including the es6-shim.

@Yaffle
Copy link
Contributor

Yaffle commented Jul 31, 2015

@ljharb , setTimeout is not supported, so

var promiseSupportsSubclassing = supportsSubclassing(globals.Promise, function(S) {
            return S.resolve(42).then(function() {}) instanceof S;
        });

promiseSupportsSubclassing test throws an error...

@ljharb
Copy link
Collaborator

ljharb commented Aug 23, 2015

@Yaffle Try the latest commit in WSH - please file a new issue for me if there's any further issues.

@Yaffle
Copy link
Contributor

Yaffle commented Aug 23, 2015

@ljharb , well... it works with var global = this; and es5-shim, but throws some errors:

  1. var NumberShim = function Number(value) {

    "Number" is a bad name for IE8 (probablye because of http://kiro.me/blog/nfe_dilemma.html), so before this line is reached, Number is called and throws the error, because isBinary is not defined yet.
  2. wrapConstructor uses Object.getOwnPropertyNames, which is not supported in IE8 event with es5-shim;
  3. Map and Set are not polyfilled in IE8 because of if (supportsDescriptors) {

@ljharb
Copy link
Collaborator

ljharb commented Aug 23, 2015

@Yaffle Can you please file an issue for 1 and 2? 3 is expected and desired.

@chmx
Copy link

chmx commented Apr 19, 2016

@ljharb Using var global = this; before loading es6-shim works with Nashorn too. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants