diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index 9d43e2bdd..4ecf44ab8 100644 --- a/CHANGELOG-Unreleased.md +++ b/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 diff --git a/src/sync/impl/__tests__/applyRemote.test.js b/src/sync/impl/__tests__/applyRemote.test.js index 45f06653b..fa3f59c52 100644 --- a/src/sync/impl/__tests__/applyRemote.test.js +++ b/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() diff --git a/src/sync/impl/applyRemote.js b/src/sync/impl/applyRemote.js index c8a988172..536ace7eb 100644 --- a/src/sync/impl/applyRemote.js +++ b/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 = 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( @@ -285,7 +355,6 @@ function prepareApplyRemoteChangesToCollection( // Insert and update records created.forEach((raw) => { - validateRemoteRaw(raw) const currentRecord = recordsMap.get(raw.id) if (currentRecord) { logError( @@ -307,7 +376,6 @@ function prepareApplyRemoteChangesToCollection( }) updated.forEach((raw) => { - validateRemoteRaw(raw) const currentRecord = recordsMap.get(raw.id) if (currentRecord) { @@ -390,6 +458,7 @@ export default async function applyRemoteChanges( ): Promise { const { db, _unsafeBatchPerCollection } = context + validateRemoteChanges(remoteChanges, db) const recordsToApply = await getAllRecordsToApply(remoteChanges, context) // Perform steps concurrently