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]

👍

[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 Sign callback function is called with successful result as a first argument #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.

[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.

[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 #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: #111 (comment)

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

[Guriqbal-Singh-Alida]

If you're using an async getKey method to retrieve your public key externally, you can do so before calling verify as Panva said: #111 (comment)

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);

Given verify return decoded data, There seems to be overhead to decode same token twice.

[Tilogorn]

Whats the state of this? The docs still say If a callback is supplied, function acts asynchronously. Can I, as package consumer, be sure that a call to verify(token, key, function() {}) is non-blocking?

Because that is what I assumed until I saw this issue here.

[pi0neerpat]

More documentation here is needed. Thanks @AcBerk for providing the example above

[ajmas]

I'd argue we should probably see functions clearly broken down into their asynchronous and synchronous forms. If a function has a callback, then it should be treated as asynchronous, and thus candidate for async/await, otherwise it should be treated as synchronous by default.

The verify() function appears to be one function that is contentious (based on this thread) since it is both async and sync, depending on the parameters passed. I am wondering whether it may just be worth in a future version having the basic verify() function that does not accept a function for the key parameter and a verifyAsync() (or something equivalent) which does support a callback or promise?

BTW the current readme has this to say on this specific function:

(Asynchronous) If a callback is supplied, function acts asynchronously. The callback is called with the decoded payload if the signature is valid and optional expiration, audience, or issuer are valid. If not, it will be called with the error.

(Synchronous) If a callback is not supplied, function acts synchronously. Returns the payload decoded if the signature is valid and optional expiration, audience, or issuer are valid. If not, it will throw the error.

Issue #511 also touches on whether verify() is really async, but we need to consider the mode of operation

[christopheblin]

maybe you could deprecate jwt.verify and create 2 distinct methods with explicit contracts

• verifySync(token: string, key: string | (jwt:Jwt) => string)
• verifyAsync(token: string, key: (jwt: Jwt) => Promise<string>)

I think this is non breaking and it becomes obvious which one to call in your code (and which one is going to introduce a real Promise)

const key = "xxx";
const decoded = jwt.verifySync(token, key)

const keys = { "1234" : "xxx", "5678": "zzz" };
const decoded = jwt.verifySync(token, (jwt) => keys[jwt.header.kid]); //this is currently NOT working with jwt.verify

//surely, no one would ever use like that without a cache ...
const decoded = await jwt.verifyAsync(token, (jwt) => http.get(`https://authserver/keys/${jwt.header.kid}`)

in my personal use case, I'd like the following

async checkJwt(token: string): Promise<MyUser> {
  if (! this.signingKeys) {
    this.signingKeys = await new JwksClient({ jwksUri }).getSigningKeys();
  }
  return jwt.verifySync(token, (jwt) => this.signingKeys.find(k => k.kid === jwt.header.kid)) as MyUser
}

[hungdao-testing]

I am just the same considering as the comment from @Tilogorn

Would anyone share your point of view

[fires3as0n]

Why can't verify function just return a falsy value in case of an incorrect token? Isn't throwing an error inside a normal application flow an antipattern? Could we imagine if native array .find() method was throwing instead of returning undefined.

[jimmywarting]

haven't read everything.
just skimmed some in the beginning.

Some say that crypto is sync, so that there is no need for any callback or promises.
but i would say that it would actually be worth having a async impl either way.

that is if we are able to one day switch away from using node Buffer and replacing it with Uint8Array instead
and also using web crypto instead of node:crypto that is more cross env friendlier. so that it one day could also run in Deno, Bun, and even browsers without any additional dependencies