Skip to content

Commit

Permalink
Alexa review fixes (#220)
Browse files Browse the repository at this point in the history 
* Fixes oauth and form-express sanitized/validated data

Signed-off-by: digitaldan <[email protected]>

* Be more specific about which values we replace in our data object

Signed-off-by: digitaldan <[email protected]>

* Use form validation for reset code

Signed-off-by: digitaldan <[email protected]>
  • Loading branch information
@digitaldan
digitaldan authored Oct 29, 2018
1 parent a3f5fa9 commit afac283
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 32 deletions.
48 changes: 27 additions & 21 deletions routes/account.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;

Expand Down Expand Up @@ -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) {
Expand All @@ -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();
Expand All @@ -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');
Expand Down Expand Up @@ -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');
Expand All @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -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);
Expand All @@ -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) {
Expand Down
2 changes: 0 additions & 2 deletions routes/applications.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion routes/devices.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down
9 changes: 8 additions & 1 deletion routes/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion routes/invitations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
8 changes: 4 additions & 4 deletions routes/users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions views/oauth2dialog.ejs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
<input name="transaction_id" type="hidden" value="<%= transactionID %>">
<input type="hidden" name="_csrf" value="<%= token %>" class="form-control">
<input type="hidden" name="scope" value="ifttt" class="form-control">
<button name="submit" type="submit" class="btn" id="allow">Allow</button>
<button name="cancel" type="submit" class="btn" id="deny">Deny</button>
<button name="submit" type="submit" class="btn" id="allow" value="Allow">Allow</button>
<button name="cancel" type="submit" class="btn" id="deny" value="Deny">Deny</button>
</form>
</p>
</div>
Expand Down

0 comments on commit afac283

Please sign in to comment.