AspNetCoreOData icon indicating copy to clipboard operation
AspNetCoreOData copied to clipboard

ODataQueryOptions binding throws an ArgumentException with a whitespace query param

Open axnetg opened this issue 2 years ago • 3 comments

Assemblies affected ASP.NET Core OData 8.x

Describe the bug Sending a request with a query param where the key is whitespace throws an ArgumentException when trying to bind the ODataQueryOptions model in a controller.

Reproduce steps

  1. Create an OData controller with the following action method:
[Route("odata/test")]
public IActionResult Get(ODataQueryOptions<TEntity> options)
{
    //...
}
  1. Send a request to the following endpoint (note the trailing &%20):
/odata/test?$top=10&$skip=0&%20

Expected behavior The ODataQueryOptions object should be bound successfully ignoring the empty query param.

Additional context The issue is reproducible using .NET 6. The empty query param doesn't need to be placed at the end. When ODataOptions.EnableNoDollarQueryOptions is false the exception is NOT thrown and binding behaves as expected.

System.ArgumentException: The argument 'queryOptionName' is null or empty. (Parameter 'queryOptionName')
   at Microsoft.AspNetCore.OData.Query.ODataQueryOptions.IsSupportedQueryOption(String queryOptionName)
   at Microsoft.AspNetCore.OData.Query.ODataQueryOptions.GetODataQueryParameters()
   at Microsoft.AspNetCore.OData.Query.ODataQueryOptions.Initialize(ODataQueryContext context)
   at Microsoft.AspNetCore.OData.Query.ODataQueryOptions`1..ctor(ODataQueryContext context, HttpRequest request)
   at Microsoft.AspNetCore.OData.Query.ODataQueryParameterBindingAttribute.ODataQueryParameterBinding.CreateODataQueryOptions[T](ODataQueryContext context, HttpRequest request)
   at Microsoft.AspNetCore.OData.Query.ODataQueryParameterBindingAttribute.ODataQueryParameterBinding.BindModelAsync(ModelBindingContext bindingContext)
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BinderTypeModelBinder.BindModelAsync(ModelBindingContext bindingContext)
   at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value, Object container)
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext()

axnetg avatar Sep 18 '23 10:09 axnetg

Thanks @axnetg. This looks like expected behavior currently. We will add a behavior flag similar to EnableNoDollarQueryOptions to allow parsing such query options as well.

On the development side, let's see if requests like odata/test?$top=10&$skip=0& also produce the same error. We should also keep in mind that a trailing whitespace query option probably isn't the only case to consider, there could be something like /odata/test?$top=10&$skip=0&%20&foo=true where the whitespace is in the middle that we would want to handle.

corranrogue9 avatar Sep 19 '23 16:09 corranrogue9

Hello @corranrogue9. Thank you for the response!

I found this issue while migrating a project at work from .NET 5 to .NET 6. I guess something changed internally in the way the framework parses the request between the two versions but I couldn't find any documentation on this.

let's see if requests like odata/test?$top=10&$skip=0& also produce the same error

I have just checked this and it doesn't throw the error. Neither do requests like /odata/test?$top=10&&$skip=0.

there could be something like /odata/test?$top=10&$skip=0&%20&foo=true where the whitespace is in the middle that we would want to handle

Yes, the whitespace in the middle of the request also throws the error. Query params like these /odata/test?$top=10&%20=foo&$skip=0 also throw.

axnetg avatar Sep 20 '23 07:09 axnetg

Thanks @axnetg. This looks like expected behavior currently.

@corranrogue9 that's not ideal though. Since this is a problem that the client can cause, it should naturally be a 4XX error instead of a server exception. Is there a chance you could raise this to the team for discussion to change this into a 4XX failure instead?

julealgon avatar Sep 27 '23 16:09 julealgon