Remove use of `eval`

[fatso83]

• FakeTimers version : 6.0
• Environment : Rollup
• Example URL : https://github.com/sinonjs/nise/issues/110

What did you expect to happen? No warnings/errors

What actually happens Rollup complaints about the use of eval.

How to reproduce See original bug report.

Judging by the comments in the original issue in nise, this issue seems to be caused by needing to support some ancient browsers. Fixing it should thus be easy, but requires a new major version.

[mantoni]

We had a discussion about this before, I recall, but can't find it. We should throw a meaningful error if a string is passed as the first argument to setTimeout or setInterval.

I suggest to throw the same error as Node:

$ node -p 'setTimeout("doSomething()",1)'

TypeError [ERR_INVALID_CALLBACK]: Callback must be a function. Received 'doSomething()'

[itayperry]

Hey everyone, I'm taking a look at this. I'll try to fix the issue :)

[itayperry]

Hey there, I removed the eval() function, it is no longer in use. It would be great if you take a look at the code and check that it doesn't break anything. There was a test for the eval() and I changed it to make sure that it fails and throws an exception, the test was completed successfully :)

327

[benjamingr]

I just did a sanity check and it looks like this feature is still supported in all major browsers.

In https://github.com/sinonjs/nise/issues/110 and specifically https://github.com/sinonjs/nise/issues/110#issuecomment-550090232 @mantoni saw that it didn't work in Chrome - but that's only because the page he was testing in had CSP (for example GitHub.com) - sites like example.com will happily execute code like setTimeout("console.log('hello world')").

Chrome:

In addition checking on the grid, it looks like this feature still works on IE11:

MSHtml Edge:

Firefox:

Safari:

So it looks like this feature (passing strings) is still supported in all major browsers (IE11, Chrome, Edge, Firefox, Chromium Edge, Safari).

It also appears that the spec we are implementing specifically requires this:

If the first argument to the invoked method is an object that has an internal [[Call]] method, then return a task that checks if the entry for handle in list has been cleared, and if it has not, calls the aforementioned [[Call]] method with as its arguments the third and subsequent arguments to the invoked method (if any), and with an undefined thisArg, and abort these steps. [ECMA262]

Setting thisArg to undefined means that the function code will be executed with the this keyword bound to the WindowProxy or the WorkerGlobalScope object, as if the code was running in the global scope.

Otherwise, continue with the remaining steps.

Apply the ToString() abstract operation to the first argument to the method, and let script source be the result. [ECMA262]

[mantoni]

Ooooh, that is very interesting. I knew it was in the spec, but thought the feature got dropped due to security concernes.

I think we all agree that we have to keep this feature then.

[itayperry]

Should I cancel the pull request?

[benjamingr]

No @itayperry I'll post a follow up comment one sec :]

[benjamingr]

@mantoni I went to #whatwg on IRC and asked them what they think we should do. I think keeping it for compatibility is probably the right thing to do (I didn't get an answer yet but I suspect they'll agree with you and with my intuition here). Update:: the whatwg people who were present (Anne) were not aware of any plans to deprecate or remove support for string arguments.

@itayperry I think the follow up for the PR should be:

• [ ] Only throw in Node.js (I like this since it better emulates what Node does). I tried searching for when we started throwing in Node and reached the end of the line at 0.10.0 that still throws just with a worse error.
• [ ] Add an option to .install and clock creation warnings to emit a warning (or throw) when passing a string to setTimeout (since it's usually a bug)
• [ ] Trick rollup not to complain about the eval by aliasing it or otherwise suppressing the error (with a comment?)

[itayperry]

Would a trick like that work? :)

var eval2 = eval;

(function () {
  var foo = 42;
  eval('console.log("with eval:",foo)');  // logs 'with eval: 42'
  eval2('console.log("with eval2:",foo)'); // throws ReferenceError
})();

How can I throw only in Node? I'm googling it but I can't find any solution.

[benjamingr]

@itayperry look at how we detect running in Node.js in the library already and try running fake-timers in Node vs browsers. You don't have to implement detection it's already there :)

As for eval2 - read about direct vs indirect eval that can impact semantics and I am not sure which is right here :)

[benjamingr]

I think the eval2 trick would work and timers are supposed to execute in the global scope. I think the old behavior is just a bug.

(function() { 
  var y = 10;
  setTimeout('console.log(y)'); // throws ReferenceError: y is not defined
})();
(function() { 
  var y = 10;
  eval('console.log(y)'); // logs 10
})();
(function() { 
  var y = 10;
  (0, eval) ('console.log(y)'); // throws ReferenceError: y is not defined
})();

[itayperry]

BTW, how did it work on Chrome for you? @benjamingr

I don't wanna make too much mess. I'll close this PR and open 3 other PRs to address your 3 recommendations. Thanks!!!

@mantoni Thank you also for the feedback :)

[benjamingr]

@itayperry you are testing this on GitHub.com, GitHub uses something called CSP to prevent usage of eval in scripts on the site to prevent eval.

In particular if you open the network tab and inspect the request for the document you tested it on - you will see something like:

Content-Security-Policy: default-src 'none'; base-uri 'self'; block-all-mixed-content; connect-src 'self' uploads.github.com www.githubstatus.com collector.githubapp.com api.github.com www.google-analytics.com github-cloud.s3.amazonaws.com github-production-repository-file-5c1aeb.s3.amazonaws.com github-production-upload-manifest-file-7fdce7.s3.amazonaws.com github-production-user-asset-6210df.s3.amazonaws.com cdn.optimizely.com logx.optimizely.com/v1/events wss://live.github.com; font-src github.githubassets.com; form-action 'self' github.com gist.github.com; frame-ancestors 'none'; frame-src render.githubusercontent.com; img-src 'self' data: github.githubassets.com identicons.github.com collector.githubapp.com github-cloud.s3.amazonaws.com *.githubusercontent.com; manifest-src 'self'; media-src 'none'; script-src github.githubassets.com; style-src 'unsafe-inline' github.githubassets.com; worker-src github.com/socket-worker.js

Test a website which doesn't use the CSP directive (like example.com or most of the internet unfortunately) and you will see it works.

(Also if you google the error you would have found that out :])

[itayperry]

Wow!!! You taught me something new :)

Found it:

[itayperry]

Hi everyone, I have a tiny question. The project uses prettier as far as I know, but when I click save I trigger the prettier in the fake-timers.js file, changing 5,475 lines. This didn't happen when I changed the fake-timers-src.js file on my latest commits. What do you think?

[itayperry]

For the moment I saved the file after disabling prettier (on this specific file). Perhaps this also needs to be handled separately. https://github.com/sinonjs/sinon/blob/master/CONTRIBUTING.md#linting-and-style

[itayperry]

Hi everyone :)

I'm trying to find out how to throw a TypeError only in Node.js Now I've seen that there's a file that recognizes the environment.

The file is a part of the shared library @sinonjs/common:

https://github.com/sinonjs/commons/blob/07b9e7a1d784771273a9a58d74945bbc7319b5d4/lib/global.js#L11

The question is, can I implement similar logic to check on which environment I am? I feel like doing something such as typeof global !== "undefined" might be wrong, because @sinonjs/common already does it for me, and it might be confusing..

Or maybe I could look for something (inside the global given to me) that exists only on Node.js, look it up with an if() {} and then execute the wanted code inside?

I'd be happy to get some advice.

[benjamingr]

Probably use the same check used to decide whether to return numbers or objects from timers

[itayperry]

This fix requires many changes, so I have a few questions before I continue:

1) Is this the check you mentioned?

var timeoutResult = _global.setTimeout(NOOP, 0);
var addTimerReturnsObject = typeof timeoutResult === "object";

2) Is it ok to use it also in a testing file? (set this var at the beginning of the file)

3) Another question which is a bit more complex for me - there are a few tests that trigger the eval (https://github.com/sinonjs/fake-timers/pull/327#discussion_r429338390), can I put an if inside each of those tests and assert two different things?

if (!addTimerReturnsObject) { 
    assert(..pass - env is not Node.js..) 
} else { 
    assert(..failing - you're in Node.js env..) 
}

[mantoni]

Answering question 3: Please create separate test cases in a nested describe and conditionally skip them like here:

https://github.com/sinonjs/fake-timers/blob/0bfffc1810990f6e5dc42c5238e48cd90bd41265/test/fake-timers-test.js#L497-L503

[itayperry]

Thank you @mantoni for your fast reply :)

Can I nest a describe inside another decribe ? Or is it better to do it separately?

[benjamingr]

Can I nest a describe inside another decribe ? Or is it better to do it separately?

Generally - yes.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.