Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am in the process of writing a simple Node.JS webserver. I am about half-way 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.

What are design and security issues that my code may have?

// 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;
}

[Note: This is my first question here so please bear with me. Is this too vague a question? Is it acceptable to improve my code and my update my question as I receive answers?]

share|improve this question

2 Answers 2

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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