Skip to content

JS: add more models for file related sinks and sources #3065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 17, 2020

Conversation

erik-krogh
Copy link
Contributor

Adds new sinks/sources related to NPM libraries that does file operations.

The sinks are mostly related to js/path-injection, but there are also a few other queries that benefit from these added sources/sinks.

Here is a LGTM run that shows some (mostly safe) uses of the new sinks.

An evaluation on relevant queries show good performance, and a single new result that looks like a TP (but is not dangerous).

I found some other sinks that use some promise flow, I'll try those out after #3036 has been merged.

I've chosen not to include the popular del library as a sink, as it protects against deleting the current working directory and above.

@erik-krogh erik-krogh added the JS label Mar 15, 2020
@erik-krogh erik-krogh requested a review from a team as a code owner March 15, 2020 16:53
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. I just have a few non-semantic requests.

Regarding ProducesFileNames and ProducedFileName, do you have other uses of those classes in the pipeline? Otherwise, it seems like a minor overkill to introduce them for a single use case in RecursiveReadDir.

/**
* Gets a file name produced by this producer.
*/
abstract DataFlow::Node getAFileNameSource();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be getAFileName as this the output of the producer. getAFileNameSource makes it sound like it is the input.
(I do realise that this is the predicate name used in FileNameSource, but that class is using a different perspective)

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Mar 17, 2020

Regarding ProducesFileNames and ProducedFileName, do you have other uses of those classes in the pipeline? Otherwise, it seems like a minor overkill to introduce them for a single use case in RecursiveReadDir.

Yes, I expect to use ProducesFileNames again in later PRs.

erik-krogh and others added 2 commits March 17, 2020 11:26
Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Now we only need a giant change note, perhaps restricted to just the most popular libraries you have modelled here.

@semmle-qlci semmle-qlci merged commit ea46873 into github:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants