Asynchronous Handling Issue in API Key Hashing Middleware

[Swarnendu0123]

What is your operating system?

Windows

Actual Behavior

In the user model, asynchronously hashes API keys within a loop during the checkApiKey middleware. However, due to the asynchronous nature of the bcrypt operations, the next() function may be called multiple times before all API keys are hashed. This can lead to unexpected behavior, such as calling next() prematurely or multiple times.

Implementation Now:

userSchema.pre('save', function checkApiKey(next) {
  // eslint-disable-line consistent-return
  const user = this;
  if (!user.isModified('apiKeys')) {
    next();
    return;
  }
  let hasNew = false;
  user.apiKeys.forEach((k) => {
    if (k.isNew) {
      hasNew = true;
      bcrypt.genSalt(10, (err, salt) => {
        // eslint-disable-line consistent-return
        if (err) {
          next(err);
          return;
        }
        bcrypt.hash(k.hashedKey, salt, (innerErr, hash) => {
          if (innerErr) {
            next(innerErr);
            return;
          }
          k.hashedKey = hash;
          next();
        });
      });
    }
  });
  if (!hasNew) next();
});

Expected Behavior

The checkApiKey middleware should correctly hash API keys using bcrypt and call next() only after all API keys have been hashed. This ensures that the middleware behaves as intended, handling each API key properly and advancing to the next middleware in the chain only when all asynchronous operations are complete.

Correct implementation:

userSchema.pre('save', function checkApiKey(next) {
  const user = this;
  if (!user.isModified('apiKeys')) {
    next();
    return;
  }
  let hasNew = false;
  let pendingTasks = 0;

  user.apiKeys.forEach((k) => {
    if (k.isNew) {
      hasNew = true;
      pendingTasks++;
      bcrypt.genSalt(10, (err, salt) => {
        if (err) {
          next(err);
          return;
        }
        bcrypt.hash(k.hashedKey, salt, (innerErr, hash) => {
          if (innerErr) {
            next(innerErr);
            return;
          }
          k.hashedKey = hash;
          pendingTasks--;
          if (pendingTasks === 0) {
            next();
          }
        });
      });
    }
  });

  if (!hasNew) {
    next();
  }
});

[Swarnendu0123]

@raclim @lindapaiste give me some feedback I have almost made the changes, should any improvement is needed? or I should raise a PR?

[lindapaiste]

Thanks for finding this! I wonder if it's possible to use the hash() function with promises rather than callbacks? Then we could just await Promise.all the array. Feel free to look into the bcypt docs and to raise a PR. This is a low priority though because the entire API key feature has never actually been released.

[lindapaiste]

https://www.npmjs.com/package/bcryptjs#gensaltrounds-seed_length-callback

I'm picturing something along the lines of

Promise.all(user.apiKeys.map(async (k) => {
    if (k.isNew) {
      const salt = await bcrypt.genSalt(10);
      const hash = await bcrypt.hash(k.hashedKey, salt);
      k.hashedKey = hash;
  }
}))
  .then(() => next())
  .catch((err) => next(err));

BTW it's possible that the scenario you are describing wouldn't come up if there can only be one new API key at a time, but I don't know if that's true. And I agree with your sentiment that we shouldn't have code that could potentially behave in unexpected ways.

Now I'm wondering where/when we set the k.isNew flag to false. But I'm resisting the urge to fall down that rabbit hole.

[Swarnendu0123]

Cool! I have almost figured out the implement :)

Now I'm wondering where/when we set the k.isNew flag to false. But I'm resisting the urge to fall down that rabbit hole.

As for the concern about setting the isNew flag to false, it's important to reset this flag after hashing each API key to prevent rehashing it unnecessarily in subsequent save operations. With the provided code, the isNew flag is set to false within the map function after hashing each API key. This ensures that each new API key is hashed only once, even if the save operation is called multiple times.

userSchema.pre('save', async function checkApiKey(next) {
  const user = this;
  if (!user.isModified('apiKeys')) {
    next();
    return;
  }

  try {
    await Promise.all(user.apiKeys.map(async (k) => {
      if (k.isNew) {
        const salt = await bcrypt.genSalt(10);
        const hash = await bcrypt.hash(k.hashedKey, salt);
        k.hashedKey = hash;
        k.isNew = false; // Set isNew flag to false after hashing
      }
    }));

    next(); // Call next if all operations are successful
  } catch (err) {
    next(err); // Pass any error to the next middleware
  }
});

This implementation maintains the flow of the middleware and ensures that the next() function is called appropriately based on the completion or failure of the asynchronous operations.

[lindapaiste]

Now I'm wondering where/when we set the k.isNew flag to false. But I'm resisting the urge to fall down that rabbit hole.

As for the concern about setting the isNew flag to false, it's important to reset this flag after hashing each API key to prevent rehashing it unnecessarily in subsequent save operations. With the provided code, the isNew flag is set to false within the map function after hashing each API key. This ensures that each new API key is hashed only once, even if the save operation is called multiple times.

I looked into it a bit more. I couldn't find anywhere else that we are setting or using the isNew flag. It turns out that it's an automatic property of the Mongoose Document object: https://mongoosejs.com/docs/api/document.html#Document.prototype.$isNew The API key would be a "subdocument" of the User object. So I think that we do not need to set it to false anywhere, and Mongoose does that for us during the save operation.

[Swarnendu0123]

Yes, Mongoose automatically handles the isNew flag for documents during the save operation. When you create a new document instance, Mongoose sets the isNew flag to true. After you save the document to the database for the first time, Mongoose updates the isNew flag to false.

Let me just modify my PR.