1
\$\begingroup\$

I am in the process of writing a simple Node.JS webserver. I am about halfway there in terms of functionality. Please see the full code in this Pastebin (the client-side code is not shown as I want to focus on server-side).

Before I implement the rest of the functionality, I would like to pause and reflect and the quality of my code, regarding (quoting points from the FAC):

  • Best practices and design pattern usage
  • Security issues

I am not a Node.JS expert, and I am sure that issues with my code will stick out straight away. I have decided to keep all my code in just one file because the server is so short. Regarding security, I have setup a Node HTTPS server, and used the session management based on cookies from Express.JS.

// TODO: Close Mongo connections upon exit
var express = require('express');
var mongoose = require('mongoose');
var fs = require('fs');
var https = require('https');
var shellCommand = require('child_process').exec;

var privateKey = fs.readFileSync('./../ssl/localhost.key').toString();
var certificate = fs.readFileSync('./../ssl/localhost.crt').toString();

// Instantiate express
var server = express()
    .use(express.static('./../'))
    .use(express.cookieParser())
    .use(express.bodyParser())
    .use(express.session({secret: 'Secret!'}))
    .set('views', './../');

https.createServer({
    key: privateKey,
    cert: certificate
}, server).listen(80, 'localhost');

// Connect to the database
mongoose.connect('localhost', 'users');

// Define our model
var User = mongoose.model('Users',
    mongoose.Schema({
        username: 'string',
        password: 'string',
        rights: 'string'
    })
);

// Clear the database
User.remove({}, function () {});

// Add admin
new User({
    username: 'admin',
    password: 'admin',
    rights: 'Administrator'
}).save();

new User({
    username: 'Steve',
    password: 'test',
    rights: 'Administrator'
}).save();

new User({
    username: 'Justin',
    password: 'test',
    rights: 'Operator'
}).save();

server.get('/usersList', function(req, res) {
    User.find({}, null, {sort: {username: 1}}, function (err, users) {
        res.send(users);
    });
});

server.get('/protocols', function(req, res) {
    var response = {};

    shellCommand('tdcli proto list  | grep -v dpi_', function (err, stdout, stderr) {
        var lines = stdout.split('\n');

        for(var i = 2; i < lines.length; i += 1) {
            var line = lines[i];
            var name = line.split(/\W+/)[1];
            var status = line.match(/(enabled|disabled)/)[0];

            response[name] = status;
        }

        res.send(response);
    });
});

server.get('/statistics', function(req, res) {
    var response = {};

    for(var i = 0; i < 3; i += 1) {
        response[i] = 0.25 + 1 / 2 * Math.random();
    }

    shellCommand('top -b -n 1', function (err, stdout, stderr) {
        var lines = stdout.split('\n');
        var line;
        var elements;
        var memory;
        var cpu;

        for(i = 0; i < lines.length; i += 1) {
            line = lines[i];
            elements = line.split(/\s+/);

            if(elements[0] == 'Mem:') {
                memory = +(elements[3].slice(0, -1));
            }

            if(elements[0] == 'Cpu(s):') {
                cpu = +((100 - +elements[4].slice(0, -4)).toFixed(1));
            }
        }

        response[3] = cpu;
        response[4] = memory;

        res.send(response);
    });
});

server.post('/login', function(req, res) {
    var receivedUsername = req.body.username;
    var receivedPassword = req.body.password;

    User.find({
        username: receivedUsername
    }, function (err, users) {
        if(printError(err)) return;

        var user = users[0];

        if(!user) {
            console.error('No user', receivedUsername);
            return;
        }

        var correctPassword = user.password;

        if(receivedPassword === correctPassword) {
            req.session.username = user.username;
            req.session.rights = user.rights;

            res.send({
                message: 'Valid'
            });
        } else {
            res.send({
                message: 'Invalid',
                correctPassword: correctPassword
            });
        }
    });
});

server.post('/logout', function(req, res) {
    req.session.username = undefined;
    req.session.rights = undefined;

    sendOK(res);
});

server.post('/newUser', function (req, res) {
    if (req.session.rights === 'Administrator') {
        var receivedUsername = req.body.username;

        User.find({
            username: receivedUsername
        }, function (err, users) {
            if(users.length !== 0) {
                res.send({
                    message: 'Error: User exists!'
                });
            } else {
                new User(req.body).save(function (err) {
                    if(printError(err)) return;
                });

                res.send({
                    message: 'OK'
                });
            }
        });
    } else {
        res.send({
            message: 'Error: Permission denied'
        });
    }
});

server.post('/removeUser', function (req, res) {
    var receivedUsername = req.body.username;

    User
        .find({username: receivedUsername})
        .remove(function (err) {
            if(printError(err)) {
                sendError(res);
            }
        });

    sendOK(res);
});

server.post('/editUser', function (req, res) {
    var oldUsername = req.body.oldUsername;
    var newUser = req.body.user;

    User.update({username: oldUsername}, {
        username: newUser.username,
        password: newUser.password,
        rights: newUser.rights
    }, function(err, numberAffected, rawResponse) {
        if(printError(err)) {
            sendError(res);
        }
    });

    sendOK(res);
});

function sendOK(res) {
    res.send({
        message: 'OK'
    });
}

function sendError(res) {
    res.send({
        message: 'Error'
    });
}

function printError(err) {
    if(err) {
        console.error('ERROR!');
    }

    return err;
}
\$\endgroup\$
2
\$\begingroup\$

First of all you need to separate your code into multiple files. Usually its something like models/controllers etc. I know it is very useful for copy pasting it to places like this but it will not do for the long run.

Even without testing your code I am pretty sure that this is not correct. Mongoose as most database related libraries are async.

    User.update({username: oldUsername}, {
        username: newUser.username,
        password: newUser.password,
        rights: newUser.rights
    }, function(err, numberAffected, rawResponse) {
        if(printError(err)) {
            sendError(res);
        }
    });

    sendOK(res);

On this example you are sending "Ok" to the client even if the update fails. You need to always send the response from inside the last callback.

One more issue with your mongoose related code is that your delete should also be nested inside your find.

User
    .find({username: receivedUsername})
    .remove(function (err) {
        if(printError(err)) {
            sendError(res);
        }
    });

Find will return an array of users which you will need to loop and .remove()

You could also use the mongoose's static methods to group user related methods. This will allow you to have user specific code in a common place rather than all over.
http://mongoosejs.com/docs/guide.html#statics

Other than that your code seems sane enough. The wrappers for keeping consistency with the responses seems fine.

\$\endgroup\$
1
\$\begingroup\$

Also, you're missing error handling for all your mongoose connection and database calls. If an update/query failed, it would be an uncaught exception which would crash your server.

\$\endgroup\$

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.