3
\$\begingroup\$

I wrote the below code which contains the following functions: establish session with the user, determine if the user is authenticated, perform ping request, search about the user from the database, reset password and import bulk product with the type text/xml.

  • Did I handle the functions correctly?
  • Is my code secure?
  • Should I correct any parts of the code?

var db = require('../models')
  var bCrypt = require('bcrypt')
  const exec = require('child_process').exec;
  var mathjs = require('mathjs')
  var libxmljs = require("libxmljs");
  var serialize = require("node-serialize")
  const Op = db.Sequelize.Op
  var express = require('express')
  var bodyParser = require('body-parser')
  var passport = require('passport')
  var session = require('express-session')
  var ejs = require('ejs')
  var morgan = require('morgan')
  const fileUpload = require('express-fileupload');
  var config = require('./config/server')
  
  var app = express()
  require('./core/passport')(passport)
  app.use(express.static('public'))
  app.set('view engine','ejs')
  app.use(morgan('tiny'))
  app.use(bodyParser.urlencoded({ extended: false }))
  app.use(fileUpload());
  
  app.use(session({
    secret: 'secret_key',
    resave: true,
    saveUninitialized: true,
    cookie: { secure: false }
  }))
  
  app.use(passport.initialize())
  app.use(passport.session())
  app.use(require('express-flash')());
  
  app.use('/app',require('./routes/app')())
  app.use('/',require('./routes/main')(passport))
  
  app.listen(config.port, config.listen)
  
  module.exports.isAuthenticated = function (req, res, next) {
      if (req.isAuthenticated()) {
          req.flash('authenticated', true)
          return next();
      
      res.redirect('/login');
  }
  
  module.exports.ping = function (req, res) {
      exec('ping -c 2 ' + req.body.address, function (err, stdout, stderr) {
          output = stdout + stderr
          res.render('app/ping', {
              output: output
          })
      })
  }
  
  module.exports.userSearch = function (req, res) {
      var query = "SELECT name,id FROM Users WHERE login='" + req.body.login + "'";
      db.sequelize.query(query, {
          model: db.User,
          logging: true
      }).then(user => {
          if (user.length) {
              var output = {
                  user: {
                      name: user[0].name,
                      id: user[0].id
                  
              
              res.render('app/usersearch', {
                  output: output
              })
          } else {
              req.flash('warning', 'User not found')
              res.render('app/usersearch', {
                  output: null
              })
          
      }).catch(err => {
          req.flash('danger', 'Internal Error')
          res.render('app/usersearch', {
              output: null
          })
      })
  }
  
  module.exports.resetpasswd = function (req, res) {
      if (req.query.login) {
          db.User.find({
              where: {
                  'login': req.query.login
              
          }).then(user => {
              if (user) {
                  if (req.query.token == md5(req.query.login)) {
                      res.render('resetpassword', {
                          login: req.query.login,
                          token: req.query.token
                     })
                 } else {
                     req.flash('danger', "Invalid reset token")
                     res.redirect('/forgotpassword')
                 
             } else {
                 req.flash('danger', "Invalid login username")
                 res.redirect('/forgotpassword')
             
         })
     } else {
         req.flash('danger', "invalid username")
         res.redirect('/forgotpassword')
     
 }
 
 module.exports.importBulkProducts =  function(req, res) {
     if (req.files.products && req.files.products.mimetype=='text/xml'){
         var products = libxmljs.parseXmlString(req.files.products.data.toString('utf8'), {noent:true,noblanks:true})
         products.root().childNodes().forEach( product => {
             var newProduct = new db.Product()
             newProduct.name = product.childNodes()[0].text()
             newProduct.code = product.childNodes()[1].text()
             newProduct.tags = product.childNodes()[2].text()
             newProduct.description = product.childNodes()[3].text()
             newProduct.save()
         })
         res.redirect('/app/products')
     }else{
         res.render('app/bulkproducts',{messages:{danger:'Invalid file'},legacy:false})
     
 }

\$\endgroup\$
3
\$\begingroup\$

The general rule is that you should never incorporate user-controlled input into a string that will be interpreted by a computer system without escaping it first. Specifically, at a quick glance, it's obvious that…

  • Your exec() call is vulnerable to command-line injection:

    exec('ping -c 2 ' + req.body.address, …)
    
  • Your SQL query is vulnerable to SQL injection:

    var query = "SELECT name,id FROM Users WHERE login='" + req.body.login + "'";
    
\$\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.