JS: provide command execution sinks for execa package

[am0o0]

Execa package before version 5 has already been modeled but newer versions up to 8 have many new APIs that I've implemented now.
@erik-krogh the SecondOrderCommandInjectionQuery doesn't work for me here. please look at the test for examples, I already put comments on which lines are vulnerable to second-order command injection.

[erik-krogh]

Here is a first round of comments, it will take a while before we can get this ready to merge.
This is not an exhaustive review, but it should give you plenty to work on.
I suggest you only continue with the other library models after we've gotten this PR into a mergable state.

---

The tests in this PR are in general very confusing to read.
You're mixing file-system access with system-command execution, and nothing in the code/comments in tst.js tells me which is which.

You also have plenty of "GOOD" and "BAD" comments, but I can't figure out what they actually mean.
Just as an example: Line 7 and 9 of tst.js are annotated with "GOOD" and "BAD" respectively.
But they both work only on string constants, and are thus completely harmless, and the only thing you're testing is that they are both recognized as system-command-executions (which they should be).
Basically: there is no connection between the test outcomes (the .expected file) and the comments in the test.

Also, the tests are not testing very much.
E.g. your test_SystemCommandExecution is only testing that a command-execution is recognized, not what the command-argument or argument-list of that command-execution is.
Adding some parameters to the test predicates can help.

---

Another thing that would be really useful is to add some end-to-end tests.
E.g. adding a new JavaScript file in javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/, where test specifically for command-execution, and annotate each line with // OK / // NOT OK similar to how other files in that directory do it.

---

A reason for second-order-command-injection not working is that you haven't implemented the optional getArgumentList predicate on the relevant class. When I add an implemtation of that predicate to ExecaExec I can get this example to work just fine: execa('git', ['ls-remote', unsafeValue]); // NOT OK.

[am0o0]

@erik-krogh sorry for the many mistakes on this PR. I tried to solve all of your review suggestions.

[erik-krogh]

@erik-krogh sorry for the many mistakes on this PR. I tried to solve all of your review suggestions.

That's OK. We set a high bar here for testing and code quality, especially when you try to get something merged into our standard query set.
(Take a look at the experimental/ folder if you want to have your PR merged faster, but then you don't have as much impact).

---

In other news.
I just opened a PR to recognize tagged template literals as a DataFlow::CallNode: #14405

I suggest you take a look at that PR and incorporate it into your PR after my PR has been merged.
There is no rush, so just wait until my PR has been merged.

[erik-krogh]

In other news.
I just opened a PR to recognize tagged template literals as a DataFlow::CallNode: #14405

#14405 has been merged, so you can incorporate that into this PR.

[am0o0]

@erik-krogh it is great that I can reach each part of the following example with the API graph:

$({shell: false}).sync`${cmd} ${arg} ${arg} ${arg2}`; // NOT OK

I think there is one problem.
if we have :

$({shell: false}).sync`ssh ${arg} ${arg} ${arg2}`; // NOT OK

then I can't specify getArgumentList and getACommandArgument because I can't determine the tag literal string value to check whether there is a hardcoded command or not.
please look at the test examples and run the ExecaScript class predicates for execa.js I tried to fully support the test cases.
Also, it seems that when I'm using a predicate like isExecaShellEnable inside the isShellInterpreted predicate of ExecaScript, I can't reach other parameters.

[erik-krogh]

I can't determine the tag literal string value to check whether there is a hardcoded command or not.

Well, you can, it's just a little tricky.
If you get the TaggedTemplateExpr (using e.g. node.asSink().asExpr().(TaggedTemplateExpr)), then you can get the template elements from that, and you can use those to determine whether the first argument encodes the command or not.

[am0o0]

@erik-krogh
should I move all of these PR changes to the experimental directory too?

[erik-krogh]

@erik-krogh should I move all of these PR changes to the experimental directory too?

That's up to you.
But this PR can be merged much faster if you do.

[am0o0]

@erik-krogh I moved this PR to experimental dir.

[erik-krogh]

@erik-krogh I moved this PR to experimental dir.

Looks OK. I'll give this a final review when you mark this PR as ready for review.

[am0o0]

@erik-krogh this PR also is not marked as ready for review but you reviewed this before, can I mark this as Ready for review too?

[erik-krogh]

can I mark this as Ready for review too?

Yes 👍

[erik-krogh]

The branch was getting old. I did a merge with main which should hopefully also trigger the CI.

Edit: Another merge with main. A CI failure was confusing, so I'll see if that gets fixed on its own.
Edit Edit: It did.