Changed response from 500 to 401 on unexpected message type 3

[bjowes]

As explained in isse #61

[einfallstoll]

I'm not sure whether I'm happy with this. The package yields a 500 Internal Server Error whenever something unexpected happens. If I would accept this PR, I would probably have to change the other places as well.

I will see how the implementation specification expects this or if it's up to me to decide this.

You can always overwrite the internalservererror handler through the options, so you could send a 401 instead.

[einfallstoll]

From [MS-NTHT]: NTLM Over HTTP Protocol:

Any return code other than a client error HTTP 401 status code (for more information, see [RFC2616] section 10.4.2), represents successful authentication. If the client is not able to access the requested resource and the response status code is not HTTP 401, the problem is HTTP protocol-specific (for more information, see [RFC2616] section 10).

How would you interpret that? On one hand "Any return code other than a client error HTTP 401 status code, [...] represents successful authentication." means a HTTP 500 would mean a successful authentication, which would be bad and should be fixed according to your PR and I would have to change the others as well, on the other hand "If the client is not able to access the requested resource and the response status code is not HTTP 401, the problem is HTTP protocol-specific [...]" could be interpreted so the usage of HTTP 500 would be acceptable and this is up to the implementation how to handle it.

[bjowes]

Understood, it is nice to have a consistent approach. I tried looking through some specs but couldn't find details on this - maybe you have access to better docs.

My thinking is that with a 401 response, most browsers would recover automatically. I am working with proxies, and when I enable/disable a proxy, chrome keeps thinking I am authenticated and sends the type 3 message. If it would receive the 401, it would re-authenticate without any user intervention.

[einfallstoll]

If the implementation is strictly "Any return code other than a client error HTTP 401 status code, [...] represents successful authentication." (as you stated with the example in Chrome), then we will have to change our implementation according to this, even though this means introducing a breaking change.

[bjowes]

Nice catch from the docs! I think it doesn't make sense to claim that 500 means authenticated. A 500 error could arise before authentication takes place. But I understand the purpose - you can receive many different status codes and still be authenticated, like 202 or 304.

[einfallstoll]

True! I will sleep over this, but I will probably merge your PR and remove internalservererror entirely. Still have to think about the impact.

[LRipi]

@einfallstoll Any news about merging the PR? 😄