8
\$\begingroup\$

So I wrote a rather primitive login logic in Node.js, which authenticates the user and handles JWT. Is it any good in terms of security, efficiency, building, async/sync, logging. SECURITY is my main concern. On in a prettier format the question would be:

  • SECURITY: Is my website secure in any way, shape, or form? I'm wondering if I could implement any security measures other than the ones that are built in to the methods provided by Node.js. Also, I know the passwords are plainly obvious to guess, but they are like that to ensure logging in as different users works.
  • EFFICIENCY: Is how I'm checking usernames and password efficient? Is there any better way to do this?
  • BUILDING: Is how I loaded my website acceptable? Reading from a file and then ending the response?
  • ASYNC/SYNC: I know I preform async and sync calls at the same time. Is there any problem to this?
  • LOGGING: I log all connections to the server, and all login attempts. Is this a good practice, or am I overdoing what logging is supposed to accomplish? (Source: Login Server with Node.js)

My code is:

// main login logic
app.post('/api/login', apiLimiter, async function (req, res) {
  // TODO: implement cookie check whether a still valid token is set and if so respond with cookie already set
  // TODO: add roles into jwt and add roles checking into other code
  // TODO: if wrong password send a response telling there's a wrong password/username
  try  {
    const pw = req.body.password;
    const submittedUser = req.body.email;
    
    User.findOne({eMail: req.body.email}, async function (err, user) {
      if (err) throw err;
      console.log(user);
      const match = await bcrypt.compare(pw, user.password);
      console.log(match);
      if (match === true && user.eMail == submittedUser) {
        jwt2.sign({user}, 'secrettokenhere', { expiresIn: '15min'}, (err, token) =>{
          res.cookie(
            'access_token', 'Bearer '+ token, {
              //domain: 'localhost',
              path: '/',
              expires: new Date(Date.now() + 900000), // cookie will be removed after 15 mins
              httpOnly: true // in production also add secure: true
            })
            .json(
              user
            );
        });
      }
      else {
        res.status(200).send("Bad login");
      }
    });    
  } catch (err) {
    res.status(500).send();
    console.log(err);
  }
});

P.S. there's gonna be a follow-up separate question with Frontend logic.

\$\endgroup\$
6
\$\begingroup\$

Async login bug The try/catch around User.findOne does not accomplish anything because findOne is asynchronous. When findOne fails, it'll pass an error to the callback, but when the callback does throw err, nothing is there to catch the asynchronous error, so no response will be sent to the user. Another issue is that you aren't checking if user exists - if it doesn't, an error will be thrown when you try to access its password property. (You also aren't checking for whether .sign results in an error or not)

You could also consider using Promises instead of callbacks - findOne already returns a Promise, and Promises are often preferable because they're more easily chainable and their error handling can be cleaner.

Security looks reasonable, though:

  • Make sure connections are only permitted over HTTPS. HTTP requests are interceptable.
  • Even if the login bug above gets fixed, your current implementation is capable of informing anyone whether a given email is registered. For example, if I know your email address, I can try to login as you and examine the response to see whether the password was wrong or if no such email exists. This may not be desirable. If you wanted to fix it, have when the password doesn't match enter the same block as when the email isn't found, to make it indistinguishable to the user - you'd want to send a reply of Bad login in both cases.
  • On a similar note, when authentication fails, the appropriate status code to send is 401 (unauthorized), not 200.

Is how I'm checking usernames and password efficient?

Looks completely normal to me.

LOGGING: I log all connections to the server, and all login attempts. Is this a good practice

If one is logging, login attempts are one of the most important things to log. But console.log is not the right way to do it, at least not alone - say that some user was concerned about their logins, how would you examine their recent login attempts? Control-F-ing through the application stdout isn't a very manageable way of doing it. I'm not sure what the industry standard for this is, but you could consider saving to a logging database.

Email login You do:

const pw = req.body.password;
const submittedUser = req.body.email;

User.findOne({eMail: req.body.email}, async function (err, user) {
  if (err) throw err;
  console.log(user);
  const match = await bcrypt.compare(pw, user.password);
  console.log(match);
  if (match === true && user.eMail == submittedUser) {

You put the request email into a variable named submittedUser, which doesn't sound all that intuitive to me - better to use a variable name that indicates that it contains an email string, not a user, like email - you could destructure both at once with:

const { password, email } = req.body;

Then, later, use those variables instead of going through req.body again.

After the .findOne, there shouldn't be any need to do user.eMail == submittedUser - that check should be superflouous given the findOne call's constraint.

The body contains email, but the database contains eMail. The capitalization is different, which is an odd inconsistency which could cause typos and bugs. I'd recommend using a single property name - probably email, since that capitalization is much more common.

Rather than comparing the match against === true, you can just check if the match is truthy - or, another option, to reduce unnecessary indentation would be to throw if the match isn't truthy, and handle errors in a .catch.

Since .sign is callback-based, but you want to work with Promises, make it Promise-based with util.promisify.

Overall I'd hope to make the code look something like this:

const jwtSignPromisified = util.promisify(jwt2.sign).bind(jwt2);
app.post('/api/login', apiLimiter, async function (req, res) {
    const failLogin = () => {
        logLoginAttempt(email, false); // or something - 2nd param indicates success
        // could also pass IP address
        res.status(401).send('Bad login');
    };
    const { password, email } = req.body;
    try {
        const user = await User.findOne({ email });
        if (!user) return failLogin();
        const match = await bcrypt.compare(password, user.password);
        if (!match) return failLogin();
        logLoginAttempt(email, true);
        const token = await jwtSignPromisified({ user }, 'secrettokenhere', { expiresIn: '15min' });
        res.cookie(
            'access_token', 'Bearer ' + token, {
            //domain: 'localhost',
            path: '/',
            expires: new Date(Date.now() + 900000), // cookie will be removed after 15 mins
            httpOnly: true // in production also add secure: true
        })
            .json(user);
    } catch (error) {
        // This should not be entered under normal circumstances:
        logServerError(error);
        res.status(500).send('Unexpected server error');
    }
});

where logLoginAttempt and logServerError save to logging databases that can be examined.

\$\endgroup\$
6
  • \$\begingroup\$ There appears to be one small mistake: jwtSignPromisified.sign should be just jwtSignPromisified, otherwise an error gets thrown out. \$\endgroup\$
    – Munchkin
    Oct 22 '20 at 13:11
  • \$\begingroup\$ also in findOne I had to do eMail: email, but that might be my mistake. \$\endgroup\$
    – Munchkin
    Oct 22 '20 at 13:53
  • \$\begingroup\$ Can you change the User schema to have email be all lowercase? That's one of the things I was trying to recommend in the answer - it's better when all variables/properties referencing a particular thing use the same casing, it'll help reduce bugs due to typos. \$\endgroup\$ Oct 22 '20 at 13:55
  • \$\begingroup\$ Changed the User schema to email and now it's giving out 401 again.. And yes I did change in the auth service of the frontend from eMail to email and in the backend from eMail to email... weird... \$\endgroup\$
    – Munchkin
    Oct 22 '20 at 14:16
  • \$\begingroup\$ If a user was previously registered with the eMail schema, maybe the same user can't login via email without restructuring the database? Try registering a new user and then logging in with them, if you haven't tried that already. Just an idea \$\endgroup\$ Oct 22 '20 at 14:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.