Return a Promise

[xpepermint]

Methods should return a Promise.

[akohout]

+1

[omsmith]

Everything that occurs during signing/verifying/decoding is completely synchronous. What benefit would a promise provide?

[junosuarez]

Verify currently offers a synchronous operation or an optional callback (which simply defers evaluation to a future turn of the event loop). It's unclear to me why the callback version exists. I also think it's an antipattern to overload the same method to behave both synchronously and asynchronously depending on the arguments.

[xpepermint]

What I wanted to say is that verify should return a Promise :).

[omsmith]

Sure, though as @jden pointed out the callback version of verify is essentially useless. The whole operation is synchronous. The callback, or a Promise, seem to be strictly detrimental in this case.

[xpepermint]

Well... as I can see the sync version of verify will throw an error which means you have to put it inside the try/catch block. I prefer using promises instead of try/catch block because ~70% of my code is async (promises). If I could do verify.then(()=>something) it would look more elegant.

@omsmith I agree that the callback is useless and I would be more pleased if the sync version would just return null in case of an error (but you have different errors). I don't know what's best...

[akohout]

As already pointed by @xpepermint, using promises would be more consistent with the rest of the code.

[junosuarez]

Could any of the maintainers comment about whether the intention is to refactor the implementation of .verify to use async versions of crypto functions?

I'm still +1 to separating async/sync into separate functions (as node core does, e.g. in fs), and +0 on callbacks vs promises

[omsmith]

While streaming would be possible, there is no async sign/verify in crypto

[junosuarez]

@omsmith good point, and at the size that JWTs are, streaming is unnecessary overhead. My vote would be to remove the async function signature then.

[dschenkelman]

Hey everyone,

There are different things here.

• Returning a promise - I don't think we would want to do that. If we accept callbacks anyone can wrap that in a promise, either using new Promise(function(...){} or something like bluebird's promisifyAll. We like promises (and use them a lot internally) but we don't think it is a good idea to expose them in APIs when they can be so easily wrapped. Some libs also both return a promise and a callback, but that leads to being tied to an implementation of promises which is not always desired.
• Separating into sync/async functions - Crypto functions in jws are not async, so as it was mentioned streaming could be used but it does not make a lot of sense based on the payloads.

CPU intensive async calls (crypto) make use of libuv's event loop to simulate asynchrony (they are of course working in another thread). We don't have the option to simulate that here. The best choice would probably be to get rid of the callbacks.

As of now it makes no sense to do so. Once we update to another major we can probably make this change.

[kara-ryli]

I'd like to point out that there's a difference between async and not throwing. The Node pattern of using a callback like (err, value) => {} is nice for anything that can throw because libraries like async.js can handle it, and libraries like Bluebird can promisify it.

So while I'm -1 on returning a promise, I'm also -1 on throwing.

[joepie91]

Some libs also both return a promise and a callback, but that leads to being tied to an implementation of promises which is not always desired.

That's not really true, though - the Promises/A+ spec exists exactly to make this not the case. You can simply return the promise from a handler provided by whichever promises library you're using, and then handle it as normal in your implementation of choice. The only way it would be "tied to" an implementation, is in that it is a dependency.

The argument that "it can be promisified" normally misses half the point of promises - one of the things that promises give you is reliable error handling, by making it hard to 'forget' to handle or propagate an error. Using promises internally in a library therefore greatly increases the reliability of said library, compared to using nodebacks (ie. the async (err, result) callbacks). Even if the end user uses the nodeback interface, they would still get the benefits of internal library reliability, as the API conversion only happens on the exposed methods.

All that being said, if the library isn't operating asynchronously to begin with, there's no point to having either a callback or a promise. I'd even say that it's misleading, as it isn't actually asynchronous. Throwing in a fully synchronous method is perfectly okay, by the way - promise libraries will automatically convert this into a rejection, and when not using promises, they can be handled using a regular try/catch.

[ide]

Chiming in here -- the most compelling reason to return Promises is for interoperability with async functions. They are the lingua franca of asynchrony in JS.

try {
  let payload = await jwt.verify(token, secret, { issuer: 'https://example.com' });
} catch (e) {
  ...
}

But if verify is internally synchronous, then returning a Promise is mostly extra overhead and asynchrony.

edit: the jws library appears to use streams, so there may actually be some small benefit to the asynchronous API

[sibelius]

:+1:

[felixfbecker]

All that being said, if the library isn't operating asynchronously to begin with, there's no point to having either a callback or a promise. I'd even say that it's misleading, as it isn't actually asynchronous. Throwing in a fully synchronous method is perfectly okay, by the way - promise libraries will automatically convert this into a rejection, and when not using promises, they can be handled using a regular try / catch .

this.

[Flaise]

All that being said, if the library isn't operating asynchronously to begin with, there's no point to having either a callback or a promise. I'd even say that it's misleading, as it isn't actually asynchronous.

I had to go digging through the source code and issue tracker in order to make sure there were no blocking operations that make the non-callback version unsuitable for production use.

[kara-ryli]

tl;dr Promises are good, but jsonwebtoken doesn't need them.

So I previously commented that I'm -1 on returning a Promise, but I've come around on promises as the ideal method of flow control for things that have potential error states. Compare the following code:

// try/catch
(function () {
  let result;
  try {
    result = jwt.verify('....token', secret);
  } catch (e) {
    if (err instance of jwt.JsonWebTokenError) {
      // ...something
    } else if (err instance of jwt.NotBeforeError) {
      // ...something else
    } else if (err instance of jwt.TokenExpiredError) {
      // ... a third thing
    } else {
      // assume the Error interface 
    }
  }
  // result should be ok
}());

// via nodebacks
jwt.verify('....token', secret, function (err, result) {
  if (err instance of jwt.JsonWebTokenError) {
    // ...something
  } else if (err instance of jwt.NotBeforeError) {
    // ...something else
  } else if (err instance of jwt.TokenExpiredError) {
    // ... a third thing
  } else if (err instance of Error) {
    // assume the Error interface 
  } else if (err) {
    // can this even happen?
  } else {
    // result should be ok
  }
});

// via promises
Bluebird.try(() => jwt.verify('....token', secret)).
  then(result => { /* result should be ok */ }).
  catch(jwt.JsonWebTokenError, err => { /* something */ }).
  catch(jwt.NotBeforeError, err => { /*something else */ }).
  catch(jwt.TokenExpiredError, err => { /* a third thing */ }).
  catch(err => { /* definitely an error */ });

Personally, I prefer the Promise interface. Also, the standard try/catch interface is an optimization killer.

The good news is that promisifying jsonwebtoken's interface is so easy that it feels out of scope for this library:

const jwt = require('jsonwebtoken');
const promisify = require('bluebird').promisify;
exports.JsonWebTokenError = jwt.JsonWebTokenError;
exports.NotBeforeError = jwt.NotBeforeError;
exports.TokenExpiredError = jwt.TokenExpiredError;
exports.decode = promisify(jwt.decode);
exports.sign = promisify(jwt.sign);
exports.verify = promisfy(jwt.verify);

[joepie91]

@Ry7n To address your points:

1. Yes, Bluebird will let you more easily and reliably catch specific types of errors, and this is a good thing. However, the library doesn't need to return Promises for this - it can simply throw synchronously, and that throw will be caught by Bluebird and repropagated as a rejection (and thus catchable using error filters/predicates).
2. Yes, it is now relatively simple to promisify the library (since #169), but that's not the point. Promisification is useful as a compatibility mechanism to deal with legacy modules, but it shouldn't be seen as a permanent measure; it is still an extra step to take with possible function name confusion being introduced (when using promisifyAll) or unnecessary extra duplicated code to maintain (when using promisify like you suggested), and the canonical representation of asynchronous operations should be a Promise out of the box.
3. This entire discussion is moot if the library isn't doing anything asynchronous anyway, and the entire asynchronous API (whether using callbacks or Promises) should just be deprecated and replaced with a fully synchronous API.

[kara-ryli]

@joepie91 I don't think we're disagreeing; neither of us think JSONWebToken should return a promise.

[joepie91]

@Ry7n Right, but it seems to be for a different reason. Hence attempting to clarify, since the callback API should probably be deprecated as well :)

[frogcjn]

+1, any progress?

[felixfbecker]

This issue should be renamed to "deprecate callback API"

[junosuarez]

or "remove async API and bump major version"

[calebmer]

Could this please be clarified in the README or updated as a breaking change? As I understand it you should really only ever use the synchronous version so having an asynchronous API is not only an anti-pattern, but also frustrating to all new developers trying to use promises.

As fs in Node.js core has taught the community, if an asynchronous version is available always use the asynchronous version. I’d be surprised if anyone who hasn’t seen this issue is using the synchronous version in production.

[leebenson]

@felixfbecker, good to see you here :-)

I totally agree with @calebmer that this should be made clear in the README. As a new jsonwebtoken user, the 'not really async' thing threw me off. Having a callback suggests there's some kind of IO penalty for synchronous execution. If it's all the same, a callback a moot - and only serves to confuse new users.

[marcushultman]

Ended up doing new Promise(resolve => resolve(jwt.verify(...))), works well enough.

[leebenson]

@marcushultman, while that works, a Promise is just as moot as a callback. jwt.verify() is a synchronous operation, so procedural code that immediately follows it is guaranteed to execute after the token has been verified. A Promise or a callback really isn't needed.

[marcushultman]

Well, since I'm doing most control flows with promises, I'll still end up using promises, whether it's inside the callback or not. I simply want to avoid any type of callback hell, and using a delegate function is tedious, so I'm using promises for synchronous operations as well as for object transformations. :) Anyway, 👎 for returning promises from jwt, so I just wanted to share how I did it, if anyone wants to do the same. :)

[leebenson]

Understood @marcushultman, but I want to make it clear to anyone reading that neither promises nor callbacks are required for JWT tokens to verify. It was a mistake to ever include a callback in this API, because it gives the impression that verification is an expensive operation that may return a value at an indeterminate point in the future -- when instead, it actually returns immediately.

i.e.

const payload = jwt.verify(/* options */);
// 'payload' is guaranteed to contain the value when this line is executed

Even if you're using Promises in your own control flow, there's never any need to call jwt.verify asynchronously. You're instantiating and tearing down an unnecessary promise object that is literally doing nothing other than running synchronous code. It makes as much sense as writing const ten = new Promise(resolve => resolve(10)) instead of plain scalar vals.

IMO, the async 'version' (I use that term loosely- they're both really synchronous) should be removed and the readme updated. There's zero benefit to introducing any type of code flow to a synchronous op.

[ide]

Would anyone want to rewrite jwt.verify to use the streams-based API of the underlying jws library and profile it? The streams-based API, if faster, could be a good reason to change jwt.verify to be an async function.

My intuition is that the async API is slower than the sync one because JWTs are so small and the overhead of waiting for the end of the event loop & creating intermediate promises is significant. We're not waiting on a lot of I/O anyway -> I expect the finding to be that using the jws sync API is faster than its streams API (for JWTs) and therefore we should make jwt.verify sync and call it a day. But it'd be nice to have some benchmarks to point at whenever this question gets asked again.

[mkvlrn]

Using this right now, works really well.

[charlesr1971]

Does this library handle JWE? And, if not, are there any future plans to do so? This will allow encrypted payloads to be handled seamlessly...

[MaffooBristol]

Promisify is also now built into node, so it's even easier:

const { promisify } = require('util');
const jwt = require('jsonwebtoken');

const token = await promisify(jwt.sign)({ key, uid }, secret);

[leebenson]

right, but the point is this isn't really async, so it shouldn't need to be wrapped in anything.

[MaffooBristol]

Yeah I didn't really read far enough through the thread, it's synchronous whatever way so it's all a bit pointless, gotcha.

[shaxbee]

jwt.verify can be async, since getKey might call out external resource asynchronously, eg jwks.

[joepie91]

Does this library handle JWE? And, if not, are there any future plans to do so? This will allow encrypted payloads to be handled seamlessly...

I would recommend looking at PASETO instead, given how many security/design issues there are with the JOSE specifications in general, and JWE in particular. PASETO is also much simpler to use correctly, and has pretty widespread language support, including for JS.

[jfbenckhuijsen]

Weighing in on this, would be nice if we could actually have a version which is implemented with promises internally. I was trying to use Google Cloud KMS instead of the node provided crypto libraries to handle signing (and verification), without the need to write all the JWT stuff myself. Given these interactions are inherently async, this seems currently not possible.

[khaledosman]

I don't get why this is such a big deal? Why do sign and verify accept callbacks? if they're synchronous I'd expect the callbacks to be removed, but that didn't happen in https://github.com/auth0/node-jsonwebtoken/issues/246 since "they can be async", "can by async" is a good enough reason to use Promises in my book. you can always use Promise.resolve() for sync values.

Promises is the standard way for doing async stuff nodejs nowadays, if you don't want to break the current sync API that it returns an unnecessary promise, at least a .promise() function similar to what aws sdk does would be beneficial.

I don't think people use bluebird anymore nor want to install an external library just for that now that ES6 Promises are part of the language.. callbacks are seen as an anti-pattern and its weird having them when the rest of the consuming codebase is using Promises.. wrapping the function with a Promise or using promisify are all workarounds and forcing the consumer of the library write unnecessary verbose code when it can easily be done from the library similar to almost every other NodeJS library as the developer would expect.

[panva]

What is the point of exposing either callback or Promise anyway i ask?

Node's crypto async (for the JWS relevant crypto ops) comes from being able to stream the signed / verified data, which isn't the case here. There's literally 0 benefit from using the callback and there would also be 0 benefit in exposing a Promise.

The only thing, that i'd see as a quirk rather than a feature is getKey being callback based, so the whole thing then needs to be a callback. That however can be achieved OOB with decode and then passing the key down to a synchronous verify.

Bottom line, callbacks should have never been included in the first place and exposing Promise now is just wasted effort. If you came here asking for Promise just because you don't like callbacks - use the sync interface instead, if you need promise because you don't like try/catch - wrap it.

@ziluvatar how about we just remove the mention of callbacks from the docs.

---

edit: Node's crypto module now supports true non-blocking and async sign/verify which we could use should we want to revisit this topic. https://github.com/panva/jose makes use of those APIs.

[jiri-vyc]

+1 for removing the callback API altogether and bump major version OR* support Promises, even though they're just useless in this case, but at least consistent with

A) current "async" callback API (quotation marks important)

B) current way of writing and error handling in asynchronous Node code:

try {
  let foo = await bar();
  let payload = await jwt.verify(token, secret, { issuer: 'https://example.com' });
} catch (e) {
  ...
}

The current way is just neither here nor there and it's very confusing. Proof: this issue discussion.

And requiring a bluebird or promisify nowadays is just... not right.

* XOR

[PorterK]

@jiri-vyc That code will execute and work exactly as expected if you just remove await from before jwt.verify... and it would look clean/normal IMO

+1 to remove the callback API which serves to confuse people into thinking that the methods of this library are async.

I say wrapping any method here in a promise is not a great idea, it's just a waste of time just to conform to a pattern that is phasing out (with async/await phasing in).

Sync interface is the only way to go here unless things become actually async, which I can't really see happening after reading this discussion.

[alexcberk]

If you're using an async getKey method to retrieve your public key externally, you can do so before calling verify as Panva said: https://github.com/auth0/node-jsonwebtoken/issues/111#issuecomment-516350938

I didn't see a good example, so here's what I ended up with.

pseudocode-only

import { verify as jwtVerify, decode as jwtDecode } from 'jsonwebtoken';
import * as JwksClient from 'jwks-rsa';
import { promisify } from 'util';

const jwkClient = JwksClient();

const getKey = async(header) => {
  const getPubKey = promisify(jwkClient.getSigningKey);
  const key = await getPubKey(header.kid);
  const pubKey = key.getPublicKey();

  return pubKey;
}

const tokenHeader = jwtDecode(token, {complete: true}).header;
const pubKey = await getKey(tokenHeader);
const decoded = jwtVerify(token, pubKey);

[saalihou]

I was confused by this as well. I almost blindly just promisified the callback thinking "if there's a callback version, there must be some benefit such as offloading the signing/verification to another thread". Any reason to not clarify this in the README ? I'm happy to do a PR.

[mi-na-bot]

Using promisify is not a solution. It adds another dependency for something absurdly trivial [promisify is included in node.js] and adds to the cognitive load for developers, requiring them to verify promisify works correctly.

To the point that callbacks are pointless, they are not. Verify accepts an asynchronous (not async) function, apparently for convenient usage of JWKS.

As noted above the key can be decoded once to extract the kid synchronously to obtain a key before running verify synchronously. It works reasonably well, but if this is the suggested way of using JWKS, the paradigm of passing a function for key should be marked depreciated.

Being hosted by Auth0, it is perplexing that this should be an awkward experience, they basically require JWKS.

[alexcberk]

@sever1an - To clarify, my solution isn't using promisify with the jsonwebtoken package. I'm using it to get my key with cleaner syntax using a separate library (jwks-rsa) to make the network call.

Agreed, the callback function signature of verify should be deprecated.

promisify is included with Node.js. I think saying it adds another dependency can be misleading.

[mi-na-bot]

@acberk - My deception was from a place of ignorance. How did I never know? Thank you for the free node.js lesson. 👍

The solution of wrapping jwks-rsa is what I finally did, per your suggestion. Aesthetically I liked the idea of never handling an unverified token decode using the callback style api, but it is not worth it to include more confusion with the jsonwebtoken library.

[jmholzinger]

I personally would not remove the "asynchronous" version of verify with haste.

The RFC is not completely implemented, which is incidentally how I came across this issue. I'm looking to perform X.509 certificate chain validation, which unfortunately is not supported by this library yet. I plan to use the "x5c" header, which likely would not cause issues if verify was made completely synchronous, but I imagine the "x5u" header would not fair as well. I suppose that requests for the certificate chain could be performed OOB, similar to the proposal of getKey in previous comments, but that feels awkward in my opinion. The certificate chain is part of the token and therefor should be part of the verify process. Requiring a separate step for certificate verification sounds like a bad user experience and would likely be prone to error.

That said, even if the decision was made to make certificate validation a separate process, I would expect it to be asynchronous (at least for "x5u"), and I would expect it to support promises natively. Sure, you can force everyone to manually wrap everything they plan to use, but why? You could manually wrap the fs methods, yet fs.promises exists.

[Jolg42]

It looks to me like jose is a good alternative here and is already quite popular. https://www.npmjs.com/package/jose https://github.com/panva/jose

What is new in v3.x?

    Revised API
    No dependencies
    Browser support (using Web Cryptography API)
    Promise-based API
    experimental (non-blocking 🎉) Node.js libuv thread pool based runtime

How is it different from jws, jwa or jsonwebtoken?

    it supports browser runtime
    it supports encrypted JWTs (i.e. in JWE format)
    supports secp256k1, Ed25519, Ed448, X25519, and X448
    it supports JWK Key Format for all four key types (oct, RSA, EC and OKP)
    it is exclusively using native platform Key object representations (CryptoKey and KeyObject)
    there is JSON Web Encryption support
    it supports the flattened JWS / JWE Serialization Syntaxes
    it supports the "crit" member validations to make sure extensions are handled correctly