From 35bd18b72555937a1b37297c44d0d4ee33c54a48 Mon Sep 17 00:00:00 2001 From: Marti Martz Date: Fri, 21 Dec 2018 06:04:56 -0700 Subject: [PATCH] Tag possible error prone code points (#1564) * These are mostly comments in this PR * Normalized a few more identifiers to their standards for easier grep'ing. I'm sure there's more. * Some of these may be silent as intended... they should be denoted as such... like `fallthrough` for `switch` should be normally * Some of these assume that AWS S3 and MongoLabs are perfectly connected and no issues... bad idea. This is what I was describing. We need [#37](Graceful failure) whether it's a comment, a console message, a page, or some combination of these. * This doesn't include MongoDB calls that just use `save()` for example... which is not recommended at all. Error traps are good when connecting to third parties whether they are local or remote. I'm not perfect either but I can get easily burned out on fixing these which is why I work in stages instead of all at once. Don't get paid for that! ;) :) Always fill out the arguments if known especially with `aErr`. That's a reason why it's done first in this coding style. * This was a very brief skim over the code... hopefully it wasn't overzealous or underzealous in code points... but they are comments to be addressed eventually. Living on four hours of sleep in 3 days isn't good so I'm taking a break not to mention a few days away from holiday days. Applies to #430 and indirectly to #1548 Signed-off-by: Martii Auto-merge --- controllers/admin.js | 10 ++++++++++ controllers/auth.js | 5 +++++ controllers/discussion.js | 6 ++++++ controllers/flag.js | 2 ++ controllers/group.js | 6 +++++- controllers/index.js | 4 ++++ controllers/issue.js | 4 ++++ controllers/remove.js | 6 ++++++ controllers/script.js | 8 ++++++++ controllers/scriptStorage.js | 6 ++++-- controllers/user.js | 15 +++++++++++++++ libs/flag.js | 2 ++ libs/markdown.js | 8 ++++---- libs/modelQuery.js | 2 +- libs/modifySessions.js | 4 ++++ libs/passportVerify.js | 2 ++ libs/remove.js | 6 ++++++ libs/repoManager.js | 2 ++ 18 files changed, 90 insertions(+), 8 deletions(-) diff --git a/controllers/admin.js b/controllers/admin.js index 42d96cb86..e060c2246 100644 --- a/controllers/admin.js +++ b/controllers/admin.js @@ -190,6 +190,8 @@ exports.adminUserUpdate = function (aReq, aRes, aNext) { // Make sure the change is reflected in the session store updateSessions(aReq, aUser, function (aErr, aSess) { + // WARNING: No err handling + aRes.redirect(user.userPageUri); }); }); @@ -509,6 +511,8 @@ exports.adminApiKeysPage = function (aReq, aRes, aNext) { // strategyListQuery tasks.push(function (aCallback) { Strategy.find({}, function (aErr, aStrats) { + // WARNING: No err handling + var stored = nil(); var strategies = null; @@ -556,6 +560,8 @@ exports.apiAdminUpdate = function (aReq, aRes, aNext) { }); Strategy.find({}, function (aErr, aStrats) { + // WARNING: No err handling + var stored = nil(); aStrats.forEach(function (aStrat) { @@ -590,6 +596,8 @@ exports.apiAdminUpdate = function (aReq, aRes, aNext) { } strategy.save(function (aErr, aStrategy) { + // WARNING: No err handling + loadPassport(aStrategy); aCallback(); }); @@ -599,6 +607,8 @@ exports.apiAdminUpdate = function (aReq, aRes, aNext) { aCallback(); }, function (aErr) { + // WARNING: No err handling + aRes.redirect('/admin/api'); }); }); diff --git a/controllers/auth.js b/controllers/auth.js index 4805ab98a..7f4545160 100644 --- a/controllers/auth.js +++ b/controllers/auth.js @@ -43,6 +43,7 @@ passport.serializeUser(function (aUser, aDone) { // Setup all the auth strategies var openIdStrategies = {}; Strategy.find({}, function (aErr, aStrategies) { + // WARNING: No err handling // Get OpenId strategies for (var name in allStrategies) { @@ -95,6 +96,8 @@ exports.auth = function (aReq, aRes, aNext) { if (aReq.session.cookie.sameSite !== 'lax') { aReq.session.cookie.sameSite = 'lax'; aReq.session.save(function (aErr) { + // WARNING: No err handling + authenticate(aReq, aRes, aNext); }); } else { @@ -342,6 +345,8 @@ exports.callback = function (aReq, aRes, aNext) { if (!aReq.session.passport.oujsOptions.authAttach) { expandSession(aReq, aUser, function (aErr) { + // WARNING: No err handling + aRes.redirect(doneUri); }); } else { diff --git a/controllers/discussion.js b/controllers/discussion.js index 18b5978a6..15b8a2b2a 100644 --- a/controllers/discussion.js +++ b/controllers/discussion.js @@ -427,6 +427,8 @@ function postComment(aUser, aDiscussion, aContent, aCreator, aUserAgent, aCallba }); comment.save(function (aErr, aComment) { + // WARNING: No err handling + ++aDiscussion.comments; aDiscussion.lastCommentor = aUser.name; aDiscussion.updated = new Date(); @@ -479,6 +481,8 @@ function postTopic(aUser, aCategory, aTopic, aContent, aIssue, aUserAgent, aCall newDiscussion = new Discussion(props); newDiscussion.save(function (aErr, aDiscussion) { + // WARNING: No err handling + // Now post the first comment postComment(aUser, aDiscussion, aContent, true, aUserAgent, function (aErr, aDiscussion) { aCallback(aDiscussion); @@ -566,6 +570,8 @@ exports.createComment = function (aReq, aRes, aNext) { postComment(authedUser, aDiscussion, content, false, userAgent, function (aErr, aDiscussion) { + // WARNING: No err handling + aRes.redirect(aDiscussion.path.split('/').map(function (aStr) { return encodeURIComponent(aStr); }).join('/') + diff --git a/controllers/flag.js b/controllers/flag.js index fe33f2b5f..7ef44bec0 100644 --- a/controllers/flag.js +++ b/controllers/flag.js @@ -43,6 +43,8 @@ exports.flag = function (aReq, aRes, aNext) { form = new formidable.IncomingForm(); form.parse(aReq, function (aErr, aFields) { + // WARNING: No err handling + var flag = aFields.flag === 'false' ? false : true; var reason = null; diff --git a/controllers/group.js b/controllers/group.js index 91070a458..06586a453 100644 --- a/controllers/group.js +++ b/controllers/group.js @@ -129,6 +129,8 @@ exports.addScriptToGroups = function (aScript, aGroupNames, aCallback) { }); group.save(function (aErr, aGroup) { + // WARNING: No err handling + aScript._groupId = aGroup._id; aCallback(); }); @@ -149,7 +151,9 @@ exports.addScriptToGroups = function (aScript, aGroupNames, aCallback) { aGroup.size = aScripts.length; aGroup.rating = getRating(aScripts); aGroup.updated = new Date(); - aGroup.save(function () { + aGroup.save(function (aErr, aGroup) { + // WARNING: No err handling + // NOTE: This is a callback that does nothing }); } diff --git a/controllers/index.js b/controllers/index.js index 542cd5609..fd76be700 100644 --- a/controllers/index.js +++ b/controllers/index.js @@ -107,6 +107,8 @@ exports.home = function (aReq, aRes) { getFlaggedListForContent('Script', options, aCallback); } ], function (aErr) { + // WARNING: No err handling + preRender(); render(); }); @@ -295,6 +297,8 @@ exports.logout = function (aReq, aRes) { } User.findOne({ _id: authedUser._id }, function (aErr, aUser) { + // WARNING: No err handling + removeSession(aReq, aUser, function () { aRes.redirect(redirectUri); }); diff --git a/controllers/issue.js b/controllers/issue.js index 8422791b2..95a021891 100644 --- a/controllers/issue.js +++ b/controllers/issue.js @@ -443,6 +443,8 @@ exports.comment = function (aReq, aRes, aNext) { discussionLib.postComment(authedUser, aIssue, content, false, userAgent, function (aErr, aDiscussion) { + // WARNING: No err handling + aRes.redirect(aDiscussion.path.split('/').map(function (aStr) { return encodeURIComponent(aStr); }).join('/') + @@ -493,6 +495,8 @@ exports.changeStatus = function (aReq, aRes, aNext) { if (changed) { aIssue.save(function (aErr, aDiscussion) { + // WARNING: No err handling + aRes.redirect(aDiscussion.path.split('/').map(function (aStr) { return encodeURIComponent(aStr); }).join('/') + diff --git a/controllers/remove.js b/controllers/remove.js index 7f6ba6d69..db62f7f4d 100644 --- a/controllers/remove.js +++ b/controllers/remove.js @@ -42,6 +42,8 @@ exports.rm = function (aReq, aRes, aNext) { form = new formidable.IncomingForm(); form.parse(aReq, function (aErr, aFields) { + // WARNING: No err handling + var reason = aFields.reason; var type = aReq.params[0]; @@ -87,6 +89,8 @@ exports.rm = function (aReq, aRes, aNext) { installName: scriptStorage.caseSensitive(installNameBase + (isLib ? '.js' : '.user.js')) }, function (aErr, aScript) { + // WARNING: No err handling + removeLib.remove(Script, aScript, authedUser, reason, function (aRemoved) { if (!aRemoved) { aNext(); @@ -101,6 +105,8 @@ exports.rm = function (aReq, aRes, aNext) { User.findOne({ name: { $regex: new RegExp('^' + username + '$', "i") } }, function (aErr, aUser) { + // WARNING: No err handling + removeLib.remove(User, aUser, authedUser, reason, function (aRemoved) { if (!aRemoved) { aNext(); diff --git a/controllers/script.js b/controllers/script.js index dbf0c1521..0e3c66498 100644 --- a/controllers/script.js +++ b/controllers/script.js @@ -247,6 +247,8 @@ var getScriptPageTasks = function (aOptions) { _scriptId: script._id, _userId: authedUser._id }, function (aErr, aVoteModel) { + // WARNING: No err handling + aOptions.voteable = !script.isOwner; if (aVoteModel) { @@ -359,6 +361,8 @@ exports.view = function (aReq, aRes, aNext) { getFlaggedListForContent('Script', options, aCallback); } ], function (aErr) { + // WARNING: No err handling + preRender(); render(); }); @@ -547,6 +551,8 @@ exports.vote = function (aReq, aRes, aNext) { Vote.findOne({ _scriptId: aScript._id, _userId: authedUser._id }, function (aErr, aVoteModel) { + // WARNING: No err handling + var votes = aScript.votes || 0; var flags = 0; var oldVote = null; @@ -554,6 +560,8 @@ exports.vote = function (aReq, aRes, aNext) { function saveScript() { if (!flags) { aScript.save(function (aErr, aScript) { + // WARNING: No err handling + var script = null; if (vote === false) { diff --git a/controllers/scriptStorage.js b/controllers/scriptStorage.js index 7957514a9..3819c1ad8 100644 --- a/controllers/scriptStorage.js +++ b/controllers/scriptStorage.js @@ -853,7 +853,7 @@ exports.sendScript = function (aReq, aRes, aNext) { // Resave affected properties aScript.save(function (aErr, aScript) { - // WARNING: No error handling at this stage + // WARNING: No err handling }); } }); @@ -885,6 +885,8 @@ exports.sendMeta = function (aReq, aRes, aNext) { Script.findOne({ installName: caseSensitive(installNameBase + '.user.js') }, function (aErr, aScript) { + // WARNING: No err handling + var script = null; var scriptOpenIssueCountQuery = null; var whitespace = '\u0020\u0020\u0020\u0020'; @@ -1942,7 +1944,7 @@ exports.deleteScript = function (aInstallName, aCallback) { function (aErr, aScript) { var s3 = new AWS.S3(); - // WARNING: No error handling at this stage + // WARNING: No err handling s3.deleteObject({ Bucket : bucketName, Key : aScript.installName}, function (aErr) { diff --git a/controllers/user.js b/controllers/user.js index 37ec725d0..ae197c1f9 100644 --- a/controllers/user.js +++ b/controllers/user.js @@ -124,6 +124,8 @@ exports.extend = function (aReq, aRes, aNext) { _id: authedUser._id, sessionIds: { "$in": [ aReq.sessionID ] } }, function (aErr, aUser) { + // WARNING: No err handling + extendSession(aReq, aUser, function (aErr) { if (aErr) { if (aErr === 'Already extended') { @@ -469,6 +471,8 @@ exports.view = function (aReq, aRes, aNext) { getFlaggedListForContent('User', options, aCallback); } ], function (aErr) { + // WARNING: No err handling + preRender(); render(); }); @@ -731,6 +735,8 @@ exports.userScriptListPage = function (aReq, aRes, aNext) { } } ], function (aErr) { + // WARNING: No err handling + preRender(); render(); }); @@ -969,6 +975,8 @@ exports.userEditPreferencesPage = function (aReq, aRes, aNext) { tasks.push(function (aCallback) { var userStrats = user.strategies.slice(0); Strategy.find({}, function (aErr, aStrats) { + // WARNING: No err handling + var defaultStrategy = userStrats[userStrats.length - 1]; var name = null; var strategy = null; @@ -1652,6 +1660,8 @@ exports.uploadScript = function (aReq, aRes, aNext) { form = new formidable.IncomingForm(); form.parse(aReq, function (aErr, aFields, aFiles) { + // WARNING: No err handling + // var isLib = aReq.params.isLib; var script = aFiles.script; @@ -1695,6 +1705,7 @@ exports.uploadScript = function (aReq, aRes, aNext) { stream.on('end', function () { User.findOne({ _id: authedUser._id }, function (aErr, aUser) { + // WARNING: No err handling var bufferConcat = Buffer.concat(bufs); @@ -1764,6 +1775,8 @@ exports.submitSource = function (aReq, aRes, aNext) { function storeScript(aMeta, aSource) { User.findOne({ _id: authedUser._id }, function (aErr, aUser) { + // WARNING: No err handling + scriptStorage.storeScript(aUser, aMeta, aSource, false, function (aErr, aScript) { var redirectUri = aScript ? ((aScript.isLib ? '/libs/' : '/scripts/') + @@ -1815,6 +1828,8 @@ exports.submitSource = function (aReq, aRes, aNext) { aScript.fork = fork; aScript.save(function (aErr, aScript) { + // WARNING: No err handling + aRes.redirect(redirectUri); }); }); diff --git a/libs/flag.js b/libs/flag.js index d2240d73e..ded8e2aee 100644 --- a/libs/flag.js +++ b/libs/flag.js @@ -210,6 +210,8 @@ exports.unflag = function (aModel, aContent, aUser, aReason, aCallback) { function removeFlag(aAuthor) { aFlag.remove(function (aErr) { + // WARNING: No err handling + saveContent(aModel, aContent, aAuthor, aUser.role < 4 ? -2 : -1, true, aCallback); }); } diff --git a/libs/markdown.js b/libs/markdown.js index 203fcebd0..0871c9ec0 100644 --- a/libs/markdown.js +++ b/libs/markdown.js @@ -238,11 +238,11 @@ marked.setOptions({ if (aLang && hljs.getLanguage(aLang)) { try { return hljs.highlight(aLang, aCode).value; - } catch (aErr) { + } catch (aE) { if (isDev) { console.error([ colors.red('Dependency named highlighting failed with:'), - aErr + aE ].join('\n')); } @@ -264,11 +264,11 @@ marked.setOptions({ } return hljs.highlightAuto(aCode, lang).value; } - } catch (aErr) { + } catch (aE) { if (isDev) { console.error([ colors.red('Dependency automatic named highlighting failed with:'), - aErr + aE ].join('\n')); } diff --git a/libs/modelQuery.js b/libs/modelQuery.js index c5276af42..e7db74934 100644 --- a/libs/modelQuery.js +++ b/libs/modelQuery.js @@ -73,7 +73,7 @@ var parseSearchConditions = function (aQ, aPrefixSearchFields, aFullSearchFields var fullStr = ''; var prefixRegex = null; var fullRegex = null; - var terms = aQ.replace(/([.*+?^=!:${}()|\[\]\/\\])/g, '\\$1').split(/\s+/).map(function (aE) { return aE.trim(); }); + var terms = aQ.replace(/([.*+?^=!:${}()|\[\]\/\\])/g, '\\$1').split(/\s+/).map(function (aEl) { return aEl.trim(); }); // Match all the terms but in any order terms.forEach(function (aTerm) { diff --git a/libs/modifySessions.js b/libs/modifySessions.js index 3d2eb920b..2016c68f5 100644 --- a/libs/modifySessions.js +++ b/libs/modifySessions.js @@ -47,6 +47,8 @@ exports.add = function (aReq, aUser, aCallback) { var store = aReq.sessionStore; function finish(aErr, aUser) { + // WARNING: No err handling + aReq.session.user = serializeUser(aUser); aCallback(); } @@ -268,6 +270,8 @@ exports.getSessionDataList = function (aReq, aOptions, aCallback) { aInnerCallback(); }], function (aErr) { + // WARNING: No err handling + aCallback(null); } ); diff --git a/libs/passportVerify.js b/libs/passportVerify.js index ff2230269..ee3e26c34 100644 --- a/libs/passportVerify.js +++ b/libs/passportVerify.js @@ -72,6 +72,8 @@ exports.verify = function (aId, aStrategy, aUsername, aLoggedIn, aDone) { if (!aUser) { User.findOne({ 'name': aUsername }, function (aErr, aUser) { + // WARNING: No err handling + if (aUser && aLoggedIn) { if (allStrategies[aStrategy].readonly) { aDone(null, false, 'readonly strategy'); diff --git a/libs/remove.js b/libs/remove.js index bc9683614..e409fcb39 100644 --- a/libs/remove.js +++ b/libs/remove.js @@ -232,6 +232,8 @@ function remove(aModel, aContent, aUser, aReason, aAutomated, aCallback) { --aResult.discussion.comments; aResult.discussion.save(function (aErr) { + // WARNING: No err handling + aOuterCallback(null); }); @@ -241,6 +243,8 @@ function remove(aModel, aContent, aUser, aReason, aAutomated, aCallback) { aResult.discussion.updated = aResult.prevComment.created; aResult.discussion.save(function (aErr) { + // WARNING: No err handling + aOuterCallback(null); }); } else { @@ -337,6 +341,8 @@ exports.remove = function (aModel, aContent, aUser, aReason, aCallback) { _authorId: aContent._id }, function (aErr, aContentArr) { + // WARNING: No err handling + async.eachSeries(aContentArr, function (aContent, aEachInnerCallback) { remove(model, aContent, aUser, '', true, aEachInnerCallback); }, aEachOuterCallback); diff --git a/libs/repoManager.js b/libs/repoManager.js index dd9b20073..a94bea310 100644 --- a/libs/repoManager.js +++ b/libs/repoManager.js @@ -19,6 +19,8 @@ var clientId = null; var clientKey = null; Strategy.findOne({ name: 'github' }, function (aErr, aStrat) { + // WARNING: No err handling + clientId = aStrat.id; clientKey = aStrat.key; });