REST card update: sort is a number, isOverTime can be turned off, a description can be cleared.

#6877 made the card PUT write `sort` whenever it is present, so 0 works -
and so would null, '' or 'abc', written as the card's position. The same
handler read `isOverTime` and `description` by truthiness, so false could
not be set and an empty description could not clear the old one.

parseCardScalarFields() in server/lib/restCardHelpers.js now decides all
three by presence and checks them before the first write: sort must be a
finite number (a form-encoded '0' becomes 0), isOverTime true/false or
'true'/'false', description a string (null clears it). Anything else is a
400 and the card is left as it was, rather than half of the edit applied.

tests/restCardSort.test.cjs drives the handler for the accepted values,
refuses the invalid ones with no writes, and fails if any of the three is
read by truthiness again.

Thanks to AhmedLukman and xet7 !

server/lib/restCardHelpers.js

@@ -4,6 +4,7 @@
 //   #5398 normalizeMoveParams  - one consistent set of board-move params
 //   #5399 computeTopSort        - sort value that lands a card on TOP of a list
 //   #5537 parseCardDate         - parse an ISO date string into a Date that persists
+//   #6877 parseCardScalarFields - sort 0, isOverTime false, an empty description
 
 /**
  * #5399 Moving a card to another list via the API must put it on TOP of the
@@ -103,3 +104,42 @@ export function parseCardDate(value) {
   }
   return null;
 }
+
+/**
+ * The card PUT fields whose valid values include a falsy one: sort 0,
+ * isOverTime false and an empty description (#6877). A truthiness check
+ * skipped them, so 0 could not be written, an overtime flag could not be
+ * turned off and a description could not be cleared. Presence decides now,
+ * and the value is checked before the handler writes anything, so a bad
+ * value is a 400 rather than a half-applied edit.
+ *
+ * Form-encoded clients send strings, so '0' is a number and 'true'/'false'
+ * are booleans here, as in JSON.
+ *
+ * @param {object} body the request body
+ * @returns {{ error: string } | { set: object }} what to $set, or why not
+ */
+export function parseCardScalarFields(body) {
+  const has = key => !!body && Object.prototype.hasOwnProperty.call(body, key) && body[key] !== undefined;
+  const set = {};
+  if (has('sort')) {
+    const raw = body.sort;
+    const sort = typeof raw === 'number' ? raw
+      : (typeof raw === 'string' && raw.trim() !== '' ? Number(raw) : NaN);
+    if (!Number.isFinite(sort)) return { error: 'Invalid sort' };
+    set.sort = sort;
+  }
+  if (has('isOverTime')) {
+    const raw = body.isOverTime;
+    if (raw === true || raw === 'true') set.isOverTime = true;
+    else if (raw === false || raw === 'false') set.isOverTime = false;
+    else return { error: 'Invalid isOverTime' };
+  }
+  if (has('description')) {
+    const raw = body.description;
+    if (raw === null) set.description = '';
+    else if (typeof raw === 'string') set.description = raw;
+    else return { error: 'Invalid description' };
+  }
+  return { set };
+}

server/models/cards.js

@@ -11,7 +11,7 @@ import { add, now } from '/imports/lib/dateUtils';
 import { Authentication } from '/server/authentication';
 import { sendJsonResult } from '/server/apiMiddleware';
 import { allowIsBoardMember, allowIsBoardMemberWithWriteAccess, computeSortForIndex, mergeLabelIds, canAssignCardMember, isCardDateClear } from '/server/lib/utils';
-import { computeTopSort, normalizeMoveParams, parseCardDate } from '/server/lib/restCardHelpers';
+import { computeTopSort, normalizeMoveParams, parseCardDate, parseCardScalarFields } from '/server/lib/restCardHelpers';
 const { coerceRestArrayParam } = require('/server/lib/restArrayParam');
 const { applyCardBoardConsistency } = require('/server/lib/cardBoardConsistency');
 const { stampCardListEntry } = require('/models/lib/cardListEntry');
@@ -1787,6 +1787,13 @@ WebApp.handlers.put(
       sendJsonResult(res, { code: 404, data: { error: 'Card not found' } });
       return;
     }
+    // sort 0, isOverTime false and '' are values, not absences (#6877); checked
+    // before the first write, so a bad one leaves the card as it was.
+    const scalars = parseCardScalarFields(req.body);
+    if (scalars.error) {
+      sendJsonResult(res, { code: 400, data: { error: scalars.error } });
+      return;
+    }
 
     if (req.body.title) {
       const newTitle =
@@ -1800,11 +1807,10 @@ WebApp.handlers.put(
       );
       updated = true;
     }
-    // Zero is a valid sort value, so check for a supplied field rather than truthiness.
-    if (req.body.sort !== undefined) {
+    if ('sort' in scalars.set) {
       await Cards.direct.updateAsync(
         { _id: paramCardId, listId: paramListId, boardId: paramBoardId, archived: false },
-        { $set: { sort: req.body.sort } },
+        { $set: { sort: scalars.set.sort } },
       );
       updated = true;
     }
@@ -1827,10 +1833,10 @@ WebApp.handlers.put(
       );
       updated = true;
     }
-    if (req.body.description) {
+    if ('description' in scalars.set) {
       await Cards.direct.updateAsync(
         { _id: paramCardId, listId: paramListId, boardId: paramBoardId, archived: false },
-        { $set: { description: req.body.description } },
+        { $set: { description: scalars.set.description } },
       );
       updated = true;
     }
@@ -1944,10 +1950,10 @@ WebApp.handlers.put(
       );
       updated = true;
     }
-    if (req.body.isOverTime) {
+    if ('isOverTime' in scalars.set) {
       await Cards.direct.updateAsync(
         { _id: paramCardId, listId: paramListId, boardId: paramBoardId, archived: false },
-        { $set: { isOverTime: req.body.isOverTime } },
+        { $set: { isOverTime: scalars.set.isOverTime } },
       );
       updated = true;
     }

tests/restCardSort.test.cjs

@@ -10,7 +10,7 @@ const route = '/api/boards/:boardId/lists/:listId/cards/:cardId';
 const plain = value => JSON.parse(JSON.stringify(value));
 
 async function fixture() {
-  const { normalizeMoveParams } = await import('../server/lib/restCardHelpers.js');
+  const { normalizeMoveParams, parseCardScalarFields } = await import('../server/lib/restCardHelpers.js');
   const writes = [];
   const card = { _id: 'card', boardId: 'board', listId: 'list', archived: false, sort: 5 };
   let handler;
@@ -22,6 +22,7 @@ async function fixture() {
   vm.runInNewContext(source.slice(start, end + '\n);'.length), {
     WebApp: { handlers: { put(url, fn) { assert.equal(url, route); handler = fn; } } },
     normalizeMoveParams,
+    parseCardScalarFields,
     Authentication: { async checkBoardWriteAccess(userId) {
       if (userId !== 'member') throw new Error('Forbidden');
     } },
@@ -52,17 +53,73 @@ async function fixture() {
   };
 }
 
-for (const sort of [0, 5, -1, 0.5, '0']) {
+// A form-encoded client sends '0'; it is written as the number 0, the type
+// every other sort value has, so ordering compares numbers with numbers.
+for (const [sort, written] of [[0, 0], [5, 5], [-1, -1], [0.5, 0.5], ['0', 0], [' 2.5 ', 2.5]]) {
   test(`card PUT writes sort ${JSON.stringify(sort)} through the scoped update`, async () => {
     const f = await fixture();
     assert.deepEqual(await f.put({ sort }), { code: 200, data: { _id: 'card' } });
     assert.deepEqual(f.writes, [{
       selector: { _id: 'card', listId: 'list', boardId: 'board', archived: false },
-      modifier: { $set: { sort } },
+      modifier: { $set: { sort: written } },
     }]);
   });
 }
 
+// Negative: since presence, not truthiness, decides, a value that is not a
+// number would otherwise be written as the card's position.
+for (const sort of [null, '', 'abc', true, {}, [], 'Infinity', Number.NaN]) {
+  test(`card PUT refuses sort ${JSON.stringify(sort)} with 400 and writes nothing`, async () => {
+    const f = await fixture();
+    assert.deepEqual(await f.put({ sort, title: 'Renamed' }), { code: 400, data: { error: 'Invalid sort' } });
+    assert.deepEqual(f.writes, [], 'not even the valid title: the edit is refused whole');
+  });
+}
+
+for (const [isOverTime, written] of [[false, false], ['false', false], [true, true], ['true', true]]) {
+  test(`card PUT writes isOverTime ${JSON.stringify(isOverTime)}`, async () => {
+    const f = await fixture();
+    assert.equal((await f.put({ isOverTime })).code, 200);
+    assert.deepEqual(f.writes.map(w => w.modifier), [{ $set: { isOverTime: written } }]);
+  });
+}
+
+for (const isOverTime of [null, 0, 'yes', {}]) {
+  test(`card PUT refuses isOverTime ${JSON.stringify(isOverTime)} with 400`, async () => {
+    const f = await fixture();
+    assert.deepEqual(await f.put({ isOverTime }), { code: 400, data: { error: 'Invalid isOverTime' } });
+    assert.deepEqual(f.writes, []);
+  });
+}
+
+for (const [description, written] of [['', ''], [null, ''], ['New text', 'New text']]) {
+  test(`card PUT writes description ${JSON.stringify(description)}`, async () => {
+    const f = await fixture();
+    assert.equal((await f.put({ description })).code, 200);
+    assert.deepEqual(f.writes.map(w => w.modifier), [{ $set: { description: written } }]);
+  });
+}
+
+for (const description of [5, {}, ['x']]) {
+  test(`card PUT refuses description ${JSON.stringify(description)} with 400`, async () => {
+    const f = await fixture();
+    assert.deepEqual(await f.put({ description }), { code: 400, data: { error: 'Invalid description' } });
+    assert.deepEqual(f.writes, []);
+  });
+}
+
+// Every field of this handler that can be legitimately falsy goes through the
+// presence check; none is read with a bare `if (req.body.<field>)` again.
+test('sort, isOverTime and description are not read by truthiness', () => {
+  const source = fs.readFileSync(path.join(__dirname, '../server/models/cards.js'), 'utf8');
+  const start = source.indexOf(`WebApp.handlers.put(\n  '${route}'`);
+  const body = source.slice(start, source.indexOf('\n);', start));
+  for (const field of ['sort', 'isOverTime', 'description']) {
+    assert.ok(!new RegExp(`if \\(req\\.body\\.${field}\\)`).test(body), field);
+    assert.ok(new RegExp(`'${field}' in scalars\\.set`).test(body), `${field} uses the parsed value`);
+  }
+});
+
 test('an omitted sort does not overwrite the position during another edit', async () => {
   const f = await fixture();
   assert.equal((await f.put({ title: 'Renamed' })).code, 200);