feat: extend detect non literal fs filename #92
Conversation
|
Good job! |
|
Any idea when these changes will be merged? |
|
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. |
|
No worries! Thanks for your help!
I only used the |
|
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 |
rules/data/fsPackagesNames.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| [ | |||
| "fs", "node:fs", "fs/promises", "node:fs/promises", "fs-extra" | |||
| ] | |||
There was a problem hiding this comment.
Can we move this into the rule file? There doesn’t appear to be a need for this to be external.
| return (argMeta || []).filter((argIndex) => node.arguments[argIndex].type !== 'Literal'); | ||
| } | ||
|
|
||
| function generateReport({ context, node, packageName, methodName, indeces }) { |
| return null; | ||
| } | ||
|
|
||
| const sinks = sinkPositions(node.parent, fsMetaData[methodName]); |
There was a problem hiding this comment.
You use the word “sink” throughout, but it’s not clear to me what this means. Can you rename so that it’s clearer?
There was a problem hiding this comment.
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.
| .filter((entry) => entry.type === 'VariableDeclaration') | ||
| .flatMap((entry) => entry.declarations) | ||
| .find( | ||
| (d) => |
There was a problem hiding this comment.
d -> declaration
for clarity
| /** | ||
| * Returns the ImportDeclaration for the import of the methodName from one of the packageNames | ||
| * import { methodName as a } from 'packageName'; | ||
| */ |
There was a problem hiding this comment.
Can you include some JSDoc comments for the parameters and return types?
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. |
|
If you could rebase and fix the conflicts, that would be great. |
|
@BuZZ-T just checking back -- do you have time to update this? |
|
@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
BuZZ-T
force-pushed
the
feature/extend-detect-non-literal-fs-filename
branch
from
76da428
to
e46d3e2
Compare
|
@nzakas: Done. Sorry that it took so long for me to respond! |
| // this only works, when imports are on top level! | ||
| const program = context.getAncestors()[0]; | ||
|
|
||
| const methodCallSinkReport = detectOnMethodCall({ |
There was a problem hiding this comment.
Looks like we've got one "sink" left. :)
|
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. |
I think the changes are now in a state where a PR can be created.
I've done the following things:
import) can be used in testsdetect-literal-fs-filenameto check from where the inspected method / function is importednode:prefix in package namesI'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