C#: Add VS Code model editor queries by koesie10 · Pull Request #14200 · github/codeql

koesie10 · 2023-09-13T11:13:06Z

This adds two queries that support the CodeQL Model Editor feature in the CodeQL VS Code extension. These queries will be used to retrieve models that can be modeled using MaD. The query results they produce can also be converted to SARIF, hence the static strings in the query.

The AutomodelVsCode.qll file is similar to ExternalApi.qll, but they are used for completely different purposes, and changes to the ExternalApi.qll do not necessarily need to propagate to AutomodelVsCode.qll.

These queries have been in use for a few weeks now in the VS Code extension (see csharp.ts). Using this, we have confirmed that these queries return the results we need. Once these queries are merged and released, we will switch to resolving the queries from the query packs. We will do so based on the tags.

csharp/ql/src/utils/modeleditor/AutomodelVsCode.qll

csharp/ql/test/utils/modeleditor/PublicClass.cs

michaelnebel · 2023-09-18T09:04:01Z

This adds two queries that support the CodeQL Model Editor feature in the CodeQL VS Code extension. These queries will be used to retrieve models that can be modeled using MaD. The query results they produce can also be converted to SARIF, hence the static strings in the query.

The AutomodelVsCode.qll file is similar to ExternalApi.qll, but they are used for completely different purposes, and changes to the ExternalApi.qll do not necessarily need to propagate to AutomodelVsCode.qll.

These queries have been in use for a few weeks now in the VS Code extension (see csharp.ts). Using this, we have confirmed that these queries return the results we need. Once these queries are merged and released, we will switch to resolving the queries from the query packs. We will do so based on the tags.

This is really cool!
Thank you very much for doing this!!!

koesie10 · 2023-09-18T13:48:36Z

@michaelnebel Thank you for the review! I've implemented your feedback, although I'm not very familiar with CodeQL, so I'm not entirely sure about the changes for the unbound declarations.

michaelnebel

Thank you once again for doing this!
I have added a couple of more comments. Besides the comments in the code, it also worth considering, whether we need a DCA test suite for these queries as they are excluded from all existing test suites (in the selectors). When doing changes to the queries going forward, it is nice to have some tooling to verify that we don't break the results they produce and/or breaks their performance.

For consistency, I will also trigger a DCA run for one of our query suites for check that these queries are indeed excluded by the selector update.

csharp/ql/src/utils/modeleditor/FetchApplicationModeMethods.ql

csharp/ql/src/utils/modeleditor/ModelEditor.qll

csharp/ql/test/utils/modeleditor/PublicClass.cs

csharp/ql/src/utils/modeleditor/FetchApplicationModeMethods.ql

csharp/ql/test/utils/modeleditor/FetchApplicationModeMethods.qlref

csharp/ql/test/utils/modeleditor/FetchFrameworkModeMethods.qlref

michaelnebel · 2023-09-19T08:29:58Z

csharp/ql/test/utils/modeleditor/FetchFrameworkModeMethods.expected

+| PublicGenericInterface.cs:7:10:7:14 | stuff | GitHub.CodeQL.PublicGenericInterface<>#stuff(T) | false | supported | PublicGenericInterface.cs | library |  | type | unknown | classification |
+| PublicGenericInterface.cs:9:10:9:19 | stuff2<> | GitHub.CodeQL.PublicGenericInterface<>#stuff2<>(T2) | false | supported | PublicGenericInterface.cs | library |  | type | unknown | classification |
+| PublicGenericInterface.cs:11:17:11:27 | staticStuff | GitHub.CodeQL.PublicGenericInterface<>#staticStuff(System.String) | false | supported | PublicGenericInterface.cs | library |  | type | unknown | classification |
+| PublicInterface.cs:7:10:7:14 | stuff | GitHub.CodeQL.PublicInterface#stuff(System.String) | false | supported | PublicInterface.cs | library |  | type | unknown | classification |


All the callables are reported to be not supported as there doesn't exist any models for them.
Maybe consider to add some positive testcases for this as well.
It is possible to add a data extension file that only applies to an individual test case (codeql test run looks for .ext.yml files with the same name as the test case). That is, if you add a file called FetchFrameworkModeMethods.ext.yml you can add models for some of your testmethods.

This will implicitly help in testing that we got the predicates for isSupported right (e.g. getAnInput and getAnOutput). The ApplicationMode testcase primarily targets "source" models.

Thanks for the suggestion! I've added 4 new methods with the 4 different types of models. The summaryModel and neutralModel seem to work, but I can't get the sinkModel or sourceModel to work. Would you be able to take a look and check why those are not working?

I think the problem is that the implementation requires that for a callable to be identified as a source or sink there needs to be an actual call to it (this works fine for the queries that are just interested in callables NOT the source code). We need to do something else for the analysis that should respect source and sink models for EndPoints in the source code (I will look into this).

michaelnebel · 2023-09-19T08:39:01Z

@michaelnebel Thank you for the review! I've implemented your feedback, although I'm not very familiar with CodeQL, so I'm not entirely sure about the changes for the unbound declarations.

Thank you and totally understandable! You are doing great!

michaelnebel · 2023-09-19T11:25:21Z

DCA using the code-scanning query suite looks good (these queries are not executed).

koesie10 · 2023-09-19T14:24:16Z

Besides the comments in the code, it also worth considering, whether we need a DCA test suite for these queries as they are excluded from all existing test suites (in the selectors). When doing changes to the queries going forward, it is nice to have some tooling to verify that we don't break the results they produce and/or breaks their performance.

How would I go about doing that? Would I need to create a new .qls file and add this to DCA?

koesie10 · 2023-09-19T14:24:16Z

Besides the comments in the code, it also worth considering, whether we need a DCA test suite for these queries as they are excluded from all existing test suites (in the selectors). When doing changes to the queries going forward, it is nice to have some tooling to verify that we don't break the results they produce and/or breaks their performance.

How would I go about doing that? Would I need to create a new .qls file and add this to DCA?

hvitved · 2023-09-20T07:01:35Z

csharp/ql/src/utils/modeleditor/ModelEditor.qll

+/** Provides classes and predicates related to handling APIs for the VS Code extension. */
+
+private import csharp
+private import dotnet


No need to import this (and no need to use DotNet::).

hvitved · 2023-09-20T07:03:09Z

csharp/ql/src/utils/modeleditor/ModelEditor.qll

+ * Otherwise the name of the nested type is prefixed with a `+` and appended to
+ * the name of the enclosing type, which might be a nested type as well.
+ */
+private string nestedName(Declaration declaration) {


Can be restricted to types instead of declarations in general

hvitved · 2023-09-20T07:08:06Z

csharp/ql/src/utils/modeleditor/ModelEditor.qll

+  }
+
+  /** Gets a node that is an output from a call to this API. */
+  private DataFlow::Node getAnOutput() {


This will include post-update nodes for arguments, e.g. for a call

Foo(x)

[post-update] x will be included, which is likely not what you want. Instead I suggest you use getAnOutNode from DataFlowDispatch.qll which will only include normal returns and out/ref returns.

hvitved · 2023-09-20T07:10:03Z

csharp/ql/src/utils/modeleditor/ModelEditor.qll

+  endpoint.isSupported() and result = true
+  or
+  not endpoint.isSupported() and
+  result = false


Suggested change

endpoint.isSupported() and result = true

or

not endpoint.isSupported() and

result = false

if endpoint.isSupported() then result = true else result = false

…rms of MaD.

michaelnebel · 2023-09-20T12:00:33Z

Besides the comments in the code, it also worth considering, whether we need a DCA test suite for these queries as they are excluded from all existing test suites (in the selectors). When doing changes to the queries going forward, it is nice to have some tooling to verify that we don't break the results they produce and/or breaks their performance.

How would I go about doing that? Would I need to create a new .qls file and add this to DCA?

Yes. I have added one here: https://github.com/github/codeql-dca/pull/1663
Also trying to run this suite against the PR now.

michaelnebel · 2023-09-20T12:27:54Z

DCA Experiment only on this branch: https://github.com/github/codeql-dca-main/issues/16281

michaelnebel · 2023-09-21T08:23:42Z

DCA looks good in terms of performance.
@koesie10 : For larger projects the queries can take more than 5 minutes to execute (at least on actions), but I would also expect significant waiting time locally for results:

michaelnebel · 2023-09-22T07:42:59Z

@hvitved : Do you have an objections on merging as is?

michaelnebel · 2023-09-22T08:49:37Z

Other comments:
(1) If this should be tested via DCA, we need to add the flag -X disable-sarif-files=true to the DCA run for the time being (due to a high number of results).
(2) @koesie10 : If you are planning to integrate this queries in VS Code (or somehow expose these to customers), I would recommend trying to execute them against large databases (maybe use MRVA for this).

csharp/ql/src/utils/modeleditor/ApplicationModeEndpointsQuery.qll

michaelnebel

This looks good to me.
Lets try an run DCA again (I will trigger one more execution).

michaelnebel · 2023-09-26T08:54:14Z

It looks like the model editor queries are now executed via DCA.
Please note that in the DCA experiment https://github.com/github/codeql-dca-main/issues/16420 Meta Queries actually are the Un-interpreted queries (this is merely a typo that will be fixed soon). Furthermore, the large discrepancy is the diagmetric and uninterpreted queries are causing by sharing cache. The execution against the feature branch executes the model editor queries before the diagmetric queries, which means that the dataflow part of the cache is created at this point in time.

I think the numbers look reasonable.

C#: Add VS Code model editor queries

585fb9d

github-actions bot added the C# label Sep 13, 2023

koesie10 marked this pull request as ready for review September 13, 2023 12:45

koesie10 requested a review from a team as a code owner September 13, 2023 12:45

koesie10 added the no-change-note-required This PR does not need a change note label Sep 13, 2023

michaelnebel self-requested a review September 18, 2023 06:39

michaelnebel reviewed Sep 18, 2023

View reviewed changes

koesie10 added 9 commits September 18, 2023 13:40

C#: Extract TestLibrary to separate module

0cc74a2

C#: Check accessor declaration for publicness

e524e35

C#: Switch from Declaration to Callable

ff2cef3

C#: Remove unnecessary isEffectivelyPublic predicate

8472b84

C#: Add tests for generic interfaces/classes/methods

f468b2a

C#: Rename CallableMethod to Endpoint

4693f72

C#: Rename AutomodelVsCode to ModelEditor

93972a4

C#: Only include unbound declarations in endpoints

81a8eee

C#: Update comment for Endpoint

948e36a

koesie10 requested a review from michaelnebel September 18, 2023 13:48

C#: Fix formatting of ExternalApi

489561f

michaelnebel reviewed Sep 19, 2023

View reviewed changes

koesie10 added 5 commits September 19, 2023 14:51

C#: Remove unnecessary isUnboundDeclaration predicates

dd79049

C#: Add tests for private methods and accessors

14a2b7f

C#: Add tests for supported framework methods

eace7a4

C#: Rename queries from fetch methods to endpoints

044fb9f

C#: Update query id/tags and documentation

3ebb9e1

koesie10 requested a review from michaelnebel September 19, 2023 14:24

hvitved reviewed Sep 20, 2023

View reviewed changes

michaelnebel added 3 commits September 20, 2023 13:01

C#: Identify whether callables in the source code are supported in te…

45432f2

…rms of MaD.

C#: Re-factor most of the logic out of the model editor query files.

50a9219

C#: Address review comments.

13dd9a6

michaelnebel requested a review from hvitved September 20, 2023 12:49

michaelnebel mentioned this pull request Sep 21, 2023

C#: Minor improvements to the ExternalApi implementation. #14272

Merged

C#: Remove unnecessary columns

0fea21f

hvitved requested changes Sep 25, 2023

View reviewed changes

csharp/ql/src/utils/modeleditor/ApplicationModeEndpointsQuery.qll Outdated Show resolved Hide resolved

koesie10 added 2 commits September 25, 2023 10:02

C#: Split API name column into separate columns

dc6def7

C#: Remove unnecessary import

922ff7b

koesie10 requested a review from hvitved September 25, 2023 09:23

michaelnebel approved these changes Sep 25, 2023

View reviewed changes

koesie10 mentioned this pull request Sep 26, 2023

Update model editor queries github/vscode-codeql#2868

Merged

3 tasks

michaelnebel requested review from hvitved and removed request for hvitved September 28, 2023 06:20

hvitved approved these changes Sep 28, 2023

View reviewed changes

koesie10 merged commit 0f4f987 into main Sep 28, 2023

koesie10 deleted the koesie10/add-csharp-model-editor-queries branch September 28, 2023 08:12

Conversation

koesie10 commented Sep 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelnebel commented Sep 18, 2023

Uh oh!

koesie10 commented Sep 18, 2023

Uh oh!

michaelnebel left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelnebel Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

koesie10 Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

michaelnebel Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Sep 19, 2023

Uh oh!

michaelnebel commented Sep 19, 2023

Uh oh!

koesie10 commented Sep 19, 2023

Uh oh!

koesie10 commented Sep 19, 2023

Uh oh!

hvitved Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

hvitved Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

hvitved Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

hvitved Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Sep 20, 2023

Uh oh!

michaelnebel commented Sep 20, 2023

Uh oh!

michaelnebel commented Sep 21, 2023

Uh oh!

michaelnebel commented Sep 22, 2023

Uh oh!

michaelnebel commented Sep 22, 2023

Uh oh!

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

michaelnebel left a comment •

edited

Loading

michaelnebel commented Sep 26, 2023 •

edited

Loading