Base solution for your next web application
Open Closed

Server returns HTTP 500 when receiving invalid Jwt token #11802

User avatar
0
hra created

I am testing authentication failure to ensure our smart client (a mobile application) is resilient.

During this testing, I will obtain an authentication token from one environment, and then use that token against another environment (yes, a crazy use-case, but this is just a byproduct of trying to debug a collateral issue).

The token is successfully decoded by the different environment, however, the user id does not exist on that environment, so it throws an exception with the relevant information. I found that, instead of returning useful information, such as "invalid refresh token", the server just returns an unhelpful HTTP 500.

I dug a little deeper, an I found that any time the server receives an invalid token, it will 500 instead of giving an appropriate response.

The reason for this is:

https://github.com/aspnetzero/aspnet-zero-core/blob/c3941a4248476419e91071448956468e7e985f16/aspnet-core/src/MyCompanyName.AbpZeroTemplate.Web.Core/Controllers/TokenAuthController.cs#L249 When an invalid token is received, either because the token payload is invalid (line 256) or because the user id within the payload is not found (264) the method throws an ValidationError. (note: line 268 checks user for null, but this code never executes because UserManager.GetUserAsync explicitly throws an Exception when no user is found - hence line 268 is redundant (this in itself is a bug)

The AbpExceptionFilter then wraps this exception, and because the type "ValidationException" does not match any of the mappable types in the abp DefaultErrorInfoConverter, the error is simply mapped to HTTP 500.

So, there are 2 bugs here. The following line is redundant because GetUserAsync cannot return null: https://github.com/aspnetzero/aspnet-zero-core/blob/c3941a4248476419e91071448956468e7e985f16/aspnet-core/src/MyCompanyName.AbpZeroTemplate.Web.Core/Controllers/TokenAuthController.cs#L268

And "ValidationException" is probably the wrong exception to throw - I think it should be something like AbpAuthorizationException - allowing the client to explain to the user, or at least figure out that token refresh is not going to work, and tell the user that they need to provide their credentials again - as opposed to retrying the same operation.

Of course, I dont know how this change will affect the rest of the application - so it would need to be assessed.

Go to accepted answer
5 Answer(s)
  • User Avatar
    0
    ismcagdas created
    Support Team

    Hi @hra

    By design, such cases return HTTP 500 but you can change source code to alter this behaviour. Since this is a hacking behaviour, I think returning auth error is not good.

  • User Avatar
    0
    hra created

    Hi - thanks for the quick response,

    Please note that my "hack" was just one way to reproduce this behavior. It could have just as easily occurred by a user account becoming unavailable (deletion?) during token refresh - or possibly there is another way for the user to not be found - the original author of TokenAuthController appears to have thought so when they wrote it.

    Here's my thinking...

    • whomever wrote TokenAuthController.Refresh was clearly intending to handle the case where a user could not be found for the user-id claim - because they are testing for "user == null"
    • when "user == null" they are throwing a UserFriendlyException - which is great - it provides some insight to the end user - instead of just HTTP500.
    • unfortunately, the code checking for null being returned from GetUserAsync will never evaluate to true - because GetUserAsync cannot return null (it throws an exception internally if null is detected).
    • the ACTUAL result of a user not being found, is GetUserAsync throws an Exception, which is caught by the Refresh method catch-all handler, which then rethrows as a ValidationException, which then gets mapped to HTTP500 - a very unhelpful error.

    Hopefully I've described my thinking clearly - I agree with the original intent of the code, to deliver useful information about the user not being found, to the end user - however the way it's implemented prevents that intent from being realised - with code that can never be reached by the instruction pointer...

    Thanks @ismcagdas

  • User Avatar
    0
    ismcagdas created
    Support Team

    Hi @hra

    Thanks. Is it possible for you to create an issue on https://github.com/aspnetzero/aspnet-zero-core ? We can re-evaluate this.

  • User Avatar
    0
    hra created

    Hi @hra

    Thanks. Is it possible for you to create an issue on https://github.com/aspnetzero/aspnet-zero-core ? We can re-evaluate this.

    Thanks, done: https://github.com/aspnetzero/aspnet-zero-core/issues/5084

  • User Avatar
    0
    ismcagdas created
    Support Team

    Thanks a lot, we will work on this for the next version.

Assignee
No one
Labels
None yet