Skip to content

Commit cf4c3d0

Browse files
hugo-synclaude
hugo-syn
and
claude
committed
C#: address further OData review comments
- 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>
1 parent 93baa40 commit cf4c3d0

8 files changed

Lines changed: 93 additions & 74 deletions

File tree

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,11 @@ extensions:
55
data:
66
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
77
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Patch", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
8-
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Patch", "(TStructuralType)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
98
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Put", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
109
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
11-
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
1210
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyUnchangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
1311
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
1412
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Patch", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
15-
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Patch", "(T)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
1613
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Put", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
1714
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "CopyChangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
1815
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "CopyUnchangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]

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

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
import csharp
2222
private import semmle.code.csharp.commons.Collections
23-
private import semmle.code.csharp.dataflow.FlowSteps
2423
private import semmle.code.csharp.security.dataflow.flowsources.Remote
2524

2625
/** The `ODataActionParameters` dictionary type, across OData library versions. */
@@ -50,22 +49,6 @@ class ODataActionParameterRead extends ElementAccess {
5049
ODataActionParameterRead() { isODataActionParametersValue(this.getQualifier()) }
5150
}
5251

53-
/**
54-
* A call to `TryGetValue` on an `ODataActionParameters` dictionary copies the value
55-
* of the looked-up entry into the `out` argument.
56-
*/
57-
private class ODataActionParametersTryGetValueTaintStep extends AdditionalTaintStep {
58-
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
59-
exists(MethodCall mc, AssignableDefinitions::OutRefDefinition def |
60-
mc.getTarget().hasName("TryGetValue") and
61-
isODataActionParametersValue(mc.getQualifier()) and
62-
node1.asExpr() = mc.getQualifier() and
63-
def.getTargetAccess() = mc.getArgumentForName("value") and
64-
node2 = DataFlow::assignableDefinitionNode(def)
65-
)
66-
}
67-
}
68-
6952
/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */
7053
private predicate isODataParameterValue(Expr e) {
7154
DataFlow::localExprFlow(any(ODataActionParameterRead r), e)

‎csharp/ql/test/library-tests/frameworks/OData/OData.cs‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,6 @@ void IsAsFromDictionary(ODataActionParameters parameters)
7373
}
7474
}
7575

76-
void TryGetValueFromDictionary(ODataActionParameters parameters)
77-
{
78-
if (parameters.TryGetValue("Entity", out var value))
79-
{
80-
var entity = (BoundEntity)value;
81-
Sink(entity.Name);
82-
}
83-
}
84-
8576
void UpcastThenIndex(ODataActionParameters parameters)
8677
{
8778
IDictionary<string, object> dict = parameters;
@@ -101,18 +92,6 @@ void DeltaGetInstance(Delta<Widget> delta)
10192
Sink(w.Name);
10293
}
10394

104-
void DeltaPatchReturnValue(Delta<Widget> delta, Widget original)
105-
{
106-
var updated = delta.Patch(original);
107-
Sink(updated.Name);
108-
}
109-
110-
void DeltaCopyChangedValuesReturnValue(Delta<Widget> delta, Widget original)
111-
{
112-
var updated = delta.CopyChangedValues(original);
113-
Sink(updated.Name);
114-
}
115-
11695
void LegacyDeltaPatch(System.Web.Http.OData.Delta<Widget> delta, Widget original)
11796
{
11897
delta.Patch(original);

‎csharp/ql/test/library-tests/frameworks/OData/OData.expected‎

Lines changed: 81 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,81 @@
1-
| OData.cs:46:55:46:64 | parameters | OData.cs:49:18:49:23 | access to local variable entity |
2-
| OData.cs:46:55:46:64 | parameters | OData.cs:50:18:50:28 | access to property Name |
3-
| OData.cs:46:55:46:64 | parameters | OData.cs:51:18:51:31 | access to property Content |
4-
| OData.cs:46:55:46:64 | parameters | OData.cs:52:18:52:38 | access to property Owner |
5-
| OData.cs:46:55:46:64 | parameters | OData.cs:55:22:55:28 | access to property Owner |
6-
| OData.cs:59:55:59:64 | parameters | OData.cs:65:26:65:35 | access to property Label |
7-
| OData.cs:59:55:59:64 | parameters | OData.cs:72:22:72:34 | access to property Category |
8-
| OData.cs:76:62:76:71 | parameters | OData.cs:81:22:81:32 | access to property Name |
9-
| OData.cs:85:52:85:61 | parameters | OData.cs:89:18:89:28 | access to property Name |
10-
| OData.cs:92:39:92:43 | delta | OData.cs:95:18:95:30 | access to property Name |
11-
| OData.cs:98:45:98:49 | delta | OData.cs:101:18:101:23 | access to property Name |
12-
| OData.cs:104:50:104:54 | delta | OData.cs:107:18:107:29 | access to property Name |
13-
| OData.cs:110:62:110:66 | delta | OData.cs:113:18:113:29 | access to property Name |
14-
| OData.cs:116:67:116:71 | delta | OData.cs:119:18:119:30 | access to property Name |
15-
| OData.cs:122:71:122:75 | delta | OData.cs:125:18:125:23 | access to property Name |
1+
edges
2+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:48:26:48:58 | (...) ... : BoundEntity | provenance | |
3+
| OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | OData.cs:49:18:49:23 | access to local variable entity | provenance | |
4+
| OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | OData.cs:50:18:50:28 | access to property Name | provenance | |
5+
| OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | OData.cs:51:18:51:31 | access to property Content | provenance | |
6+
| OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | OData.cs:52:18:52:38 | access to property Owner | provenance | |
7+
| OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | OData.cs:55:22:55:28 | access to property Owner | provenance | |
8+
| OData.cs:48:26:48:58 | (...) ... : BoundEntity | OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | provenance | |
9+
| OData.cs:59:55:59:64 | parameters : ODataActionParameters | OData.cs:65:26:65:35 | access to property Label | provenance | |
10+
| OData.cs:59:55:59:64 | parameters : ODataActionParameters | OData.cs:69:26:69:72 | ... as ... : IEnumerable<RelatedItem> | provenance | |
11+
| OData.cs:69:17:69:22 | access to local variable items2 : IEnumerable<RelatedItem> | OData.cs:72:22:72:34 | access to property Category | provenance | |
12+
| OData.cs:69:26:69:72 | ... as ... : IEnumerable<RelatedItem> | OData.cs:69:17:69:22 | access to local variable items2 : IEnumerable<RelatedItem> | provenance | |
13+
| OData.cs:76:52:76:61 | parameters : ODataActionParameters | OData.cs:78:41:78:44 | access to local variable dict : ODataActionParameters | provenance | |
14+
| OData.cs:78:41:78:44 | access to local variable dict : ODataActionParameters | OData.cs:79:26:79:52 | (...) ... : BoundEntity | provenance | |
15+
| OData.cs:79:17:79:22 | access to local variable entity : BoundEntity | OData.cs:80:18:80:28 | access to property Name | provenance | |
16+
| OData.cs:79:26:79:52 | (...) ... : BoundEntity | OData.cs:79:17:79:22 | access to local variable entity : BoundEntity | provenance | |
17+
| OData.cs:83:39:83:43 | delta : Delta<Widget> | OData.cs:85:13:85:17 | access to parameter delta : Delta<Widget> | provenance | |
18+
| OData.cs:85:13:85:17 | access to parameter delta : Delta<Widget> | OData.cs:85:25:85:32 | [post] access to parameter original : Widget | provenance | MaD:143 |
19+
| OData.cs:85:25:85:32 | [post] access to parameter original : Widget | OData.cs:86:18:86:30 | access to property Name | provenance | |
20+
| OData.cs:89:45:89:49 | delta : Delta<Widget> | OData.cs:91:21:91:25 | access to parameter delta : Delta<Widget> | provenance | |
21+
| OData.cs:91:17:91:17 | access to local variable w : Widget | OData.cs:92:18:92:23 | access to property Name | provenance | |
22+
| OData.cs:91:21:91:25 | access to parameter delta : Delta<Widget> | OData.cs:91:21:91:39 | call to method GetInstance : Widget | provenance | MaD:142 |
23+
| OData.cs:91:21:91:39 | call to method GetInstance : Widget | OData.cs:91:17:91:17 | access to local variable w : Widget | provenance | |
24+
| OData.cs:95:67:95:71 | delta : Delta<Widget> | OData.cs:97:13:97:17 | access to parameter delta : Delta<Widget> | provenance | |
25+
| OData.cs:97:13:97:17 | access to parameter delta : Delta<Widget> | OData.cs:97:25:97:32 | [post] access to parameter original : Widget | provenance | MaD:153 |
26+
| OData.cs:97:25:97:32 | [post] access to parameter original : Widget | OData.cs:98:18:98:30 | access to property Name | provenance | |
27+
| OData.cs:101:71:101:75 | delta : Delta<Widget> | OData.cs:103:21:103:25 | access to parameter delta : Delta<Widget> | provenance | |
28+
| OData.cs:103:17:103:17 | access to local variable w : Widget | OData.cs:104:18:104:23 | access to property Name | provenance | |
29+
| OData.cs:103:21:103:25 | access to parameter delta : Delta<Widget> | OData.cs:103:21:103:37 | call to method GetEntity : Widget | provenance | MaD:152 |
30+
| OData.cs:103:21:103:37 | call to method GetEntity : Widget | OData.cs:103:17:103:17 | access to local variable w : Widget | provenance | |
31+
nodes
32+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | semmle.label | parameters : ODataActionParameters |
33+
| OData.cs:48:17:48:22 | access to local variable entity : BoundEntity | semmle.label | access to local variable entity : BoundEntity |
34+
| OData.cs:48:26:48:58 | (...) ... : BoundEntity | semmle.label | (...) ... : BoundEntity |
35+
| OData.cs:49:18:49:23 | access to local variable entity | semmle.label | access to local variable entity |
36+
| OData.cs:50:18:50:28 | access to property Name | semmle.label | access to property Name |
37+
| OData.cs:51:18:51:31 | access to property Content | semmle.label | access to property Content |
38+
| OData.cs:52:18:52:38 | access to property Owner | semmle.label | access to property Owner |
39+
| OData.cs:55:22:55:28 | access to property Owner | semmle.label | access to property Owner |
40+
| OData.cs:59:55:59:64 | parameters : ODataActionParameters | semmle.label | parameters : ODataActionParameters |
41+
| OData.cs:65:26:65:35 | access to property Label | semmle.label | access to property Label |
42+
| OData.cs:69:17:69:22 | access to local variable items2 : IEnumerable<RelatedItem> | semmle.label | access to local variable items2 : IEnumerable<RelatedItem> |
43+
| OData.cs:69:26:69:72 | ... as ... : IEnumerable<RelatedItem> | semmle.label | ... as ... : IEnumerable<RelatedItem> |
44+
| OData.cs:72:22:72:34 | access to property Category | semmle.label | access to property Category |
45+
| OData.cs:76:52:76:61 | parameters : ODataActionParameters | semmle.label | parameters : ODataActionParameters |
46+
| OData.cs:78:41:78:44 | access to local variable dict : ODataActionParameters | semmle.label | access to local variable dict : ODataActionParameters |
47+
| OData.cs:79:17:79:22 | access to local variable entity : BoundEntity | semmle.label | access to local variable entity : BoundEntity |
48+
| OData.cs:79:26:79:52 | (...) ... : BoundEntity | semmle.label | (...) ... : BoundEntity |
49+
| OData.cs:80:18:80:28 | access to property Name | semmle.label | access to property Name |
50+
| OData.cs:83:39:83:43 | delta : Delta<Widget> | semmle.label | delta : Delta<Widget> |
51+
| OData.cs:85:13:85:17 | access to parameter delta : Delta<Widget> | semmle.label | access to parameter delta : Delta<Widget> |
52+
| OData.cs:85:25:85:32 | [post] access to parameter original : Widget | semmle.label | [post] access to parameter original : Widget |
53+
| OData.cs:86:18:86:30 | access to property Name | semmle.label | access to property Name |
54+
| OData.cs:89:45:89:49 | delta : Delta<Widget> | semmle.label | delta : Delta<Widget> |
55+
| OData.cs:91:17:91:17 | access to local variable w : Widget | semmle.label | access to local variable w : Widget |
56+
| OData.cs:91:21:91:25 | access to parameter delta : Delta<Widget> | semmle.label | access to parameter delta : Delta<Widget> |
57+
| OData.cs:91:21:91:39 | call to method GetInstance : Widget | semmle.label | call to method GetInstance : Widget |
58+
| OData.cs:92:18:92:23 | access to property Name | semmle.label | access to property Name |
59+
| OData.cs:95:67:95:71 | delta : Delta<Widget> | semmle.label | delta : Delta<Widget> |
60+
| OData.cs:97:13:97:17 | access to parameter delta : Delta<Widget> | semmle.label | access to parameter delta : Delta<Widget> |
61+
| OData.cs:97:25:97:32 | [post] access to parameter original : Widget | semmle.label | [post] access to parameter original : Widget |
62+
| OData.cs:98:18:98:30 | access to property Name | semmle.label | access to property Name |
63+
| OData.cs:101:71:101:75 | delta : Delta<Widget> | semmle.label | delta : Delta<Widget> |
64+
| OData.cs:103:17:103:17 | access to local variable w : Widget | semmle.label | access to local variable w : Widget |
65+
| OData.cs:103:21:103:25 | access to parameter delta : Delta<Widget> | semmle.label | access to parameter delta : Delta<Widget> |
66+
| OData.cs:103:21:103:37 | call to method GetEntity : Widget | semmle.label | call to method GetEntity : Widget |
67+
| OData.cs:104:18:104:23 | access to property Name | semmle.label | access to property Name |
68+
subpaths
69+
#select
70+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:49:18:49:23 | access to local variable entity | $@ | OData.cs:49:18:49:23 | access to local variable entity | access to local variable entity |
71+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:50:18:50:28 | access to property Name | $@ | OData.cs:50:18:50:28 | access to property Name | access to property Name |
72+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:51:18:51:31 | access to property Content | $@ | OData.cs:51:18:51:31 | access to property Content | access to property Content |
73+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:52:18:52:38 | access to property Owner | $@ | OData.cs:52:18:52:38 | access to property Owner | access to property Owner |
74+
| OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:46:55:46:64 | parameters : ODataActionParameters | OData.cs:55:22:55:28 | access to property Owner | $@ | OData.cs:55:22:55:28 | access to property Owner | access to property Owner |
75+
| OData.cs:59:55:59:64 | parameters : ODataActionParameters | OData.cs:59:55:59:64 | parameters : ODataActionParameters | OData.cs:65:26:65:35 | access to property Label | $@ | OData.cs:65:26:65:35 | access to property Label | access to property Label |
76+
| OData.cs:59:55:59:64 | parameters : ODataActionParameters | OData.cs:59:55:59:64 | parameters : ODataActionParameters | OData.cs:72:22:72:34 | access to property Category | $@ | OData.cs:72:22:72:34 | access to property Category | access to property Category |
77+
| OData.cs:76:52:76:61 | parameters : ODataActionParameters | OData.cs:76:52:76:61 | parameters : ODataActionParameters | OData.cs:80:18:80:28 | access to property Name | $@ | OData.cs:80:18:80:28 | access to property Name | access to property Name |
78+
| OData.cs:83:39:83:43 | delta : Delta<Widget> | OData.cs:83:39:83:43 | delta : Delta<Widget> | OData.cs:86:18:86:30 | access to property Name | $@ | OData.cs:86:18:86:30 | access to property Name | access to property Name |
79+
| OData.cs:89:45:89:49 | delta : Delta<Widget> | OData.cs:89:45:89:49 | delta : Delta<Widget> | OData.cs:92:18:92:23 | access to property Name | $@ | OData.cs:92:18:92:23 | access to property Name | access to property Name |
80+
| OData.cs:95:67:95:71 | delta : Delta<Widget> | OData.cs:95:67:95:71 | delta : Delta<Widget> | OData.cs:98:18:98:30 | access to property Name | $@ | OData.cs:98:18:98:30 | access to property Name | access to property Name |
81+
| OData.cs:101:71:101:75 | delta : Delta<Widget> | OData.cs:101:71:101:75 | delta : Delta<Widget> | OData.cs:104:18:104:23 | access to property Name | $@ | OData.cs:104:18:104:23 | access to property Name | access to property Name |

‎csharp/ql/test/library-tests/frameworks/OData/OData.ql‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
1+
/**
2+
* @kind path-problem
3+
*/
4+
15
import csharp
6+
import Taint::PathGraph
27

38
module TaintConfig implements DataFlow::ConfigSig {
49
predicate isSource(DataFlow::Node n) {
@@ -18,6 +23,6 @@ module TaintConfig implements DataFlow::ConfigSig {
1823

1924
module Taint = TaintTracking::Global<TaintConfig>;
2025

21-
from DataFlow::Node source, DataFlow::Node sink
22-
where Taint::flow(source, sink)
23-
select source, sink
26+
from Taint::PathNode source, Taint::PathNode sink
27+
where Taint::flowPath(source, sink)
28+
select source, source, sink, "$@", sink, sink.toString()

‎csharp/ql/test/library-tests/frameworks/OData/options‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
semmle-extractor-options: /nostdlib /noconfig
2-
semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj
2+
semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj
3+
semmle-extractor-options: ${testdir}/../../../resources/stubs/Microsoft.AspNet.OData.cs
34
semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.Http.OData.cs

csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.cs renamed to csharp/ql/test/resources/stubs/Microsoft.AspNet.OData.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ public class Delta<TStructuralType> where TStructuralType : class
99
{
1010
public Delta() => throw null;
1111
public TStructuralType GetInstance() => throw null;
12-
public TStructuralType Patch(TStructuralType original) => throw null;
12+
public void Patch(TStructuralType original) => throw null;
1313
public void Put(TStructuralType original) => throw null;
14-
public TStructuralType CopyChangedValues(TStructuralType original) => throw null;
14+
public void CopyChangedValues(TStructuralType original) => throw null;
1515
public void CopyUnchangedValues(TStructuralType original) => throw null;
1616
}
1717
}

‎csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj‎

Lines changed: 0 additions & 12 deletions
This file was deleted.

0 commit comments

Comments
 (0)