Incorrectly rejects valid JWTs

abeluck commented 3 years ago

Support plan

is this issue currently blocking your project? (yes/no): yes
is this issue affecting a production system? (yes/no): no

Context

node version: 14.15.1
module version with issue: 2.0.1
last module version without issue: n/a
environment (e.g. node, browser, native): node
used with (e.g. hapi application, another framework, standalone, ...): hapi app
any other relevant information:

What are you trying to achieve or the steps to reproduce?

This plugin rejects a valid JWT, just because it does not have "type": "JWT" in the token header.

Valid JWT:

eyJhbGciOiJIUzUxMiJ9.eyJuYW1lIjoidGVzdCIsImVtYWlsIjoidGVzdEB0ZXN0LnRlc3QiLCJpYXQiOjE2MDYwNTI0MzMsImV4cCI6MTYwODY0NDQzM30.HZ3jD7iZxTCsDjavj2E5OgeYE4maCs5BEVQaIeQQ5eqbzWEJjxgKGYvwe8wjLHmojyeel9XbQ9w4FonJJj048A

  server.auth.strategy("jwt", "jwt", {
    keys: {
      key: "123",
      algorithms: ["HS512"],
      kid: "...",
    },
    verify: false,
    validate: (artifacts, request, h) => {
      return {
        isValid: true,
        credentials: { user: artifacts.decoded.payload.user },
      };
    },
  });

What was the result you got?

{
    "attributes": {
        "error": "Token is not a JWT"
    },
    "error": "Unauthorized",
    "message": "Token is not a JWT",
    "statusCode": 401
}

What result did you expect?

This token should be accepted by the plugin, because as-per the JWT spec (https://tools.ietf.org/html/rfc7519#section-5.1) the typ header is optional.

devinstewart commented 3 years ago

Nice catch in the RFC!

While fixing this in the decode section is rather easy, if this were to be implemented I would like to add an option to token.generate that allows the typ to not be optional, and document said feature.

This helps with test coverage and makes the module consistent in all areas.

That all said, unless anybody has objection or a PR ready to go, this does not look like a big lift, and I could have something raised within the next 24-48 hours.

abeluck commented 3 years ago

We're not using the plugin to generate JWTs, and have no plans to, so no opinion re adding an option there one way or another.

The correct fix I assume would check for the presence of the header, and if present, assert that is JWT?

Was going to work on a PR for this later this week, but if you can make it happen faster (and add the feat to the generate function), then I'm all for it!

devinstewart commented 3 years ago

Yes, in fact I was thinking something as simple as

if (header.typ !== 'JWT') { becoming if (header.typ && header.typ !== 'JWT') {

While I understand that just that line would satisfy your requirements, I tend to look at the module in its entirety.

Stay tuned!

hapijs / jwt