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

Use a hardcoded no-op instead of instancing #1032

Merged
merged 1 commit into from
Aug 23, 2017
Merged

Use a hardcoded no-op instead of instancing #1032

merged 1 commit into from
Aug 23, 2017

Conversation

Alhadis
Copy link
Contributor

@Alhadis Alhadis commented Aug 23, 2017

Using new Function() is considered unsafe evaluation, which means it breaks if executed in an environment with a strict Content Security Policy (CSP).

In my case, it's causing an EvalError for a test-runner I wrote for Atom:

EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self'".

	at new Function (<anonymous>)
	at Object.addProperty (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai/utils/addProperty.js:39:35)
	at Function.Assertion.addProperty (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai/assertion.js:94:10)
	at /Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai/core/assertions.js:46:15
	at Array.forEach (native)
	at module.exports (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai/core/assertions.js:45:35)
	at Object.exports.use (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai.js:39:5)
	at Object.<anonymous> (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai.js:71:9)
	at Object.<anonymous> (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/lib/chai.js:94:3)
	at Module._compile (/Applications/Atom.app/Contents/Resources/app/src/native-compile-cache.js:87:30)
	at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/src/compile-cache.js:234:23)
	at Module.load (module.js:488:32)
	at tryModuleLoad (module.js:447:12)
	at Function.Module._load (module.js:439:3)
	at Module.require (module.js:498:17)
	at require (/Applications/Atom.app/Contents/Resources/app/src/native-compile-cache.js:47:27)
	at Object.<anonymous> (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/index.js:1:173)
	at Object.<anonymous> (/Users/johngardner/Labs/Atom-Mocha/node_modules/chai/index.js:3:3)
	at Module._compile (/Applications/Atom.app/Contents/Resources/app/src/native-compile-cache.js:87:30)
	at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/src/compile-cache.js:234:23)
	at Module.load (module.js:488:32)
	at tryModuleLoad (module.js:447:12)
	at Function.Module._load (module.js:439:3)
	at Module.require (module.js:498:17)
	at require (/Applications/Atom.app/Contents/Resources/app/src/native-compile-cache.js:47:27)
	at Object.<anonymous> (/Users/johngardner/Labs/Atom-Mocha/lib/main.js:6:18)
	at Object.<anonymous> (/Users/johngardner/Labs/Atom-Mocha/lib/main.js:527:3)
	at Module._compile (/Applications/Atom.app/Contents/Resources/app/src/native-compile-cache.js:87:30)
	at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/src/compile-cache.js:234:23)
	at Module.load (module.js:488:32)
handleSetupError @ index.js:87

This is only being used to provide a no-op function, so there's no need to use new Function() when an anonymous function will suffice.

@Alhadis Alhadis requested a review from a team as a code owner August 23, 2017 18:38
keithamus
keithamus previously approved these changes Aug 23, 2017
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

Seems like a good idea. Let's get one more from @chaijs/chai to review and merge 😄

@@ -36,7 +36,7 @@ var transferFlags = require('./transferFlags');
*/

module.exports = function addProperty(ctx, name, getter) {
getter = getter === undefined ? new Function() : getter;
getter = getter === undefined ? function(){} : getter;
Copy link
Member

Choose a reason for hiding this comment

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

This is very nitpicky, but for the sake of consistency with other parts of the code, could you use function () {} instead please? Thanks 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@astorije I've force-pushed an amended commit.

The use of `new Function()` causes an EvalError if the environment's CSP
forbids execution of unsafe-eval. As there's no runtime evaluation being
performed, it's better to use a plain anonymous function instead.
@astorije astorije merged commit c592551 into chaijs:master Aug 23, 2017
@Alhadis Alhadis deleted the csp-fix branch August 23, 2017 20:01
@Alhadis
Copy link
Contributor Author

Alhadis commented Aug 29, 2017

@astorije Is there an ETA for the next release? The atom-mocha module is currently unusable in Atom at the moment, since it bundles Chai by default...

@astorije
Copy link
Member

Sorry @Alhadis, I can't say I have been the best project owner thus far, so releases are a bit out of my reach. @keithamus, maybe you would know more? :)

@keithamus
Copy link
Member

We currently have a mostly open release cycle. What it takes for a release is the following:

It's important to note anyone can make this PR. For example a community member released 4.1.0 (#998). We want to - generally - get the community involved in every aspect including release management if we can!

@Alhadis
Copy link
Contributor Author

Alhadis commented Aug 29, 2017

That's... a refreshingly open and welcoming attitude I've not seen before, and certainly an inviting one. 😄 Since I'm the one badgering for a release, I may as well handle the accompanying pull-request. That'll give me a version to set in a new atom-mocha release, which will in turn help me to test a myriad of potentially complicated issues with recent changes to Atom packages.

... yeah, there's a lot that's hinging on such a little change, heh. :D

@Alhadis Alhadis mentioned this pull request Aug 31, 2017
@Alhadis
Copy link
Contributor Author

Alhadis commented Aug 31, 2017

I'm so sorry about the delay! 😞 I got sidetracked.

PR submitted! See #1037. Hope the release notes cover everything adequately.

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.

3 participants