diff --git a/resources/crdt.ts b/resources/crdt.ts index dd934db8fe..0df776b35e 100644 --- a/resources/crdt.ts +++ b/resources/crdt.ts @@ -1,11 +1,18 @@ +// Single definition of the numeric `add` fold, shared by the storage-time merge (crdt.add, via +// updateAndFreeze) and the read/serialize path (tracked.ts Addition.update). One implementation +// keeps the two from diverging: a numeric string was concatenated on the storage path but coerced +// on the read path, and a bigint folded on the storage path but threw (`+bigint`) on the read path. +export function addValues(previousValue: any, value: any) { + if (typeof previousValue === 'bigint') return previousValue + BigInt(value); + if (previousValue == null) return value; // no prior value: the add establishes it + const previous = +previousValue; // coerce so a numeric string adds instead of concatenating + if (isNaN(previous)) return value; // non-numeric prior: the add establishes it + // A number-typed field stays a number even if a stray bigint delta arrives, rather than throwing + // a number+bigint TypeError on the apply path. + return previous + (typeof value === 'bigint' ? Number(value) : value); +} export function add(record, property, action) { - const previousValue = record[property]; - if (typeof previousValue === 'bigint') { - record[property] = previousValue + BigInt(action.value); - } else if (isNaN(record[property])) record[property] = action.value; - else { - record[property] = previousValue + action.value; - } + record[property] = addValues(record[property], action.value); } add.reverse = function (record, property, action) { const previousValue = record[property]; @@ -15,9 +22,16 @@ add.reverse = function (record, property, action) { record[property] = previousValue - action.value; } }; -const operations = { +// The CRDT operation registry. Exported so the storage/apply path (tracked.ts updateAndFreeze) +// resolves ops against this explicit set rather than the module's export namespace — otherwise a +// crafted/corrupt `__op__` naming any exported function (addValues, getRecordAtTime, …) would +// resolve truthy and be invoked with the wrong arguments (throwing and wedging the apply path, or +// silently corrupting the field). applyReverse/applyForward below already resolve through this. +// Null-prototype so a crafted `__op__` naming an Object.prototype member (toString, valueOf, +// constructor, …) resolves to undefined rather than an inherited function. +export const operations = Object.assign(Object.create(null), { add, -}; +}); /** * Rebuild a record update that has a timestamp before the provided newer update @@ -179,30 +193,22 @@ export function getRecordAtTime(currentEntry, timestamp, store, tableId: number, const boundaryEntry = auditStore.get(auditTime, tableId, recordId); if (boundaryEntry?.type === 'delete') return null; } - // some patches may leave properties in an unknown state, so we need to fill in the blanks - // first we determine if there any unknown properties - // then continue to iterate back through the audit history, filling in the blanks - while (unknowns.size > 0 && auditTime > 0) { - const auditEntry = auditStore.get(auditTime, tableId, recordId); - // The history needed to resolve the remaining unknowns may have been pruned away; stop - // rather than dereferencing a missing entry (mirrors the reverse-walk guard above). - if (!auditEntry) break; - let priorRecord: any; - switch (auditEntry.type) { - case 'put': - priorRecord = auditEntry.getValue(store); - break; - case 'patch': - priorRecord = auditEntry.getValue(store); - break; - } - for (const key in priorRecord) { - if (unknowns.has(key)) { - record[key] = priorRecord[key]; - unknowns.delete(key); + // A reversed patch that set a field to a plain value can't be undone (a plain set has no inverse), + // so those fields are left "unknown": their value at `timestamp` must come from older history. + // Reconstruct that state forward from the nearest base so CRDT ops accumulate correctly — e.g. a + // counter built up by `add` patches and later overwritten by a plain set. The prior code copied + // the nearest audit entry's raw field, which for a patch is an unresolved `{ __op__ }` object or a + // single delta rather than the folded value at `timestamp`. If the history needed to reconstruct a + // key is unavailable (pruned), the key keeps its live value (best effort, as before). + if (unknowns.size > 0 && auditTime > 0) { + const priorRecord = reconstructForward(auditStore, store, tableId, recordId, auditTime, timestamp); + if (priorRecord) { + for (const key of unknowns) { + // Object.hasOwn, not `in`: an unknown field named like a prototype member (toString, + // constructor, …) must not be filled from priorRecord's prototype chain. + if (Object.hasOwn(priorRecord, key)) record[key] = priorRecord[key]; } } - auditTime = auditEntry.previousVersion; } // finally return the record in the state it was at the requested timestamp return record; diff --git a/resources/tracked.ts b/resources/tracked.ts index 1446dde85f..ad2c500251 100644 --- a/resources/tracked.ts +++ b/resources/tracked.ts @@ -1,6 +1,7 @@ import { ClientError } from '../utility/errors/hdbError.ts'; import * as crdtOperations from './crdt.ts'; import { Blob } from './blob.ts'; +import * as harperLogger from '../utility/logging/harper_logger.ts'; function getChanges(target) { let changes = target.getChanges(); @@ -423,9 +424,18 @@ export function updateAndFreeze(target, changes = target.getChanges?.()) { let value = changes[key]; if (value && typeof value === 'object') { if (value.__op__) { - const operation = crdtOperations[value?.__op__]; - if (!operation) throw new Error('Invalid CRDT operation ' + value.__op__); - else operation(mergedUpdatedObject, key, value); + // Resolve against the explicit operation registry, not the crdt.ts export namespace, + // so a crafted/corrupt `__op__` naming another export can't be invoked as an operation. + const operation = crdtOperations.operations[value.__op__]; + if (operation) operation(mergedUpdatedObject, key, value); + else { + // An unrecognized CRDT operation — corruption, or an op type from a newer node this + // version can't apply. This runs on the write/replication apply path, where throwing + // would abort the commit and can wedge a subscription, so skip the op instead: the + // field keeps its base value and a full-copy re-converges the record. Read-path + // reconstruction (crdt.applyForward/applyReverse) still throws loudly by design. + harperLogger.warn(`Skipping unrecognized CRDT operation "${value.__op__}" on property "${key}"`); + } continue; } else value = updateAndFreeze(value); } @@ -584,6 +594,7 @@ export class Addition { this.value = value; } update(previousValue) { - return (+previousValue || 0) + this.value; + // Shared with crdt.add so the read path folds identically to the storage path. + return crdtOperations.addValues(previousValue, this.value); } } diff --git a/unitTests/resources/crdt.test.js b/unitTests/resources/crdt.test.js index 3d33b8a3d7..5067c368ad 100644 --- a/unitTests/resources/crdt.test.js +++ b/unitTests/resources/crdt.test.js @@ -1,5 +1,5 @@ const assert = require('assert'); -const { getRecordAtTime, applyForward } = require('#src/resources/crdt'); +const { getRecordAtTime, applyForward, addValues } = require('#src/resources/crdt'); // Build a mock audit history and the `store` shape getRecordAtTime expects. Each event is // { version, type: 'put'|'patch'|'delete', value, previousVersion }. The audit store resolves an @@ -172,6 +172,50 @@ describe('crdt getRecordAtTime', () => { assert.deepStrictEqual(getRecordAtTime(current, 10, store, 1, 'N'), { id: 'N', v: 1 }); }); }); + + it('reconstructs a counter later overwritten by a plain set without leaking the op object', () => { + // put(count:5) -> patch(+3) [count=8] -> patch(count:100, plain overwrite) -> current. + // The reverse walk marks `count` unknown at the plain set (a plain set has no inverse). Filling + // it must forward-replay the add to 8; the prior code copied the nearest audit entry's raw + // field and returned the op object { __op__: 'add', value: 3 } instead of the value. + const events = [ + { version: 10, type: 'put', value: { id: 'X', count: 5 }, previousVersion: 0 }, + { version: 20, type: 'patch', value: { count: { __op__: 'add', value: 3 } }, previousVersion: 10 }, + { version: 30, type: 'patch', value: { count: 100 }, previousVersion: 20 }, + ]; + const store = makeStore(events); + const current = currentEntry({ id: 'X', count: 100 }, 30); + assert.deepStrictEqual(getRecordAtTime(current, 25, store, 1, 'X'), { id: 'X', count: 8 }); + }); + + it('does not fill an unknown field from a prototype member (own-property check)', () => { + // put(v:1, no `toString`) -> patch(v:2) -> patch(toString:'b') -> current. `toString` is + // unknown at t=25 and is NOT an own key of the reconstructed record, so it must keep its live + // value rather than being assigned Object.prototype.toString (which `key in priorRecord` did). + const events = [ + { version: 10, type: 'put', value: { id: 'P', v: 1 }, previousVersion: 0 }, + { version: 20, type: 'patch', value: { v: 2 }, previousVersion: 10 }, + { version: 30, type: 'patch', value: { toString: 'b' }, previousVersion: 20 }, + ]; + const store = makeStore(events); + const current = currentEntry({ id: 'P', v: 2, toString: 'b' }, 30); + const result = getRecordAtTime(current, 25, store, 1, 'P'); + assert.strictEqual(result.toString, 'b'); + assert.strictEqual(typeof result.toString, 'string'); + }); + + it('fills an unknown field from an older plain put when reconstructing forward', () => { + // put(status:'a', v:1) -> patch(v:2) -> patch(status:'b') -> current. At t=25 status is 'a' + // (the plain set at 30 is unknown to the reverse walk and resolved from the put at 10). + const events = [ + { version: 10, type: 'put', value: { id: 'S', status: 'a', v: 1 }, previousVersion: 0 }, + { version: 20, type: 'patch', value: { v: 2 }, previousVersion: 10 }, + { version: 30, type: 'patch', value: { status: 'b' }, previousVersion: 20 }, + ]; + const store = makeStore(events); + const current = currentEntry({ id: 'S', status: 'b', v: 2 }, 30); + assert.deepStrictEqual(getRecordAtTime(current, 25, store, 1, 'S'), { id: 'S', status: 'a', v: 2 }); + }); }); describe('crdt applyForward', () => { @@ -184,4 +228,47 @@ describe('crdt applyForward', () => { it('throws on an unsupported operation', () => { assert.throws(() => applyForward({}, { x: { __op__: 'multiply', value: 2 } }), /Unsupported operation multiply/); }); + + it('does not resolve an Object.prototype member as an operation', () => { + // The registry is null-prototype, so `__op__: 'toString'` (a prototype member on a plain + // object) resolves to undefined and is rejected like any other unsupported op, rather than + // invoking Object.prototype.toString. + assert.throws(() => applyForward({}, { x: { __op__: 'toString', value: 2 } }), /Unsupported operation toString/); + }); +}); + +describe('crdt addValues', () => { + // Single fold shared by the storage path (crdt.add) and the read/serialize path + // (tracked.ts Addition.update), so the two can no longer diverge. + it('adds two numbers', () => { + assert.strictEqual(addValues(5, 3), 8); + }); + + it('establishes the value when there is no prior numeric value', () => { + assert.strictEqual(addValues(undefined, 3), 3); + assert.strictEqual(addValues(NaN, 3), 3); + assert.strictEqual(addValues('not a number', 3), 3); + }); + + it('coerces a numeric-string prior value instead of concatenating', () => { + // Regression: crdt.add did "5" + 3 = "53" while the read path coerced to 8. + assert.strictEqual(addValues('5', 3), 8); + }); + + it('folds bigint prior values without throwing', () => { + // Regression: the read path (Addition.update) did +bigint and threw a TypeError. + assert.strictEqual(addValues(5n, 3), 8n); + }); + + it('establishes the value from a null/undefined prior, including a bigint delta', () => { + assert.strictEqual(addValues(null, 3), 3); + assert.strictEqual(addValues(undefined, 3), 3); + assert.strictEqual(addValues(null, 3n), 3n); // would throw `0 + 3n` without the null guard + }); + + it('keeps a number field a number when a stray bigint delta arrives', () => { + // number + bigint would otherwise throw a TypeError on the apply path. + assert.strictEqual(addValues(5, 3n), 8); + assert.strictEqual(typeof addValues(5, 3n), 'number'); + }); }); diff --git a/unitTests/resources/tracked.test.js b/unitTests/resources/tracked.test.js index 137cabb907..5c26205de5 100644 --- a/unitTests/resources/tracked.test.js +++ b/unitTests/resources/tracked.test.js @@ -6,6 +6,7 @@ const { collapseData, GenericTrackedObject, } = require('#src/resources/tracked'); +const harperLogger = require('#src/utility/logging/harper_logger'); describe('Tracked Object', () => { let source = { str: 'string', @@ -89,3 +90,53 @@ describe('Tracked Object', () => { assert.equal(collapseData(instance).arrayOfStrings[2], 'another string'); }); }); + +describe('updateAndFreeze CRDT operations', () => { + it('applies a recognized add operation', () => { + const result = updateAndFreeze({ count: 5 }, { count: { __op__: 'add', value: 3 } }); + assert.strictEqual(result.count, 8); + }); + + it('skips an unrecognized operation instead of throwing (apply path must not wedge)', () => { + // On the write/replication apply path a throw would abort the commit and can wedge a + // subscription, so an op this version can't apply is warned + skipped; the field keeps its + // base value and a full-copy re-converges the record. + const original = harperLogger.warn; + let warned = ''; + harperLogger.warn = (message) => { + warned = message; + }; + try { + let result; + assert.doesNotThrow(() => { + result = updateAndFreeze({ count: 5 }, { count: { __op__: 'multiply', value: 3 } }); + }); + assert.strictEqual(result.count, 5); // unchanged base value + assert.match(warned, /unrecognized CRDT operation "multiply"/); + } finally { + harperLogger.warn = original; + } + }); + + it('does not invoke a non-operation crdt export named by a crafted __op__', () => { + // Ops resolve against the explicit registry, not the crdt.ts export namespace. Before that, + // `__op__: 'getRecordAtTime'` resolved to the exported function and was invoked with the + // wrong arguments — throwing and wedging the apply path. It must now warn + skip like any + // other unrecognized op. + const original = harperLogger.warn; + let warned = ''; + harperLogger.warn = (message) => { + warned = message; + }; + try { + let result; + assert.doesNotThrow(() => { + result = updateAndFreeze({ count: 5 }, { count: { __op__: 'getRecordAtTime', value: 3 } }); + }); + assert.strictEqual(result.count, 5); + assert.match(warned, /unrecognized CRDT operation "getRecordAtTime"/); + } finally { + harperLogger.warn = original; + } + }); +});