-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[WIP] Fix CSP issue while obtaining ref to global #353
Conversation
Prevent violating Content Security Policy fixes parcel-bundler#335
It appears that this breaks the globals integration-test, as I'd love to get a reviewer's eye on this, as the proposed changed in this PR did fix my CSP 'unsafe-eval' issue and the packaged code still worked. |
@@ -7,7 +7,7 @@ const VARS = { | |||
asset.addDependency('process'); | |||
return 'var process = require("process");'; | |||
}, | |||
global: () => 'var global = (1,eval)("this");', | |||
global: () => 'var global = typeof global === "object" ? global : typeof window === "object" ? window :typeof self === "object" ? self : this;', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other implementations seem to default to {}
instead of this
. Trade-offs / concerns with either approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In global context this
always refers to the global object. As we check before with typeof global === "object"
whether the global object exists, it might be a better solution to use {}
. I don't feel expert enough to give a definitive answer though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cosmetic nitpick: there's a space missing in :typeof
.
FWIW the tests pass on macOS (node 8.9.3 and 9.6.1) and Linux (node 9.6.1). |
@mBeierl You'll also have to replace |
I’m running into this issue trying to use parcel to build a Chrome extension and all Chrome extensions do not allow 'unsafe-eval'. |
I'm having the same problem with @mrcoles. I think this problem is very critical. |
merge in this PR parcel-bundler#353 to make some extra changes
Expands on parcel-bundler#353 Also, prettier updates were auto-applied to hrm-runtime.js.
Can this be replaced with the following instead:
|
Closing in favor of #1133 |
Prevent violating Content Security Policy
fixes #335