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

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

Merged
merged 16 commits into from Sep 23, 2021

Conversation

@archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 30, 2021

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
Copy link

@eslint-github-bot eslint-github-bot bot commented Jul 30, 2021

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
Copy link

@eslint-github-bot eslint-github-bot bot commented Aug 2, 2021

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

@archmoj 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
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 3, 2021

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.

@mdjermanovic mdjermanovic added this to Feedback Needed in Triage Aug 3, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Aug 3, 2021

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

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Aug 3, 2021

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
Copy link
Contributor Author

@archmoj archmoj commented Aug 5, 2021

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

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 6, 2021

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
Copy link
Contributor Author

@archmoj archmoj commented Aug 6, 2021

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

@nzakas nzakas requested review from btmills and mdjermanovic Aug 26, 2021
@mdjermanovic mdjermanovic self-assigned this Aug 26, 2021
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 26, 2021

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?

Copy link
Member

@btmills btmills left a comment

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.

@archmoj archmoj force-pushed the no-new-func-apply branch from c36bfe9 to 05e750e Aug 27, 2021
@archmoj archmoj requested a review from mdjermanovic Aug 27, 2021
@btmills btmills moved this from Feedback Needed to Pull Request Opened in Triage Aug 29, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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

docs/rules/no-new-func.md Outdated Show resolved Hide resolved
docs/rules/no-new-func.md Show resolved Hide resolved
parent.type === "CallExpression"
)) {
evalNode = parent;
} else if (parent.type === "MemberExpression" && node === parent.object) {
Copy link
Member

@mdjermanovic mdjermanovic Sep 6, 2021

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
tests/lib/rules/no-new-func.js Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@linux-foundation-easycla
Copy link

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

CLA Signed

The committers are authorized under a signed CLA.

archmoj and others added 3 commits Sep 7, 2021
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 20, 2021

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

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Sep 20, 2021

@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
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 22, 2021

@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

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

Copy link
Contributor

@snitin315 snitin315 left a comment

Looks good. Thanks for contributing.

@mdjermanovic 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 mdjermanovic merged commit 14a4739 into eslint:master Sep 23, 2021
13 checks passed
Triage automation moved this from Pull Request Opened to Complete Sep 23, 2021
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 23, 2021

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

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Sep 23, 2021

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
Copy link
Member

@nzakas nzakas commented Oct 7, 2021

@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
Copy link
Contributor Author

@archmoj archmoj commented Oct 8, 2021

@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
Copy link
Member

@nzakas nzakas commented Oct 9, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Triage
Complete
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants