Update: `no-new-func` rule catching eval case of `MemberExpression` #14860

archmoj · 2021-07-30T17:19:58Z

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Bug fix (template)
[x] Include tests for this change
[x] Update documentation for this change

What changes did you make? (Give an overview)

The no-new-func rule should be able to catch calls to Function.apply (see the example below) concerning dynamic Function() constructor in the form of eval().

var fn = Function.apply('sum', ['a', 'b', 'return a+b;']);
fn(1, 2);

Is there anything you'd like reviewers to focus on?

This would allow testing libraries to comply with CSP as demonstrated in plotly/plotly.js#5868

cc: @alexcjohnson

eslint-github-bot · 2021-07-30T17:20:02Z

Hi @archmoj!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The commit message tag must be one of the following:

The Tag is one of the following:
- Fix - for a bug fix.
- Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
- New - implements a new feature.
- Breaking - for a backwards-incompatible enhancement or feature.
- Docs - changes to documentation only.
- Build - changes to build process only.
- Upgrade - for a dependency upgrade.
- Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).
You can use the labels of the issue you are working on to determine the best tag.
There should be a space following the initial tag and colon, for example 'New: Message'.
The first letter of the tag should be in uppercase

Read more about contributing to ESLint here

eslint-github-bot · 2021-08-02T21:28:45Z

Hi @archmoj!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

mdjermanovic · 2021-08-03T18:30:10Z

Hi @archmoj, thanks for the PR!

This change makes sense to me 👍, but I think we should treat it as an enhancement, so I'd like to hear more opinions from the team before accepting and reviewing the PR.

nzakas · 2021-08-03T19:28:21Z

I’m okay with the change, but the docs need to be updated to explain that these forms are akin to “new Function”.

archmoj · 2021-08-03T20:38:54Z

I’m okay with the change, but the docs need to be updated to explain that these forms are akin to “new Function”.

Good call. Addressed in cc988e1.

archmoj · 2021-08-05T14:07:29Z

Is there any remaining item(s) on this PR?

nzakas · 2021-08-06T20:26:44Z

We just need time to review it. A lot of people are away, so it may take some time. We'll update if we need any further changes.

archmoj · 2021-08-06T20:33:39Z

We just need time to review it. A lot of people are away, so it may take some time. We'll update if we need any further changes.

Thanks very much for the info @nzakas

mdjermanovic · 2021-08-26T12:50:28Z

Do we have a consensus? I'm willing to champion this enhancement, but it needs one more 👍

Or, shall we treat it as a bug fix?

btmills

I agree the rule should catch this case. Thanks for adding the check! Since it’s improving the rule’s ability to detect eval-like calls, I’d lean toward this being a semver-minor change.

mdjermanovic

Sorry for the delay! This looks very good, I left several comments about certain details.

docs/rules/no-new-func.md

mdjermanovic · 2021-09-06T11:40:11Z

lib/rules/no-new-func.js

+                                parent.type === "CallExpression"
+                            )) {
+                                evalNode = parent;
+                            } else if (parent.type === "MemberExpression" && node === parent.object) {


I think we should also check if the property name is one of "call", "apply" or "bind", as this looks like a false positive:

/* eslint no-new-func: error */ Function.toString(); // false positive

For this check, we can use astUtils.getStaticPropertyName() so that it covers code such as Function["call"]()

lib/rules/no-new-func.js

tests/lib/rules/no-new-func.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

linux-foundation-easycla · 2021-09-07T17:31:27Z

The committers are authorized under a signed CLA.

✅ Mojtaba Samimi (d20c16e, 8e4f5cf, 81b1c60, 8dbcb44, e78dc5e, cc988e1, 7a83a49, 05e750e, 885ed3a, ef39ca7, 083ad40, 13f61ea, 8a41cc9, c49f69f)

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic · 2021-09-20T09:08:23Z

@archmoj I think #14860 (comment) is the last remaining issue, let us know if you need any help with that.

archmoj · 2021-09-20T09:20:32Z

@archmoj I think #14860 (comment) is the last remaining issue, let us know if you need any help with that.

I would appreciate if you could possibly push a commit to address that.

mdjermanovic · 2021-09-22T11:19:54Z

@archmoj I think #14860 (comment) is the last remaining issue, let us know if you need any help with that.

I would appreciate if you could possibly push a commit to address that.

Done e6421b3

mdjermanovic

LGTM, thanks!

snitin315

Looks good. Thanks for contributing.

mdjermanovic · 2021-09-23T20:38:03Z

Merged, thanks for contributing! This will be included in ESLint v8.0.0-rc.0 release, scheduled for tomorrow (#15059)

archmoj · 2021-09-23T20:43:41Z

Merged, thanks for contributing! This will be included in ESLint v8.0.0-rc.0 release, scheduled for tomorrow (#15059)

Thanks very much @mdjermanovic for taking over this PR as well as the note.
I'll try v8.0.0-rc.0 once published :)

nzakas · 2021-10-07T20:50:14Z

@archmoj we'd like to thank you for your work on this. Can you drop us an email at contact (at) eslint (dot) org?

archmoj · 2021-10-08T20:35:42Z

@archmoj we'd like to thank you for your work on this. Can you drop us an email at contact (at) eslint (dot) org?

It was not really a big deal to kick start a PR like this one.
But yeah thanks to @mdjermanovic who helped with the hard work of finishing it. 🥇 🏆

nzakas · 2021-10-09T00:21:47Z

We’d like to pay you from our contributor pool, but we need an email address to do that. :)

eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jul 30, 2021

archmoj mentioned this pull request Jul 30, 2021

Propose fixing eslint module to catch calls to Function.apply plotly/plotly.js#5868

Merged

archmoj force-pushed the no-new-func-apply branch from 80b2c35 to d20c16e Compare July 30, 2021 17:26

archmoj added 4 commits July 30, 2021 13:29

Fix: no-new-fuc should catch calls to Function.apply

d20c16e

Chore: adjust ES6 syntax in no-new-func

8e4f5cf

Docs: examples for incorrect no-new-func rule

81b1c60

Chore: tests for invalid no-new-func cases

8dbcb44

archmoj changed the title ~~Fix: no-new-fuc should catch calls to Function.apply in the form of eval()~~ Fix: no-new-func rule catching eval case of MemberExpression Aug 2, 2021

Docs: example and test for incorrect no-new-func rule

e78dc5e

mdjermanovic added this to Feedback Needed in Triage Aug 3, 2021

Docs: enhance documentation for no-new-func rule

cc988e1

nzakas requested review from btmills and mdjermanovic August 26, 2021 00:35

mdjermanovic self-assigned this Aug 26, 2021

btmills approved these changes Aug 26, 2021

View reviewed changes

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 26, 2021

archmoj added 2 commits August 27, 2021 11:15

Chore: additional test for Function.bind

885ed3a

Docs: note regarding Function.bind without immediate call

ef39ca7

archmoj requested a review from mdjermanovic August 27, 2021 16:55

Fix: report CallExpression node instead of MemberExpression node

083ad40

btmills moved this from Feedback Needed to Pull Request Opened in Triage Aug 29, 2021

archmoj mentioned this pull request Sep 2, 2021

Include gl3d in strict bundle plotly/plotly.js#5888

Merged

mdjermanovic requested changes Sep 6, 2021

View reviewed changes

Docs: modify no-new-func docs

13f61ea

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

archmoj and others added 3 commits September 7, 2021 13:32

Docs: add no-new-func example

8a41cc9

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Fix: use maybeCallee instead of parent in no-new-func

c49f69f

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

add tests

0d11a0e

check method name

e6421b3

mdjermanovic approved these changes Sep 22, 2021

View reviewed changes

snitin315 approved these changes Sep 23, 2021

View reviewed changes

mdjermanovic changed the title ~~Fix: no-new-func rule catching eval case of MemberExpression~~ Update: no-new-func rule catching eval case of MemberExpression Sep 23, 2021

mdjermanovic merged commit 14a4739 into eslint:master Sep 23, 2021

Triage automation moved this from Pull Request Opened to Complete Sep 23, 2021

archmoj mentioned this pull request Oct 4, 2021

Bump eslint to v8 release candidate including fixes for no-new-func test plotly/plotly.js#5969

Merged

sync-by-unito bot mentioned this pull request Nov 1, 2021

build(deps-dev): bump eslint from 7.32.0 to 8.1.0 numbersprotocol/capture-lite#1029

Closed

eslint-github-bot bot locked and limited conversation to collaborators Mar 23, 2022

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: `no-new-func` rule catching eval case of `MemberExpression` #14860

Update: `no-new-func` rule catching eval case of `MemberExpression` #14860

archmoj commented Jul 30, 2021 •

edited

eslint-github-bot bot commented Jul 30, 2021

eslint-github-bot bot commented Aug 2, 2021

mdjermanovic commented Aug 3, 2021

nzakas commented Aug 3, 2021

archmoj commented Aug 3, 2021

archmoj commented Aug 5, 2021

nzakas commented Aug 6, 2021

archmoj commented Aug 6, 2021

mdjermanovic commented Aug 26, 2021

btmills left a comment

mdjermanovic left a comment

mdjermanovic Sep 6, 2021

linux-foundation-easycla bot commented Sep 7, 2021 •

edited

mdjermanovic commented Sep 20, 2021

archmoj commented Sep 20, 2021

mdjermanovic commented Sep 22, 2021

mdjermanovic left a comment

snitin315 left a comment

mdjermanovic commented Sep 23, 2021

archmoj commented Sep 23, 2021

nzakas commented Oct 7, 2021 •

edited

archmoj commented Oct 8, 2021 •

edited by nzakas

nzakas commented Oct 9, 2021

Update: no-new-func rule catching eval case of MemberExpression #14860

Update: no-new-func rule catching eval case of MemberExpression #14860

Conversation

archmoj commented Jul 30, 2021 • edited

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

eslint-github-bot bot commented Jul 30, 2021

eslint-github-bot bot commented Aug 2, 2021

mdjermanovic commented Aug 3, 2021

nzakas commented Aug 3, 2021

archmoj commented Aug 3, 2021

archmoj commented Aug 5, 2021

nzakas commented Aug 6, 2021

archmoj commented Aug 6, 2021

mdjermanovic commented Aug 26, 2021

btmills left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic Sep 6, 2021

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Sep 7, 2021 • edited

mdjermanovic commented Sep 20, 2021

archmoj commented Sep 20, 2021

mdjermanovic commented Sep 22, 2021

mdjermanovic left a comment

Choose a reason for hiding this comment

snitin315 left a comment

Choose a reason for hiding this comment

mdjermanovic commented Sep 23, 2021

archmoj commented Sep 23, 2021

nzakas commented Oct 7, 2021 • edited

archmoj commented Oct 8, 2021 • edited by nzakas

nzakas commented Oct 9, 2021

Update: `no-new-func` rule catching eval case of `MemberExpression` #14860

Update: `no-new-func` rule catching eval case of `MemberExpression` #14860

archmoj commented Jul 30, 2021 •

edited

linux-foundation-easycla bot commented Sep 7, 2021 •

edited

nzakas commented Oct 7, 2021 •

edited

archmoj commented Oct 8, 2021 •

edited by nzakas