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

no-eval don't match on `window.eval()` #4399

Closed
BaggersIO opened this Issue Nov 12, 2015 · 22 comments

Comments

Projects
None yet
7 participants
@BaggersIO
Copy link

BaggersIO commented Nov 12, 2015

Hey,

the no-eval rule works fine with eval(), but eslint don't detect window.eval() expressions.

My rule:

"no-eval" 2

My env:

"browser": true

ESLint version: 1.8.0

Cheers,
Peter

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 12, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 12, 2015

@nzakas nzakas added bug rule accepted and removed triage labels Nov 12, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 12, 2015

It looks like no-eval only detects direct calls to eval, which makes sense, since those are the only ones that give you dynamic scope and kill performance. 👎 This should not be changed.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 12, 2015

@michaelficarra Wait, window.eval does not evaluate in global/dynamic scope? And I can't imagine the eval variable lookup is the rate-limiting step when calling that function.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 12, 2015

@platinumazure You should look into the difference between direct/indirect eval.

Direct eval:

(function() {
  var x = 0;
  eval("x"); // 0
})();
(function() {
  eval("var x = 0");
  x; // 0
})();

Indirect eval:

(function() {
  var x = 0;
  (0, eval)("x"); // ReferenceError
})();
(function() {
  (0, eval)("var x = 0");
  x; // ReferenceError
})();

Performance dies because you pretty much can't predict anything at all about what's going to happen when direct eval is used, so the JS engine will drop down into a naïve interpreter mode.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 12, 2015

Crazy. Learn something new every day. Does window.eval count as an indirect eval?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 12, 2015

Yes. A direct call to eval must appear as a call to the identifier eval, and it must resolve to the original value of the eval property of the global object (called %eval% in the spec).

(function(eval) {
  eval(...) // direct call to eval
})(eval);
(function(eval) {
  eval(...) // indirect call to eval
})(function(x){
  return eval(x); // direct call to eval
});
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 13, 2015

This rule is about blocking all uses of eval, so it makes sense to block window.eval as well.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 13, 2015

Then an option should be added to allow safe forms of eval. Also, it is impossible to detect all indirect calls to eval statically. Where are you going to draw the line?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 13, 2015

Can you file an issue for that?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 13, 2015

"If we ever change no-eval to detect indirect calls to eval, we should add an option to disable it"? I'm not creating an issue for that, it should be discussed here.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 13, 2015

This issue is a bug, we did not mean to miss window.eval. we will fix that bug and this issue will be closed. You're requesting a new option that distinguishes between direct and indirect eval, which is an enhancement, so we need a separate issue.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 15, 2015

I will work on this.

My plan:

  • Checks evel property of the host object.
    • When env: "browser" is specified, the host object is window.
    • When env: "node" is specified, the host object is global.
    • Other idea?
  • Checks indirect-calls in an expression.
    • (0, eval)(...)
    • [eval][0](...)
    • {a: eval}.a(...)
  • Does NOT check indirect-calls via other variables. To detect those is too hard.
    • var foo = eval; foo(...)
    • (function(foo) { foo(...) }(eval))
    • var i = 0; [eval][i](...)
    • var eval = "eval"; window[eval](....)

@mysticatea mysticatea self-assigned this Nov 15, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 16, 2015

@mysticatea How about top-level this.eval in scripts? eval.call(null, "...")? Function.prototype.call.call(eval, null, "...")?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 16, 2015

@nzakas The rule currently distinguishes between direct/indirect eval, whether intentional or not. If you remove this capability in a bugfix, those that are okay with indirect eval and not direct eval (and it is not unreasonable to have this preference) will have no way to configure the rule for their preference. I think the best way to move forward with this for eslint users is to give an option to preserve the current functionality in the same release it is removed. I can open a separate issue if you still want, but I strongly believe it should be a blocker for this issue.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 16, 2015

Oh, I had not noticed those. eval.bind(null, "...")(), haha, I have a headache.

I'm going to change strategy.

  • no-eval just checks the variable object of the global eval.
    Then reports all references (contains readonly) of the variable.
    This should block most indirect eval calls.
  • Checks evel property of the host object.
    • When env: "browser" is specified, the host object is window.
    • When env: "node" is specified, the host object is global.
    • no-eval reports accesses, not calls. (but miss the following pattern: var a = window; a.eval)
  • Checks this.eval for functions (not methods, not strict mode).
    Hmm, I want to share function's info with no-invalid-this rule.
    Or may I not check this.eval, because no-invalid-this is catching those?

When allowing indirect calls (#4441), the detection logic is as is.
In short, all eval() calls are reported.

And I will write this known limitation in the doc.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 16, 2015

No, no-invalid-this does not check sloppy mode code....

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 17, 2015

@michaelficarra every bug fix is backwards incompatible, we don't always need to keep previous behavior if it was incorrect.

@nzakas nzakas closed this Nov 17, 2015

@nzakas nzakas reopened this Nov 17, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 17, 2015

Oops, hit wrong button.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 17, 2015

@nzakas Sorry, that only works when the observed ("incorrect") behaviour could not have been reasonably expected as intentional behaviour. Since the current behaviour could be reasonably expected (if you thought "eval" meant "direct eval") and useful, it should be preserved. Anyway, this shouldn't matter as it seems @mysticatea is already handling it this way.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 20, 2015

Oh...
I cannot get env setting from RuleContext...

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 20, 2015

@michaelficarra you're assuming users have a much deeper understanding of the language than is typical. When we say "disallow eval", I'd be shocked to see any number of people believe that what we really meant was "disallow direct eval." I've already agreed to add an option so you can make such a distinction if you want, so I'm not sure why you're still debating this.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 20, 2015

I agree with @nzakas While @michaelficarra has a good point, I know for a fact that every JavaScript developer I ever worked with does not know the distinction between direct and indirect eval, it's just not something you run into on a daily bases.

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 21, 2015

Fix: `no-eval` come to catch indirect eval (fixes eslint#4399, fixes e…
…slint#4441)

- Functions to detect `this` binding is default were extracted to AST
Utils from `no-invalid-this`. And `no-eval` is using this utility.
- `allowIndirect` option was added to `no-eval` in order to keep old
behavior.

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 21, 2015

Fix: `no-eval` come to catch indirect eval (fixes eslint#4399, fixes e…
…slint#4441)

- Functions to detect `this` binding is default were extracted to AST
Utils from `no-invalid-this`. And `no-eval` is using this utility.
- `allowIndirect` option was added to `no-eval` in order to keep old
behavior.

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 21, 2015

Fix: `no-eval` come to catch indirect eval (fixes eslint#4399, fixes e…
…slint#4441)

- Functions to detect `this` binding is default were extracted to AST
Utils from `no-invalid-this`. And `no-eval` is using this utility.
- `allowIndirect` option was added to `no-eval` in order to keep old
behavior.

@nzakas nzakas closed this in #4505 Dec 1, 2015

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.