JS: Add library for exporting graphs as type models #15386

asgerf · 2024-01-19T20:13:36Z

Adds a parameterised module GraphExport which converts an arbitrary graph labelled with access paths, into a set of typeModel rows. This module doesn't really know about API graphs, it just sees its input as a directed graph, which simplified the logic a lot IMO.

In ApiGraphModelsExport.qll we defined a wrapper around GraphExport which additionally re-exported typeModels from upstream packages (see example below). This is complicated enough that it's nice to factor it out, and it also know a bit more about API nodes and currently needs access to the other ApiGraphModels*.qll files.

Lastly, we expose a JS-specific module ModelExport which exports the paths leading to a specific set of API nodes. The idea is that each dynamic language eventually adds its own version of ModelExport which uses the above shared components under the hood. I've prototyped one for Ruby in this draft PR, but I'd like to move ahead with just JS for now.

Re-exporting type information

This also supports re-exporting information about types from upstream libraries.

This part is contained in the second commit, and to manage complexity, it is implemented as a wrapper module around GraphExport.

Below are a few examples to demonstrate what it means to re-export type information and some of the complexity involved.

JavaScript example 1

For example, if the following is the entry point for a package bar:

// bar.js
module.exports.xxx = require('foo');

then this would generate the following type model:

foo; bar; Member[xxx]

That is, we export the fact that require('bar').xxx is an alias for the foo package.

JavaScript example 2

For a more complex case, suppose the following type model exists:

foo.XYZ; foo; Member[x].Member[y].Member[z]

And the package exports something that matches a prefix of the access path above:

module.exports.blah = require('foo').x.y;

This would result in the following type model:

foo.XYZ; bar; Member[blah].Member[z]

That is, we export the fact that require('bar').blah.z is an instance of foo.XYZ.

Notice that the access path Member[blah].Member[z] consists of an access path generated from the API
graph, with pieces of the access path from the original type model appended to it.

shared/mad/codeql/mad/dynamic/GraphExport.qll

…classes

erik-krogh

Looks good 👍
I only have a few minor questions/comments.

shared/mad/codeql/mad/dynamic/GraphExport.qll

javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExport.qll

RasmusWL

Overall this work LGTM 👍 There are some points I would like to understand better though (see inline discussions).

While reading through the commits I noticed that javascript/ql/test/library-tests/ModelGeneration/ModelGeneration.expected is nearing a level of complexity where I'm starting to think inline-expectation-tests would pay off

RasmusWL · 2024-04-12T09:18:26Z

javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll

+ * Holds if the edge `pred -> succ` labelled with `path` exists in the API graph.
+ */
+bindingset[pred]
+predicate apiGraphHasEdge(API::Node pred, string path, API::Node succ) {


do we have any guarantees (or tests) to show that the edge names we generate here are valid MaD access-paths?

Say we changed Member[xxx] to be Property[xxx] in the rest of our MaD modeling, would the whole type-model export just silently produce models that couldn't be parsed until a human realized there was a problem? 🤔

I can imagine scenarios where we try to parse the access-path generated as if it had been in a YAML file, maybe even checking that if we start from a node that we generate an MaD access-path for, we will reach the same node from parsing and following that access-path

(I realize it's not so much a question about this specific predicate, as a thing in whole... and the question might be answered from reading some of the next commits)

No guarantees and no currently no tests. I spent a bit of time refactoring the ModelOutput::getAWarning into a parameterised module so we could check these as well, but it gets a bit fiddly. I backed out of it for now.

Some tests are going to exist in an internal repo where we use model generation, in the sense that those tests would fail if there was a mismatch here.

I think it would be nice with these kinds of tests, however I'm not going to block this PR to get them in NOW.

however I'm going to block this PR to get them in NOW.

I assume a "not" was accidentally left out of that sentence 😅

indeed 🙈 (fixed by edit)

javascript/ql/test/library-tests/ModelGeneration/ModelGeneration.expected

RasmusWL · 2024-04-12T09:40:05Z

shared/mad/codeql/mad/dynamic/GraphExport.qll

+   * Holds if `name` is a good name for `node` that should be used in case the node needs
+   * to be named with a type name.
+   *
+   * Should not hold for nodes that are named via `exposedName`.


sounds like we could make some consistency check for this 😊

But that would be more work 😱

javascript/ql/lib/semmle/javascript/frameworks/data/ModelsAsData.qll

shared/mad/codeql/mad/dynamic/GraphExport.qll

RasmusWL · 2024-04-12T10:09:29Z

javascript/ql/test/library-tests/ModelGeneration/ModelGeneration.expected

+| (return-this).FluentInterface.prototype | (return-this).FluentInterface.prototype.bar | ReturnValue |
+| (return-this).FluentInterface.prototype | (return-this).FluentInterface.prototype.baz | ReturnValue |


Why is there not this row?

| (return-this).FluentInterface.prototype | (return-this).FluentInterface.prototype.foo | ReturnValue |

Huh, good question. The flow is captured by the summary model so it shouldn't be a problem, but it is a bit strange that it's generated for bar,baz but not foo.

I don't quite know what to say around this. I think it would be nice if we could at least explain why it's happening, but I might be a little too cautious around this.

I figured it out, turns out this was due to a bug in API graphs. See 3c885f3.

Nice catch, btw!

javascript/ql/test/library-tests/ModelGeneration/ModelGeneration.expected

javascript/ql/test/library-tests/ModelGeneration/ModelGeneration.ext.yml

…ta.qll Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

RasmusWL

just a few qldoc suggestions

shared/mad/codeql/mad/dynamic/GraphExport.qll

RasmusWL · 2024-04-16T09:10:35Z

shared/mad/codeql/mad/dynamic/GraphExport.qll

+  }
+
+  /**
+   * Holds if a named type exists or will be generated for `node`.


Suggested change

* Holds if a named type exists or will be generated for `node`.

* Holds if a named type exists or will be generated for `node`, and `node` doesn't have a pretty name.

Suggested change

* Holds if a named type exists or will be generated for `node`.

* Holds if a synthetic name must be generated for `node`.

Actually the existing parts of the qldoc was wrong, let me just fix that.

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

…aph-export

javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExport.qll

This only worked when the RHS was a SourceNode, which is not generally the case

asgerf · 2024-04-18T09:56:14Z

Thanks for the reviews everyone!

github-actions bot added the JS label Jan 19, 2024

asgerf force-pushed the js/graph-export branch 2 times, most recently from 4751f68 to 2f8bbb0 Compare April 4, 2024 13:41

github-advanced-security bot found potential problems Apr 4, 2024

View reviewed changes

shared/mad/codeql/mad/dynamic/GraphExport.qll Dismissed Show dismissed Hide dismissed

asgerf force-pushed the js/graph-export branch 2 times, most recently from 9042409 to d2a3093 Compare April 8, 2024 08:16

asgerf added 15 commits April 9, 2024 14:32

Dynamic/JS: Add library for exporting models

acef9b7

Dynamic/JS: Add support for re-exporting type models

c55e03c

JS: Add a test case with fluent flow

348c95e

JS: Add test for class with aliases

946f0b4

JS: Add tests with semi-internal class problem

f4e05cc

JS: Add test where root export object is a function

ab3c03d

JS: Add access path alias test

3022c59

JS: Add partial test for subclassing

ef7767b

JS: Add subclassing test and fix lack of subclassing handling

9313564

JS: Add test showing missing re-export of base class relationship

f2ea88a

JS: More re-export logic to handle subclass export

56ebe6c

JS: Add test case showing problem with chains going through internal …

29a6145

…classes

JS: Ensure MkClassInstance exists for base classes

81b96a8

JS: Switch from hasLocationInfo to Location

8cb80d6

Dynamic: Add hasPrettyName()

8210143

asgerf force-pushed the js/graph-export branch from d2a3093 to 8210143 Compare April 9, 2024 12:34

Dynamic: Sync ApiGraphModels.qll

f5355cf

github-actions bot added Python Ruby labels Apr 9, 2024

asgerf added the no-change-note-required This PR does not need a change note label Apr 9, 2024

asgerf mentioned this pull request Apr 9, 2024

Ruby: prototype instantiation of graph export #16165

Closed

asgerf marked this pull request as ready for review April 9, 2024 13:29

asgerf requested review from a team as code owners April 9, 2024 13:29

asgerf requested a review from a team as a code owner April 9, 2024 13:29

erik-krogh previously approved these changes Apr 10, 2024

View reviewed changes

JS: Address review comments

15eabb4

asgerf dismissed erik-krogh’s stale review via 15eabb4 April 12, 2024 09:35

RasmusWL reviewed Apr 12, 2024

View reviewed changes

asgerf and others added 2 commits April 12, 2024 15:00

Update javascript/ql/lib/semmle/javascript/frameworks/data/ModelsAsDa…

330229c

…ta.qll Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

Update shared/mad/codeql/mad/dynamic/GraphExport.qll

3949ae4

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

erik-krogh previously approved these changes Apr 12, 2024

View reviewed changes

RasmusWL reviewed Apr 16, 2024

View reviewed changes

Update shared/mad/codeql/mad/dynamic/GraphExport.qll

844b29b

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

asgerf dismissed erik-krogh’s stale review via 844b29b April 16, 2024 18:09

asgerf added 4 commits April 16, 2024 20:10

Update shared/mad/codeql/mad/dynamic/GraphExport.qll

ee5cb6f

Merge branch 'main' into js/graph-export

be64daf

Merge branch 'js/graph-export' of github.com:asgerf/codeql into js/gr…

c0db40d

…aph-export

Sync files

3335d48

asgerf mentioned this pull request Apr 16, 2024

JS: Add library for exporting graphs as type models (v2) #16235

Closed

Merge branch 'main' into js/graph-export

93a9c62

hvitved reviewed Apr 17, 2024

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExport.qll Outdated Show resolved Hide resolved

asgerf added 2 commits April 17, 2024 13:31

JS: Use AccessPath as parameter type

5e7026c

JS: Fix bug in MkClassInstance use-nodes

3c885f3

This only worked when the RHS was a SourceNode, which is not generally the case

RasmusWL approved these changes Apr 18, 2024

View reviewed changes

asgerf merged commit decd576 into github:main Apr 18, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: Add library for exporting graphs as type models #15386

JS: Add library for exporting graphs as type models #15386

asgerf commented Jan 19, 2024 •

edited

Loading

erik-krogh left a comment

RasmusWL left a comment

RasmusWL Apr 12, 2024

asgerf Apr 12, 2024 •

edited

Loading

RasmusWL Apr 16, 2024 •

edited

Loading

asgerf Apr 18, 2024

RasmusWL Apr 18, 2024

RasmusWL Apr 12, 2024

asgerf Apr 12, 2024

RasmusWL Apr 12, 2024

asgerf Apr 12, 2024

RasmusWL Apr 16, 2024

asgerf Apr 18, 2024

RasmusWL left a comment

RasmusWL Apr 16, 2024

asgerf Apr 16, 2024

asgerf commented Apr 18, 2024

		\| (return-this).FluentInterface.prototype \| (return-this).FluentInterface.prototype.bar \| ReturnValue \|
		\| (return-this).FluentInterface.prototype \| (return-this).FluentInterface.prototype.baz \| ReturnValue \|

	* Holds if a named type exists or will be generated for `node`.
	* Holds if a named type exists or will be generated for `node`, and `node` doesn't have a pretty name.

	* Holds if a named type exists or will be generated for `node`.
	* Holds if a synthetic name must be generated for `node`.

JS: Add library for exporting graphs as type models #15386

JS: Add library for exporting graphs as type models #15386

Conversation

asgerf commented Jan 19, 2024 • edited Loading

Re-exporting type information

JavaScript example 1

JavaScript example 2

erik-krogh left a comment

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

RasmusWL Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf commented Apr 18, 2024

asgerf commented Jan 19, 2024 •

edited

Loading

asgerf Apr 12, 2024 •

edited

Loading

RasmusWL Apr 16, 2024 •

edited

Loading