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

README.md inappropriately references functions as asynchronous when they are in fact all synchronous with inline callback support #929

Open
kzgrey opened this issue Aug 11, 2023 · 4 comments

Comments

@kzgrey
Copy link

kzgrey commented Aug 11, 2023

Please do not report security vulnerabilities here. The Responsible Disclosure Program details the procedure for disclosing security issues.

Thank you in advance for helping us to improve this library! Please read through the template below and answer all relevant questions. Your additional work here is greatly appreciated and will help us respond as quickly as possible. For general support or usage questions, use the Auth0 Community or Auth0 Support. Finally, to avoid duplicates, please search existing Issues before submitting one here.

By submitting an Issue to this repository, you agree to the terms within the Auth0 Code of Conduct.

Description

In multiple places in the README.md file it lists the API functions as both synchronous and asynchronous depending on the input parameters for the function. For example, the verify() operation:
Link to doc

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

In reality, the function is synchronous and when a callback is provided, it is still behaves as a synchronous callback. The callback is invoked inline.

The term Asynchronous should be reserved for methods that utilize the Promise contract and the async keyword.

Reproduction

Detail the steps taken to reproduce this error, what was expected, and whether this issue can be reproduced consistently or if it is intermittent.

Where applicable, please include:

  • Code sample to reproduce the issue
  • Log files (redact/remove sensitive information)
  • Application settings (redact/remove sensitive information)
  • Screenshots

Environment

Please provide the following:

  • Version of this library used:
  • Version of the platform or framework used, if applicable:
  • Other relevant versions (language, server software, OS, browser):
  • Other modules/plugins/libraries that might be involved:
@ekwoka
Copy link

ekwoka commented Aug 30, 2023

And so many people use the "async" api when they could just use the return...

@isimmons
Copy link

Combine this with incorrect tutorials on the internet and it causes a lot of confusion and uncertainty if testing is done correctly.

Testing with Vitest:

export function generateToken(userEmail, doneFn) {
  jwt.sign({ email: userEmail }, "secret123", doneFn);
}

The test

// false positive because it's not async
it("should generate a token", () => {
    generateToken("foo@foomail.com", (err, token) => {
      expect(token).toBeDefined();
      expect(token).toBe(2);
    });
  });

// works when callback marked async
it("should generate a token", () => {
    generateToken("foo@foomail.com", async (err, token) => {
      // expect(token).toBeDefined();
      expect(token).toBe(2); // correctly fails when marked async
    });
  });

// Also works when return expect but not marked async
it("should generate a token", () => {
    generateToken("foo@foomail.com",  (err, token) => {
      // expect(token).toBeDefined();
      return expect(token).toBe(2); // correctly fails
    });
  });

As @ekwoka says, using the return works but is this intended behavior or should we be explicitly marking this as async or is it supposed to be working without marking async as the readme shows?

@ekwoka
Copy link

ekwoka commented Feb 14, 2024

Oh see what I meant was just using the return of jwt.sign

const token = jwt.sign(args)
expect(token).toBe(2)

Absolutely no reason to pass a callback at all.

@isimmons
Copy link

isimmons commented Feb 14, 2024

Ah, thank you. This sure simplifies things :-)

export function generateTokenNoCallback(userEmail) {
  return jwt.sign({ email: userEmail }, "secret123");
}

// test
describe("generateTokenNoCallback", () => {
  it("should return a token", () => {
    expect(generateTokenNoCallback("foo@foomail.com")).toBeDefined(); // correctly passes
    expect(generateTokenNoCallback("foo@foomail.com")).toBe(2); // correctly fails
  });
});

UPDATE: My 3rd option above didn't work anyway. Simply adding return to the callback worked once and then didn't work a second time. Apparently I got lucky on the timing the first time which made it look like "return" was doing something. So if using callback then it needs to be marked async but no callback at all seems to be working too.

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

3 participants