From afac28320ac352d253bcc38ba93a78650b0b1aa6 Mon Sep 17 00:00:00 2001 From: Dan Cunningham Date: Mon, 29 Oct 2018 10:23:50 -0700 Subject: [PATCH] Alexa review fixes (#220) * Fixes oauth and form-express sanitized/validated data Signed-off-by: digitaldan * Be more specific about which values we replace in our data object Signed-off-by: digitaldan * Use form validation for reset code Signed-off-by: digitaldan --- routes/account.js | 48 ++++++++++++++++++++++++------------------ routes/applications.js | 2 -- routes/devices.js | 2 +- routes/index.js | 9 +++++++- routes/invitations.js | 2 +- routes/users.js | 8 +++---- views/oauth2dialog.ejs | 4 ++-- 7 files changed, 43 insertions(+), 32 deletions(-) diff --git a/routes/account.js b/routes/account.js index 668d807..0c559b5 100644 --- a/routes/account.js +++ b/routes/account.js @@ -33,7 +33,12 @@ exports.lostpasswordget = function(req, res) { } exports.lostpasswordpostvalidate = form( - field("email", "E-Mail").trim().isEmail().required() + field("email", "E-Mail").trim().toLower().isEmail().required() +); + +exports.loginpostvalidate = form( + field("username", "E-Mail").trim().toLower().isEmail().required(), + field("password", "Password").trim().required() ); exports.lostpasswordpost = function(req, res) { @@ -42,7 +47,7 @@ exports.lostpasswordpost = function(req, res) { // errormessages:req.flash('error'), infomessages:req.flash('info')}); res.redirect('/lostpassword'); } else { - User.findOne({username: req.body.email}, function(error, lostUser) { + User.findOne({username: req.form.email}, function(error, lostUser) { // resetSuccess indicates, if there's no technical error, only, it doesn't mean, thatan account with the e-mail address exists. var resetSuccess = true; @@ -93,16 +98,17 @@ exports.lostpasswordresetget = function(req, res) { exports.lostpasswordresetpostvalidate = form( field("password", "New password").trim().required(), - field("password2", "Repeat new password").trim().required() + field("password2", "Repeat new password").trim().required(), + field("resetCode", "Reset Code").required() ); exports.lostpasswordresetpost = function(req, res) { if (!req.form.isValid) { - res.redirect('/lostpasswordreset?resetCode=' + req.body.resetCode); + res.redirect('/lostpasswordreset?resetCode=' + req.form.resetCode); } else { - if (req.body.password != req.body.password2) { + if (req.form.password != req.form.password2) { req.flash('error', 'Passwords don\'t match'); - res.redirect('/lostpasswordreset?resetCode=' + req.body.resetCode); + res.redirect('/lostpasswordreset?resetCode=' + req.form.resetCode); } else { LostPassword.findOne({recoveryCode: resetCode, used: false}, function(error, lostPassword) { if (lostPassword && !error) { @@ -111,7 +117,7 @@ exports.lostpasswordresetpost = function(req, res) { var userPassword, result; userPassword = new UserPassword(lostUser); - result = userPassword.setPassword(req.body.password, function(error) { + result = userPassword.setPassword(req.form.password, function(error) { if (!error) { lostPassword.used = true; lostPassword.save(); @@ -125,7 +131,7 @@ exports.lostpasswordresetpost = function(req, res) { if (!result) { UserPassword.printPasswordNotComplexEnoughError(req); - res.redirect('/lostpasswordreset?resetCode=' + req.body.resetCode); + res.redirect('/lostpasswordreset?resetCode=' + req.form.resetCode); } } else { req.flash('error', 'There was an error while processing your request'); @@ -172,8 +178,8 @@ exports.accountpost = function(req, res) { } else { req.user.openhab(function(error, openhab) { if (!error && openhab) { - openhab.uuid = req.body.openhabuuid; - openhab.secret = req.body.openhabsecret; + openhab.uuid = req.form.openhabuuid; + openhab.secret = req.form.openhabsecret; openhab.save(); req.flash('info', 'openHAB settings successfully updated'); res.redirect('/account'); @@ -195,14 +201,14 @@ exports.accountpasswordpost = function(req, res) { } // first check, if both new passwords match each other - if (req.body.password !== req.body.password1) { + if (req.form.password !== req.form.password1) { req.flash('error', 'Passwords don\'t match'); res.redirect('/account'); return; } // make sure, that the old password is correct before changing it - req.user.checkPassword(req.body.oldpassword, function (err, isCorrect) { + req.user.checkPassword(req.form.oldpassword, function (err, isCorrect) { if (err) { req.flash('error', 'Could not check old password due to an unknown authentication error.'); return; @@ -216,7 +222,7 @@ exports.accountpasswordpost = function(req, res) { // save the new password and redirect userPassword = new UserPassword(req.user); - if (!userPassword.setPassword(req.body.password)) { + if (!userPassword.setPassword(req.form.password)) { UserPassword.printPasswordNotComplexEnoughError(req); res.redirect('/account'); } else { @@ -342,14 +348,14 @@ exports.accountdeletepost = function(req, res) { exports.registerpostvalidateall = form( field("agree", "Agreeing to terms and privacy policy").trim().required(), - field("username", "Username").trim().isEmail().required(), + field("username", "Username").trim().toLower().isEmail().required(), field("password", "Password").trim().required(), field("openhabuuid", "openHAB UUID").trim().required(), field("openhabsecret", "openHAB secret").trim().required() ); exports.registerpostvalidate = form( - field("username", "Username").trim().isEmail().required(), + field("username", "Username").trim().toLower().isEmail().required(), field("password", "Password").trim().required(), field("openhabuuid", "openHAB UUID").trim().required(), field("openhabsecret", "openHAB secret").trim().required() @@ -366,25 +372,25 @@ exports.registerpost = function(req, res) { res.render('login', { title: "Login / Sign up", user: req.user, errormessages:req.flash('error'), infomessages:req.flash('info') }); } else { - User.findOne({username: req.body.username.trim().toLowerCase()}, function(err, existingUser) { + User.findOne({username: req.form.username}, function(err, existingUser) { if (existingUser) { req.flash('error', "A user with this e-mail is already registered."); res.render('login', { title: "Login / Sign up", user: req.user, errormessages:req.flash('error'), infomessages:req.flash('info') }); } else if (!err) { - Openhab.findOne({uuid: req.body.openhabuuid},function(err, existingOpenhab) { + Openhab.findOne({uuid: req.form.openhabuuid},function(err, existingOpenhab) { if (existingOpenhab) { req.flash('error', "UUID is already in use on another account."); res.render('login', { title: "Login / Sign up", user: req.user, errormessages:req.flash('error'), infomessages:req.flash('info') }); } else { - if (!UserPassword.isComplexEnough(req.body.password)) { + if (!UserPassword.isComplexEnough(req.form.password)) { UserPassword.printPasswordNotComplexEnoughError(req); res.render('login', { title: "Login / Sign up", user: req.user, errormessages:req.flash('error'), infomessages:req.flash('info') }); return; } - User.register(req.body.username, req.body.password, function(err, user) { + User.register(req.form.username, req.form.password, function(err, user) { if (err) { req.flash('error', "An error occured during registration, please contact support"); logger.error(err); @@ -399,8 +405,8 @@ exports.registerpost = function(req, res) { errormessages:req.flash('error'), infomessages:req.flash('info') }); } else { var openhab = new Openhab({ - account: user.account, uuid: req.body.openhabuuid, - secret: req.body.openhabsecret + account: user.account, uuid: req.form.openhabuuid, + secret: req.form.openhabsecret }); openhab.save(function (error) { if (error) { diff --git a/routes/applications.js b/routes/applications.js index fe88d0a..af66cab 100644 --- a/routes/applications.js +++ b/routes/applications.js @@ -10,8 +10,6 @@ var gcm = require('node-gcm'); var gcmSender = require('../gcmsender.js') , appleSender = require('../aps-helper'); var redis = require('../redis-helper'); -var form = require('express-form'), - field = form.field; var OAuth2Token = require('../models/oauth2token'); exports.applicationsget = function(req, res) { diff --git a/routes/devices.js b/routes/devices.js index 206218f..350e22e 100644 --- a/routes/devices.js +++ b/routes/devices.js @@ -58,7 +58,7 @@ exports.devicessendmessage = function(req, res) { } else { logger.info("openHAB-cloud: sending message to device " + req.params.id); var sendMessageDeviceId = mongoose.Types.ObjectId(req.params.id); - var message = req.body.messagetext; + var message = req.form.messagetext; UserDevice.findOne({owner: req.user.id, _id: sendMessageDeviceId}, function (error, sendMessageDevice) { if (!error && sendMessageDevice) { if (sendMessageDevice.deviceType == 'ios') { diff --git a/routes/index.js b/routes/index.js index b26884e..6f17929 100644 --- a/routes/index.js +++ b/routes/index.js @@ -99,7 +99,14 @@ Routes.prototype.setupLoginLogoutRoutes = function (app) { }); }); - app.post('/login', passport.authenticate('local', { + app.post('/login', account_routes.loginpostvalidate, + //use express-form sanitized data for passport + function(req, res, next) { + req.body.username = req.form.username; + req.body.password = req.form.password; + next(); + }, + passport.authenticate('local', { successReturnToOrRedirect: '/', failureRedirect: '/login', failureFlash: true diff --git a/routes/invitations.js b/routes/invitations.js index 5027cf0..ac3d25c 100644 --- a/routes/invitations.js +++ b/routes/invitations.js @@ -21,7 +21,7 @@ exports.invitationspost = function(req, res) { res.redirect('/invitations'); }); } else { - Invitation.send(req.body.email, function(error, invite) { + Invitation.send(req.form.email, function(error, invite) { if (!error && invite) { req.flash('info', 'Invitation sent!'); res.redirect('/invitations'); diff --git a/routes/users.js b/routes/users.js index c01929d..c020c26 100644 --- a/routes/users.js +++ b/routes/users.js @@ -60,19 +60,19 @@ exports.usersaddpost = function(req, res) { if (!req.form.isValid) { res.redirect('/users/add'); } else { - if (req.body.password == req.body.password1) { - if (!UserPassword.isComplexEnough(req.body.password)) { + if (req.form.password == req.form.password1) { + if (!UserPassword.isComplexEnough(req.form.password)) { UserPassword.printPasswordNotComplexEnoughError(req); res.redirect('/users/add'); return; } - User.findOne({username: req.body.username}, function(error, checkUser) { + User.findOne({username: req.form.username}, function(error, checkUser) { if (!error) { if (checkUser) { req.flash('error', 'This username already exists'); res.redirect('/users/add'); } else { - User.registerToAccount(req.body.username, req.body.password, req.user.account, req.body.role, function(error, newUser) { + User.registerToAccount(req.form.username, req.form.password, req.user.account, req.form.role, function(error, newUser) { if (!error) { req.flash('info', 'User was added successfully'); res.redirect('/users'); diff --git a/views/oauth2dialog.ejs b/views/oauth2dialog.ejs index 9ea7d21..8e3194b 100644 --- a/views/oauth2dialog.ejs +++ b/views/oauth2dialog.ejs @@ -34,8 +34,8 @@ - - + +