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

Raise jws.decode error to avoid confusion with "invalid token" error #294

Merged
merged 4 commits into from
Feb 9, 2017

Conversation

evolvah
Copy link
Contributor

@evolvah evolvah commented Jan 11, 2017

jws.decode() never throws an error. At least, in its current version. However, if it were to throw an exception, the diagnostics would be indistinguishable from a soft failure to decode a token. I had an extra trailing space on my JWT and it took me some additional debugging work to trace the actual root cause because the error message was not distinct.

jws.decode() never throws an error. At least, in its current version. However, if it were to throw an exception, the diagnostics would be indistinguishable from a soft failure to decode a token. I had an extra trailing space on my JWT and it took me some additional debugging work to trace the actual root cause because the error message was not distinct.
@ziluvatar
Copy link
Contributor

Hi @evolvah ! What do you think to simply forward the unexpected error? We do that in other places for calling jws, at least we unify the way we treat them and we remove your concern as well.

If that's ok for you, can you modify your commit/PR message as well to reflect the new change?

…aller. Currently, jws.decode never throws an exception. The change is made per discsussion in the original PR
@evolvah
Copy link
Contributor Author

evolvah commented Feb 8, 2017

That's a good idea! Please see the latest update to this PR, @ziluvatar.

@evolvah
Copy link
Contributor Author

evolvah commented Feb 8, 2017

I do like PRs with negative net LOC count :)

@ziluvatar
Copy link
Contributor

Me too!!! But it seems I expressed my suggestion badly :(
I meant to just forward the error:

  var decodedToken;
  try {
    decodedToken = jws.decode(jwtString);
  } catch(err) {
    return done(err);
  }

It is not negative net LOC count but some letters are removed ;)

@evolvah
Copy link
Contributor Author

evolvah commented Feb 8, 2017

Ah, I see now! Let me make the change and add a test case. Once I'm done, I will update the PR.

@evolvah
Copy link
Contributor Author

evolvah commented Feb 8, 2017

Done! With a new test case being added.

@@ -77,6 +77,21 @@ describe('HS256', function() {
done();
});
});
});

describe('should fail verification graceflly with trailing space in the jwt', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, gracefully
Thanks for the new test :)
Modify that and I'll merge.

@evolvah
Copy link
Contributor Author

evolvah commented Feb 9, 2017

Bummer! Fixed the typo, updated the PR. Ready to merge.

@ziluvatar ziluvatar changed the title Corrected indistinguishable error messages Raise jws.decode error to avoid confusion with "invalid token" error Feb 9, 2017
@ziluvatar ziluvatar merged commit 7f68fe0 into auth0:master Feb 9, 2017
@ziluvatar
Copy link
Contributor

Thanks @evolvah 👏 , I modified the PR description to make more explicit the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants