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

Remove eval usage. Fixes CSP cases. #1133

Merged
merged 3 commits into from May 1, 2018

Conversation

CvX
Copy link
Contributor

@CvX CvX commented Apr 4, 2018

Picks up on work done by @mBeierl (closes #353).
Fixes #335.

@CvX CvX force-pushed the eval-fix-2 branch 3 times, most recently from 023af73 to 6bb6cfa Compare April 4, 2018 14:44
@@ -7,7 +7,8 @@ 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 : {};',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessarily very reliable. What if someone declares window or self in the local scope of their module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Replaced it with a more reliable (and succinct!) solution: 1fe753e

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but won't Function constructor have the same problem as eval with CSP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're, right, my bad.
I don't know why I assumed that another variant of arbitrary code execution would fly. 🙃 Should have read W3C Working Draft for CSP

About the case of window and self in a local scope – you're talking about a situation where there's a module that either:

  1. uses global object, redefines window, and is used in a browser
  2. uses global object, redefines self, and is used in a Web Worker

Do we have to choose between supporting those two cases and supporting CSP?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... we may be able to get it working by moving that code to get the global object you originally added outside of each module and into the prelude where we can guarantee that nothing will mess with those variables. In there, the global object should be this (unless strict mode is enabled). Then we can attach it to module or something, and here assign var global = module.global, or maybe pass it into the module function. What do you think?

See this article for more info: https://www.contentful.com/blog/2017/01/17/the-global-object-in-javascript/

@CvX CvX force-pushed the eval-fix-2 branch 3 times, most recently from 5c707c4 to e35413b Compare April 9, 2018 14:41
@CvX
Copy link
Contributor Author

CvX commented Apr 9, 2018

@devongovett Moving acquisition of global to the prelude is a great idea. I've updated the PR – let me know what you think. It's using this since prelude seems to be in a non-strict scope. Is there any case where it would become strict?

Btw, it would be a good idea to run some of the tests in an actual browser environment, maybe via Puppeteer. I'll have to look into that. Some actual CSP tests would be nice.

embiem and others added 3 commits May 1, 2018 08:32
Prevent violating Content Security Policy

fixes parcel-bundler#335
The `global` object is aquired via `this` in a non-strict scope and then passed into each module.
@devongovett
Copy link
Member

There was one additional case to handle: when you redeclare global as a variable inside a module, e.g. const global = 2. This would cause a syntax error complaining that global was already defined (in the module function wrapper). I solved this by accessing global from arguments[3] when needed instead of always passing to the function.

@devongovett devongovett merged commit b032b85 into parcel-bundler:master May 1, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
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

Successfully merging this pull request may close these issues.

🐛 CSP 'unsafe-eval' not allowed
3 participants