JS: Add steps through date-formatting functions#4567
Conversation
| */ | ||
| private class MomentFormatStep extends TaintTracking::AdditionalTaintStep, DataFlow::CallNode { | ||
| MomentFormatStep() { | ||
| this = API::moduleImport("moment").getASuccessor*().getMember("format").getACall() |
There was a problem hiding this comment.
I think getASuccessor() is a bit too general.
If you look at this LGTM query you can see some stuff that is included by getASuccessor(), that I don't think should be included.
I think a recursive predicate that uses getAMember() and getReturn() is the way to go.
| @@ -0,0 +1,61 @@ | |||
| private import javascript | |||
|
|
|||
| private API::Node formatFunction() { | |||
There was a problem hiding this comment.
This is date-fns specific, but the predicate name doesn't convey that message.
How about moving the date-fns specific classes/predicates into a DateFns module?
| private API::Node formatFunctionCurried() { | ||
| result = | ||
| API::moduleImport(["date-fns/fp", "date-fns/fp/utc"]).getMember(["format", "lightFormat"]) | ||
| or | ||
| result = | ||
| API::moduleImport(["date-fns/fp/format", "date-fns/fp/lightFormat", "date-fns/fp/utc/format", | ||
| "date-fns/fp/utc/lightFormat"]) | ||
| } |
There was a problem hiding this comment.
Looks like you missed the date-fns/esm folder. (Documentation)
The esm folder also contains a esm/fp sub folder.
There was a problem hiding this comment.
Good catch, After a closer look it seems the utc folder doesn't actually exist in the latest version of date-fns 1.x nor in 2.x, so I removed that at the same time.
|
I snuck in support for the |
|
...and I better stop here and run and evaluation. |
erik-krogh
left a comment
There was a problem hiding this comment.
You're just missing the new libraries from the change-log.
Otherwise LGTM (assuming the evaluation works out).
432c62b to
1e45bc7
Compare
|
A smoke-test evaluation looks fine. Rebased and reverted changes to the 1.26 change notes and added some for 1.27 in the new format ✨ |
Not sure these will have much impact tbh, but now we've got them covered at least.