diff --git a/src/node/db/API.js b/src/node/db/API.js index be3e73486..21c958809 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -137,15 +137,13 @@ exports.getRevisionChangeset = function(padID, rev, callback) if (rev !== undefined && typeof rev !== "number") { // try to parse the number - if (!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if (isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } // ensure this is not a negative number @@ -184,17 +182,17 @@ exports.getRevisionChangeset = function(padID, rev, callback) callback(null, changeset); }) - } - //the client wants the latest changeset, lets return it to him - else - { - pad.getRevisionChangeset(pad.getHeadRevisionNumber(), function(err, changeset) - { - if(ERR(err, callback)) return; - callback(null, changeset); - }) + return; } + + //the client wants the latest changeset, lets return it to him + pad.getRevisionChangeset(pad.getHeadRevisionNumber(), function(err, changeset) + { + if(ERR(err, callback)) return; + + callback(null, changeset); + }) }); } @@ -219,15 +217,13 @@ exports.getText = function(padID, rev, callback) if(rev !== undefined && typeof rev != "number") { //try to parse the number - if(!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if(isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } //ensure this is not a negativ number @@ -268,13 +264,13 @@ exports.getText = function(padID, rev, callback) callback(null, data); }) + + return; } + //the client wants the latest text, lets return it to him - else - { - var padText = exportTxt.getTXTFromAtext(pad, pad.atext); - callback(null, {"text": padText}); - } + var padText = exportTxt.getTXTFromAtext(pad, pad.atext); + callback(null, {"text": padText}); }); } @@ -359,15 +355,13 @@ exports.getHTML = function(padID, rev, callback) if (rev !== undefined && typeof rev != "number") { - if (!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if (isNaN(parseInt(rev))) { callback(new customError("rev is not a number","apierror")); return; } + + rev = parseInt(rev); } if(rev !== undefined && rev < 0) @@ -405,19 +399,19 @@ exports.getHTML = function(padID, rev, callback) var data = {html: html}; callback(null, data); }); + + return; } + //the client wants the latest text, lets return it to him - else + exportHtml.getPadHTML(pad, undefined, function (err, html) { - exportHtml.getPadHTML(pad, undefined, function (err, html) - { - if(ERR(err, callback)) return; - html = "" +html; // adds HTML head - html += ""; - var data = {html: html}; - callback(null, data); - }); - } + if(ERR(err, callback)) return; + html = "" +html; // adds HTML head + html += ""; + var data = {html: html}; + callback(null, data); + }); }); } @@ -448,11 +442,10 @@ exports.setHTML = function(padID, html, callback) if(e){ callback(new customError("HTML is malformed","apierror")); return; - }else{ - //update the clients on the pad - padMessageHandler.updatePadClients(pad, callback); - return; } + + //update the clients on the pad + padMessageHandler.updatePadClients(pad, callback); }); }); } @@ -641,15 +634,13 @@ exports.saveRevision = function(padID, rev, callback) if(rev !== undefined && typeof rev != "number") { //try to parse the number - if(!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if(isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } //ensure this is not a negativ number @@ -732,8 +723,9 @@ exports.createPad = function(padID, text, callback) callback(new customError("createPad can't create group pads","apierror")); return; } + //check for url special characters - else if(padID.match(/(\/|\?|&|#)/)) + if(padID.match(/(\/|\?|&|#)/)) { callback(new customError("malformed padID: Remove special characters","apierror")); return; @@ -782,15 +774,13 @@ exports.restoreRevision = function (padID, rev, callback) if (rev !== undefined && typeof rev != "number") { //try to parse the number - if (!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if (isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } //ensure this is not a negativ number @@ -959,11 +949,10 @@ exports.getPadID = function(roID, callback) if(retrievedPadID == null) { callback(new customError("padID does not exist","apierror")); + return; } - else - { - callback(null, {padID: retrievedPadID}); - } + + callback(null, {padID: retrievedPadID}); }); } @@ -1127,9 +1116,9 @@ exports.sendClientsMessage = function (padID, msg, callback) { getPadSafe(padID, true, function (err, pad) { if (ERR(err, callback)) { return; - } else { - padMessageHandler.handleCustomMessage(padID, msg, callback); } + + padMessageHandler.handleCustomMessage(padID, msg, callback); } ); } @@ -1177,30 +1166,26 @@ exports.createDiffHTML = function(padID, startRev, endRev, callback){ if(startRev !== undefined && typeof startRev != "number") { //try to parse the number - if(!isNaN(parseInt(startRev))) - { - startRev = parseInt(startRev, 10); - } - else + if(isNaN(parseInt(startRev))) { callback({stop: "startRev is not a number"}); return; } + + startRev = parseInt(startRev, 10); } //check if rev is a number if(endRev !== undefined && typeof endRev != "number") { //try to parse the number - if(!isNaN(parseInt(endRev))) - { - endRev = parseInt(endRev, 10); - } - else + if(isNaN(parseInt(endRev))) { callback({stop: "endRev is not a number"}); return; } + + endRev = parseInt(endRev, 10); } //get the pad diff --git a/src/node/db/AuthorManager.js b/src/node/db/AuthorManager.js index 1f2a736be..c7ebf47f4 100644 --- a/src/node/db/AuthorManager.js +++ b/src/node/db/AuthorManager.js @@ -104,16 +104,16 @@ function mapAuthorWithDBKey (mapperkey, mapper, callback) //return the author callback(null, author); }); - } - //there is a author with this mapper - else - { - //update the timestamp of this author - db.setSub("globalAuthor:" + author, ["timestamp"], new Date().getTime()); - //return the author - callback(null, {authorID: author}); + return; } + + //there is a author with this mapper + //update the timestamp of this author + db.setSub("globalAuthor:" + author, ["timestamp"], new Date().getTime()); + + //return the author + callback(null, {authorID: author}); }); } @@ -209,20 +209,19 @@ exports.listPadsOfAuthor = function (authorID, callback) if(author == null) { callback(new customError("authorID does not exist","apierror")) + return; } + //everything is fine, return the pad IDs - else + var pads = []; + if(author.padIDs != null) { - var pads = []; - if(author.padIDs != null) + for (var padId in author.padIDs) { - for (var padId in author.padIDs) - { - pads.push(padId); - } + pads.push(padId); } - callback(null, {padIDs: pads}); } + callback(null, {padIDs: pads}); }); } diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index 82c14c39d..0c9be1221 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -62,13 +62,12 @@ exports.deleteGroup = function(groupID, callback) if(_group == null) { callback(new customError("groupID does not exist","apierror")); + return; } + //group exists, everything is fine - else - { - group = _group; - callback(); - } + group = _group; + callback(); }); }, //iterate trough all pads of this groups and delete them @@ -213,34 +212,35 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback) //try to get a group for this mapper db.get("mapper2group:"+groupMapper, function(err, groupID) { - if(ERR(err, callback)) return; + function createGroupForMapper(cb) { + exports.createGroup(function(err, responseObj) + { + if(ERR(err, cb)) return; + + //create the mapper entry for this group + db.set("mapper2group:"+groupMapper, responseObj.groupID); + + cb(null, responseObj); + }); + } + + if(ERR(err, callback)) return; - // there is a group for this mapper - if(groupID) { - exports.doesGroupExist(groupID, function(err, exists) { - if(ERR(err, callback)) return; - if(exists) return callback(null, {groupID: groupID}); - - // hah, the returned group doesn't exist, let's create one - createGroupForMapper(callback) - }) - } - //there is no group for this mapper, let's create a group - else { - createGroupForMapper(callback) - } - - function createGroupForMapper(cb) { - exports.createGroup(function(err, responseObj) - { - if(ERR(err, cb)) return; - - //create the mapper entry for this group - db.set("mapper2group:"+groupMapper, responseObj.groupID); - - cb(null, responseObj); - }); - } + // there is a group for this mapper + if(groupID) { + exports.doesGroupExist(groupID, function(err, exists) { + if(ERR(err, callback)) return; + if(exists) return callback(null, {groupID: groupID}); + + // hah, the returned group doesn't exist, let's create one + createGroupForMapper(callback) + }) + + return; + } + + //there is no group for this mapper, let's create a group + createGroupForMapper(callback) }); } @@ -261,12 +261,11 @@ exports.createGroupPad = function(groupID, padName, text, callback) if(exists == false) { callback(new customError("groupID does not exist","apierror")); + return; } + //group exists, everything is fine - else - { - callback(); - } + callback(); }); }, //ensure pad does not exists @@ -280,12 +279,11 @@ exports.createGroupPad = function(groupID, padName, text, callback) if(exists == true) { callback(new customError("padName does already exist","apierror")); + return; } + //pad does not exist, everything is fine - else - { - callback(); - } + callback(); }); }, //create the pad @@ -320,19 +318,18 @@ exports.listPads = function(groupID, callback) if(exists == false) { callback(new customError("groupID does not exist","apierror")); + return; } + //group exists, let's get the pads - else + db.getSub("group:" + groupID, ["pads"], function(err, result) { - db.getSub("group:" + groupID, ["pads"], function(err, result) - { - if(ERR(err, callback)) return; - var pads = []; - for ( var padId in result ) { - pads.push(padId); - } - callback(null, {padIDs: pads}); - }); - } + if(ERR(err, callback)) return; + var pads = []; + for ( var padId in result ) { + pads.push(padId); + } + callback(null, {padIDs: pads}); + }); }); } diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index 0cb01cace..91ab7f792 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -476,28 +476,27 @@ Pad.prototype.copy = function copy(destinationID, force, callback) { // if it's a group pad, let's make sure the group exists. function(callback) { - if (destinationID.indexOf("$") != -1) + if (destinationID.indexOf("$") === -1) { - destGroupID = destinationID.split("$")[0] - groupManager.doesGroupExist(destGroupID, function (err, exists) - { - if(ERR(err, callback)) return; - - //group does not exist - if(exists == false) - { - callback(new customError("groupID does not exist for destinationID","apierror")); - return; - } - //everything is fine, continue - else - { - callback(); - } - }); - } - else callback(); + return; + } + + destGroupID = destinationID.split("$")[0] + groupManager.doesGroupExist(destGroupID, function (err, exists) + { + if(ERR(err, callback)) return; + + //group does not exist + if(exists == false) + { + callback(new customError("groupID does not exist for destinationID","apierror")); + return; + } + + //everything is fine, continue + callback(); + }); }, // if the pad exists, we should abort, unless forced. function(callback) @@ -506,27 +505,29 @@ Pad.prototype.copy = function copy(destinationID, force, callback) { { if(ERR(err, callback)) return; - if(exists == true) - { - if (!force) - { - console.error("erroring out without force"); - callback(new customError("destinationID already exists","apierror")); - console.error("erroring out without force - after"); - return; - } - else // exists and forcing - { - padManager.getPad(destinationID, function(err, pad) { - if (ERR(err, callback)) return; - pad.remove(callback); - }); - } - } - else + /* + * this is the negation of a truthy comparison. Has been left in this + * wonky state to keep the old (possibly buggy) behaviour + */ + if (!(exists == true)) { callback(); + return; } + + if (!force) + { + console.error("erroring out without force"); + callback(new customError("destinationID already exists","apierror")); + console.error("erroring out without force - after"); + return; + } + + // exists and forcing + padManager.getPad(destinationID, function(err, pad) { + if (ERR(err, callback)) return; + pad.remove(callback); + }); }); }, // copy the 'pad' entry @@ -622,29 +623,28 @@ Pad.prototype.remove = function remove(callback) { //is it a group pad? -> delete the entry of this pad in the group function(callback) { - //is it a group pad? - if(padID.indexOf("$")!=-1) - { - var groupID = padID.substring(0,padID.indexOf("$")); - - db.get("group:" + groupID, function (err, group) - { - if(ERR(err, callback)) return; - - //remove the pad entry - delete group.pads[padID]; - - //set the new value - db.set("group:" + groupID, group); - - callback(); - }); - } - //its no group pad, nothing to do here - else + if(padID.indexOf("$") === -1) { + // it isn't a group pad, nothing to do here callback(); + return; } + + // it is a group pad + var groupID = padID.substring(0,padID.indexOf("$")); + + db.get("group:" + groupID, function (err, group) + { + if(ERR(err, callback)) return; + + //remove the pad entry + delete group.pads[padID]; + + //set the new value + db.set("group:" + groupID, group); + + callback(); + }); }, //remove the readonly entries function(callback) diff --git a/src/node/db/PadManager.js b/src/node/db/PadManager.js index 2ecd6e274..035ef3e5e 100644 --- a/src/node/db/PadManager.js +++ b/src/node/db/PadManager.js @@ -159,21 +159,20 @@ exports.getPad = function(id, text, callback) if(pad != null) { callback(null, pad); + return; } - //try to load pad - else - { - pad = new Pad(id); - //initalize the pad - pad.init(text, function(err) - { - if(ERR(err, callback)) return; - globalPads.set(id, pad); - padList.addPad(id); - callback(null, pad); - }); - } + //try to load pad + pad = new Pad(id); + + //initalize the pad + pad.init(text, function(err) + { + if(ERR(err, callback)) return; + globalPads.set(id, pad); + padList.addPad(id); + callback(null, pad); + }); } exports.listAllPads = function(cb) @@ -206,30 +205,28 @@ exports.sanitizePadId = function(padId, callback) { if(transform_index >= padIdTransforms.length) { callback(padId); + return; } + //check if padId exists - else + exports.doesPadExists(padId, function(junk, exists) { - exports.doesPadExists(padId, function(junk, exists) + if(exists) { - if(exists) - { - callback(padId); - } - else - { - //get the next transformation *that's different* - var transformedPadId = padId; - while(transformedPadId == padId && transform_index < padIdTransforms.length) - { - transformedPadId = padId.replace(padIdTransforms[transform_index][0], padIdTransforms[transform_index][1]); - transform_index += 1; - } - //check the next transform - exports.sanitizePadId(transformedPadId, callback, transform_index); - } - }); - } + callback(padId); + return; + } + + //get the next transformation *that's different* + var transformedPadId = padId; + while(transformedPadId == padId && transform_index < padIdTransforms.length) + { + transformedPadId = padId.replace(padIdTransforms[transform_index][0], padIdTransforms[transform_index][1]); + transform_index += 1; + } + //check the next transform + exports.sanitizePadId(transformedPadId, callback, transform_index); + }); } exports.isValidPadId = function(padId) diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index 98feafb3a..f930b9618 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -90,13 +90,13 @@ exports.checkAccess = function (padID, sessionCookie, token, password, callback) // grant or deny access, with author of token callback(null, statusObject); }); + + return; } + // user may create new pads - no need to check anything - else - { - // grant access, with author of token - callback(null, statusObject); - } + // grant access, with author of token + callback(null, statusObject); }); //don't continue diff --git a/src/node/db/SessionManager.js b/src/node/db/SessionManager.js index 518c70c7a..954203758 100644 --- a/src/node/db/SessionManager.js +++ b/src/node/db/SessionManager.js @@ -90,15 +90,13 @@ exports.createSession = function(groupID, authorID, validUntil, callback) if(typeof validUntil != "number") { //try to parse the number - if(!isNaN(parseInt(validUntil))) - { - validUntil = parseInt(validUntil); - } - else + if(isNaN(parseInt(validUntil))) { callback(new customError("validUntil is not a number","apierror")); return; } + + validUntil = parseInt(validUntil); } //ensure this is not a negativ number diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index d316faf01..c52565de3 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -244,59 +244,71 @@ exports.handleMessage = function(client, message) } }; - if (message) { - async.series([ - handleMessageHook, - //check permissions - function(callback) - { - // client tried to auth for the first time (first msg from the client) - if(message.type == "CLIENT_READY") { - createSessionInfo(client, message); - } - - // Note: message.sessionID is an entirely different kind of - // session from the sessions we use here! Beware! - // FIXME: Call our "sessions" "connections". - // FIXME: Use a hook instead - // FIXME: Allow to override readwrite access with readonly - - // Simulate using the load testing tool - if(!sessioninfos[client.id].auth){ - console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") - return; - }else{ - var auth = sessioninfos[client.id].auth; - var checkAccessCallback = function(err, statusObject) - { - if(ERR(err, callback)) return; - - //access was granted - if(statusObject.accessStatus == "grant") - { - callback(); - } - //no access, send the client a message that tell him why - else - { - client.json.send({accessStatus: statusObject.accessStatus}) - } - }; - //check if pad is requested via readOnly - if (auth.padID.indexOf("r.") === 0) { - //Pad is readOnly, first get the real Pad ID - readOnlyManager.getPadId(auth.padID, function(err, value) { - ERR(err); - securityManager.checkAccess(value, auth.sessionID, auth.token, auth.password, checkAccessCallback); - }); - } else { - securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback); - } - } - }, - finalHandler - ]); + /* + * In a previous version of this code, an "if (message)" wrapped the + * following async.series(). + * This ugly "!Boolean(message)" is a lame way to exactly negate the truthy + * condition and replace it with an early return, while being sure to leave + * the original behaviour unchanged. + * + * A shallower code could maybe make more evident latent logic errors. + */ + if (!Boolean(message)) { + return; } + + async.series([ + handleMessageHook, + //check permissions + function(callback) + { + // client tried to auth for the first time (first msg from the client) + if(message.type == "CLIENT_READY") { + createSessionInfo(client, message); + } + + // Note: message.sessionID is an entirely different kind of + // session from the sessions we use here! Beware! + // FIXME: Call our "sessions" "connections". + // FIXME: Use a hook instead + // FIXME: Allow to override readwrite access with readonly + + // Simulate using the load testing tool + if(!sessioninfos[client.id].auth){ + console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") + return; + } + + var auth = sessioninfos[client.id].auth; + var checkAccessCallback = function(err, statusObject) + { + if(ERR(err, callback)) return; + + //access was granted + if(statusObject.accessStatus == "grant") + { + callback(); + } + //no access, send the client a message that tell him why + else + { + client.json.send({accessStatus: statusObject.accessStatus}) + } + }; + + //check if pad is requested via readOnly + if (auth.padID.indexOf("r.") === 0) { + //Pad is readOnly, first get the real Pad ID + readOnlyManager.getPadId(auth.padID, function(err, value) { + ERR(err); + securityManager.checkAccess(value, auth.sessionID, auth.token, auth.password, checkAccessCallback); + }); + } else { + securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback); + } + }, + finalHandler + ]); } diff --git a/src/node/hooks/express/adminsettings.js b/src/node/hooks/express/adminsettings.js index 73691837a..baa0bb70a 100644 --- a/src/node/hooks/express/adminsettings.js +++ b/src/node/hooks/express/adminsettings.js @@ -28,15 +28,13 @@ exports.socketio = function (hook_name, args, cb) { if (err) { return console.log(err); } - else - { - //if showSettingsInAdminPage is set to false, then return NOT_ALLOWED in the result - if(settings.showSettingsInAdminPage === false) { - socket.emit("settings", {results:'NOT_ALLOWED'}); - } - else { - socket.emit("settings", {results: data}); - } + + //if showSettingsInAdminPage is set to false, then return NOT_ALLOWED in the result + if(settings.showSettingsInAdminPage === false) { + socket.emit("settings", {results:'NOT_ALLOWED'}); + } + else { + socket.emit("settings", {results: data}); } }); }); diff --git a/src/node/hooks/express/padurlsanitize.js b/src/node/hooks/express/padurlsanitize.js index a9972220b..be3ffb1b4 100644 --- a/src/node/hooks/express/padurlsanitize.js +++ b/src/node/hooks/express/padurlsanitize.js @@ -8,26 +8,25 @@ exports.expressCreateServer = function (hook_name, args, cb) { if(!padManager.isValidPadId(padId) || /\/$/.test(req.url)) { res.status(404).send('Such a padname is forbidden'); + return; } - else - { - padManager.sanitizePadId(padId, function(sanitizedPadId) { - //the pad id was sanitized, so we redirect to the sanitized version - if(sanitizedPadId != padId) - { - var real_url = sanitizedPadId; - real_url = encodeURIComponent(real_url); - var query = url.parse(req.url).query; - if ( query ) real_url += '?' + query; - res.header('Location', real_url); - res.status(302).send('You should be redirected to ' + real_url + ''); - } - //the pad id was fine, so just render it - else - { - next(); - } - }); - } + + padManager.sanitizePadId(padId, function(sanitizedPadId) { + //the pad id was sanitized, so we redirect to the sanitized version + if(sanitizedPadId != padId) + { + var real_url = sanitizedPadId; + real_url = encodeURIComponent(real_url); + var query = url.parse(req.url).query; + if ( query ) real_url += '?' + query; + res.header('Location', real_url); + res.status(302).send('You should be redirected to ' + real_url + ''); + } + //the pad id was fine, so just render it + else + { + next(); + } + }); }); }