[Sync] Validate the whole pulled changeset before applying any of it

CHANGELOG-Unreleased.md

@@ -8,6 +8,7 @@
 
 ### Fixes
 
+- [Sync] A pulled changeset is now validated in full before any of it is applied (table and record shape, string ids, ids listed more than once in a table's created/updated/deleted). Before, an invalid record found halfway through left already-prepared records broken in memory, and updating them failed until the app restarted. Records with a non-string `id` are now rejected instead of being given a random id
 - [LokiJS] Multitab sync issue fix
 - [Android] Added linker flag for building with 16kB page alignment
 - [TS] make catchError visible to typescript

src/sync/impl/__tests__/applyRemote.test.js

@@ -476,9 +476,151 @@ describe('applyRemoteChanges', () => {
     await expectUpdateFails({ id: 'foo', _status: 'created' })
     await expectUpdateFails({ id: 'foo', _changed: 'bla' })
     await expectUpdateFails({ foo: 'bar' })
+    await expectUpdateFails({ id: 1 })
 
     expect(await fetchLocalChanges(database)).toEqual(emptyLocalChanges)
   })
+  describe('changeset validation', () => {
+    const expectNothingTouched = async (database, pSynced, tSynced) => {
+      const projects = database.get('mock_projects')
+      const tasks = database.get('mock_tasks')
+      expect(await getRaw(projects, 'pSynced')).toEqual(pSynced)
+      expect(await getRaw(tasks, 'tSynced')).toEqual(tSynced)
+
+      // cached records must still be usable
+      await database.write(async () => {
+        const project = await projects.find('pSynced')
+        expect(project._raw).toEqual(pSynced)
+        await project.update((p) => {
+          p.name = 'local again'
+        })
+      })
+    }
+
+    it('validates the whole changeset before preparing any record', async () => {
+      const { database } = makeDatabase()
+      await makeLocalChanges(database)
+      const pSynced = { ...(await getRaw(database.get('mock_projects'), 'pSynced')) }
+      const tSynced = { ...(await getRaw(database.get('mock_tasks'), 'tSynced')) }
+
+      await expectToRejectWithMessage(
+        testApplyRemoteChanges(database, {
+          mock_projects: { updated: [{ id: 'pSynced', name: 'remote' }] },
+          mock_tasks: { updated: [{ id: 'tSynced', name: 'remote', _status: 'synced' }] },
+        }),
+        /invalid raw record/i,
+      )
+      await expectNothingTouched(database, pSynced, tSynced)
+    })
+    it('rejects records that appear more than once in a table', async () => {
+      const { database } = makeDatabase()
+      await makeLocalChanges(database)
+      const pSynced = { ...(await getRaw(database.get('mock_projects'), 'pSynced')) }
+      const tSynced = { ...(await getRaw(database.get('mock_tasks'), 'tSynced')) }
+
+      const expectDuplicateFails = (taskChanges) =>
+        expectToRejectWithMessage(
+          testApplyRemoteChanges(database, {
+            mock_projects: { updated: [{ id: 'pSynced', name: 'remote' }] },
+            mock_tasks: taskChanges,
+          }),
+          /mock_tasks#tSynced appears more than once/,
+        )
+
+      await expectDuplicateFails({
+        updated: [{ id: 'tSynced', name: 'remote' }],
+        deleted: ['tSynced'],
+      })
+      await expectDuplicateFails({
+        created: [{ id: 'tSynced', name: 'remote' }],
+        updated: [{ id: 'tSynced', name: 'remote' }],
+      })
+      await expectDuplicateFails({
+        updated: [
+          { id: 'tSynced', name: 'remote' },
+          { id: 'tSynced', name: 'remote 2' },
+        ],
+      })
+      await expectDuplicateFails({ deleted: ['tSynced', 'tSynced'] })
+      await expectNothingTouched(database, pSynced, tSynced)
+    })
+    it('validates inherited table changes too', async () => {
+      const { database } = makeDatabase()
+      await makeLocalChanges(database)
+      const pSynced = { ...(await getRaw(database.get('mock_projects'), 'pSynced')) }
+      const tSynced = { ...(await getRaw(database.get('mock_tasks'), 'tSynced')) }
+
+      // for-in (used when applying changes) enumerates inherited keys
+      const changes = Object.create({
+        mock_tasks: {
+          created: [],
+          updated: [{ id: 'tSynced', name: 'remote', _status: 'synced' }],
+          deleted: [],
+        },
+      })
+      changes.mock_projects = {
+        created: [],
+        updated: [{ id: 'pSynced', name: 'remote' }],
+        deleted: [],
+      }
+      await expectToRejectWithMessage(
+        database.write(() => applyRemoteChanges(changes, { db: database })),
+        /invalid raw record/i,
+      )
+      await expectNothingTouched(database, pSynced, tSynced)
+    })
+    it('rejects malformed table changes and ids', async () => {
+      const { database } = makeDatabase()
+      await makeLocalChanges(database)
+      const pSynced = { ...(await getRaw(database.get('mock_projects'), 'pSynced')) }
+      const tSynced = { ...(await getRaw(database.get('mock_tasks'), 'tSynced')) }
+
+      const apply = (taskChanges) =>
+        database.write(() =>
+          applyRemoteChanges(
+            {
+              mock_projects: {
+                created: [],
+                updated: [{ id: 'pSynced', name: 'remote' }],
+                deleted: [],
+              },
+              mock_tasks: taskChanges,
+            },
+            { db: database },
+          ),
+        )
+
+      await expectToRejectWithMessage(
+        apply({ created: [], updated: {}, deleted: [] }),
+        /invalid changeset supplied to sync for table mock_tasks/i,
+      )
+      await expectToRejectWithMessage(apply(null), /invalid changeset/i)
+      // changes are copied with object spread, which drops inherited and non-enumerable keys
+      await expectToRejectWithMessage(
+        apply(Object.create({ created: [], updated: [], deleted: [] })),
+        /invalid changeset supplied to sync for table mock_tasks/i,
+      )
+      const nonEnumerable = { created: [], updated: [] }
+      Object.defineProperty(nonEnumerable, 'deleted', { value: [], enumerable: false })
+      await expectToRejectWithMessage(
+        apply(nonEnumerable),
+        /invalid changeset supplied to sync for table mock_tasks/i,
+      )
+      await expectToRejectWithMessage(
+        apply({ created: [], updated: [], deleted: [{ id: 'tSynced' }] }),
+        /deleted ids in table mock_tasks must be strings/i,
+      )
+      await expectToRejectWithMessage(
+        apply({ created: [], updated: [{ id: 1, name: 'remote' }], deleted: [] }),
+        /invalid raw record/i,
+      )
+      await expectToRejectWithMessage(
+        apply({ created: [JSON.parse('{"id": "t1", "__proto__": {}}')], updated: [], deleted: [] }),
+        /__proto__/i,
+      )
+      await expectNothingTouched(database, pSynced, tSynced)
+    })
+  })
   it(`safely skips collections that don't exist`, async () => {
     const { database } = makeDatabase()
 

src/sync/impl/applyRemote.js

@@ -253,9 +253,79 @@ function validateRemoteRaw(raw: DirtyRaw): void {
   // TODO: I think other code is actually resilient enough to handle illegal _status and _changed
   // would be best to change that part to a warning - but tests are needed
   invariant(
-    raw && typeof raw === 'object' && 'id' in raw && !('_status' in raw || '_changed' in raw),
-    `[Sync] Invalid raw record supplied to Sync. Records must be objects, must have an 'id' field, and must NOT have a '_status' or '_changed' fields`,
+    raw &&
+      typeof raw === 'object' &&
+      typeof raw.id === 'string' &&
+      !('_status' in raw || '_changed' in raw),
+    `[Sync] Invalid raw record supplied to Sync. Records must be objects, must have a string 'id' field, and must NOT have a '_status' or '_changed' fields`,
   )
+  invariant(
+    // $FlowFixMe
+    !Object.prototype.hasOwnProperty.call(raw, '__proto__'),
+    'Malicious dirtyRaw detected - contains a __proto__ key',
+  )
+}
+
+// Table changes are copied with object spread when applied, which only keeps own enumerable keys
+const isOwnArray = (obj: Object, key: string): boolean =>
+  // $FlowFixMe
+  Object.prototype.propertyIsEnumerable.call(obj, key) && Array.isArray(obj[key])
+
+// Checks the whole pulled changeset before anything is prepared. Preparing a record mutates its
+// cached JS model, so if a bad record were only found halfway through, the records prepared before
+// it would stay out of sync with the database (and couldn't be updated until the app restarts).
+function validateRemoteChanges(remoteChanges: SyncDatabaseChangeSet, db: Database): void {
+  invariant(
+    remoteChanges && typeof remoteChanges === 'object',
+    `[Sync] Invalid changeset supplied to Sync. Changes must be an object of per-table changes`,
+  )
+
+  // Enumerating the same way as getAllRecordsToApply (filterObj uses for-in), so that every table
+  // that is applied is validated first
+  // eslint-disable-next-line no-restricted-syntax, guard-for-in
+  for (const tableName in remoteChanges) {
+    // Unknown tables are skipped (for forward-compatibility), so their changes are never applied
+    if (!db.get((tableName: any))) {
+      // eslint-disable-next-line no-continue
+      continue
+    }
+
+    const changes = remoteChanges[(tableName: any)]
+    invariant(
+      changes &&
+        typeof changes === 'object' &&
+        isOwnArray(changes, 'created') &&
+        isOwnArray(changes, 'updated') &&
+        isOwnArray(changes, 'deleted'),
+      `[Sync] Invalid changeset supplied to Sync for table ${tableName}. Table changes must be an object with own 'created', 'updated', and 'deleted' arrays`,
+    )
+
+    // mutating - perf critical
+    const seenIds: Set<RecordId> = new Set()
+    const checkUnique = (id: RecordId): void => {
+      invariant(
+        !seenIds.has(id),
+        `[Sync] Invalid changeset supplied to Sync. Record ${tableName}#${id} appears more than once in created/updated/deleted`,
+      )
+      seenIds.add(id)
+    }
+
+    changes.created.forEach((raw) => {
+      validateRemoteRaw(raw)
+      checkUnique(raw.id)
+    })
+    changes.updated.forEach((raw) => {
+      validateRemoteRaw(raw)
+      checkUnique(raw.id)
+    })
+    changes.deleted.forEach((id) => {
+      invariant(
+        typeof id === 'string',
+        `[Sync] Invalid changeset supplied to Sync. Deleted ids in table ${tableName} must be strings`,
+      )
+      checkUnique(id)
+    })
+  }
 }
 
 function prepareApplyRemoteChangesToCollection<T: Model>(
@@ -285,7 +355,6 @@ function prepareApplyRemoteChangesToCollection<T: Model>(
 
   // Insert and update records
   created.forEach((raw) => {
-    validateRemoteRaw(raw)
     const currentRecord = recordsMap.get(raw.id)
     if (currentRecord) {
       logError(
@@ -307,7 +376,6 @@ function prepareApplyRemoteChangesToCollection<T: Model>(
   })
 
   updated.forEach((raw) => {
-    validateRemoteRaw(raw)
     const currentRecord = recordsMap.get(raw.id)
 
     if (currentRecord) {
@@ -390,6 +458,7 @@ export default async function applyRemoteChanges(
 ): Promise<void> {
   const { db, _unsafeBatchPerCollection } = context
 
+  validateRemoteChanges(remoteChanges, db)
   const recordsToApply = await getAllRecordsToApply(remoteChanges, context)
 
   // Perform steps concurrently