Every Error Is a 400 and Says Too Much

Every request in this estate passes through one try/catch — the ErrorHandlerMiddleware that turns an unhandled exception into a JSON response. It is the last line of defence, the thing that decides what a caller learns when something breaks. It is also fourteen lines long, and it contains two independent bugs that happen to sit one on top of the other: it reports every failure as the same HTTP status, and it leaks the raw exception text after carefully computing a safe alternative it never uses. Both are the kind of thing a code review catches in ten seconds and a running demo never reveals.

The whole middleware

public async Task Invoke(HttpContext context)
{
    try { await _next(context); }
    catch (Exception exception)
    {
        _logger.LogError(exception, exception.Message);
        await HandleErrorAsync(context, exception);
    }
}

private static Task HandleErrorAsync(HttpContext context, Exception exception)
{
    var errorCode = "error";
    var statusCode = HttpStatusCode.BadRequest;
    var message = "There was an error.";
    switch (exception)
    {
        case DShopException e:
            errorCode = e.Code;
            message = e.Message;
            break;
    }
    var response = new { code = errorCode, message = exception.Message };
    var payload = JsonConvert.SerializeObject(response);
    context.Response.ContentType = "application/json";
    context.Response.StatusCode = (int)statusCode;
    return context.Response.WriteAsync(payload);
}

Read HandleErrorAsync line by line, because the whole method is a study in machinery built and then not wired up.

Bug one: every error is a 400

statusCode is initialised to HttpStatusCode.BadRequest and never reassigned. The switch sets errorCode and message for a DShopException, but it never touches statusCode, and there is no other branch. So the final line — context.Response.StatusCode = (int)statusCode — always writes 400. A domain validation failure is a 400. A NullReferenceException is a 400. A Mongo timeout, a Consul lookup that found no instances, a bug that should be a 500 — all 400 Bad Request.

That is not a cosmetic mislabel; HTTP status codes are a contract that other software reads. A 400 tells the caller "your request was wrong, don't retry it as-is." When the truth is “our database was briefly unreachable” (a 503, retriable) or “we threw a null reference” (a 500, our fault), reporting 400 tells clients to stop retrying transient failures and tells your monitoring that the server is healthy while it is falling over. Load balancers, retry policies, uptime dashboards and API consumers all make decisions off the status code, and this middleware feeds every one of them the same wrong answer. The distinction the whole switch exists to draw — domain fault versus everything else — is exactly the distinction that should have set the status, and it sets the message instead.

Bug two: the safe message it throws away

Look carefully at where message goes. The method computes it with visible care: default it to a bland "There was an error.", and only for a known DShopException — a controlled, safe-to-expose domain message — replace it with e.Message. That is precisely the right instinct: expose domain errors, hide everything else behind a generic string.

And then the response object is built with the wrong variable:

var response = new { code = errorCode, message = exception.Message };

exception.Message, not message. The sanitised local that took three lines to compute is discarded, and the raw exception.Message — from any exception, DShopException or not — is serialised straight to the client. So an unhandled NullReferenceException doesn't return “There was an error.” It returns "Object reference not set to an instance of an object." A Mongo failure returns the driver's connection string-adjacent diagnostics. A cast failure names your internal types. This is textbook information disclosure: the estate wrote the code to prevent it, computed the safe value, and then referenced the unsafe one by a single-word slip. The message variable is assigned twice and read never.

What the status code is supposed to say

The reason “everything is 400” matters is that the status code is the one part of an error response that machines read, and different failures demand different machine behaviour:

Collapse all of those to 400 and every automated consumer draws the wrong conclusion: retry policies abandon transient failures they should have retried, health checks stay green through a 500-storm because 400s read as “clients being clients,” and a client that would have re-authenticated on a 401 instead surfaces a nonsensical “bad request” to a user whose token simply expired. The switch in this middleware already distinguishes the one case that is a 400 — a DShopException — from everything else; it just spends that distinction on the message and leaves the status hardcoded.

Two one-line fixes, one real lesson

Both bugs are one line each. Set the status from the exception type — case DShopException: statusCode = HttpStatusCode.BadRequest; ... default: statusCode = HttpStatusCode.InternalServerError — so infrastructure faults report as 500 and clients can tell “my fault” from “your fault.” And serialise the variable you computed: message = message, not exception.Message, so only vetted text escapes. Neither is hard. Both were intended — the machinery for per-type codes and sanitised messages is right there, half-connected.

That is the durable lesson, and it generalises well past this file. Computing a safe value and then using the unsafe one is a whole category of bug, and it hides perfectly: the safe code is present, so a skim of the method reassures you, and the demo works because the happy path never hits the catch. The two habits that kill it are a security review that reads what actually reaches WriteAsync (not what the method seems to intend) and an integration test that asserts on the body and status of a deliberately-triggered non-domain exception. Neither existed here — the estate's test story, as the second series shows, is mostly folders that don't test anything.

To be fair, a single global error handler that logs and returns JSON is the right shape, and this one has the right ideas — distinguish domain from infrastructure, sanitise the message. It just left both ideas one identifier short of working. When the authors rebuilt this as Convey, exception-to-status-code mapping became a first-class, extensible concern rather than a hardcoded BadRequest, and that is the fix this middleware is quietly asking for.

Next, the other cross-cutting middleware every service runs — authentication — and the genuinely clever thing it does to make a stateless JWT revocable, at the cost of a Redis round-trip on every request.