feat: extend detect non literal fs filename #92

BuZZ-T · 2022-09-02T16:30:53Z

I think the changes are now in a state where a PR can be created.

I've done the following things:

Added "@typescript-eslint/parser" so that TypeScript syntax (in this case: import) can be used in tests
Rewritten the detect-literal-fs-filename to check from where the inspected method / function is imported
Added "fs/promises" and "fs-extra" as packages to check
Also check for node: prefix in package names
Added many tests to cover the implemented logic
Created utils functions for getting the import or require of a method / function

I'm not sure if the last bullet point makes the code clearer or more complex. It's in an extra commit, so i can revert this part.
Additionally i'm pretty sure there are more useful test-cases, especially valid ones, to prevent false positives.

I appreciate comments and hints for improvement and i hope i'll have the time to implement them fast!

P.S.: This covers #26, #54, #65 and #88

revelt · 2022-09-18T14:02:59Z

Good job!

sakirma · 2022-10-19T14:15:20Z

Any idea when these changes will be merged?

BuZZ-T · 2022-10-25T15:08:48Z

Any idea when these changes will be merged?

Hi @sakirma ! Looks like i still need a reviewer + approval for this. I'm not aware of the review process (if there is one ;) ) in this repository, maybe @nzakas may step in here?

nzakas · 2022-10-27T23:48:23Z

So sorry, for some reason I didn’t get a notification about this! I’m buried under notifications at the moment but I’ll plan on looking at this next week.

Question: do any of the tests actually use TypeScript syntax? I just looked quickly and didn’t see any.

BuZZ-T · 2022-10-28T08:22:13Z

No worries! Thanks for your help!

Question: do any of the tests actually use TypeScript syntax? I just looked quickly and didn’t see any.

I only used the import statement. Is there an eslint-parser that supports that? Then i wouldn't need the typescript-parser.

nzakas · 2022-11-03T17:44:48Z

The default parser will work with import statements. You just need to set parserOptions.ecmaVersion to 6 or higher and then set parserOptions.sourceType to “module”. See https://eslint.org/docs/latest/developer-guide/nodejs-api#ruletester

nzakas

Thanks for this. Overall looks good, just some cleanup tasks to improve clarity for readers.

nzakas · 2022-11-03T17:46:34Z

rules/data/fsPackagesNames.json

@@ -0,0 +1,3 @@
+[
+    "fs", "node:fs", "fs/promises", "node:fs/promises", "fs-extra"
+]


Can we move this into the rule file? There doesn’t appear to be a need for this to be external.

nzakas · 2022-11-03T17:50:53Z

rules/detect-non-literal-fs-filename.js

+  return (argMeta || []).filter((argIndex) => node.arguments[argIndex].type !== 'Literal');
+}
+
+function generateReport({ context, node, packageName, methodName, indeces }) {


indeces -> indices

nzakas · 2022-11-03T17:52:09Z

rules/detect-non-literal-fs-filename.js

+    return null;
+  }
+
+  const sinks = sinkPositions(node.parent, fsMetaData[methodName]);


You use the word “sink” throughout, but it’s not clear to me what this means. Can you rename so that it’s clearer?

That's a taint analysis terminology (https://owasp.org/www-community/controls/Static_Code_Analysis). But i agree that's not a commonly known term. I tried to rename it.

nzakas · 2022-11-03T17:52:54Z

rules/detect-non-literal-fs-filename.js

+    .filter((entry) => entry.type === 'VariableDeclaration')
+    .flatMap((entry) => entry.declarations)
+    .find(
+      (d) =>


d -> declaration

for clarity

nzakas · 2022-11-03T17:53:40Z

rules/utils/import-utils.js

+/**
+ * Returns the ImportDeclaration for the import of the methodName from one of the packageNames
+ * import { methodName as a } from 'packageName';
+ */


Can you include some JSDoc comments for the parameters and return types?

BuZZ-T · 2022-11-06T22:24:25Z

The default parser will work with import statements. You just need to set parserOptions.ecmaVersion to 6 or higher and then set parserOptions.sourceType to “module”. See https://eslint.org/docs/latest/developer-guide/nodejs-api#ruletester

Ah great. I changed it accordingly.

Thanks for your review! I tried to include all your comments.

I also need to resolve the merge conflicts. Do you prefer merging main in the feature branch or rebasing? I can do both.

nzakas · 2022-11-10T19:54:09Z

If you could rebase and fix the conflicts, that would be great.

nzakas · 2022-11-24T19:01:32Z

@BuZZ-T just checking back -- do you have time to update this?

BuZZ-T · 2022-11-25T08:04:57Z

@nzakas: Oh sorry, i missed the notification. I'll fix the conflict

* detects multiple cases of possible imports of fs methods * many tests added to cover that cases

…tion in utils

BuZZ-T · 2022-11-26T22:35:12Z

@nzakas: Done. Sorry that it took so long for me to respond!

nzakas · 2022-11-30T21:41:07Z

rules/detect-non-literal-fs-filename.js

+        // this only works, when imports are on top level!
+        const program = context.getAncestors()[0];
+
+        const methodCallSinkReport = detectOnMethodCall({


Looks like we've got one "sink" left. :)

nzakas · 2022-11-30T21:42:42Z

It also looks like we have failing tests in Node.js 12 due to using ?..

BuZZ-T · 2022-12-01T22:56:25Z

It also looks like we have failing tests in Node.js 12 due to using ?..

Ah, i didn't know node 12 should also be supported. I hope they are running now.

nzakas

That works. Thanks so much for your time and effort.

BuZZ-T mentioned this pull request Sep 17, 2022

detect-non-literal-fs-filename - add a check is it fs method at the first place #89

Closed

nzakas requested changes Nov 3, 2022

View reviewed changes

BuZZ-T and others added 4 commits November 26, 2022 23:31

feat: rewritten detect-non-literal-fs-filename rule

a1ae157

* detects multiple cases of possible imports of fs methods * many tests added to cover that cases

refactor: extract searching for ImportDeclaration and VariableDeclara…

b6d7c3d

…tion in utils

chore: added JSdoc, inlined fsPackagesNames, renamed sink terminology

b9569e6

chore: changed report params to object

e46d3e2

BuZZ-T force-pushed the feature/extend-detect-non-literal-fs-filename branch from 76da428 to e46d3e2 Compare November 26, 2022 22:33

nzakas reviewed Nov 30, 2022

View reviewed changes

chore: rename 'sink', remove '?.' for node 12

e5e51b1

nzakas approved these changes Dec 6, 2022

View reviewed changes

nzakas merged commit 08ba476 into eslint-community:main Dec 6, 2022

This was referenced Dec 8, 2022

Improve detect-child-process rule #104

Closed

feat(detect-non-literal-fs-filename): change to track non-top-level require() as well #105

Merged

This was referenced Jan 12, 2023

False positive detect-non-literal-fs-filename on _.exists #26

Closed

False positive for detect-non-literal-fs-filename #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extend detect non literal fs filename #92

feat: extend detect non literal fs filename #92

BuZZ-T commented Sep 2, 2022 •

edited

Loading

revelt commented Sep 18, 2022

sakirma commented Oct 19, 2022

BuZZ-T commented Oct 25, 2022

nzakas commented Oct 27, 2022

BuZZ-T commented Oct 28, 2022

nzakas commented Nov 3, 2022

nzakas left a comment

nzakas Nov 3, 2022

nzakas Nov 3, 2022

nzakas Nov 3, 2022

BuZZ-T Nov 6, 2022

nzakas Nov 3, 2022

nzakas Nov 3, 2022

BuZZ-T commented Nov 6, 2022

nzakas commented Nov 10, 2022

nzakas commented Nov 24, 2022

BuZZ-T commented Nov 25, 2022

BuZZ-T commented Nov 26, 2022

nzakas Nov 30, 2022

nzakas commented Nov 30, 2022

BuZZ-T commented Dec 1, 2022

nzakas left a comment

feat: extend detect non literal fs filename #92

feat: extend detect non literal fs filename #92

Conversation

BuZZ-T commented Sep 2, 2022 • edited Loading

revelt commented Sep 18, 2022

sakirma commented Oct 19, 2022

BuZZ-T commented Oct 25, 2022

nzakas commented Oct 27, 2022

BuZZ-T commented Oct 28, 2022

nzakas commented Nov 3, 2022

nzakas left a comment

Choose a reason for hiding this comment

nzakas Nov 3, 2022

Choose a reason for hiding this comment

nzakas Nov 3, 2022

Choose a reason for hiding this comment

nzakas Nov 3, 2022

Choose a reason for hiding this comment

BuZZ-T Nov 6, 2022

Choose a reason for hiding this comment

nzakas Nov 3, 2022

Choose a reason for hiding this comment

nzakas Nov 3, 2022

Choose a reason for hiding this comment

BuZZ-T commented Nov 6, 2022

nzakas commented Nov 10, 2022

nzakas commented Nov 24, 2022

BuZZ-T commented Nov 25, 2022

BuZZ-T commented Nov 26, 2022

nzakas Nov 30, 2022

Choose a reason for hiding this comment

nzakas commented Nov 30, 2022

BuZZ-T commented Dec 1, 2022

nzakas left a comment

Choose a reason for hiding this comment

BuZZ-T commented Sep 2, 2022 •

edited

Loading