Skip to content

Commit 867d483

Browse files
StefHStefH
authored
By default the RestrictOrderByToPropertyOrField is now set to true in the ParsingConfig (#884)
* By default the RestrictOrderByToPropertyOrField is now set to true in the ParsingConfig * . * fix * x * if
1 parent bd4b8c7 commit 867d483

9 files changed

+89
-35
lines changed

‎README.md

+7-1
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,15 @@ If it's not possible to add that attribute, you need to implement a custom [Cust
5050
Or provide a list of addtional types in the [DefaultDynamicLinqCustomTypeProvider.cs](https://github.com/zzzprojects/System.Linq.Dynamic.Core/blob/master/src/System.Linq.Dynamic.Core/CustomTypeProviders/DefaultDynamicLinqCustomTypeProvider.cs).
5151

5252
### v1.6.0-preview-01, 02, 03
53-
A breaking change is introduced in this version to solve CVE-2024-51417.
53+
#### Change 1
5454
It's not allowed anymore to call any methods on the `object` type. By default also the `ToString` and `Equals` methods are not allowed.
5555
To allow these methods set `AllowEqualsAndToStringMethodsOnObject` to `true` in the `ParsingConfig` and provide that config to all dynamic calls.
56+
This is done to mitigate the risk of calling methods on the `object` type which could lead to security issues (CVE-2024-51417).
57+
58+
#### Change 2
59+
By default the `RestrictOrderByToPropertyOrField` is now set to `true` in the `ParsingConfig`.
60+
Which means that only properties and fields can be used in the `OrderBy` / `ThenBy`.
61+
This is done to mitigate the risk of calling methods or other expressions in the `OrderBy` / `ThenBy` which could lead to security issues.
5662

5763
---
5864

‎src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs

+3-1
Original file line numberDiff line numberDiff line change
@@ -996,7 +996,9 @@ private Expression ParseIdentifier()
996996

997997
if (isValid && shouldPrioritizeType)
998998
{
999-
var keywordOrFunctionAllowed = !_usedForOrderBy || _usedForOrderBy && !_parsingConfig.RestrictOrderByToPropertyOrField;
999+
var keywordOrFunctionAllowed =
1000+
!_usedForOrderBy ||
1001+
(_usedForOrderBy && (_keywordsHelper.IsItOrRootOrParent(keywordOrType) || !_parsingConfig.RestrictOrderByToPropertyOrField));
10001002
if (!keywordOrFunctionAllowed)
10011003
{
10021004
throw ParseError(Res.UnknownPropertyOrField, _textParser.CurrentToken.Text, TypeHelper.GetTypeName(_it?.Type));

‎src/System.Linq.Dynamic.Core/Parser/IKeywordsHelper.cs

+2
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,7 @@ namespace System.Linq.Dynamic.Core.Parser;
55

66
internal interface IKeywordsHelper
77
{
8+
bool IsItOrRootOrParent(AnyOf<string, Expression, Type> value);
9+
810
bool TryGetValue(string text, out AnyOf<string, Expression, Type> value);
911
}

‎src/System.Linq.Dynamic.Core/Parser/KeywordsHelper.cs

+10
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,16 @@ public KeywordsHelper(ParsingConfig config)
8484
}
8585
}
8686

87+
public bool IsItOrRootOrParent(AnyOf<string, Expression, Type> value)
88+
{
89+
if (value.IsFirst)
90+
{
91+
return value.First is KEYWORD_IT or KEYWORD_ROOT or KEYWORD_PARENT or SYMBOL_IT or SYMBOL_PARENT or SYMBOL_ROOT;
92+
}
93+
94+
return false;
95+
}
96+
8797
public bool TryGetValue(string text, out AnyOf<string, Expression, Type> value)
8898
{
8999
// 1. Try to get as constant-expression, keyword, symbol or functions

‎src/System.Linq.Dynamic.Core/ParsingConfig.cs

+3-3
Original file line numberDiff line numberDiff line change
@@ -307,11 +307,11 @@ public IQueryableAnalyzer QueryableAnalyzer
307307
public StringLiteralParsingType StringLiteralParsing { get; set; } = StringLiteralParsingType.Default;
308308

309309
/// <summary>
310-
/// When set to <c>true</c>, the parser will restrict the OrderBy method to only allow properties or fields.
310+
/// When set to <c>true</c>, the parser will restrict the OrderBy and ThenBy methods to only allow properties or fields. If set to <c>false</c>, any expression is allowed.
311311
///
312-
/// Default value is <c>false</c>.
312+
/// Default value is <c>true</c>.
313313
/// </summary>
314-
public bool RestrictOrderByToPropertyOrField { get; set; }
314+
public bool RestrictOrderByToPropertyOrField { get; set; } = true;
315315

316316
/// <summary>
317317
/// When set to <c>true</c>, the parser will allow the use of the Equals(object obj), Equals(object objA, object objB), ReferenceEquals(object objA, object objB) and ToString() methods on the <see cref="object"/> type.

‎test/System.Linq.Dynamic.Core.Tests/EntitiesTests.OrderBy.cs

+24-21
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Linq.Dynamic.Core.Exceptions;
2+
using System.Linq.Dynamic.Core.Parser;
23
using System.Linq.Dynamic.Core.Tests.Helpers.Entities;
34
using FluentAssertions;
45
using Xunit;
@@ -7,57 +8,58 @@ namespace System.Linq.Dynamic.Core.Tests;
78

89
public partial class EntitiesTests
910
{
10-
[Fact]
11-
public void Entities_OrderBy_RestrictOrderByIsFalse()
11+
[Theory]
12+
[InlineData("IIF(1 == 1, 1, 0)")]
13+
[InlineData("np(Name, \"x\")")]
14+
public void Entities_OrderBy_RestrictOrderByIsFalse_ShouldAllowAnyExpression(string expression)
1215
{
16+
// Arrange
17+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
18+
1319
// Act
14-
var resultBlogs = _context.Blogs.OrderBy(b => true).ToArray();
15-
var dynamicResultBlogs = _context.Blogs.OrderBy("IIF(1 == 1, 1, 0)").ToDynamicArray<Blog>();
20+
Action action = () => _ = _context.Blogs.OrderBy(config, expression).ToDynamicArray<Blog>();
1621

1722
// Assert
18-
Assert.Equal(resultBlogs, dynamicResultBlogs);
23+
action.Should().NotThrow();
1924
}
2025

2126
[Fact]
22-
public void Entities_OrderBy_RestrictOrderByIsTrue_ValidExpressionShouldNotThrow()
27+
public void Entities_OrderBy_RestrictOrderByIsTrue_NonRestrictedExpressionShouldNotThrow()
2328
{
24-
// Arrange
25-
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = true };
26-
2729
// Act 1
2830
var resultBlogs = _context.Blogs.OrderBy(b => b.Name).ToArray();
29-
var dynamicResultBlogs = _context.Blogs.OrderBy(config, "Name").ToDynamicArray<Blog>();
31+
var dynamicResultBlogs = _context.Blogs.OrderBy("Name").ToDynamicArray<Blog>();
3032

3133
// Assert 1
3234
Assert.Equal(resultBlogs, dynamicResultBlogs);
3335

3436
// Act 2
3537
var resultPosts = _context.Posts.OrderBy(p => p.Blog.Name).ToArray();
36-
var dynamicResultPosts = _context.Posts.OrderBy(config, "Blog.Name").ToDynamicArray<Post>();
38+
var dynamicResultPosts = _context.Posts.OrderBy("Blog.Name").ToDynamicArray<Post>();
3739

3840
// Assert 2
3941
Assert.Equal(resultPosts, dynamicResultPosts);
4042
}
41-
42-
[Fact]
43-
public void Entities_OrderBy_RestrictOrderByIsTrue_InvalidExpressionShouldThrow()
43+
44+
[Theory]
45+
[InlineData("IIF(1 == 1, 1, 0)")]
46+
[InlineData("np(Name, \"x\")")]
47+
public void Entities_OrderBy_RestrictOrderByIsTrue_RestrictedExpressionShouldThrow(string expression)
4448
{
45-
// Arrange
46-
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = true };
47-
4849
// Act
49-
Action action = () => _context.Blogs.OrderBy(config, "IIF(1 == 1, 1, 0)");
50+
Action action = () => _context.Blogs.OrderBy(expression);
5051

5152
// Assert
52-
action.Should().Throw<ParseException>().WithMessage("No property or field 'IIF' exists in type 'Blog'");
53+
action.Should().Throw<ParseException>().WithMessage("No property or field '*' exists in type 'Blog'");
5354
}
5455

5556
[Fact]
5657
public void Entities_OrderBy_NullPropagation_Int()
5758
{
5859
// Arrange
60+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
5961
var resultBlogs = _context.Blogs.OrderBy(b => b.NullableInt ?? -1).ToArray();
60-
var dynamicResultBlogs = _context.Blogs.OrderBy("np(NullableInt, -1)").ToArray();
62+
var dynamicResultBlogs = _context.Blogs.OrderBy(config, "np(NullableInt, -1)").ToArray();
6163

6264
// Assert
6365
Assert.Equal(resultBlogs, dynamicResultBlogs);
@@ -67,8 +69,9 @@ public void Entities_OrderBy_NullPropagation_Int()
6769
public void Entities_OrderBy_NullPropagation_String()
6870
{
6971
// Arrange
72+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
7073
var resultBlogs = _context.Blogs.OrderBy(b => b.X ?? "_").ToArray();
71-
var dynamicResultBlogs = _context.Blogs.OrderBy("np(X, \"_\")").ToArray();
74+
var dynamicResultBlogs = _context.Blogs.OrderBy(config, "np(X, \"_\")").ToArray();
7275

7376
// Assert
7477
Assert.Equal(resultBlogs, dynamicResultBlogs);

‎test/System.Linq.Dynamic.Core.Tests/ExpressionTests.cs

+9-3
Original file line numberDiff line numberDiff line change
@@ -1384,6 +1384,8 @@ public void ExpressionTests_IsNull_ThrowsException()
13841384
[Fact]
13851385
public void ExpressionTests_Indexer_Issue57()
13861386
{
1387+
// Arrange
1388+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = true };
13871389
var rows = new List<JObject>
13881390
{
13891391
new JObject {["Column1"] = "B", ["Column2"] = 1},
@@ -1392,9 +1394,11 @@ public void ExpressionTests_Indexer_Issue57()
13921394
new JObject {["Column1"] = "A", ["Column2"] = 2}
13931395
};
13941396

1397+
// Act
13951398
var expected = rows.OrderBy(x => x["Column1"]).ToList();
1396-
var result = rows.AsQueryable().OrderBy(@"it[""Column1""]").ToList();
1399+
var result = rows.AsQueryable().OrderBy(config, @"it[""Column1""]").ToList();
13971400

1401+
// Assert
13981402
Assert.Equal(expected, result);
13991403
}
14001404

@@ -1727,14 +1731,15 @@ public void ExpressionTests_NullPropagation_Method_WithDefaultValue()
17271731
public void ExpressionTests_NullPropagating_DateTime()
17281732
{
17291733
// Arrange
1734+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
17301735
var q = new[]
17311736
{
17321737
new { id = 1, date1 = (DateTime?) DateTime.Now, date2 = DateTime.Now.AddDays(-1) }
17331738
}.AsQueryable();
17341739

17351740
// Act
17361741
var result = q.OrderBy(x => x.date2).Select(x => x.id).ToArray();
1737-
var resultDynamic = q.OrderBy("np(date1)").Select("id").ToDynamicArray<int>();
1742+
var resultDynamic = q.OrderBy(config, "np(date1)").Select("id").ToDynamicArray<int>();
17381743

17391744
// Assert
17401745
Check.That(resultDynamic).ContainsExactly(result);
@@ -1744,14 +1749,15 @@ public void ExpressionTests_NullPropagating_DateTime()
17441749
public void ExpressionTests_NullPropagation_NullableDateTime()
17451750
{
17461751
// Arrange
1752+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
17471753
var q = new[]
17481754
{
17491755
new { id = 1, date1 = (DateTime?) DateTime.Now, date2 = DateTime.Now.AddDays(-1)}
17501756
}.AsQueryable();
17511757

17521758
// Act
17531759
var result = q.OrderBy(x => x.date1.Value.Year).Select(x => x.id).ToArray();
1754-
var resultDynamic = q.OrderBy("np(date1.Value.Year)").Select("id").ToDynamicArray<int>();
1760+
var resultDynamic = q.OrderBy(config, "np(date1.Value.Year)").Select("id").ToDynamicArray<int>();
17551761

17561762
// Assert
17571763
Check.That(resultDynamic).ContainsExactly(result);

‎test/System.Linq.Dynamic.Core.Tests/QueryableTests.OrderBy.cs

+30-4
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
using System.Collections;
22
using System.Collections.Generic;
33
using System.Linq.Dynamic.Core.Exceptions;
4+
using System.Linq.Dynamic.Core.Parser;
45
using System.Linq.Dynamic.Core.Tests.Helpers.Models;
6+
using FluentAssertions;
57
using Xunit;
68

79
namespace System.Linq.Dynamic.Core.Tests;
@@ -36,12 +38,13 @@ public int Compare(object? x, object? y)
3638
public void OrderBy_Dynamic_NullPropagation_Int()
3739
{
3840
// Arrange
41+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
3942
var testList = User.GenerateSampleModels(2);
4043
var qry = testList.AsQueryable();
4144

4245
// Act
4346
var orderBy = testList.OrderBy(x => x.NullableInt ?? -1).ToArray();
44-
var orderByDynamic = qry.OrderBy("np(NullableInt, -1)").ToArray();
47+
var orderByDynamic = qry.OrderBy(config, "np(NullableInt, -1)").ToArray();
4548

4649
// Assert
4750
Assert.Equal(orderBy, orderByDynamic);
@@ -51,12 +54,13 @@ public void OrderBy_Dynamic_NullPropagation_Int()
5154
public void OrderBy_Dynamic_NullPropagation_String()
5255
{
5356
// Arrange
57+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
5458
var testList = User.GenerateSampleModels(2);
5559
var qry = testList.AsQueryable();
5660

5761
// Act
5862
var orderBy = testList.OrderBy(x => x.NullableString ?? "_").ToArray();
59-
var orderByDynamic = qry.OrderBy("np(NullableString, \"_\")").ToArray();
63+
var orderByDynamic = qry.OrderBy(config, "np(NullableString, \"_\")").ToArray();
6064

6165
// Assert
6266
Assert.Equal(orderBy, orderByDynamic);
@@ -66,12 +70,13 @@ public void OrderBy_Dynamic_NullPropagation_String()
6670
public void OrderBy_Dynamic_NullPropagation_NestedObject()
6771
{
6872
// Arrange
73+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
6974
var testList = User.GenerateSampleModels(2);
7075
var qry = testList.AsQueryable();
7176

7277
// Act
7378
var orderBy = testList.OrderBy(x => x.Profile?.Age ?? -1).ToArray();
74-
var orderByDynamic = qry.OrderBy("np(Profile.Age, -1)").ToArray();
79+
var orderByDynamic = qry.OrderBy(config, "np(Profile.Age, -1)").ToArray();
7580

7681
// Assert
7782
Assert.Equal(orderBy, orderByDynamic);
@@ -81,6 +86,7 @@ public void OrderBy_Dynamic_NullPropagation_NestedObject()
8186
public void OrderBy_Dynamic_NullPropagation_NestedObject_Query()
8287
{
8388
// Arrange
89+
var config = new ParsingConfig { RestrictOrderByToPropertyOrField = false };
8490
var qry = User.GenerateSampleModels(2)
8591
.Select(u => new
8692
{
@@ -93,7 +99,7 @@ public void OrderBy_Dynamic_NullPropagation_NestedObject_Query()
9399
.AsQueryable();
94100

95101
// Act
96-
var orderByDynamic = qry.OrderBy("np(X.Age, -1)").ToArray();
102+
var orderByDynamic = qry.OrderBy(config, "np(X.Age, -1)").ToArray();
97103

98104
// Assert
99105
Assert.NotNull(orderByDynamic);
@@ -238,4 +244,24 @@ public void OrderBy_Dynamic_Exceptions()
238244
Assert.Throws<ArgumentException>(() => qry.OrderBy(""));
239245
Assert.Throws<ArgumentException>(() => qry.OrderBy(" "));
240246
}
247+
248+
[Theory]
249+
[InlineData(KeywordsHelper.KEYWORD_IT)]
250+
[InlineData(KeywordsHelper.SYMBOL_IT)]
251+
[InlineData(KeywordsHelper.KEYWORD_ROOT)]
252+
[InlineData(KeywordsHelper.SYMBOL_ROOT)]
253+
[InlineData("\"User\" + \"Name\"")]
254+
[InlineData("\"User\" + \"Name\" asc")]
255+
[InlineData("\"User\" + \"Name\" desc")]
256+
public void OrderBy_RestrictOrderByIsTrue_NonRestrictedExpressionShouldNotThrow(string expression)
257+
{
258+
// Arrange
259+
var queryable = User.GenerateSampleModels(3).AsQueryable();
260+
261+
// Act
262+
Action action = () => _ = queryable.OrderBy(expression);
263+
264+
// Assert 2
265+
action.Should().NotThrow();
266+
}
241267
}

‎test/System.Linq.Dynamic.Core.Tests/QueryableTests.OrderByDescending.cs

+1-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
using System.Linq.Dynamic.Core.Exceptions;
2-
using System.Linq.Dynamic.Core.Tests.Helpers.Models;
1+
using System.Linq.Dynamic.Core.Tests.Helpers.Models;
32
using Xunit;
43

54
namespace System.Linq.Dynamic.Core.Tests

0 commit comments

Comments
 (0)