JS: Refactorization of NoSQL.qll to use API-graphs #4363

erik-krogh · 2020-09-29T13:44:21Z

A bit of precision has been lost compared to the previous implementation, due to limitations of the API-graphs API.
But interestingly, no test case failed as a result.
The imprecisions are in the top of NoSQL.qll (getAMongoClient() / getAMongoDb()).

I added a new convenience method to API::EntryPoint, which allows me to easily get an API::Node from an EntryPoint.

I had to change the implementation in some places to avoid API-graph related gotchas.
For example: instead of Mongoose::InvokeNode (modelling a call to a Mongoose function) I now have a Mongoose::MongooseFunction (modelling the Mongoose function itself).

max-schaefer · 2020-10-02T09:03:12Z

javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll

    or
-    exists(DataFlow::TypeBackTracker t2 | result = getAMongoDbCallback(t2).backtrack(t2, t))
+    // slightly imprecise, is not supposed to have a result if the parameter name is "db" (that would be a mongodb v2 `Db`).
+    result = getAMongoDbCallback().getParameter(1)


Can't you use the newly introduced getAnImmediateUse to rule this out? The immediate use would be the DataFlow::Parameter itself, so you can check its name.

erik-krogh · 2020-10-03T20:09:18Z

An evaluation came back bad because of a bad join-order (see below tuple counts).

I changed the predicate a bit such that the performance should be good now.

[2020-10-03 18:44:59] (458s) Tuple counts for NoSQL::Mongoose::Document::getAMongooseDocument#f#join_rhs#1:
1         ~0%     {4} r1 = CONSTANT(unique string, unique string, unique string, unique int)["then","0","parameter 0",0]
1         ~0%     {4} r2 = SELECT r1 ON r1.<3> >= -1
1         ~0%     {4} r3 = SELECT r2 ON 0 >= -1
1         ~0%     {2} r4 = SCAN r3 OUTPUT r3.<2>, ("member " ++ r3.<0>)
1         ~0%     {4} r5 = CONSTANT(unique string, unique string, unique string, unique int)["exec","1","parameter 1",1]
1         ~0%     {4} r6 = SELECT r5 ON r5.<3> >= -1
1         ~0%     {4} r7 = SELECT r6 ON 0 >= -1
1         ~0%     {2} r8 = SCAN r7 OUTPUT r7.<2>, ("member " ++ r7.<0>)
2         ~0%     {2} r9 = r4 \/ r8
272334    ~4%     {3} r10 = JOIN r9 WITH ApiGraphs::API::Impl::edge#2#fff_102#join_rhs AS R ON FIRST 1 OUTPUT r9.<1>, R.<1>, R.<2>
548812365 ~0%     {4} r11 = JOIN r10 WITH ApiGraphs::API::Impl::edge#2#fff_102#join_rhs AS R ON FIRST 1 OUTPUT R.<1>, r10.<1>, r10.<2>, R.<2>
                  return r11

erik-krogh · 2020-10-04T20:46:19Z

A new evaluation came back, and it's looking good.

I think it might actually be a performance improvement.
(And no changes to --metric TaintSinks.ql, which is a good).

max-schaefer

Fantastic work, thanks! I have a few suggestions for improving terminological consistency, and one TypeScript-related question. I'm fine with addressing the latter in a follow-up PR to avoid upsetting the performance apple cart.

javascript/ql/src/semmle/javascript/ApiGraphs.qll

javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll

max-schaefer · 2020-10-05T07:50:39Z

javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll

    or
-    exists(DataFlow::TypeBackTracker t2 | result = getAMongoDbCallback(t2).backtrack(t2, t))
+    result = getAMongoDbCallback().getParameter(1) and
+    not result.getAnImmediateUse().(DataFlow::ParameterNode).getName() = "db" // mongodb v2 provides a `Db` here


If we end up doing this a lot, it might be nice to have a predicate asParameter() on API::Node, similar to how we have asExpr() and similar on data-flow nodes (but no need to squeeze it into this PR).

max-schaefer · 2020-10-05T07:52:01Z

javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll

+  private class TypedMongoCollection extends API::EntryPoint {
+    TypedMongoCollection() { this = "TypedMongoCollection" }
+
+    override DataFlow::SourceNode getAUse() { result.hasUnderlyingType("mongodb", "Collection") }


API graphs are meant to support canonical type names. Have you tried using them instead? (They aren't particularly well exposed in the API at the moment, but they probably should be.)

To expand on the above: you are meant to be able to do root().getMember("mongodb").getMember("Collection").getInstance() (cf this test). If that doesn't work, I'll fix it.

To expand on the above: you are meant to be able to do root().getMember("mongodb").getMember("Collection").getInstance() (cf this test). If that doesn't work, I'll fix it.

That worked, except it ends up looking like:

result = API::root().getASuccessor(API::EdgeLabel::mod("mongodb")).getMember("Collection").getInstance()

The mongodb type has edge "mod mongodb", but it is not a module-import so using API::moduleImport("mongodb") does not work.

The API could use a cleanup, but lets do that later.

FWIW I ran into the same issue with the Angular2 model and held off on using API graphs for this reason.

The snippet you posted leaves more questions than it answers. Is root().getMember(x) the same as API::moduleImport(x)? If Collection is the name of an interface, what exactly does getInstance mean?

That worked, except it ends up looking like: (...)

🙈

The mongodb type has edge "mod mongodb", but it is not a module-import so using API::moduleImport("mongodb") does not work.

That's an oversight. I'll fix it, then we can see about using it in the NoSQL model.

If Collection is the name of an interface, what exactly does getInstance mean?

Ah, I had hoped that the concept of being an instance of a type would be natural, but if it's not we can introduce a new kind of edge for it. Any ideas for what to call it?

Ah, I had hoped that the concept of being an instance of a type would be natural, but if it's not we can introduce a new kind of edge for it. Any ideas for what to call it?

I think getInstance() is natural. We just need some utility-predicate to work with CanonicalNames.

I'd be in favor of a classless predicate that's a direct replacement for hasUnderlyingType:

API::nodeOfType("mongodb", "Collection")

I think representing values, type names, and type-namespaces as the same kind of node will lead to confusion, as most of the API::Node predicates are meaningless for type names and type-namespaces. Going straight to the "value" space means you can never mix up the ~~two~~ three kinds of nodes.

I don't think we need to change the underlying encoding.

Something like this?

javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll

Co-authored-by: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

erik-krogh · 2020-10-05T14:46:52Z

I think I've found a potential performance improvement. But that can wait for later, this PR is good to merge as is (when it comes to performance).

@max-schaefer
Have you considered making a newtype Label in API-Graphs?
Currently the ApiGraphs::API::Node::getAMember predicate performs quite badly on vscode, and the reason looks to be the string labels (tuple counts and clause timings).

I did a quick-and-dirty experiment trying to make a newtype Label, and vscode became significantly faster.
(See clause timings in notes).

max-schaefer · 2020-10-05T14:56:06Z

Have you considered making a newtype Label in API-Graphs?

Yes. It's something I've gone back and forth about a couple of times. The problem with the newtype approach is that it requires us to specify at the definition site which labels we want, which is a little error-prone. With the bindingset annotation we can get away with doing that at the use site, which is more convenient. I experimented with having an abstract class of label names that is used in the definition and extended at the use sites, but that became so verbose that I went back to bindingsets.

Definitely worth revisiting that decision if it benefits performance.

erik-krogh · 2020-10-05T15:20:57Z

Definitely worth revisiting that decision if it benefits performance.

This is the implementation I used in my small experiment: erik-krogh@0ec5e0e

(I forgot to add the link in the previous comment).

max-schaefer · 2020-10-05T15:35:17Z

Thanks for sharing. I think that implementation nicely highlights the duplication you get with that approach.

max-schaefer · 2020-10-05T15:52:10Z

I'd prefer to remove the last commit. Then we have something that's identical (modulo comments) to the version that was evaluated. I'll fix the TypeScript API in a separate PR.

erik-krogh · 2020-10-05T17:07:23Z

I'd prefer to remove the last commit. Then we have something that's identical (modulo comments) to the version that was evaluated. I'll fix the TypeScript API in a separate PR.

I've reverted the last commit.

erik-krogh added the WIP This is a work-in-progress, do not merge yet! label Sep 29, 2020

github-actions bot added the JS label Sep 29, 2020

erik-krogh force-pushed the nosql-api branch 7 times, most recently from 1b525de to 0060473 Compare October 1, 2020 13:10

erik-krogh removed the WIP This is a work-in-progress, do not merge yet! label Oct 1, 2020

erik-krogh force-pushed the nosql-api branch from 0060473 to 5aa1960 Compare October 1, 2020 13:56

refactor the NoSQL model to use API graphs

abdbe92

erik-krogh force-pushed the nosql-api branch from 5aa1960 to abdbe92 Compare October 2, 2020 08:43

erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Oct 2, 2020

erik-krogh marked this pull request as ready for review October 2, 2020 08:43

erik-krogh requested a review from a team as a code owner October 2, 2020 08:43

max-schaefer reviewed Oct 2, 2020

View reviewed changes

improve precision using getAnImmediateUse to check parameter names

6acb199

erik-krogh changed the title ~~JS: In progress refactorization of NoSQL.qll to use API-graphs~~ JS: Refactorization of NoSQL.qll to use API-graphs Oct 2, 2020

join-order improvement in NoSQL.qll

856ad07

max-schaefer suggested changes Oct 5, 2020

View reviewed changes

Apply suggestions from code review

2753a4f

Co-authored-by: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

erik-krogh force-pushed the nosql-api branch from f564161 to 2753a4f Compare October 5, 2020 17:07

max-schaefer approved these changes Oct 5, 2020

View reviewed changes

codeql-ci merged commit e95b665 into github:main Oct 5, 2020

max-schaefer mentioned this pull request Oct 9, 2020

JavaScript: Improve integration of TypeScript types into API graphs #4449

Merged

JS: Refactorization of NoSQL.qll to use API-graphs #4363

JS: Refactorization of NoSQL.qll to use API-graphs #4363

Uh oh!

Conversation

erik-krogh commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Oct 3, 2020

Uh oh!

erik-krogh commented Oct 4, 2020

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erik-krogh commented Oct 5, 2020

Uh oh!

max-schaefer commented Oct 5, 2020

Uh oh!

erik-krogh commented Oct 5, 2020

Uh oh!

max-schaefer commented Oct 5, 2020

Uh oh!

max-schaefer commented Oct 5, 2020

Uh oh!

erik-krogh commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erik-krogh commented Sep 29, 2020 •

edited

Loading

asgerf Oct 5, 2020 •

edited

Loading