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 <martii@users.noreply.github.com> Auto-merge
OpenUserJS · Dec 21, 2018 · 35bd18b · 35bd18b
1 parent 0d61c78
commit 35bd18b
Show file tree

Hide file tree

Showing 18 changed files with 90 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,13 +551,17 @@ 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;
 
           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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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');