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

Improve fastify.jwt.verify by either making errors easier to handle via callback, or returning a promise. #294

Open
2 tasks done
autopulated opened this issue Jun 15, 2023 · 1 comment

Comments

@autopulated
Copy link

autopulated commented Jun 15, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Since it isn't possible to await this function (as opposed to calling the synchronous version), for async execution we have to use a callback, which makes handling both the callback-returned and thrown errors ugly.

So I think, either:

  • The JWT validation errors always be returned via the callback, and never thrown if the callback is passed
  • Or, just to make .verify and .sign await-able. The existing synchronous versions makes that difficult though.
  • Or, new .verifyAsync and .signAsync that returns a promise?

Motivation

For context, since eddsa signatures are relatively expensive to verify (at least, more expensive than signing), I'm assuming that it's generally good practise not to block the event loop / use synchronous node:crypto apis when verifying them, but maybe I have a misunderstanding here.

Maybe the synchronous API should just go away, and be replaced by an awaitable version returning a promise, I suggest calling it verifyAsync to avoid confusion with the current synchronous version.

Example

// error handling for current async .verify api:
async function myRoute(request, reply) {
    return new Promise((resolve, reject) => {
        try {
            fastify.jwt.verify(request.body.token, (err, decoded) => {
                if (err) {
                    // handle user-facing errors / internal errors  here 
                } else {
                    // ...
                }
            });
        } catch (e) {
            // handle user-facing errors / internal errors also here
        }
    });
}

// proposed api:
async function myRoute2(request, reply) {
    try {
        const decoded = await fastify.jwt.verifyAsync(request.body.token);
        // ...
    } catch (e) {
        // handle user-facing errors / internal errors here
    }
}
@maslennikov
Copy link
Contributor

maslennikov commented Jun 25, 2023

I agree, there are two different interfaces (and implementations) of what is perceived to be the same functionality: fastify.jwt.verify() and response.jwtVerify(). Right now using the fastify.jwt.verify() is very bare bones and requires separate error handling for bleeding errors from encapsulated fast-jwt.

I suggest to refactor them in a way they share a common verify step implementation, returned errors, and async+callback styles of invocation, so that the only thing different would be providing a token to the former, and relying on request headers/cookies with the latter.

Relevant parts:

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

No branches or pull requests

2 participants