From fef82f6407eda86e70305d44dc17822abd5b7801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6hler?= Date: Mon, 2 Nov 2026 10:39:33 +0100 Subject: [PATCH] Validate the whole pulled changeset before applying any of it * Validate the whole pulled changeset before applying any of it applyRemoteChanges validated records one by one while preparing them, so a malformed record (or one id listed in both updated and deleted) threw after earlier records had already been prepared. Those records kept their pending state and mutated raw in the cache, so later update() calls on them failed until the app restarted. The changeset is now checked up front: table shape, record shape (string id, no _status/_changed, no __proto__ key), string deleted ids, and duplicate ids across created/updated/deleted. Unknown tables are still skipped for forward-compatibility. Co-Authored-By: Claude Opus 5.5 * Validate the same tables that are applied, and add a changelog entry getAllRecordsToApply enumerates tables with for-in (through filterObj), which includes inherited keys, while validation used Object.keys. Validation now enumerates the same way, so no applied table skips it. Co-Authored-By: Claude Opus 5.5 * Require own enumerable created/updated/deleted arrays Table changes are copied with object spread when applied, which drops inherited and non-enumerable keys. Changes whose arrays were only inherited passed validation and then failed after earlier tables were prepared. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG-Unreleased.md | 1 + src/sync/impl/__tests__/applyRemote.test.js | 142 ++++++++++++++++++++ src/sync/impl/applyRemote.js | 77 ++++++++++- 3 files changed, 216 insertions(+), 4 deletions(-) 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