Skip to content

Commit 6c5734e

Browse files
skofman1skofman1
authored
When V1 Odata requests OrderBy is invalid, return 400 (#8144)
* When V1 Odata requests OrderBy is invalid, return 400 * When V2 Odata requests OrderBy is invalid, return 400
1 parent 4c77f91 commit 6c5734e

6 files changed

Lines changed: 99 additions & 46 deletions

File tree

‎src/NuGetGallery/Controllers/ODataV1FeedController.cs‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
using System.Web.Http;
1010
using System.Web.Http.OData;
1111
using System.Web.Http.OData.Query;
12+
using Microsoft.Data.OData;
1213
using NuGet.Services.Entities;
1314
using NuGetGallery.Configuration;
1415
using NuGetGallery.OData;
@@ -58,11 +59,19 @@ public IHttpActionResult Get(ODataQueryOptions<V1FeedPackage> options)
5859
{
5960
return BadRequest(ODataQueryVerifier.GetValidationFailedMessage(options));
6061
}
62+
63+
bool result = TryShouldIgnoreOrderById(options, out var shouldIgnoreOrderById);
64+
65+
if (!result)
66+
{
67+
return BadRequest("Invalid OrderBy parameter");
68+
}
69+
6170
var queryable = GetAll()
6271
.Where(p => !p.IsPrerelease && p.PackageStatusKey == PackageStatus.Available)
6372
.Where(SemVerLevelKey.IsUnknownPredicate())
6473
.WithoutSortOnColumn(Version)
65-
.WithoutSortOnColumn(Id, ShouldIgnoreOrderById(options))
74+
.WithoutSortOnColumn(Id, shouldIgnoreOrderById)
6675
.ToV1FeedPackageQuery(_configurationService.GetSiteRoot(UseHttps()));
6776

6877
return TrackedQueryResult(options, queryable, MaxPageSize, customQuery: true);

‎src/NuGetGallery/Controllers/ODataV2FeedController.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,19 @@ public async Task<IHttpActionResult> Get(
6161
ODataQueryOptions<V2FeedPackage> options,
6262
[FromUri]string semVerLevel = null)
6363
{
64+
bool result = TryShouldIgnoreOrderById(options, out var shouldIgnoreOrderById);
65+
66+
if (!result)
67+
{
68+
return BadRequest("Invalid OrderBy parameter");
69+
}
70+
6471
// Setup the search
6572
var packages = GetAll()
6673
.Where(p => p.PackageStatusKey == PackageStatus.Available)
6774
.Where(SemVerLevelKey.IsPackageCompliantWithSemVerLevelPredicate(semVerLevel))
6875
.WithoutSortOnColumn(Version)
69-
.WithoutSortOnColumn(Id, ShouldIgnoreOrderById(options))
76+
.WithoutSortOnColumn(Id, shouldIgnoreOrderById)
7077
.InterceptWith(new NormalizeVersionInterceptor());
7178

7279
var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel);

‎src/NuGetGallery/OData/NuGetODataController.cs‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using System.Web.Http;
88
using System.Web.Http.OData;
99
using System.Web.Http.OData.Query;
10+
using Microsoft.Data.OData;
1011
using NuGetGallery.Configuration;
1112
using NuGetGallery.OData.QueryFilter;
1213
using NuGetGallery.WebApi;
@@ -110,14 +111,25 @@ protected virtual IHttpActionResult TrackedQueryResult<TModel>(
110111
/// </summary>
111112
/// <param name="options">The request OData options.</param>
112113
/// <returns></returns>
113-
protected bool ShouldIgnoreOrderById<T>(ODataQueryOptions<T> options)
114+
protected bool TryShouldIgnoreOrderById<T>(ODataQueryOptions<T> options, out bool shouldIgnoreOrderById)
114115
{
115-
return options.OrderBy != null &&
116-
!options.OrderBy.OrderByNodes.Any((node) =>
117-
{
118-
string nodeName = ((OrderByPropertyNode)node).Property.Name;
119-
return string.Equals(nodeName, Id, StringComparison.Ordinal);
120-
});
116+
shouldIgnoreOrderById = false;
117+
118+
try
119+
{
120+
shouldIgnoreOrderById = options.OrderBy != null &&
121+
!options.OrderBy.OrderByNodes.Any((node) =>
122+
{
123+
string nodeName = ((OrderByPropertyNode)node).Property.Name;
124+
return string.Equals(nodeName, Id, StringComparison.Ordinal);
125+
});
126+
}
127+
catch (ODataException)
128+
{
129+
return false;
130+
}
131+
132+
return true;
121133
}
122134
}
123135
}

‎tests/NuGetGallery.Facts/Controllers/ODataFeedControllerFactsBase.cs‎

Lines changed: 36 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ protected async Task<IReadOnlyCollection<TFeedPackage>> GetCollection<TFeedPacka
100100
string requestPath)
101101
where TFeedPackage : class
102102
{
103-
var queryResult = InvokeODataFeedControllerAction(controllerAction, requestPath);
103+
var queryResult = (QueryResult<TFeedPackage>)GetActionResult(controllerAction, requestPath);
104104

105105
return await GetValueFromQueryResult(queryResult);
106106
}
@@ -110,7 +110,7 @@ protected async Task<IReadOnlyCollection<TFeedPackage>> GetCollection<TFeedPacka
110110
string requestPath)
111111
where TFeedPackage : class
112112
{
113-
var queryResult = await InvokeODataFeedControllerActionAsync(asyncControllerAction, requestPath);
113+
var queryResult = (QueryResult<TFeedPackage>)await GetActionResultAsync(asyncControllerAction, requestPath);
114114

115115
return await GetValueFromQueryResult(queryResult);
116116
}
@@ -120,7 +120,7 @@ protected async Task<int> GetInt<TFeedPackage>(
120120
string requestPath)
121121
where TFeedPackage : class
122122
{
123-
var queryResult = InvokeODataFeedControllerAction(controllerAction, requestPath);
123+
var queryResult = (QueryResult<TFeedPackage>)GetActionResult(controllerAction, requestPath);
124124

125125
return int.Parse(await GetValueFromQueryResult(queryResult));
126126
}
@@ -130,11 +130,43 @@ protected async Task<int> GetInt<TFeedPackage>(
130130
string requestPath)
131131
where TFeedPackage : class
132132
{
133-
var queryResult = await InvokeODataFeedControllerActionAsync(asyncControllerAction, requestPath);
133+
var queryResult = (QueryResult<TFeedPackage>)await GetActionResultAsync(asyncControllerAction, requestPath);
134134

135135
return int.Parse(await GetValueFromQueryResult(queryResult));
136136
}
137137

138+
protected static ODataQueryContext CreateODataQueryContext<TFeedPackage>()
139+
where TFeedPackage : class
140+
{
141+
var oDataModelBuilder = new ODataConventionModelBuilder();
142+
oDataModelBuilder.EntitySet<TFeedPackage>("Packages");
143+
144+
return new ODataQueryContext(oDataModelBuilder.GetEdmModel(), typeof(TFeedPackage));
145+
}
146+
147+
protected IHttpActionResult GetActionResult<TFeedPackage>(
148+
Func<TController, ODataQueryOptions<TFeedPackage>, IHttpActionResult> controllerAction,
149+
string requestPath)
150+
where TFeedPackage : class
151+
{
152+
var request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}{requestPath}");
153+
var controller = CreateTestableODataFeedController(request);
154+
155+
return controllerAction(controller, new ODataQueryOptions<TFeedPackage>(CreateODataQueryContext<TFeedPackage>(), request));
156+
}
157+
158+
protected async Task<IHttpActionResult> GetActionResultAsync<TFeedPackage>(
159+
Func<TController, ODataQueryOptions<TFeedPackage>, Task<IHttpActionResult>> asyncControllerAction,
160+
string requestPath)
161+
where TFeedPackage : class
162+
{
163+
var request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}{requestPath}");
164+
var controller = CreateTestableODataFeedController(request);
165+
166+
return await asyncControllerAction(controller,
167+
new ODataQueryOptions<TFeedPackage>(CreateODataQueryContext<TFeedPackage>(), request));
168+
}
169+
138170
private static IQueryable<Package> CreatePackagesQueryable()
139171
{
140172
var packageRegistration = new PackageRegistration { Id = TestPackageId };
@@ -227,15 +259,6 @@ private static IQueryable<Package> CreatePackagesQueryable()
227259
return list.AsQueryable();
228260
}
229261

230-
protected static ODataQueryContext CreateODataQueryContext<TFeedPackage>()
231-
where TFeedPackage : class
232-
{
233-
var oDataModelBuilder = new ODataConventionModelBuilder();
234-
oDataModelBuilder.EntitySet<TFeedPackage>("Packages");
235-
236-
return new ODataQueryContext(oDataModelBuilder.GetEdmModel(), typeof(TFeedPackage));
237-
}
238-
239262
private static async Task<dynamic> GetValueFromQueryResult<TEntity>(QueryResult<TEntity> queryResult)
240263
{
241264
var httpResponseMessage = await queryResult.ExecuteAsync(CancellationToken.None);
@@ -256,29 +279,5 @@ private static async Task<dynamic> GetValueFromQueryResult<TEntity>(QueryResult<
256279
return ((IQueryable<TEntity>)objectContent.Value).ToList();
257280
}
258281
}
259-
260-
private async Task<QueryResult<TFeedPackage>> InvokeODataFeedControllerActionAsync<TFeedPackage>(
261-
Func<TController, ODataQueryOptions<TFeedPackage>, Task<IHttpActionResult>> asyncControllerAction,
262-
string requestPath)
263-
where TFeedPackage : class
264-
{
265-
var request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}{requestPath}");
266-
var controller = CreateTestableODataFeedController(request);
267-
268-
return (QueryResult<TFeedPackage>)await asyncControllerAction(controller,
269-
new ODataQueryOptions<TFeedPackage>(CreateODataQueryContext<TFeedPackage>(), request));
270-
}
271-
272-
private QueryResult<TFeedPackage> InvokeODataFeedControllerAction<TFeedPackage>(
273-
Func<TController, ODataQueryOptions<TFeedPackage>, IHttpActionResult> controllerAction,
274-
string requestPath)
275-
where TFeedPackage : class
276-
{
277-
var request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}{requestPath}");
278-
var controller = CreateTestableODataFeedController(request);
279-
280-
return (QueryResult<TFeedPackage>)controllerAction(controller,
281-
new ODataQueryOptions<TFeedPackage>(CreateODataQueryContext<TFeedPackage>(), request));
282-
}
283282
}
284283
}

‎tests/NuGetGallery.Facts/Controllers/ODataV1FeedControllerFacts.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Generic;
55
using System.Linq;
66
using System.Threading.Tasks;
7+
using System.Web.Http.Results;
78
using NuGet.Services.Entities;
89
using NuGetGallery.Configuration;
910
using NuGetGallery.OData;
@@ -28,6 +29,18 @@ public async Task Get_FiltersSemVerV2PackageVersions()
2829
Assert.Equal(NonSemVer2Packages.Where(p => !p.IsPrerelease).Count(), resultSet.Count);
2930
}
3031

32+
[Fact]
33+
public void Get_ReturnsBadRequestWhenOrderByInvalidColumn()
34+
{
35+
// Act
36+
var resultSet = GetActionResult<V1FeedPackage>(
37+
(controller, options) => controller.Get(options),
38+
"/api/v1/Packages?$orderby=abc");
39+
40+
// Assert
41+
Assert.IsType<BadRequestErrorMessageResult>(resultSet);
42+
}
43+
3144
[Fact]
3245
public async Task GetCount_FiltersSemVerV2PackageVersions()
3346
{

‎tests/NuGetGallery.Facts/Controllers/ODataV2FeedControllerFacts.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Generic;
55
using System.Linq;
66
using System.Threading.Tasks;
7+
using System.Web.Http.Results;
78
using Moq;
89
using NuGet.Services.Entities;
910
using NuGet.Versioning;
@@ -46,6 +47,18 @@ public async Task Get_IncludesSemVerV2PackageVersionsWhenSemVerLevel2OrHigher(st
4647
Assert.Equal(AvailablePackages.Count, resultSet.Count);
4748
}
4849

50+
[Fact]
51+
public async Task Get_ReturnsBadRequestWhenOrderByInvalidColumn()
52+
{
53+
// Act
54+
var resultSet = await GetActionResultAsync<V2FeedPackage>(
55+
(controller, options) => controller.Get(options),
56+
"/api/v2/Packages?$orderby=abc");
57+
58+
// Assert
59+
Assert.IsType<BadRequestErrorMessageResult>(resultSet);
60+
}
61+
4962
[Fact]
5063
public async Task GetCount_FiltersSemVerV2PackageVersionsByDefault()
5164
{

0 commit comments

Comments
 (0)