Some OData exceptions are not caught by the WebApi, but swallowed by Odata

mabouwhui commented 5 years ago

We have a WebApi in AspNet core 2.2 that uses AspNetCore.OData version 7.2.1. For security reasons we want to have full control over the responses when exceptions are thrown, so OData stacktraces are not send back to our Api consumers. However, some OData exceptions cannot be caught in the API; They are swallowed by the OData layer.

We are looking for a way to catch these exceptions, but are unable to do so. Below is a way to reproduce these kind of exceptions.

Assemblies affected

Microsoft.AspNetCore.OData 7.2.1

Reproduce steps

Reproduction steps:

In visual studio create a new ASP.NET Core Web Application project, Target the ASP.NET Core framework version 2.2 and add the latest nuget package Microsoft.AspNetCore.OData (currently at version 7.2.1) and configure OData

Add an example controller, e.g.:

[Route("api/[controller]")]
public class BooksController : ODataController
{
    // GET api/books
    [HttpGet]
    [ODataSampleEnableQuery]
    public IQueryable<Book> Get(ODataQueryOptions queryOptions)
    {
        ODataSampleEnableQueryAttribute.Validate(queryOptions);
        return new Book[]
        {
            new Book() { Id = 1, Title = "1984", Author = "George Orwell", Available = true},
            new Book() { Id = 2, Title = "The Hobbit", Author = "J.R.R. Tolkien", Available = false}
        }.AsQueryable();
    }
}

public class Book
{
    public long Id { get; set; }
    public string Title { get; set; }
    public string Author { get; set; }
    public bool Available { get; set; }
}

Make sure all exceptionhandling is in place
A. Catch application exceptions:

Add in Startup.cs.Configure(..,..) (above the statement "app.UseMvc(..)"):

app.UseExceptionHandler(appError =>
    {
        appError.Run(async context =>
        {
            context.Response.StatusCode = (int)HttpStatusCode.InternalServerError;
            context.Response.ContentType = "application/json";
            //Error logging goes here
            await context.Response.WriteAsync("An error has occurred. Check the log.");
        });
    });

B. Catch OData validation exceptions: Create an inherited class from the EnableQueryAttribute to verify the ODataQueryOptions in your application code:

public class ODataSampleEnableQueryAttribute : EnableQueryAttribute
{
public override void ValidateQuery(HttpRequest request, ODataQueryOptions queryOptions)
{
    try
    {
        Validate(queryOptions);
        base.ValidateQuery(request, queryOptions);
    }
    //Errors are of type ODataException or ArgumentOutOfRangeException
    catch (Exception e)
    {
        //Will be a specific exception in production code
        throw new Exception("An error occurred", e)
            { };
    }
}

public static void Validate(ODataQueryOptions queryOptions)
{
    try
    {
        queryOptions.Validate(new ODataValidationSettings() { });
    }
    catch (Exception e)
    {
        //Will be a specific exception in production code
        throw new Exception(e.Message, e)
            { };
    }
}
}

Send an url to the API that is not caught bu the application: GET https://localhost:44304/api/Books?$count=True GET https://localhost:44304/api/Books?$filter=startswith(title, null)

Expected result

Exception is caught by the application's exception handler OR Exception is thrown by the ODataQueryOptions.Validate() (and in our case redirected to the application's exception handler)

Actual result

No exception is caught by the application. OData swallows/catches the exception and returnes a 400 response with full stacktrace:

{
    "Message": "The query specified in the URI is not valid. The 'startswith' function cannot be applied to an enumeration-typed argument.",
    "ExceptionMessage": "The 'startswith' function cannot be applied to an enumeration-typed argument.",
    "ExceptionType": "Microsoft.OData.ODataException",
    "StackTrace": "   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.ValidateAllStringArguments(String functionName, Expression[] arguments)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.BindStartsWith(SingleValueFunctionCallNode node)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.BindSingleValueFunctionCallNode(SingleValueFunctionCallNode node)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.BindSingleValueNode(SingleValueNode node)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.Bind(QueryNode node)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.BindExpression(SingleValueNode expression, RangeVariable rangeVariable, Type elementType)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.BindFilterClause(FilterBinder binder, FilterClause filterClause, Type filterType)\r\n   at Microsoft.AspNet.OData.Query.Expressions.FilterBinder.Bind(IQueryable baseQuery, FilterClause filterClause, Type filterType, ODataQueryContext context, ODataQuerySettings querySettings)\r\n   at Microsoft.AspNet.OData.Query.FilterQueryOption.ApplyTo(IQueryable query, ODataQuerySettings querySettings)\r\n   at Microsoft.AspNet.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query, ODataQuerySettings querySettings)\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.ApplyQuery(IQueryable queryable, ODataQueryOptions queryOptions)\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.ExecuteQuery(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, Func`2 modelFunction, IWebApiRequestMessage request, Func`2 createQueryOptionFunction)\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.OnActionExecuted(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, IWebApiRequestMessage request, Func`2 modelFunction, Func`2 createQueryOptionFunction, Action`1 createResponseAction, Action`3 createErrorAction)"
}

j-childers commented 4 years ago

Seeing the same problem here in .NET Core 3.1 and OData 7.3.0

shoguns6 commented 4 years ago

Facing the same problem in .NET Core 3.1 and OData 7.3.0

shoguns6 commented 4 years ago

Seeing the same problem here in .NET Core 3.1 and OData 7.3.0

Me too!!

ishan30 commented 4 years ago

I'm facing the same issue! Does anyone know a way to handle this exception?

ishan30 commented 4 years ago

@KanishManuja-MS Is there a way to handle this?

shoguns6 commented 4 years ago

@KanishManuja-MS Is there a way to handle this?

I'm facing the same issue! Does anyone know a way to handle this exception?

One way it worked for me is to explicitly call 'queryOption.Validate()'. this will throw an exception if there is any issue. The same can be caught my middleware.

SenyaMur commented 4 years ago

My solution:

 public class ErrorHandlingMiddleware {
    private readonly RequestDelegate next;
    public ErrorHandlingMiddleware(RequestDelegate next) {
      this.next = next;
    }

    public async Task Invoke(HttpContext context /* other dependencies */ ) {
      bool modifyResponse = true;
      Stream originBody = null;

      if (modifyResponse) {
        //uncomment this line only if you need to read context.Request.Body stream
        //context.Request.EnableRewind();

        originBody = ReplaceBody(context.Response);
      }
      try {
        await next(context);
      } catch (Exception ex) {
        await HandleExceptionAsync(context, ex);
      } finally {
        //as we replaced the Response.Body with a MemoryStream instance before,
        //here we can read/write Response.Body
        //containing the data written by middlewares down the pipeline 

        //finally, write modified data to originBody and set it back as Response.Body value
        ReturnBody(context.Response, originBody);
      }
    }

    private Stream ReplaceBody(HttpResponse response) {
      var originBody = response.Body;
      response.Body = new MemoryStream();
      return originBody;
    }

    private void ReturnBody(HttpResponse response, Stream originBody) {
      response.Body.Seek(0, SeekOrigin.Begin);
      response.Body.CopyTo(originBody);
      response.Body = originBody;
    }
    private Task HandleExceptionAsync(HttpContext context, Exception ex) {
      var code = HttpStatusCode.InternalServerError; // 500 if unexpected
      var result = JsonConvert.SerializeObject(new {Error= ex.Message});
      context.Response.Body.Seek(0, SeekOrigin.Begin);
      context.Response.ContentType = "application/json";
      context.Response.StatusCode = (int) code;
      return context.Response.WriteAsync(result);
    }
  }

In Startup.cs

//Add in Configure before UseMvc
app.UseMiddleware(typeof(ErrorHandlingMiddleware));

hidegh commented 4 years ago

Usually errors that occur outside the MVC pipeline has to be handled by a middleware. These errors include:

any errors prior MVC pipeline is entered
any errors after the MVC pipeline (deferred iqueryable errors, serialization errors)

If you are returning an un-evaluated IQueryable (deferred) then you might have SQL execution errors, but also if there's some Serialization issue (circular dependencies, etc.)...

ishan30 commented 4 years ago

One way it worked for me is to explicitly call 'queryOption.Validate()'. this will throw an exception if there is any issue. The same can be caught my middleware.

Thanks for the revert.

When I pass some query as expand, my api gives correct result and in case of in vague query parameter for example "$expand=qwe123" it gives me 400 bad request with exception message : "Query specified in URI is invalid"

type : ODataException

Is there any way that I can handle this in my controller?

shoguns6 commented 4 years ago

in the controller get method.... use OdataQueryOptions explicitly... like the one below.. or u can catch that error in middleware. Supporting ODataQueryOptions in existing Web API

|

Supporting ODataQueryOptions in existing Web API

I have a Web API project which has been used for several years without OData support, just with standard URL par... |

|

On Thursday, 18 June, 2020, 06:44:52 pm IST, Ishan Shah <notifications@github.com> wrote:

One way it worked for me is to explicitly call 'queryOption.Validate()'. this will throw an exception if there is any issue. The same can be caught my middleware.

Thanks for the revert.

When I pass some query as expand, my api gives correct result and in case of in vague query parameter for example "$expand=qwe123" it gives me 400 bad request with exception message : "Query specified in URI is invalid"

type : ODataException

Is there any way that I can handle this in my controller?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

flipdoubt commented 4 years ago

Using the custom EnableQueryAttribute in the original comment by @mabouwhui fixes the problem in our application in that it redirects the ODataException to our own error handling logic, but I want to add my +1 to the original issue because the current error handling exposes the full stack trace and implementation detail in production. This seems like a big security leak the OData team seems willing to handle in most other cases. Severity on this one should be escalated, IMHO.

ambitiousrahul commented 3 years ago

Using the custom EnableQueryAttribute in the original comment by @mabouwhui fixes the problem in our application in that it redirects the ODataException to our own error handling logic, but I want to add my +1 to the original issue because the current error handling exposes the full stack trace and implementation detail in production. This seems like a big security leak the OData team seems willing to handle in most other cases. Severity on this one should be escalated, IMHO.

I have created the CustomEnableQueryAttribute and used it for endpoints. Debugger strikes try block , but exception is not caught. Debugger does not enters the catch block.

This is with oData-7.5

SymbioticKilla commented 3 years ago

Any Update on this?

NinjaCross commented 1 year ago

Any updates on this ? I'm still relying on @mabouwhui solution, but it really feels to me that in 2023 a better solution should be available... I'm I wrong ? I'm I missing something ?

mabouwhui commented 1 year ago

This issue is closed, because - I think - a workaround is available. For clarification I will write a wrap-up.

To catch ALL OData exceptions it is not a good enough solution to create a catch in the override of the EnableQueryAttribute class. Under some circumstances exceptions will not be caught and may 'leak' back to the API consumer. This was the reason why I added this issue.

The workaround is to create a middleware that will catch all exceptions in your API, as suggested by @SenyaMur. This is a good idea anyway, because exceptions from other middleware might also 'leak'. Probably an open door, but a small addition to the suggested solution: Be aware of the position in startup.cs of the code where you add the middleware (app.UseMiddleware(... ). It is typically the first line in the Configure() method.

hexagonite commented 1 year ago

If you are using the AspNetCoreOData package, the solution I've found is to combine a custom ODataSerializer with an error-handling middleware. This way you can control the response body and thus clear the stack trace from it. Some exceptions will be caught in your middleware, and some in the serializer (because some exceptions are being handled automatically within the private methods of this package and no middleware will help you).

Sayan751 commented 1 year ago

The approach with the custom ODataErrorSerializer, as mentioned by @hexagonite is very interesting. However, I find it to be still very limiting. For my case, I am getting a SerializableError which is practically a Dictionary<string, string>. This makes a problem when I want to handle certain errors differently, forcing me to parse the strings. The API must be friendlier. I think it might be a fair expectation that at minimum the ErrorCode key is set.

Viech3 commented 11 months ago

My solution:

 public class ErrorHandlingMiddleware {
    private readonly RequestDelegate next;
    public ErrorHandlingMiddleware(RequestDelegate next) {
      this.next = next;
    }

    public async Task Invoke(HttpContext context /* other dependencies */ ) {
      bool modifyResponse = true;
      Stream originBody = null;

      if (modifyResponse) {
        //uncomment this line only if you need to read context.Request.Body stream
        //context.Request.EnableRewind();

        originBody = ReplaceBody(context.Response);
      }
      try {
        await next(context);
      } catch (Exception ex) {
        await HandleExceptionAsync(context, ex);
      } finally {
        //as we replaced the Response.Body with a MemoryStream instance before,
        //here we can read/write Response.Body
        //containing the data written by middlewares down the pipeline 

        //finally, write modified data to originBody and set it back as Response.Body value
        ReturnBody(context.Response, originBody);
      }
    }

    private Stream ReplaceBody(HttpResponse response) {
      var originBody = response.Body;
      response.Body = new MemoryStream();
      return originBody;
    }

    private void ReturnBody(HttpResponse response, Stream originBody) {
      response.Body.Seek(0, SeekOrigin.Begin);
      response.Body.CopyTo(originBody);
      response.Body = originBody;
    }
    private Task HandleExceptionAsync(HttpContext context, Exception ex) {
      var code = HttpStatusCode.InternalServerError; // 500 if unexpected
      var result = JsonConvert.SerializeObject(new {Error= ex.Message});
      context.Response.Body.Seek(0, SeekOrigin.Begin);
      context.Response.ContentType = "application/json";
      context.Response.StatusCode = (int) code;
      return context.Response.WriteAsync(result);
    }
  }

In Startup.cs

//Add in Configure before UseMvc
app.UseMiddleware(typeof(ErrorHandlingMiddleware));

For .NET6 you have to make ReturnBody async. Otherwise an exception 'Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead.' is thrown.

public class ErrorHandlingMiddleware {
    private readonly RequestDelegate next;
    public ErrorHandlingMiddleware(RequestDelegate next) {
      this.next = next;
    }

    public async Task Invoke(HttpContext context /* other dependencies */ ) {
      bool modifyResponse = true;
      Stream originBody = null;

      if (modifyResponse) {
        //uncomment this line only if you need to read context.Request.Body stream
        //context.Request.EnableRewind();

        originBody = ReplaceBody(context.Response);
      }
      try {
        await next(context);
      } catch (Exception ex) {
        await HandleExceptionAsync(context, ex);
      } finally {
        //as we replaced the Response.Body with a MemoryStream instance before,
        //here we can read/write Response.Body
        //containing the data written by middlewares down the pipeline 

        //finally, write modified data to originBody and set it back as Response.Body value
        await ReturnBodyAsync(context.Response, originBody); // <-- await Call
      }
    }

    private Stream ReplaceBody(HttpResponse response) {
      var originBody = response.Body;
      response.Body = new MemoryStream();
      return originBody;
    }

    private async Task ReturnBodyAsync(HttpResponse response, Stream originBody) { // <-- changed to async method
      response.Body.Seek(0, SeekOrigin.Begin);
      await response.Body.CopyToAsync(originBody); // <-- changed to CopyAsync
      response.Body = originBody;
    }
    private Task HandleExceptionAsync(HttpContext context, Exception ex) {
      var code = HttpStatusCode.InternalServerError; // 500 if unexpected
      var result = JsonConvert.SerializeObject(new {Error= ex.Message});
      context.Response.Body.Seek(0, SeekOrigin.Begin);
      context.Response.ContentType = "application/json";
      context.Response.StatusCode = (int) code;
      return context.Response.WriteAsync(result);
    }
  }

AirtonBorges commented 2 weeks ago

I've found a way to get the exception when you have a ModelState:

    private Exception? GetExceptionFromModelState(ModelStateDictionary pModelState)
    {
        var xError = pModelState.Values.SelectMany(p => p.Errors)
            .Where(p => p.Exception != null)
            .Select(p => p.Exception!)
            .FirstOrDefault();

        return xError;
    }

With this, you can rethow the exception, or deal with it in another way. Example:

    [HttpPatch]
    public virtual async Task<IActionResult> Patch(int key
        , Delta<Item> pItem)
    {
        var xError = GetExceptionFromModelState(ModelState);
        if (xError != null)
           throw xError;
     ...

OData / WebApi