ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.32k stars 1.63k forks source link

When delegate handler throws an exception, service returns 404 (not found) #335

Closed Dilsy99 closed 6 years ago

Dilsy99 commented 6 years ago

Expected

When a delegate handler throws an exception I would expect the service to return 500 (internal server error)

Actual

When a delegate handler throws an exception the service returns 404 (not found)

Steps to Reproduce the Problem

Create a delegate handler (inherit from DelegatingHandler) and configure it in startup/config Get the handler to throw and exception Postman to the endpoint that contains the handler and 404 is returned

Possible fix

Add the following code to the ErrorsToHttpStatusCodeMapper:

if (errors.Any(e => e.Code == OcelotErrorCode.UnableToCompleteRequestError))
{
        return 500;
}

Other

I am not sure if the current behavior is for a reason so will not do anything until I hear this is not a daft thing to change!

Specifications

TomPallister commented 6 years ago

@Dilsy99 yep I pretty much agree with this. I don't think there is any reason for this other than I haven't accounted for delegating handlers throwing exceptions since I did the error mapper stuff. I did the error mapping stuff maybe a year and a half ago and delegating handlers are 6 months old so probably my bad :(

I guess this should really be configurable because some places might want to return a 404 instead of 500 to discourage hackers because they think that a 500 means vulnerable to buffer overflow etc. I personally don't see a problem returning a 500 in .net but maybe I'm naive :( However for now happy for you to make this map to a 500 if you want to submit a PR!? :)

Dilsy99 commented 6 years ago

@TomPallister Many thanks for the response

I did think about configuration and how that would work in practice but it rather overwhelmed my Friday afternoon brain!

I have made the change and the only byproduct that I have noticed (because the test failed :-)) is that the ssl certificate configuration error now returns 500 rather than 404 (which makes sense as the handler is throwing the error).

Are you OK that this breaks existing functionality that people "may" rely on?

public void should_not_dangerous_accept_any_server_certificate_validator()
TomPallister commented 6 years ago

@Dilsy99 this is all fine with me :)

I've just merged the PR and will do a release some time today so we can close this issue! :)

Dilsy99 commented 6 years ago

@TomPallister Excellent. Many thanks

TomPallister commented 6 years ago

@Dilsy99 released in version 6.0.0