csharp: odata lib by hugo-syn · Pull Request #22384 · github/codeql

hugo-syn · 2026-08-19T13:57:57Z

Add predicates and classes for the OData library https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnet.odata?view=odata-aspnetcore-7.0&viewFallbackFrom=odata-aspnetcore-8.0

Adds semmle.code.csharp.frameworks.OData, following the WCF.qll/JsonNET.qll convention: values cast, as-converted, or type-tested out of an untyped ODataActionParameters dictionary, and entities tracked by Delta<T> (via GetInstance/Patch/Put/CopyChangedValues/CopyUnchangedValues), have no static type relationship to the action method's own parameter types, so their members aren't picked up by the existing AspNetRemoteFlowSourceMember modeling. This adds a TaintedMember for those bound types (with the same nested-type/collection recursion as AspNetRemoteFlowSourceMember), plus two AdditionalTaintStep steps for the Delta<T> method calls, which don't fit the member-read shape TaintedMember covers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Match WCF.qll's convention: only the TaintedMember/AdditionalTaintStep wiring classes stay private, everything else that identifies a reusable OData domain concept (ODataActionParametersClass, DeltaClass, ODataBoundType, DeltaMutatingMethod, DeltaGetInstanceMethod) is public. Also renames the test fixtures to generic placeholder names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

import csharp already publicly imports semmle.code.csharp.dataflow.TaintTracking (and DataFlow), same as WCF.qll/JsonNET.qll rely on implicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

michaelnebel

Thank you very much! It is really good, if we can get our modelling extended even further!

I have added some initial comments / questions. Maybe OData parameter like types are only relevant for classes that extend ODataController. Should that somehow be incorporated in the logic?

michaelnebel · 2026-08-20T11:48:52Z

+private class DeltaGetInstanceTaintStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(MethodCall mc |
+      mc.getTarget().getUnboundDeclaration() instanceof DeltaGetInstanceMethod and
+      node1.asExpr() = mc.getQualifier() and
+      node2.asExpr() = mc
+    )
+  }
+}


Perhaps, the QL implementation can be replaced by Models as Data?
Below is the row for one of the GetInstance methods.

extensions: - addsTo: pack: codeql/csharp-all extensible: summaryModel data: - ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]

michaelnebel · 2026-08-20T11:51:48Z

+namespace Microsoft.AspNet.OData
+{
+    public class ODataActionParameters : Dictionary<string, object>
+    {
+    }
+
+    public class Delta<TStructuralType> where TStructuralType : class
+    {
+        private TStructuralType instance;
+
+        public Delta() { instance = default(TStructuralType); }
+
+        public TStructuralType GetInstance() => instance;
+
+        public void Patch(TStructuralType original) { }
+
+        public void Put(TStructuralType original) { }
+
+        public void CopyChangedValues(TStructuralType original) { }
+
+        public void CopyUnchangedValues(TStructuralType original) { }
+    }
+}


Ideally, we would like to keep stub implementations separate from the test and store them in test/resources/stubs.
This will require an options file for the test; If possible, it is also preferred, if the test relies fully on stubs and not any .dll files.

michaelnebel · 2026-08-20T12:00:37Z

+  TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)
+}
+
+/** The generic `Delta<TStructuralType>` change-tracking class, across OData library versions. */


Maybe refer to the unbound declaration with "Delta1" instead of Delta<TStructuralType> as the type parameter is named T for Microsoft.AspNetCore.OData.Deltas.Delta<T>

Per review feedback on github#22384, replace the hand-written DeltaGetInstanceMethod/DeltaGetInstanceTaintStep taint step with a Models-as-Data summaryModel row for both the Microsoft.AspNet.OData and Microsoft.AspNetCore.OData.Deltas variants of Delta<T>.GetInstance().

Per review feedback on github#22384, OData.qll's CandidateODataMember was an exact copy of CandidateMemberToTaint from Remote.qll. Make that class public and import it instead of duplicating it.

Per review feedback on github#22384, keep the ODataActionParameters/Delta<T> stub implementations out of the test .cs file and store them in test/resources/stubs instead, following the pattern used by other frameworks (e.g. JsonNET, Aws). The test now loads the stub project via an options file and relies on no .dll files.

Per review feedback on github#22384, the doc comment named the type parameter TStructuralType, but the AspNetCore variant of Delta<T> names it T. Refer to the unbound generic as \`Delta\`1\`\` instead.

hugo-syn · 2026-08-21T09:15:17Z

Hi @michaelnebel I think I've made changes for all your requests let me know if it's ok

I'm not sure to get you question:

Maybe OData parameter like types are only relevant for classes that extend ODataController. Should that somehow be incorporated in the logic?

Can you give more details / examples ?

jzabroski · 2026-08-24T22:44:29Z

Maybe OData parameter like types are only relevant for classes that extend ODataController.

I don't agree with this advice.

OData can work without inheriting from ODataController by using standard ASP.NET Core Controller or ApiController classes combined with the [EnableQuery] attribute or manual ODataQueryOptions parsing.

Therefore, the filter to speed up CodeQL database matches should have all 3 possibilities:

Extend ODataController; or
Extend Controller; or
Extend ApiController

For the latter two, either

[EnableQuery] attribute; or
ODataQueryOptions detected

github-actions · 2026-08-25T11:18:48Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

csharp

Generated file changes for csharp

Changes to framework-coverage-csharp.rst:

-    System,"``System.*``, ``System``",48,12495,59,5
+    System,"``System.*``, ``System``",48,12500,59,5
-    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Data.SqlClient``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``NHibernate``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",60,2406,162,4
+    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNet.OData``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.OData.Deltas``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Data.SqlClient``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``NHibernate``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",60,2419,162,4
-    Totals,,108,14908,415,9
+    Totals,,108,14926,415,9

Changes to framework-coverage-csharp.csv:

+ Microsoft.AspNet.OData,,,7,,,,,,,,,,,,,,,,,,,7,
+ Microsoft.AspNetCore.OData.Deltas,,,6,,,,,,,,,,,,,,,,,,,6,
- System,59,48,12495,,6,5,12,,,4,1,,31,2,,6,15,17,5,3,,6382,6113
+ System,59,48,12500,,6,5,12,,,4,1,,31,2,,6,15,17,5,3,,6387,6113

michaelnebel

I am sorry for the delay in review; Thank you for your diligence @hugo-syn.
Will also start a DCA run (automated testing against a set of repositories)

michaelnebel · 2026-08-25T11:39:43Z

@@ -0,0 +1,19 @@
+// This file contains auto-generated code.
+// Generated from `Microsoft.AspNet.OData, Version=7.7.5.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`.


Is this indeed auto-generated? Did you use the make_stubs_nuget.py to generate the file?

If it is not auto-generated, could you then move this file to csharp/ql/test/resources/stubs (and then remove comments about code being auto generated)?
If it is auto generated, then please leave it here (sorry about being a bit pushy about this - otherwise I will be really confused when trying to update all stubs later in the future) and then add the package to the list in make_stubs_all.py.

michaelnebel · 2026-08-25T11:51:44Z

+
+/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */
+private predicate isODataParameterValue(Expr e) {
+  TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)


Suggested change

TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)

DataFlow::localExprFlow(any(ODataActionParameterRead r), e)

Maybe we should consider using local data flow instead (and not only taint tracking), then it becomes a bit more strict, which types we consider to be ODataBound (and it appears that all test-cases pass). Or do you know of a real world example, where this wouldn't be good enough?

Copilot

Pull request overview

Adds OData action-parameter and Delta<T> taint tracking to the C# analysis libraries.

Changes:

Models OData-bound types, members, and mutating Delta operations.
Adds GetInstance flow summaries.
Adds classic OData test fixtures and release notes.

Show a summary per file

File	Description
`Microsoft.AspNet.OData.csproj`	Configures the test stub project.
`Microsoft.AspNet.OData.cs`	Provides generated OData API stubs.
`OData/options`	Loads the OData test stubs.
`OData/OData.ql`	Defines the taint test query.
`OData/OData.expected`	Records expected flows.
`OData/OData.cs`	Exercises dictionary and Delta flows.
`Remote.qll`	Exposes the reusable member candidate class.
`OData.qll`	Implements OData taint modeling.
`TaintTrackingPrivate.qll`	Registers the OData models.
`Microsoft.AspNet.OData.model.yml`	Models `GetInstance` return flow.
`2026-08-19-odata-taint-step.md`	Documents the feature.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Files reviewed: 11/11 changed files
Comments generated: 4
Review effort level: Balanced

+      this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or
+      this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1")


michaelnebel · 2026-08-25T12:31:07Z

+/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */
+private predicate isODataParameterValue(Expr e) {
+  TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)


This is used for constructing an approximation of a set of types where we want to taint the members - maybe the existing implementation suffices for most real world examples (I will leave it to you @hugo-syn , if you want to improve further - IMO this is not something that blocks the current PR)

I'm not sure so I've added it just in case

michaelnebel · 2026-08-25T12:27:44Z

+      pack: codeql/csharp-all
+      extensible: summaryModel
+    data:
+      - ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]


They return void.

I've added both cases

I believe that CoPilot is mistaken. Both Patch and CopyChangedValues return void - so we shouldn't add any models for ReturnValue (as these method doesn't return anything - they modify their input).

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

hugo-syn · 2026-08-25T14:12:48Z

Hey @michaelnebel I've taken into account comments from Copilot and your comments let me know if its better

hugo-syn · 2026-08-25T14:15:11Z

Also curious about the DCA, do you have a list of projects using OData ?

michaelnebel

I have added some further comments; I would prefer that we keep TryGetValue out of scope for this PR - it is getting a bit complicated for me to review in one go 😄

From the first DCA run it appears that there are no issues with performance or changes to alerts - this could easily be because none of these projects use ODataParameters (haven't checked).
Do you know of an open source project, where the changes in this PR will lead to changes in alerts? 😄

michaelnebel · 2026-08-26T07:14:24Z

+      pack: codeql/csharp-all
+      extensible: summaryModel
+    data:
+      - ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]


I believe that CoPilot is mistaken. Both Patch and CopyChangedValues return void - so we shouldn't add any models for ReturnValue (as these method doesn't return anything - they modify their input).

michaelnebel · 2026-08-26T07:28:24Z

        public Delta() => throw null;
        public TStructuralType GetInstance() => throw null;
-        public void Patch(TStructuralType original) => throw null;
+        public TStructuralType Patch(TStructuralType original) => throw null;


According to the documentation here Patch returns void (and the same goes for CopyChangedValues). I think your original stub implementation was correct.

michaelnebel · 2026-08-26T07:30:41Z

@@ -0,0 +1,17 @@
+namespace Microsoft.AspNet.OData


Perhaps move this file to the stubs folder and remove the project file.

michaelnebel · 2026-08-26T09:04:13Z

+/**
+ * A call to `TryGetValue` on an `ODataActionParameters` dictionary copies the value
+ * of the looked-up entry into the `out` argument.
+ */
+private class ODataActionParametersTryGetValueTaintStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(MethodCall mc, AssignableDefinitions::OutRefDefinition def |
+      mc.getTarget().hasName("TryGetValue") and
+      isODataActionParametersValue(mc.getQualifier()) and
+      node1.asExpr() = mc.getQualifier() and
+      def.getTargetAccess() = mc.getArgumentForName("value") and
+      node2 = DataFlow::assignableDefinitionNode(def)
+    )
+  }
 }


I am not sure that this works fully as intended - and it doesn't fully address CoPilots comment as it only adds an additional taint step. Note, that the testcase on 76-83 only works because some of the other testcases causes BoundEntity to have its members tainted.
Perhaps remove for now and consider making a follow up PR to also cover TryGetValue (if needed).

michaelnebel · 2026-08-26T09:09:52Z

@@ -1 +1 @@
 import csharp


Please convert the testcase to a path-problem testcase. It is/was getting a bit hard to understand "why" there is flow since the logic for taint tracking and taint members are a bit mixed and it can somewhat hard to understand whether a taint step is caused by a model or due to members being tainted.

- Revert Patch/CopyChangedValues to void-only per michaelnebel (defer to maintainer over docs citation despite conflicting reflection evidence). - Move Microsoft.AspNet.OData.cs stub to a flat file, drop its wrapper project. - Drop the TryGetValue AdditionalTaintStep: it only added a taint step and didn't make cast targets recognized as ODataBoundType, so it doesn't fully address the underlying gap; left for a follow-up PR. - Convert OData.ql to a path-problem query for clearer test output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

hugo-syn · 2026-08-26T15:36:07Z

Hello @michaelnebel , I'm also a bit lost, I've reverted some changes based on your comments I hope it will be better

hugo-syn and others added 3 commits August 19, 2026 15:30

C#: drop redundant TaintTracking import in OData.qll

3440d4a

import csharp already publicly imports semmle.code.csharp.dataflow.TaintTracking (and DataFlow), same as WCF.qll/JsonNET.qll rely on implicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

hugo-syn requested a review from a team as a code owner August 19, 2026 13:57

github-actions Bot added C# documentation labels Aug 19, 2026

michaelnebel reviewed Aug 20, 2026

View reviewed changes

hugo-syn added 4 commits August 21, 2026 09:51

C#: reuse CandidateMemberToTaint from Remote.qll in OData.qll

a44bfec

Per review feedback on github#22384, OData.qll's CandidateODataMember was an exact copy of CandidateMemberToTaint from Remote.qll. Make that class public and import it instead of duplicating it.

C#: fix DeltaClass doc comment to not name a specific type parameter

9c3a9b4

Per review feedback on github#22384, the doc comment named the type parameter TStructuralType, but the AspNetCore variant of Delta<T> names it T. Refer to the unbound generic as \`Delta\`1\`\` instead.

michaelnebel reviewed Aug 25, 2026

View reviewed changes

michaelnebel requested a balanced review from Copilot August 25, 2026 12:14

Copilot started reviewing on behalf of michaelnebel August 25, 2026 12:14 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

hugo-syn and others added 4 commits August 25, 2026 15:04

Update csharp/ql/lib/ext/Microsoft.AspNet.OData.model.yml

746b806

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

Update csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll

131b6be

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

Update csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll

de9eae2

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

fix: add fix from review

93baa40

michaelnebel reviewed Aug 26, 2026

View reviewed changes

		@@ -0,0 +1,19 @@
		// This file contains auto-generated code.
		// Generated from `Microsoft.AspNet.OData, Version=7.7.5.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`.

	TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)
	DataFlow::localExprFlow(any(ODataActionParameterRead r), e)

		this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or
		this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1")

Uh oh!

Conversation

hugo-syn commented Aug 19, 2026

Uh oh!

michaelnebel left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugo-syn commented Aug 21, 2026

Uh oh!

jzabroski commented Aug 24, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

csharp

Generated file changes for csharp

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel Aug 25, 2026 • 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

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!

Uh oh!

hugo-syn commented Aug 25, 2026

Uh oh!

hugo-syn commented Aug 25, 2026

Uh oh!

michaelnebel left a comment

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!

michaelnebel Aug 26, 2026 • 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!

hugo-syn commented Aug 26, 2026

Uh oh!

Reviewers

Assignees

github-actions Bot commented Aug 25, 2026 •

edited

Loading

michaelnebel Aug 25, 2026 •

edited

Loading

michaelnebel Aug 26, 2026 •

edited

Loading