JS: Added support for Array.prototype.[findLastIndex, findLast] ES2022 feature #18005
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
04830fb
to
8795d1a
Compare
8795d1a
to
04830fb
Compare
b025fc6
to
0648392
Compare
0648392
to
1b0f8aa
Compare
erik-krogh
reviewed
Nov 19, 2024
Comment on lines
125
to
129
| { // Test for findLastIndex function | ||
| const list = ["source"]; | ||
| const element = list.findLastIndex((item) => sink(item)); // NOT OK -- Not caught, currently missing dataflow tracking. | ||
| const element = list.findLastIndex((item) => sink(item)); // NOT OK | ||
| sink(element); // OK | ||
| } |
There was a problem hiding this comment.
Could you also add a test like:
const element = source.findLastIndex((item) => sink(item)); // NOT OK - only found with taint-tracking.
sink(element); // OK
There was a problem hiding this comment.
Adds test case: f1e95a8
449c51b
to
28ead40
Compare
erik-krogh
reviewed
Nov 20, 2024
| override predicate step(DataFlow::Node obj, DataFlow::Node element) { | ||
| exists(DataFlow::MethodCallNode call | | ||
| call.getMethodName() = ["findLast", "find", "findLastIndex"] and | ||
| obj = call.getReceiver().getALocalSource() and |
There was a problem hiding this comment.
Suggested change
| obj = call.getReceiver().getALocalSource() and | |
| obj = call.getReceiver() and |
I don't think you need .getALocalSource() here.
Try to see if the tests work with this change.
| @@ -492,7 +492,20 @@ private module ArrayLibraries { | |||
| exists(DataFlow::MethodCallNode call | | |||
| call.getMethodName() = ["findLast", "find", "findLastIndex"] and | |||
| prop = arrayLikeElement() and | |||
| obj = call.getReceiver() and | |||
| obj = call.getReceiver().getALocalSource() and | |||
There was a problem hiding this comment.
Suggested change
| obj = call.getReceiver().getALocalSource() and | |
| obj = call.getReceiver() and |
In the storeStep you definitely need .getALocalSource(), but I'm not sure you need it here?
See if this suggestion still passes the tests.
erik-krogh
approved these changes
Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added support for ES2023 feature,
Arrray.protype.findLastIndex,Arrray.protype. findLast.DCA run shows that the run time stays approximately the same. Meta queries revealed new taint for
Arrray.protype.findcalls with lambda, which was expected.