C#: Models as Data Documentation #15563

egregius313 · 2024-02-09T02:29:49Z

Introduces Models-as-Data documentation for C#

Changes Java to use the language-agnostic beta notice
Introduces C#-specific models-as-data doc

Later follow-up will be to add the threat modeling documentation.

hvitved

Thanks a lot for writing this up. Some comments.

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

hvitved · 2024-02-09T10:12:30Z

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

+About data extensions
+---------------------
+
+You can customize analysis by defining models (summaries, sinks, and sources) of your code's C#/.NET dependencies in data extension files. Each model defines the behavior of one or more elements of your library or framework, such as methods, properties, and callables. When you run dataflow analysis, these models expand the potential sources and sinks tracked by dataflow analysis and improve the precision of results.


customize analysis -> customize analyses?

And dataflow analysis -> dataflow analyses.

Not sure which is better, but that wording is taken from the Java customizing-library-models docs.

I think analyses is probably better in both contexts, but I don't think the gain in clarity is enough to justify making edits here and in the Java docs, since "analysis" does also technically make sense (at least I think it does!), and it wouldn't just affect this paragraph.

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

hvitved · 2024-02-09T10:19:34Z

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

+      }
+   }
+
+We need to add a tuple to the ``sinkModel``\(namespace, type, subtypes, name, signature, ext, input, kind, provenance) extensible predicate by updating a data extension file.


I'm not familiar with .rst syntax, but the above looks wrong?

I think the purpose of that is so that only the word sinkModel is rendered as code and be highlighted. But if we think that it's fine to highlight the entirety of sinkModel(namespace, ..., I can change it over.

cf The Java MaD docs:

Ok, that looks weird to me.

But if we think that it's fine to highlight the entirety of sinkModel(namespace, ..., I can change it over.

I think this would probably would be best, but it would involve numerous edits to both this documentation and the Java documentation. If you have time to do this, it'd be great, but it's not urgent 👍

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

michaelnebel

@egregius313 : Really good examples!
It would be great, if we could also make something similar to extensible-predicates.rst which is currently java specific (and for some reason not linked from the java documentation).

egregius313 · 2024-02-13T20:40:00Z

It would be great, if we could also make something similar to extensible-predicates.rst which is currently java specific (and for some reason not linked from the java documentation).

@michaelnebel The existing extensible-predicates.rst mentions synthetic fields/globals, but does not explain them? Would fleshing out the documentation for them make sense as well?

michaelnebel · 2024-02-14T10:11:47Z

It would be great, if we could also make something similar to extensible-predicates.rst which is currently java specific (and for some reason not linked from the java documentation).

@michaelnebel The existing extensible-predicates.rst mentions synthetic fields/globals, but does not explain them? Would fleshing out the documentation for them make sense as well?

That is an excellent question!
I think we could do much more extensive documentation, if we want to. If you are up for providing examples for synthetics that would be great, but if I think that examples for covering Field and Property (only the latter is relevant for C#) would be more important.

That is, if you want to do more examples, then the order of "importance" is probably:

Field and Property (the latter only relevant for C#).
ArrayElement, MapKey, MapValue (these are only relevant for Java).
Attributes (the ext field in the MaD format can be used to capture summaries for callables based on their annotation (java) and attribute (C#).
SyntheticField.
SyntheticGlobal: Note that there are no C# examples, but I expect this to work, because the FlowSummaryImpl implementation is now shared - but this is untested for C# (at least from a MaD stand point - there are some synthetic globals but they are declared in QL.

You are more than welcome to add more examples - this would be highly appreciated (I do know this is scope creep, but it would be really valuable, if we had these examples), but not strictly required I think.

egregius313 · 2024-02-15T17:51:11Z

You are more than welcome to add more examples - this would be highly appreciated (I do know this is scope creep, but it would be really valuable, if we had these examples), but not strictly required I think.

@michaelnebel I am going to work on extensible-predicate, but in order to avoid scope-creep in this PR, I am going to do those updates in a separate PR. It will make it easier to make sure this is reviewed and ready to merge.

subatoi

Thank you! Only minor and super minor (purely docs) comments. This is very clear and matches the Java docs really well.

subatoi · 2024-02-16T14:57:15Z

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

+      }
+   }
+
+We need to add a tuple to the ``sinkModel``\(namespace, type, subtypes, name, signature, ext, input, kind, provenance) extensible predicate by updating a data extension file.


But if we think that it's fine to highlight the entirety of sinkModel(namespace, ..., I can change it over.

I think this would probably would be best, but it would involve numerous edits to both this documentation and the Java documentation. If you have time to do this, it'd be great, but it's not urgent 👍

subatoi · 2024-02-16T15:27:55Z

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

+About data extensions
+---------------------
+
+You can customize analysis by defining models (summaries, sinks, and sources) of your code's C#/.NET dependencies in data extension files. Each model defines the behavior of one or more elements of your library or framework, such as methods, properties, and callables. When you run dataflow analysis, these models expand the potential sources and sinks tracked by dataflow analysis and improve the precision of results.


I think analyses is probably better in both contexts, but I don't think the gain in clarity is enough to justify making edits here and in the Java docs, since "analysis" does also technically make sense (at least I think it does!), and it wouldn't just affect this paragraph.

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

docs/codeql/codeql-language-guides/customizing-library-models-for-java-and-kotlin.rst

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

docs/codeql/codeql-language-guides/codeql-for-csharp.rst

michaelnebel

Looks good to me!

Co-authored-by: Michael Nebel <michaelnebel@github.com>

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

michaelnebel · 2024-02-27T09:10:16Z

@subatoi @hvitved @jf205 : Are there any objections on merging the documentation?

subatoi · 2024-02-27T09:30:52Z

@michaelnebel not from me from a Docs point of view 👍

subatoi · 2024-02-27T14:17:07Z

@michaelnebel Sorry for the late thought: it occurred to us to check this is due to ship with CLI 2.16.3. If so, all good 👍

Edit: 2.16.4, apologies

michaelnebel · 2024-02-27T14:33:39Z

@michaelnebel Sorry for the late thought: it occurred to us to check this is due to ship with CLI 2.16.3. If so, all good 👍

Alright thank you!
In case it is not shipped, I suppose we can merge most of the content of the PR and just make orphan and not link it from the main documentation?

intrigus-lgtm · 2024-02-27T20:35:06Z

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst

+- The second value ``SqlCommand`` is the name of the class (type) that contains the method.
+- The third value ``False`` is a flag that indicates whether or not the sink also applies to all overrides of the method.
+- The fourth value ``SqlCommand`` is the method name. Constructors are named after the class.
+- The fifth value ``(System.String,System.Data.SqlClient.SqlConnection)`` is the method input type signature. The type names must be fully qualified.


The type names must be fully qualified.

More curiosity than an actual problem, but why does C# require fully qualified names, but Java doesn't?

docs/codeql/reusables/beta-note-model-packs-java.rst

egregius313 requested a review from a team February 9, 2024 02:29

github-actions bot added the documentation label Feb 9, 2024

egregius313 force-pushed the egregius313/csharp/docs/mad-docs branch 2 times, most recently from 4d31893 to 4e75726 Compare February 9, 2024 03:43

hvitved requested changes Feb 9, 2024

View reviewed changes

michaelnebel reviewed Feb 12, 2024

View reviewed changes

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 12, 2024

View reviewed changes

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 12, 2024

View reviewed changes

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 12, 2024

View reviewed changes

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst Show resolved Hide resolved

michaelnebel reviewed Feb 12, 2024

View reviewed changes

docs/codeql/codeql-language-guides/customizing-library-models-for-csharp.rst Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 12, 2024

View reviewed changes

egregius313 force-pushed the egregius313/csharp/docs/mad-docs branch from d95c5f8 to 18465d8 Compare February 13, 2024 17:08

egregius313 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Feb 15, 2024

subatoi self-assigned this Feb 16, 2024

subatoi previously approved these changes Feb 16, 2024

View reviewed changes

egregius313 dismissed subatoi’s stale review via be4c14d February 19, 2024 17:04

michaelnebel approved these changes Feb 20, 2024

View reviewed changes

egregius313 and others added 10 commits February 20, 2024 14:16

Modify Java threat model link to be Java-specific

54d7805

Modify Java docs to use common models-as-data beta notice

77ef63a

Introduce C# MaD documentation

698109a

Remove references to model editor

bb86ce5

Remove threat model mentions

65db990

Correct indices in example

3f10dd0

Oxford comma

ae59ea3

Review suggestions

6665248

Co-authored-by: Michael Nebel <michaelnebel@github.com>

Add note about neutrals to Java documentation

8058096

Add neutral model note to C# documentation

e2511cd

egregius313 and others added 2 commits February 20, 2024 14:17

Add note about collapsing multiple rows into one

8be9b8b

Docs team review suggestions

c5dbaa6

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

egregius313 force-pushed the egregius313/csharp/docs/mad-docs branch from be4c14d to c5dbaa6 Compare February 20, 2024 19:17

hvitved approved these changes Feb 27, 2024

View reviewed changes

egregius313 merged commit a743683 into github:main Feb 27, 2024

intrigus-lgtm reviewed Feb 27, 2024

View reviewed changes

felicitymay reviewed Feb 28, 2024

View reviewed changes

docs/codeql/reusables/beta-note-model-packs-java.rst Show resolved Hide resolved

C#: Models as Data Documentation #15563

C#: Models as Data Documentation #15563

Uh oh!

Conversation

egregius313 commented Feb 9, 2024

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

egregius313 commented Feb 13, 2024

Uh oh!

michaelnebel commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egregius313 commented Feb 15, 2024

Uh oh!

subatoi 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Feb 27, 2024

Uh oh!

subatoi commented Feb 27, 2024

Uh oh!

subatoi commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelnebel commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelnebel commented Feb 14, 2024 •

edited

Loading

subatoi left a comment •

edited

Loading

subatoi commented Feb 27, 2024 •

edited

Loading