csharp: odata lib - #22384
Conversation
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
left a comment
There was a problem hiding this comment.
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?
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.
|
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:
Can you give more details / examples ? |
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:
For the latter two, either
|
Click to show differences in coveragecsharpGenerated file changes for csharp
- 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,2416,162,4
- Totals,,108,14908,415,9
+ Totals,,108,14923,415,9
+ Microsoft.AspNet.OData,,,5,,,,,,,,,,,,,,,,,,,5,
+ Microsoft.AspNetCore.OData.Deltas,,,5,,,,,,,,,,,,,,,,,,,5,
- 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
left a comment
There was a problem hiding this comment.
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)
| @@ -0,0 +1,19 @@ | |||
| // This file contains auto-generated code. | |||
| // Generated from `Microsoft.AspNet.OData, Version=7.7.5.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`. | |||
There was a problem hiding this comment.
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.
|
|
||
| /** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */ | ||
| private predicate isODataParameterValue(Expr e) { | ||
| TaintTracking::localExprTaint(any(ODataActionParameterRead r), e) |
There was a problem hiding this comment.
| 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?
There was a problem hiding this comment.
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
GetInstanceflow 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") |
Co-authored-by: Michael Nebel <michaelnebel@github.com>
Co-authored-by: Michael Nebel <michaelnebel@github.com>
Co-authored-by: Michael Nebel <michaelnebel@github.com>
|
Hey @michaelnebel I've taken into account comments from Copilot and your comments let me know if its better |
|
Also curious about the DCA, do you have a list of projects using OData ? |
michaelnebel
left a comment
There was a problem hiding this comment.
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? 😄
- 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>
|
Hello @michaelnebel , I'm also a bit lost, I've reverted some changes based on your comments I hope it will be better |
Excellent! Thank you! |
I find this discussion about dataflow vs additional taint bit abstract, so I thought I'd chime in again and drive direct business value through actionable insights CodeQL could provide. RaythaCMS has an active vulnerability in their ODATA filtering. I just asked Google Gemini via Google Search AI Mode, "odata cve". Gemini responded with:
I cherry-picked the fourth issue - it turns out Raytha CMS is open source and has done done a release since December 2025, so the June 2026 CVE is currently valid ☠️ . I am guessing the actual vulnerability here is in https://github.com/RaythaHQ/raytha/blob/main/src/Raytha.Infrastructure/JsonQueryEngine/Postgres/ODataFilterToPostgres.cs#L77 |
… update test expected output.
… each other in relation to identifying which members will be tainted).
|
@jzabroski : Thank you for the information! It appears that there are no expressions in @hugo-syn : Thank you for updating the PR. I checked approximately 1k open source repositories for expressions of type |
In this case, RaythaCMS uses It seems like CodeQL is generally weak at detecting SQL Injection c.f. #16353 (for example, compared to GLM-5.2, which can plainly see the problem the way a human would) What is not clear to me is if by adding explicit OData support, e.g. for ODataQueryOptionsParser, suddenly the CWE-089 would flag via existing rules. From how I understand CodeQL, these rules do not "compose up" across the database. |
|
@jzabroski : The quality/coverage of a query can be different from language to language. Most of the security related queries (for instance the SQL injection) are using the CodeQL data flow library to detect paths from a set of sources to a set of sinks. |
|
Asked Claude Fable 5:
|
Got it. So, while GLM-5.2 could see this, perhaps the right way to approach this is to e-mail Kyle Daigle and Mario Rogriguez and suggest a parsimonious use of Microsoft AI Research resources to test/prove out MAI models for cybersecurity threat modeling. The general sketch of the prompt would be to harden SQL Injection CWE-089 by using package analytics across major ecosystems add add taint support for major data libraries. I predict the total budget for such robustness additions would be <$50,000 based on my personal experiences doing large scale refactorings for <$2,000. |
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