johnbrett / hapi-auth-bearer-token

Simple Bearer authentication scheme plugin for hapi, accepts token by Header, Cookie or Query parameter.
MIT License
218 stars 46 forks source link

Error 500 on invalid token #160

Closed wkocmann closed 5 years ago

wkocmann commented 6 years ago

A request with an invalid token results in

{"statusCode":500,"error":"Internal Server Error","message":"An internal server error occurred"}

due to the line

            return h.unauthenticated(settings.unauthorized(message, settings.tokenType), { credentials, artifacts });

if is is changed to

            return h.unauthenticated(settings.unauthorized(message), { credentials, artifacts });

the statusCode is 401 as it should be.

johnbrett commented 5 years ago

It's interesting that this did not pop up before now, or whether this was a recent change caused this. Can you submit a failing test for this, I'd like to check this out?

wkocmann commented 5 years ago

I'm using it with hapi-swagger and the options object as a response property. Your test does fail if I modify the test this way:

            { method: 'GET', path: '/always_reject', handler: defaultHandler, options: { auth: 'always_reject', response: {
                status: {
                  401: {
                    statusCode: 401,
                    error: 'Unauthorized',
                    message: 'Missing authentication'
                }
            } } } },

the output of the lab test is

      actual expected

      {
        "attributes": {
          "error": "InternalBad Servertoken"
        Error},
        "error": "Unauthorized",
        "message": "AnBad internal server error occurredtoken",
        "statusCode": 500401
      }

      Expected { statusCode: 500,
  error: 'Internal Server Error',
  message: 'An internal server error occurred' } to equal specified value: { statusCode: 401,
  error: 'Unauthorized',
  message: 'Bad token',
  attributes: { error: 'Bad token' } }
wkocmann commented 5 years ago

but the Error 500 only appears if there is a response for 401

nvs2394 commented 5 years ago

I got same issue. When I check token invalid it always return 500 internal server error. Do you have any update ?

kraniebrud commented 5 years ago

I just made a token in my project to test this, I set my jwt with a very short expiration. When I hit up a route through my swagger gui (hapi-swagger) this is the response that I get from it

{
  "statusCode": 401,
  "error": "Unauthorized",
  "message": "Bad token",
  "attributes": {
    "error": "Bad token"
  }
}

If change my header bearer with some nonsense or nothing at all I get

{
  "statusCode": 401,
  "error": "Unauthorized",
  "message": "Missing authentication"
}

When I try the exact same with curl to those endpoints I get the same response as when using swagger.

This is the version that I am running "hapi-auth-bearer-token": "6.1.0"

I am not saying that there is no problem here, It just seems that I cannot reproduce the issue that you guys are encountering and that it bugs me.

That being said you mentioned that you used options on a route. I suppose that means that you wrote a schema to validate the response and to output an example in swagger? If this schema is wrong you WILL encounter an "Internal Server Error" -- and that would be expected. Now if it is an "Bad Token". The response will ALSO contain a property named attributes in the response, so if you provided a schema for a 401 response and this is missing you will get an "Internal Server Error" and that is correct.

kraniebrud commented 5 years ago

From that the test you mentioned would probably more be expected to be

{
  statusCode: 401,
  error: 'Unauthorized',
  message: 'Bad token',
  attributes: { error: 'Bad token' }
}

if you wish it to be

{
  statusCode: 401,
  error: 'Unauthorized',
  message: 'Missing authentication'
}

Try to write a custom unauthorized function for hapi-auth-bearer-token, in the options like this

{
  // some options
  unauthorized: (message, scheme) => { 
    throw boom.unauthorized(null, scheme)
  }

}

Let me know if this changed anything :)

wkocmann commented 5 years ago

Actually the custom unauthorized is working

Actually it doesn't change anything

kraniebrud commented 5 years ago

Oh shoot. Would have made sense though. Did you change the first argument in the unauthorized function ? If it is changed to something custom, it will produce the attribute object to the response. Not very intuitive I know :/

Alexis-Bize commented 5 years ago

I had the same issue and... According to the documentation, the credentials node is mandatory, even if your token is invalid. Without it, the server will respond with an error 500.

{ isValid: false, credentials: {} }

This "issue" is now fixed on my side :)

kraniebrud commented 5 years ago

If so it seems to be familiar to this one https://github.com/johnbrett/hapi-auth-bearer-token/issues/152 ?

wkocmann commented 5 years ago

I my case the credentials property is present.

    validate: async (request, token, h) => {
      return {
        isValid: isTokenValid(token),
        credentials: { token }
      };
    },
johnbrett commented 5 years ago

sorry everyone, currently on vacation, will try get to this when I can

johnbrett commented 5 years ago

I'm back and can deal with this now, sorry for delay on this. @kraniebrud thanks for jumping in and debugging this!

@wkocmann is this still an issue for you?

wkocmann commented 5 years ago

it is still an issue, have you tried to test with

            { method: 'GET', path: '/always_reject', handler: defaultHandler, options: { auth: 'always_reject', response: {
                status: {
                  401: {
                    statusCode: 401,
                    error: 'Unauthorized',
                    message: 'Missing authentication'
                }
            } } } },
marcuspoehls commented 5 years ago

@johnbrett Hey John, I just ran into the same issue. When returning { isValid: false } and nothing else from the validate function, the plugin crashes. It's because you're taking credentials out of the returned object from the validate function and just pass it through to the h.unauthenticated() call.

Should I go for a PR? Or do you fix it on your own?

Let me know if I can help any further :) Thanks!

johnbrett commented 5 years ago

Would be delighted if you could provide a PR @marcuspoehls thanks a mil!!

marcuspoehls commented 5 years ago

@johnbrett Opened the PR. If anything doesn't match your idea, let's change it :)

johnbrett commented 5 years ago

That's perfect @marcuspoehls ! Thanks! merged and will publish a new version asap

marcuspoehls commented 5 years ago

@johnbrett Awesome! Thank you!

johnbrett commented 5 years ago

Published in v6.1.1

marcuspoehls commented 5 years ago

Thank you!

wkocmann commented 5 years ago

Thanks