fix #310: wrap ObjectId creation in try-catch

qbreader · Sep 9, 2024 · 928ebd9 · 928ebd9
1 parent a7fe83b
commit 928ebd9
Show file tree

Hide file tree

Showing 21 changed files with 78 additions and 77 deletions.
diff --git a/database/account-info/question-stats/record-bonus-data.js b/database/account-info/question-stats/record-bonus-data.js
@@ -25,8 +25,13 @@ async function recordBonusData (username, data) {
     }
   }
 
-  newData.bonus_id = new ObjectId(bonus._id);
-  newData.set_id = new ObjectId(bonus.set._id);
+  try {
+    newData.bonus_id = new ObjectId(bonus._id);
+    newData.set_id = new ObjectId(bonus.set._id);
+  } catch (e) {
+    return false;
+  }
+
   newData.user_id = userId;
   return await bonusData.insertOne(newData);
 }

diff --git a/database/account-info/question-stats/record-tossup-data.js b/database/account-info/question-stats/record-tossup-data.js
@@ -25,8 +25,12 @@ async function recordTossupData (username, data) {
     }
   }
 
-  newData.tossup_id = new ObjectId(tossup._id);
-  newData.set_id = new ObjectId(tossup.set._id);
+  try {
+    newData.tossup_id = new ObjectId(tossup._id);
+    newData.set_id = new ObjectId(tossup.set._id);
+  } catch (e) {
+    return false;
+  }
   newData.user_id = userId;
   return await tossupData.insertOne(newData);
 }

diff --git a/database/account-info/verify-email.js b/database/account-info/verify-email.js
@@ -1,15 +1,13 @@
 import { users } from './collections.js';
 
-import { ObjectId } from 'mongodb';
-
 /**
  *
- * @param {String} userId
+ * @param {ObjectId} userId
  * @returns {Promise<Result>}
  */
 async function verifyEmail (userId) {
   return await users.updateOne(
-    { _id: new ObjectId(userId) },
+    { _id: userId },
     { $set: { verifiedEmail: true } }
   );
 }

diff --git a/database/qbreader/report-question.js b/database/qbreader/report-question.js
@@ -1,33 +1,23 @@
 import { bonuses, tossups } from './collections.js';
 
-import { ObjectId } from 'mongodb';
-
 /**
  * Report question with given id to the database.
- * @param {string} _id - the id of the question to report
+ * @param {ObjectId} _id - the id of the question to report
  * @param {'wrong-category' | 'text-error' | 'answer-checking' | 'other'} reason - the reason for reporting
  * @param {string} description - a description of the problem
  * @param {boolean} [verbose=true] - whether to log the result to the console
  * @returns {Promise<boolean>} true if successful, false otherwise.
  */
 async function reportQuestion (_id, reason, description, verbose = true) {
-  await tossups.updateOne({ _id: new ObjectId(_id) }, {
-    $push: {
-      reports: {
-        reason,
-        description
-      }
-    }
-  });
+  await tossups.updateOne(
+    { _id },
+    { $push: { reports: { reason, description } } }
+  );
 
-  await bonuses.updateOne({ _id: new ObjectId(_id) }, {
-    $push: {
-      reports: {
-        reason,
-        description
-      }
-    }
-  });
+  await bonuses.updateOne(
+    { _id },
+    { $push: { reports: { reason, description } } }
+  );
 
   if (verbose) {
     console.log('Reported question with id ' + _id);

diff --git a/routes/api/admin/geoword.js b/routes/api/admin/geoword.js
@@ -44,8 +44,9 @@ router.get('/protests', async (req, res) => {
 });
 
 router.post('/resolve-protest', async (req, res) => {
-  const { buzz_id: buzzId, decision, reason } = req.body;
-  const result = await resolveProtest(new ObjectId(buzzId), decision, reason);
+  let { buzz_id: buzzId, decision, reason } = req.body;
+  try { buzzId = new ObjectId(buzzId); } catch (e) { return res.status(400).send('Invalid buzz ID'); }
+  const result = await resolveProtest(buzzId, decision, reason);
 
   if (result) {
     res.sendStatus(200);

diff --git a/routes/api/admin/update-subcategory.js b/routes/api/admin/update-subcategory.js
@@ -6,8 +6,9 @@ import { ObjectId } from 'mongodb';
 const router = Router();
 
 router.put('/', async (req, res) => {
-  const { _id, type, subcategory, alternate_subcategory: alternateSubcategory } = req.body;
-  const result = await updateSubcategory(new ObjectId(_id), type, subcategory, alternateSubcategory);
+  let { _id, type, subcategory, alternate_subcategory: alternateSubcategory } = req.body;
+  try { _id = new ObjectId(_id); } catch (e) { return res.status(400).send('Invalid ID'); }
+  const result = await updateSubcategory(_id, type, subcategory, alternateSubcategory);
 
   if (result) {
     res.sendStatus(200);

diff --git a/routes/api/bonus-by-id.js b/routes/api/bonus-by-id.js
@@ -6,20 +6,13 @@ const router = Router();
 
 router.get('/', async (req, res) => {
   if (!req.query.id) {
-    res.status(400).send('Bonus ID not specified');
-    return;
+    return res.status(400).send('Bonus ID not specified');
   }
-  let oid;
-  try {
-    oid = ObjectId(req.query.id);
-  } catch (b) {
-    res.status(400).send('Invalid Bonus ID');
-    return;
-  }
-  const bonus = await getBonus(oid);
+  let _id;
+  try { _id = new ObjectId(req.query.id); } catch (b) { return res.status(400).send('Invalid Bonus ID'); }
+  const bonus = await getBonus(_id);
   if (bonus === null) {
-    res.status(404).send(`Bonus with ID ${req.query.id} was not found`);
-    return;
+    return res.status(404).send(`Bonus with ID ${req.query.id} was not found`);
   }
 
   res.header('Access-Control-Allow-Origin', '*');

diff --git a/routes/api/report-question.js b/routes/api/report-question.js
@@ -1,20 +1,26 @@
+import { ObjectId } from 'mongodb';
 import reportQuestion from '../../database/qbreader/report-question.js';
 
 import { Router } from 'express';
 
 const router = Router();
 
 router.post('/', async (req, res) => {
-  const _id = req.body._id;
+  let _id;
+  try { _id = new ObjectId(req.body._id); } catch (e) {
+    res.header('Access-Control-Allow-Origin', '*');
+    return res.status(400).send('Invalid ID');
+  }
+
   const reason = req.body.reason ?? '';
   const description = req.body.description ?? '';
   const successful = await reportQuestion(_id, reason, description);
   if (successful) {
     res.header('Access-Control-Allow-Origin', '*');
-    res.sendStatus(200);
+    return res.sendStatus(200);
   } else {
     res.header('Access-Control-Allow-Origin', '*');
-    res.sendStatus(500);
+    return res.sendStatus(500);
   }
 });
 

diff --git a/routes/api/tossup-by-id.js b/routes/api/tossup-by-id.js
@@ -9,14 +9,9 @@ router.get('/', async (req, res) => {
     res.status(400).send('Tossup ID not specified');
     return;
   }
-  let oid;
-  try {
-    oid = ObjectId(req.query.id);
-  } catch (b) {
-    res.status(400).send('Invalid Tossup ID');
-    return;
-  }
-  const tossup = await getTossup(oid);
+  let _id;
+  try { _id = new ObjectId(req.query.id); } catch (b) { return res.status(400).send('Invalid Tossup ID'); }
+  const tossup = await getTossup(_id);
   if (tossup === null) {
     res.status(404).send(`Tossup with ID ${req.query.id} was not found`);
     return;

diff --git a/routes/api/webhook.js b/routes/api/webhook.js
@@ -31,8 +31,9 @@ router.post('/', (req, res) => {
     case 'payment_intent.succeeded': {
       const paymentIntentSucceeded = event.data.object;
       // Then define and call a function to handle the event payment_intent.succeeded
-      const { user_id: userId, packetName } = paymentIntentSucceeded.metadata;
-      recordPayment(packetName, new ObjectId(userId));
+      let { user_id: userId, packetName } = paymentIntentSucceeded.metadata;
+      try { userId = new ObjectId(userId); } catch (e) { return res.status(400).send('Invalid user ID'); }
+      recordPayment(packetName, userId);
       break;
     }
 

diff --git a/routes/auth/question-stats/single-bonus.js b/routes/auth/question-stats/single-bonus.js
@@ -6,7 +6,9 @@ import { ObjectId } from 'mongodb';
 const router = Router();
 
 router.get('/', async (req, res) => {
-  const stats = await getSingleBonusStats(new ObjectId(req.query.bonus_id));
+  let _id;
+  try { _id = new ObjectId(req.query.bonus_id); } catch (e) { return res.status(400).send('Invalid Bonus ID'); }
+  const stats = await getSingleBonusStats(_id);
   res.json({ stats });
 });
 

diff --git a/routes/auth/question-stats/single-tossup.js b/routes/auth/question-stats/single-tossup.js
@@ -6,7 +6,9 @@ import { ObjectId } from 'mongodb';
 const router = Router();
 
 router.get('/', async (req, res) => {
-  const stats = await getSingleTossupStats(new ObjectId(req.query.tossup_id));
+  let _id;
+  try { _id = new ObjectId(req.query.tossup_id); } catch (e) { return res.status(400).send('Invalid Tossup ID'); }
+  const stats = await getSingleTossupStats(_id);
   res.json({ stats });
 });
 

diff --git a/routes/auth/reset-password.js b/routes/auth/reset-password.js
@@ -7,13 +7,14 @@ import { ObjectId } from 'mongodb';
 const router = Router();
 
 router.post('/', async (req, res) => {
-  const { user_id: userId, verifyResetPassword } = req.session;
+  let { user_id: userId, verifyResetPassword } = req.session;
+  try { userId = new ObjectId(userId); } catch (e) { return res.sendStatus(400); }
   if (!verifyResetPassword) {
     res.sendStatus(401);
     return;
   }
 
-  const username = await getUsername(new ObjectId(userId));
+  const username = await getUsername(userId);
   const password = req.body.password;
   await updatePassword(username, password);
   req.session = null;

diff --git a/routes/auth/stars/is-starred-bonus.js b/routes/auth/stars/is-starred-bonus.js
@@ -9,12 +9,9 @@ const router = Router();
 router.get('/', async (req, res) => {
   const username = req.session.username;
   const userId = await getUserId(username);
-  try {
-    const bonusId = new ObjectId(req.query.bonus_id);
-    res.json({ isStarred: await isStarredBonus(userId, bonusId) });
-  } catch { // Invalid ObjectID
-    res.json({ isStarred: false });
-  }
+  let bonusId;
+  try { bonusId = new ObjectId(req.query.bonus_id); } catch { return res.status(400).send('Invalid Bonus ID'); }
+  res.json({ isStarred: await isStarredBonus(userId, bonusId) });
 });
 
 export default router;
diff --git a/routes/auth/stars/is-starred-tossup.js b/routes/auth/stars/is-starred-tossup.js
@@ -9,12 +9,9 @@ const router = Router();
 router.get('/', async (req, res) => {
   const username = req.session.username;
   const userId = await getUserId(username);
-  try {
-    const tossupId = new ObjectId(req.query.tossup_id);
-    res.json({ isStarred: await isStarredTossup(userId, tossupId) });
-  } catch { // Invalid ObjectID
-    res.json({ isStarred: false });
-  }
+  let tossupId;
+  try { tossupId = new ObjectId(req.query.tossup_id); } catch { return res.status(400).send('Invalid Tossup ID'); }
+  res.json({ isStarred: await isStarredTossup(userId, tossupId) });
 });
 
 export default router;
diff --git a/routes/auth/stars/star-bonus.js b/routes/auth/stars/star-bonus.js
@@ -9,7 +9,8 @@ const router = Router();
 router.put('/', async (req, res) => {
   const username = req.session.username;
   const userId = await getUserId(username);
-  const bonusId = new ObjectId(req.body.bonus_id);
+  let bonusId;
+  try { bonusId = new ObjectId(req.body.bonus_id); } catch { return res.status(400).send('Invalid Bonus ID'); }
   await starBonus(userId, bonusId);
   res.sendStatus(200);
 });

diff --git a/routes/auth/stars/star-tossup.js b/routes/auth/stars/star-tossup.js
@@ -9,7 +9,8 @@ const router = Router();
 router.put('/', async (req, res) => {
   const username = req.session.username;
   const userId = await getUserId(username);
-  const tossupId = new ObjectId(req.body.tossup_id);
+  let tossupId;
+  try { tossupId = new ObjectId(req.body.tossup_id); } catch { return res.status(400).send('Invalid Tossup ID'); }
   await starTossup(userId, tossupId);
   res.sendStatus(200);
 });

diff --git a/routes/auth/stars/unstar-bonus.js b/routes/auth/stars/unstar-bonus.js
@@ -9,7 +9,8 @@ const router = Router();
 router.put('/', async (req, res) => {
   const username = req.session.username;
   const userId = await getUserId(username);
-  const bonusId = new ObjectId(req.body.bonus_id);
+  let bonusId;
+  try { bonusId = new ObjectId(req.body.bonus_id); } catch { return res.status(400).send('Invalid Bonus ID'); }
   await unstarBonus(userId, bonusId);
   res.sendStatus(200);
 });

diff --git a/routes/auth/stars/unstar-tossup.js b/routes/auth/stars/unstar-tossup.js
@@ -9,7 +9,8 @@ const router = Router();
 router.put('/', async (req, res) => {
   const username = req.session.username;
   const userId = await getUserId(username);
-  const tossupId = new ObjectId(req.body.tossup_id);
+  let tossupId;
+  try { tossupId = new ObjectId(req.body.tossup_id); } catch { return res.status(400).send('Invalid Tossup ID'); }
   await unstarTossup(userId, tossupId);
   res.sendStatus(200);
 });

diff --git a/server/authentication.js b/server/authentication.js
@@ -8,6 +8,7 @@ import verifyEmail from '../database/account-info/verify-email.js';
 
 import { createHash } from 'crypto';
 import jsonwebtoken from 'jsonwebtoken';
+import { ObjectId } from 'mongodb';
 const { sign, verify } = jsonwebtoken;
 
 const baseURL = process.env.BASE_URL ?? (process.env.NODE_ENV === 'production' ? 'https://www.qbreader.org' : 'http://localhost:3000');
@@ -181,6 +182,8 @@ function verifyEmailLink (userId, token) {
       return false;
     }
 
+    try { userId = new ObjectId(userId); } catch (e) { return false; }
+
     verifyEmail(userId);
     return true;
   });

diff --git a/test/database.test.js b/test/database.test.js
@@ -11,6 +11,7 @@ import reportQuestion from '../database/qbreader/report-question.js';
 
 import { assert } from 'chai';
 import mocha from 'mocha';
+import { ObjectId } from 'mongodb';
 
 const packetNumbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24];
 
@@ -57,7 +58,7 @@ async function testTiming (count) {
       mocha.it('reportQuestion', async () => {
         const results = [];
         for (let i = 0; i < count; i++) {
-          results.push(reportQuestion('630020e3cab8fa6d1490b8ea', 'other', 'test'));
+          results.push(reportQuestion(new ObjectId('630020e3cab8fa6d1490b8ea'), 'other', 'test'));
         }
         await Promise.all(results);
       });