Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid JWT token in local storage will crash App / NbAuthJWTToken #517

Closed
1 of 2 tasks
schuettecarsten opened this issue Jun 25, 2018 · 10 comments
Closed
1 of 2 tasks

Comments

@schuettecarsten
Copy link

Issue type

I'm submitting a ... (check one with "x")

  • bug report
  • feature request

Issue description

Current behavior:
When login response of a NbPasswordAuthStrategy returns an broken JWT token, it is stored in the local storage without validation. On the next request, the app will fail because NbAuthJWTToken cannot parse the token.

Expected behavior:
A broken token should be rejected during login.

Related code:
See the exception that occurs when token from login response is invalid:

ERROR Error: The token yeah! is not valid JWT token and must consist of three parts.
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.getPayload (token.js:112)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.getTokenExpDate (token.js:139)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.isValid (token.js:160)
    at MapSubscriber.project (auth.service.js:75)
    at MapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/map.js.MapSubscriber._next (map.js:35)
    at MapSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:54)
    at RefCountSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber._next (Subscriber.js:77)
    at RefCountSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:54)
    at Subject.push../node_modules/rxjs/_esm5/internal/Subject.js.Subject.next (Subject.js:47)
    at ConnectableSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber._next (Subscriber.js:77)

This exception is shown in Console log when the App tries to use the broken Token, the app crashes:

core.js:1601 ERROR Error: Uncaught (in promise): Error: The token yeah! is not valid JWT token and must consist of three parts.
Error: The token yeah! is not valid JWT token and must consist of three parts.
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.getPayload (token.js:112)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.getTokenExpDate (token.js:139)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.isValid (token.js:160)
    at MapSubscriber.project (auth.service.js:44)
    at MapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/map.js.MapSubscriber._next (map.js:35)
    at MapSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:54)
    at Observable._subscribe (scalar.js:5)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable._trySubscribe (Observable.js:42)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable.subscribe (Observable.js:28)
    at MapOperator.push../node_modules/rxjs/_esm5/internal/operators/map.js.MapOperator.call (map.js:18)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.getPayload (token.js:112)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.getTokenExpDate (token.js:139)
    at NbAuthJWTToken.push../node_modules/@nebular/auth/services/token/token.js.NbAuthJWTToken.isValid (token.js:160)
    at MapSubscriber.project (auth.service.js:44)
    at MapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/map.js.MapSubscriber._next (map.js:35)
    at MapSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:54)
    at Observable._subscribe (scalar.js:5)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable._trySubscribe (Observable.js:42)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable.subscribe (Observable.js:28)
    at MapOperator.push../node_modules/rxjs/_esm5/internal/operators/map.js.MapOperator.call (map.js:18)
    at resolvePromise (zone.js:814)
    at resolvePromise (zone.js:771)
    at zone.js:873
    at ZoneDelegate.push../node_modules/zone.js/dist/zone.js.ZoneDelegate.invokeTask (zone.js:421)
    at Object.onInvokeTask (core.js:4062)
    at ZoneDelegate.push../node_modules/zone.js/dist/zone.js.ZoneDelegate.invokeTask (zone.js:420)
    at Zone.push../node_modules/zone.js/dist/zone.js.Zone.runTask (zone.js:188)
    at drainMicroTaskQueue (zone.js:595)
    at push../node_modules/zone.js/dist/zone.js.ZoneTask.invokeTask (zone.js:500)
    at ZoneTask.invoke (zone.js:485)

Other information:

npm, node, OS, Browser

OS: Windows 10, Chrome

Angular, Nebular

Angular 6.0, Nebular 2.0.0-rc.9
@nnixaa
Copy link
Collaborator

nnixaa commented Jun 25, 2018

Hey @schuettecarsten, good catch! Thanks for reporting.

@nnixaa
Copy link
Collaborator

nnixaa commented Jul 30, 2018

My suggestion to resolve this one would be as following:

  1. We don't fail the token when parsing it. So that if the token has incorrect JSON or any other cause for parsing to fail, we just leave the payload as empty. Empty payload will make the isValid method to return false (with an exception for NbAuthSimpleToken).

  2. Upon token creation, during the authentication (login/register/refreshToken) phase we check the token, and if it is not valid - NbAuthResult will be marked as success=false.

  3. For the cases, when we don't expect the token to be presented or valid (for example registration which requires user to login separately) we introduce a new option called requireToken which is true by default. When set to false it won't fail the authentication request and returns the NbAuthResult as it is (possible with an invalid token inside).

@schuettecarsten @alain-charles does this sound as a solution to you?

/cc @Tibing

@alain-charles
Copy link
Contributor

I think your made an excellent analysis.
I would only give the name requireValidToken in the last item instead of requireToken .
Do you want me to send a PR ?

@nnixaa
Copy link
Collaborator

nnixaa commented Jul 30, 2018

@alain-charles thanks! Agree, requireValidToken makes more sense. Let's see if this resolves @schuettecarsten's issue and then we can schedule it.

@schuettecarsten
Copy link
Author

schuettecarsten commented Jul 30, 2018

This sounds absolutely great! 👍

@alain-charles
Copy link
Contributor

I have begun to work on that.
The behavior of the framework is now not exactly the same as presented here, because we introduced the tokens created_at attribute. So, the payload is parsed at token creation, so that the app now "crashes" at login if the token is malformed.

So we have to decide what to do.
If the jwt token is malformed, i don't think we shoud accept it and store it, because it is really useless. we won't be able to use it for authenticating against backend.

If the token is malformed, maybe it is a misconfiguration of the strategy, maybe a backend problem.
I propose that instead of crashing, we send NbAuthResult with success=false, and the error description "invalid jwt token".

What do you think ?

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 1, 2018

@alain-charles yes, I completely agree.

Just thinking what is the best way to do this.
One of the options would be to re-do the createToken method so that it throws an exception if the token is not valid, so that in the following example:

    return this.http.post(url, this.buildCodeRequestData(code),
                         this.buildAuthHeader())
      .pipe(
        map((res) => {
          return new NbAuthResult(
            true,
            res,
            this.getOption('redirect.success'),
            [],
            this.getOption('defaultMessages'),
            this.createToken(res)); // here we call, it creates a token, checks if it's invalid, and if it is and `requireValidToken === true` - throws an exception new Error('invalid token ... reason ...')
        }),
        catchError((res) => this.handleResponseError(res)),
      );
  }

if the createToken method throws an exception, it will be catched by handleResponseError so that the amout of changes we need to do through all strateges is quite small. The only thing we have to make sure is the all createToken calls are handled by some catchError in case of an exception.

@alain-charles
Copy link
Contributor

I verified.
For the moment, it is almost always the case, but not in :

  • NbDummyAuthStrategy.createDummyResult()
  • NbOAuth2AuthStrategy.redirectResultHandlers

@alain-charles
Copy link
Contributor

alain-charles commented Aug 2, 2018

@nnixaa I am getting near a satisfying solution for this issue.

If you configure the token as jwt and if backend sends malformed token, you get an error in any case with a detailed message as we decided above:

If the token is malformed, maybe it is a misconfiguration of the strategy, maybe a backend problem.
I propose that instead of crashing, we send NbAuthResult with success=false, and the error description "invalid jwt token".

if the token is well formed but invalid, and if requireValidToken is set to true, you get an error 'invalid token'

However we are running into concurrent issue between this new requireValidToken flag positionned at the strategy level and the existing failWhenNoToken flag positionned at the login/register/refresh levels in NbAuthPasswordStrategy.

requireValidToken takes precedence so that if set to true, positionning failWhenNoToken to false is useless.
=> possible breaking change if we set by default requireValidToken to true

What do you think ? Do we maintain this requireValidToken flag since we decided to reject malformed token and since there is this existing failWhenNoToken flag ?

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 6, 2018

@alain-charles since we never released the failWhenNoToken flag feature we can just remove it, as requireValidToken replaces this functionality.

@nnixaa nnixaa modified the milestones: rc.10, 2.0.0 Aug 8, 2018
@nnixaa nnixaa closed this as completed in 127b5d2 Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants