1

I have an ASP.NET Core 6.0 Web API application with the below ExceptionHandlerMiddleware:

public class ExceptionHandlerMiddleware
{
        private readonly RequestDelegate next;

        public ExceptionHandlerMiddleware(RequestDelegate next)
        {
            this.next = next;
        }

        public async Task InvokeAsync(HttpContext context)
        {
            try
            {
                await this.next(context).ConfigureAwait(false);
            }
            catch (Exception ex)
            {
                await this.ConvertExceptionAsync(context, ex).ConfigureAwait(false);
            }
        }

        private Task ConvertExceptionAsync(HttpContext context, Exception exception)
        {
            HttpStatusCode httpStatusCode = HttpStatusCode.InternalServerError;

            context.Response.ContentType = "application/json";

            var errorResponse = new ErrorResponse();

            var result = string.Empty;

            switch (exception)
            {
                case ValidationException validationException:
                    httpStatusCode = HttpStatusCode.BadRequest;
                    errorResponse.Error.Errors = JsonSerializer.Serialize(validationException.ValidationErrors);
                    break;

                case BadRequestException badRequestException:
                    httpStatusCode = HttpStatusCode.BadRequest;
                    errorResponse.Error.Message = badRequestException.Message;
                    break;

                case NotFoundException notFoundException:
                    httpStatusCode = HttpStatusCode.NotFound;
                    break;

                case NotAcceptableException notAcceptableException:
                    httpStatusCode = HttpStatusCode.NotAcceptable;
                    errorResponse.Error.Message = notAcceptableException.Message;
                    break;

                case Exception ex:
                    httpStatusCode = HttpStatusCode.BadRequest;
                    break;
            }

            errorResponse.ApiVersion = context.GetRequestedApiVersion()?.ToString();
            errorResponse.Error.Code = (int)httpStatusCode;
            context.Response.StatusCode = (int)httpStatusCode;

            if (string.IsNullOrEmpty(result))
            {
                errorResponse.Error.Message = exception.Message;
            }

            var serializeOptions = new JsonSerializerOptions
            {
                PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
                WriteIndented = true,
            };

            result = JsonSerializer.Serialize(errorResponse, serializeOptions);

            return context.Response.WriteAsync(result);
        }
}

It returns the error in the custom format like mentioned below

{
  "error": {
    "code": 404,
    "message": "Education (a65f7f0c-2a29-4da0-bd4b-d737320730c6) is not found",
    "errors": "[]"
  },
  "apiVersion": "1.0"
}

During the security scan, it is reported that

The file handles an Exception or runtime Error ex. During the exception handling code, the application exposes the exception details to WriteAsync, in method ConvertExceptionAsync

Avoid information leak through error messages

  1. Don’t expose users exception output data
  2. Properly handle exception for each methods
  3. Configure a global handler to prevent unhandled errors

Should I ask the security team to suppress this issue? Or this is something that can be addressed at the application level?

Update: As I just want to address the reported issue, "context.Response.WriteAsync" is moved to finally block. I hope it should address the above security issue.

    public async Task InvokeAsync(HttpContext context)
    {
        bool isExceptionReported = false;
        string result = string.Empty;

        try
        {
            await this.next(context).ConfigureAwait(false);
        }
        catch (Exception ex)
        {
            isExceptionReported = true;
            result = this.ConvertException(context, ex);
        }
        finally
        {
            if (isExceptionReported)
            {
               await context.Response.WriteAsync(result).ConfigureAwait(false);
            }
        }
    }

    private string ConvertException(HttpContext context, Exception exception)
    {
        HttpStatusCode httpStatusCode = HttpStatusCode.InternalServerError;

        context.Response.ContentType = "application/json";

        var errorResponse = new ErrorResponse();

        var result = string.Empty;

        switch (exception)
        {
            case ValidationException validationException:
                httpStatusCode = HttpStatusCode.BadRequest;
                errorResponse.Error.Errors = JsonSerializer.Serialize(validationException.ValidationErrors);
                break;
            case BadRequestException badRequestException:
                httpStatusCode = HttpStatusCode.BadRequest;
                errorResponse.Error.Message = badRequestException.Message;
                break;
            case NotFoundException notFoundException:
                httpStatusCode = HttpStatusCode.NotFound;
                break;
            case NotAcceptableException notAcceptableException:
                httpStatusCode = HttpStatusCode.NotAcceptable;
                errorResponse.Error.Message = notAcceptableException.Message;
                break;
            case Exception ex:
                httpStatusCode = HttpStatusCode.BadRequest;
                break;
        }

        errorResponse.ApiVersion = context.GetRequestedApiVersion()?.ToString();
        errorResponse.Error.Code = (int)httpStatusCode;
        context.Response.StatusCode = (int)httpStatusCode;

        if (string.IsNullOrEmpty(result))
        {
            errorResponse.Error.Message = exception.Message;
        }

        var serializeOptions = new JsonSerializerOptions
        {
            PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
            WriteIndented = true,
        };

        result = JsonSerializer.Serialize(errorResponse, serializeOptions);

        return result;
    }
One Developer
  • 99
  • 5
  • 43
  • 103
  • 1
    Are you sure you need this middleware? Or even that it's a good idea? Not Found is an HTTP response with status 404, not an OK (200) containing 404 in the payload. A ValidationException results in a BadRequest (400) response with a well formatted, RFC-standard ProblemDetails response. It's a bad idea to replace standard responses with custom ones. ASP.NET itself won't return exception details in a production environment. Your middleware on the other hand *does* return the messages even when there's no need to – Panagiotis Kanavos Aug 23 '22 at 16:29
  • 1
    Clients know how to handle 404, 406 or 400. Resilience libraries like Polly will execute the appropriate retry or recovery policy based on the HTTP status code. No client knows how to handle something that claims to be OK but isn't. Worse yet, **caches and proxies** will cache the fake-OK response and return that even if the API has recovered. Even if an `Education` item with ID `a65f7f0c-2a29-4da0-bd4b-d737320730c6` is added, caches will still return the fake OK response – Panagiotis Kanavos Aug 23 '22 at 16:31
  • @PanagiotisKanavos, Thank you for pointing it out. I will work on this. Do you have any thoughts on addressing the security scan issue? – One Developer Aug 23 '22 at 16:47
  • Yes, remove that middleware. It's bad. You shouldn't be using *exceptions* as a way to return a 4xx response from an API in the first place. You shouldn't be using 400 for unexpected exceptions either. It wasn't the request that was bad, it's the server that failed. The client can retry the exception later and succeed. 400 tells the client that they shouldn't retry at all. – Panagiotis Kanavos Aug 23 '22 at 16:50
  • 3
    As for the security issue - you *are* returning `Exception.Message` in most cases. Instead of hiding the exceptions you're actually sending them to the client. I repeat, ASP.NET Core itself wouldn't return anything beyond the status code in production mode. Your own code exposes the details ASP.NET Core would hide – Panagiotis Kanavos Aug 23 '22 at 16:50

1 Answers1

1

Here is now I handle this "Exception to Http-Status-Code" tension.

[ExcludeFromCodeCoverage, Serializable]
public class BusinessLayerFinalException : Exception
{
    public const int PreferredHttpStatusCodeDefault = 500;

    public const bool ShowInnerExceptionToCustomerDefault = false;

    public const string
        CustomerFacingMessageDefault = "There was an error with your request";

    public int? PreferredHttpStatusCode { get; set; } = PreferredHttpStatusCodeDefault;

    public bool ShowInnerExceptionToCustomer { get; set; } = ShowInnerExceptionToCustomerDefault;

    public string CustomerFacingMessage { get; set; } = CustomerFacingMessageDefault;

    public BusinessLayerFinalException()
    {
    }

    public BusinessLayerFinalException(string message)
        : base(message)
    {
    }

    public BusinessLayerFinalException(string message, Exception inner)
        : base(message, inner)
    {
    }

    protected BusinessLayerFinalException(
        SerializationInfo info,
        StreamingContext context) : base(info, context)
    {
        if (null != info)
        {
            this.PreferredHttpStatusCode = info.GetInt32(nameof(this.PreferredHttpStatusCode));
            this.ShowInnerExceptionToCustomer = info.GetBoolean(nameof(this.ShowInnerExceptionToCustomer));
            this.CustomerFacingMessage = info.GetString(nameof(this.CustomerFacingMessage));
        }
    }

    public override void GetObjectData(
        SerializationInfo info,
        StreamingContext context)
    {
        base.GetObjectData(info, context);

        if (null != info)
        {
            info.AddValue(nameof(this.PreferredHttpStatusCode), this.PreferredHttpStatusCode);
            info.AddValue(nameof(this.ShowInnerExceptionToCustomer), this.ShowInnerExceptionToCustomer);
            info.AddValue(nameof(this.CustomerFacingMessage), this.CustomerFacingMessage);
        }
    }

    public BusinessLayerFinalException(
        string message,
        int preferredHttpStatusCode,
        bool showInnerExceptionToCustomer,
        string customerFacingMessage) : base(message)
    {
        this.PreferredHttpStatusCode = preferredHttpStatusCode;
        this.ShowInnerExceptionToCustomer = showInnerExceptionToCustomer;
        this.CustomerFacingMessage = customerFacingMessage;
    }

    public BusinessLayerFinalException(
        string message,
        Exception innerException,
        int preferredHttpStatusCode,
        bool showInnerExceptionToCustomer,
        string customerFacingMessage) : base(message, innerException)
    {
        this.PreferredHttpStatusCode = preferredHttpStatusCode;
        this.ShowInnerExceptionToCustomer = showInnerExceptionToCustomer;
        this.CustomerFacingMessage = customerFacingMessage;
    }
}

Then I write 2 (and only 2) Middleware.

One to handle "Exception" (where I give a 500 and NO extra context information.

And one more to handle "BusinessLayerFinalException", which (intelligently?? :) ) looks at BusinessLayerFinalException and figures out what to show.

I'll show the more interesting one below:

   /// <summary>
    /// Middleware to handle a specific exception 'BusinessLayerFinalException'.  Loosely based on : https://learn.microsoft.com/en-us/aspnet/core/web-api/handle-errors?view=aspnetcore-5.0#use-exceptions-to-modify-the-response
    /// </summary>
    public class BusinessLayerFinalExceptionFilter : IActionFilter, IOrderedFilter
    {
        public int Order { get; } = int.MaxValue - 10;

        public const string FullErrorMessageTemplate = "({0}), ({1})";

        public void OnActionExecuting(ActionExecutingContext context)
        {
            /* as per the microsoft example, this method is currently empty */
        }

        public void OnActionExecuted(ActionExecutedContext context)
        {
            if (context.Exception is BusinessLayerFinalException exception)
            {
                string errorMsg = exception.ShowInnerExceptionToCustomer ? string.Format(FullErrorMessageTemplate, exception.CustomerFacingMessage, ExceptionExtensions.GenerateFullFlatMessage(exception, false)) : exception.CustomerFacingMessage;
                
                context.Result = new ObjectResult(errorMsg)
                {
                    StatusCode = exception.PreferredHttpStatusCode ?? (int)HttpStatusCode.InternalServerError
                };

                context.ExceptionHandled = true;
            }
        }
    }

ExceptionExtensions.GenerateFullFlatMessage

(this loops over all .InnerException(s) to provide a full stack trace.

..

You could also inject a "IsDevelopment" (https://learn.microsoft.com/en-us/aspnet/core/fundamentals/environments?view=aspnetcore-6.0 ) boolean to always show the super-details in that environment.

The BusinessLogic then has the "responsibility" to always throw a BusinessLayerFinalException...or anything else will be caught with the "Exception" middleware and give a no-information 500 to the outside world.

The benefit of BusinessLayerFinalException (IMHO) is that I am allowing the business logic to give the top-rest layer the "hints" that it needs ... WITHOUT breaking the best practice seen below.

=====

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/exception-throwing

❌ DO NOT return error codes. Exceptions are the primary means of reporting errors in frameworks.

✔️ DO report execution failures by throwing exceptions.

=====

I have seen (too many) code bases that started going back to VB6 mentality and returning error-codes all over the place....with http numbers. A complete hijack of the OO language.

granadaCoder
  • 26,328
  • 10
  • 113
  • 146