mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(sync): sync review follow-up fixes (hydration data-loss + journal privacy) (#9045)
* fix(sync): authenticate the project-move footprint on encrypted LWW ops
SuperSync E2EE (AES-256-GCM) covers only `op.payload`; `op.entityId`/`entityIds`
travel as plaintext beside the ciphertext. The LWW project-repair reducer trusts
`meta.entityIds` (copied verbatim from that envelope) to move EVERY declared task
out of its project, so a compromised sync server could append victim task ids to
the envelope of an otherwise-valid encrypted move and orphan those tasks — without
touching the authenticated payload.
Bind the envelope footprint to the authenticated one: on the decrypt path, require
exact-set equality between `op.entityIds` and `{op.entityId} ∪
payload.projectMoveSubTaskIds`.
Interim hardening: enforced only when the authenticated payload carries a
`projectMoveSubTaskIds` array. Synthetic conflict-resolution LWW ops legitimately
carry `entityIds` with no such payload field (footprint lives only in the
envelope), so they are left untouched to avoid rejecting valid ops. Fully closing
the envelope-injection vector needs the durable fix (bind the footprint as GCM
AAD behind an envelope-versioned migration), which remains open. GHSA-8pxh-mgc7-gp3g.
* fix(sync): two data-loss fixes on the snapshot-hydration path
Both bugs surface when a remote snapshot replaces live state, and both live in
sync-hydration.service.ts — kept together as one change.
(1) Atomic rejection of superseded local ops. The file-based bootstrap rejected
pending local ops via a standalone markRejected() that commits in its own
transaction BEFORE commitFileSnapshotBaseline(). If that baseline commit then
threw (e.g. the op-log tail changed), the old state survived but the ops were
already durably rejected — permanently non-uploadable local edits. Fold the
rejection into the baseline transaction via a new `rejectOpIds` param so it is
atomic with the state replacement, and notify the user only after it commits.
(2) Flush the tracked-time accumulator before replacing NgRx. Time tracking
buffers a delta in memory for up to five minutes; a hydration in that window
replaced live state without flushing it, so the later flush dispatched a LOCAL
syncTimeSpent the reducer ignores — silently dropping the tracked time. Flush the
accumulator into a durable op before loadAllData (mirroring how clean-slate and
backup handle their own replacement paths), placed after the baseline commit so
the appended op can't move the tail past the asserted seq.
* test(sync): cover the #3 tracked-time flush-across-hydration path
Adds a deterministic scenario to the task-time replay state machine: a delta
tracked but not yet flushed, flush()ed before a snapshot replaces the store,
becomes a durable syncTimeSpent op that re-applies additively on replay onto the
delta-less remote baseline (and, without the flush, the baseline stays
under-counted). Together with the sync-hydration unit test (flush runs before
loadAllData) this covers the flush → durable-op → replay chain the #3 fix relies
on.
* fix(sync): fail-safe the conflict-journal clear on profile switch (privacy)
ConflictJournalService.clearAll() swallows IndexedDB clear() failures — by
contract, it must not throw after the dataset was already replaced during a
profile switch. But if the bulk clear failed, the next profile could still see
the previous dataset's task titles / field values in the conflict-review UI: a
cross-profile privacy leak.
Persist a durable "cleared before" timestamp in localStorage (which does not
depend on the journal's own IndexedDB being healthy) before the bulk clear. The
read paths — list(), getEntry(), and the badge count — hide every entry resolved
at or before that boundary, so a swallowed clear failure can never surface stale
content. The marker is dropped on a successful clear, and pruneOnStart physically
reclaims any hidden survivors and drops the marker on the next app start.
The boundary favors hiding (entries resolved at the exact clear instant are
treated as pre-clear), matching the privacy-fail-safe intent. Known edge-case
limitations (timestamp vs monotonic clock, localStorage durability, a future
localStorage.clear()) are documented on the marker constant.
Held from public issue tracking (privacy). 4 tests added; 31/31 green.
Hardened per a 3-agent review (correctness / privacy-completeness / lean).
* test(sync): round-trip the encrypted project-move footprint through real crypto (#2)
Exercises the #2 integrity gate through the actual encrypt→decrypt flow (real
AES-GCM), not just the isolated integrity function: a legitimate move round-trips
(entityIds ride OUTSIDE the ciphertext — no false positive), a server-tampered
entityIds footprint is rejected on both the single and batch decrypt paths, and a
synthetic envelope-only op (no authenticated projectMoveSubTaskIds) is accepted.
Closes the one gap flagged in the confidence review: proves the attack model and
the fail-closed behaviour in the real pipeline.
This commit is contained in:
Johannes Millan
GitHub
parent
953d680d34
commit
6fefd741c5
10 changed files with 696 additions and 34 deletions
|
|
@ -1721,6 +1721,56 @@ describe('OperationLogStoreService', () => {
|
|||
expect((await service.loadStateCache())?.state).toEqual(priorState);
|
||||
expect(await service.getVectorClock()).toEqual({ testClient: 1 });
|
||||
});
|
||||
|
||||
it('marks rejectOpIds rejected atomically within the baseline commit', async () => {
|
||||
const supersededLocalOp = createTestOperation({ id: 'superseded-local' });
|
||||
await service.append(supersededLocalOp, 'local');
|
||||
expect((await service.getUnsynced()).map(({ op }) => op.id)).toEqual([
|
||||
'superseded-local',
|
||||
]);
|
||||
|
||||
await service.commitFileSnapshotBaseline({
|
||||
state: { sentinel: 'hydrated' },
|
||||
lastAppliedOpSeq: 1,
|
||||
vectorClock: { remote: 3 },
|
||||
compactedAt: 5,
|
||||
snapshotIncludedOps: [],
|
||||
rejectOpIds: [supersededLocalOp.id],
|
||||
});
|
||||
|
||||
// Rejected in the same commit as the state replacement → no longer uploadable.
|
||||
expect(await service.getUnsynced()).toEqual([]);
|
||||
expect((await service.loadStateCache())?.state).toEqual({ sentinel: 'hydrated' });
|
||||
});
|
||||
|
||||
it('does not reject rejectOpIds when the baseline commit rolls back (tail changed)', async () => {
|
||||
// The exact Finding #5 scenario: a standalone markRejected() would have
|
||||
// committed before this failing baseline, stranding the op as permanently
|
||||
// non-uploadable while the old state survived. Folding it into the commit
|
||||
// ties its fate to the rollback.
|
||||
const supersededLocalOp = createTestOperation({ id: 'superseded-local-2' });
|
||||
await service.append(supersededLocalOp, 'local');
|
||||
|
||||
// Stale lastAppliedOpSeq (0 ≠ current tail 1) trips the tail-changed guard.
|
||||
await expectAsync(
|
||||
service.commitFileSnapshotBaseline({
|
||||
state: { sentinel: 'should-not-apply' },
|
||||
lastAppliedOpSeq: 0,
|
||||
vectorClock: { remote: 9 },
|
||||
compactedAt: 7,
|
||||
snapshotIncludedOps: [],
|
||||
rejectOpIds: [supersededLocalOp.id],
|
||||
}),
|
||||
).toBeRejectedWithError(/operation-log tail changed/);
|
||||
|
||||
// The op remains unsynced/uploadable — rejection never outlived the commit.
|
||||
expect((await service.getUnsynced()).map(({ op }) => op.id)).toEqual([
|
||||
'superseded-local-2',
|
||||
]);
|
||||
expect((await service.loadStateCache())?.state).not.toEqual({
|
||||
sentinel: 'should-not-apply',
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('appendMixedSourceBatchSkipDuplicates', () => {
|
||||
|
|
|
|||
|
|
@ -816,6 +816,13 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
vectorClock: VectorClock;
|
||||
compactedAt: number;
|
||||
snapshotIncludedOps: readonly Operation[];
|
||||
// Superseded local ops to mark rejected atomically within this same commit.
|
||||
// A standalone markRejected() commits in its own transaction, so it would
|
||||
// persist even when this baseline transaction later rolls back (e.g. the
|
||||
// op-log tail changed): those ops become non-uploadable while the old state
|
||||
// was never replaced — a permanent local edit loss. Rejecting them here ties
|
||||
// their fate to the state replacement.
|
||||
rejectOpIds?: readonly string[];
|
||||
archiveYoung?: ArchiveStoreEntry['data'];
|
||||
archiveOld?: ArchiveStoreEntry['data'];
|
||||
}): Promise<{ seqs: number[]; writtenOps: Operation[]; skippedCount: number }> {
|
||||
|
|
@ -857,6 +864,23 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
);
|
||||
}
|
||||
|
||||
// Reject superseded local ops inside this transaction so their
|
||||
// rejection is atomic with the state replacement — never orphaned by
|
||||
// a rolled-back baseline (see rejectOpIds doc above).
|
||||
if (opts.rejectOpIds?.length) {
|
||||
for (const opId of opts.rejectOpIds) {
|
||||
const rejectEntry = await tx.getFromIndex<StoredOperationLogEntry>(
|
||||
STORE_NAMES.OPS,
|
||||
OPS_INDEXES.BY_ID,
|
||||
opId,
|
||||
);
|
||||
if (rejectEntry) {
|
||||
rejectEntry.rejectedAt = opts.compactedAt;
|
||||
await tx.put(STORE_NAMES.OPS, rejectEntry);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const seqs: number[] = [];
|
||||
const writtenOps: Operation[] = [];
|
||||
let skippedCount = 0;
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ import { SnackService } from '../../core/snack/snack.service';
|
|||
import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.service';
|
||||
import { LockService } from '../sync/lock.service';
|
||||
import { LOCK_NAMES } from '../core/operation-log.const';
|
||||
import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service';
|
||||
|
||||
describe('SyncHydrationService', () => {
|
||||
let service: SyncHydrationService;
|
||||
|
|
@ -28,6 +29,7 @@ describe('SyncHydrationService', () => {
|
|||
let mockSnackService: jasmine.SpyObj<SnackService>;
|
||||
let mockArchiveDbAdapter: jasmine.SpyObj<ArchiveDbAdapter>;
|
||||
let mockLockService: jasmine.SpyObj<LockService>;
|
||||
let mockTaskTimeSyncService: jasmine.SpyObj<TaskTimeSyncService>;
|
||||
|
||||
// Default local sync config for tests
|
||||
const defaultLocalSyncConfig = {
|
||||
|
|
@ -80,6 +82,12 @@ describe('SyncHydrationService', () => {
|
|||
mockArchiveDbAdapter.saveArchiveOld.and.resolveTo();
|
||||
mockLockService = jasmine.createSpyObj('LockService', ['request']);
|
||||
mockLockService.request.and.callFake(async (_lockName, callback) => callback());
|
||||
mockTaskTimeSyncService = jasmine.createSpyObj('TaskTimeSyncService', [
|
||||
'flush',
|
||||
'clear',
|
||||
'accumulate',
|
||||
'shouldFlush',
|
||||
]);
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
|
|
@ -93,6 +101,7 @@ describe('SyncHydrationService', () => {
|
|||
{ provide: SnackService, useValue: mockSnackService },
|
||||
{ provide: ArchiveDbAdapter, useValue: mockArchiveDbAdapter },
|
||||
{ provide: LockService, useValue: mockLockService },
|
||||
{ provide: TaskTimeSyncService, useValue: mockTaskTimeSyncService },
|
||||
],
|
||||
});
|
||||
service = TestBed.inject(SyncHydrationService);
|
||||
|
|
@ -392,6 +401,40 @@ describe('SyncHydrationService', () => {
|
|||
);
|
||||
});
|
||||
|
||||
// Regression: the in-memory time-sync delta must be captured as a durable op
|
||||
// before loadAllData wipes the live state it was applied to. Otherwise a later
|
||||
// flush dispatches a local syncTimeSpent the reducer ignores, silently dropping
|
||||
// the tracked time. Order matters: flush must precede loadAllData.
|
||||
const loadAllDataAlreadyDispatched = (): boolean =>
|
||||
mockStore.dispatch.calls
|
||||
.all()
|
||||
.some((c) => (c.args[0] as { type?: string })?.type === loadAllData.type);
|
||||
|
||||
it('flushes the tracked-time accumulator BEFORE replacing NgRx state', async () => {
|
||||
let flushedBeforeLoad = false;
|
||||
mockTaskTimeSyncService.flush.and.callFake(() => {
|
||||
flushedBeforeLoad = !loadAllDataAlreadyDispatched();
|
||||
});
|
||||
|
||||
await service.hydrateFromRemoteSync({ task: { ids: ['t1'] } });
|
||||
|
||||
expect(mockTaskTimeSyncService.flush).toHaveBeenCalledTimes(1);
|
||||
expect(flushedBeforeLoad).toBe(true);
|
||||
});
|
||||
|
||||
it('flushes the accumulator on the file-based bootstrap path too', async () => {
|
||||
let flushedBeforeLoad = false;
|
||||
mockTaskTimeSyncService.flush.and.callFake(() => {
|
||||
flushedBeforeLoad = !loadAllDataAlreadyDispatched();
|
||||
});
|
||||
|
||||
// createSyncImportOp = false → file-based bootstrap (no SYNC_IMPORT).
|
||||
await service.hydrateFromRemoteSync({ task: { ids: ['t1'] } }, undefined, false);
|
||||
|
||||
expect(mockTaskTimeSyncService.flush).toHaveBeenCalledTimes(1);
|
||||
expect(flushedBeforeLoad).toBe(true);
|
||||
});
|
||||
|
||||
it('should use repaired state when validation detects issues', async () => {
|
||||
const downloadedData = { task: { ids: ['t1'] } };
|
||||
const repairedState = { task: { ids: ['t1'], repaired: true } } as any;
|
||||
|
|
@ -544,9 +587,13 @@ describe('SyncHydrationService', () => {
|
|||
|
||||
await service.hydrateFromRemoteSync({ task: {} }, undefined, false);
|
||||
|
||||
expect(mockOpLogStore.markRejected).toHaveBeenCalledOnceWith([
|
||||
pendingBeforeHydration.op.id,
|
||||
]);
|
||||
// The rejection is now folded into the atomic baseline commit (rejectOpIds)
|
||||
// rather than a standalone markRejected() that could outlive a failed
|
||||
// commit. Only the op captured before the snapshot read is rejected — the
|
||||
// one that arrived during hydration is preserved.
|
||||
expect(mockOpLogStore.markRejected).not.toHaveBeenCalled();
|
||||
const commitCall = mockOpLogStore.commitFileSnapshotBaseline.calls.mostRecent();
|
||||
expect(commitCall.args[0].rejectOpIds).toEqual([pendingBeforeHydration.op.id]);
|
||||
});
|
||||
|
||||
it('should commit the file snapshot baseline when createSyncImportOp is false', async () => {
|
||||
|
|
|
|||
|
|
@ -30,6 +30,7 @@ import {
|
|||
} from '../../features/config/local-only-sync-settings.util';
|
||||
import { LockService } from '../sync/lock.service';
|
||||
import { LOCK_NAMES } from '../core/operation-log.const';
|
||||
import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service';
|
||||
|
||||
interface SnapshotHydrationHooks {
|
||||
/** Remote operations already represented by a file-based snapshot. */
|
||||
|
|
@ -70,6 +71,7 @@ export class SyncHydrationService {
|
|||
private snackService = inject(SnackService);
|
||||
private archiveDbAdapter = inject(ArchiveDbAdapter);
|
||||
private lockService = inject(LockService);
|
||||
private taskTimeSyncService = inject(TaskTimeSyncService);
|
||||
|
||||
/**
|
||||
* Handles hydration after a remote sync download.
|
||||
|
|
@ -247,26 +249,13 @@ export class SyncHydrationService {
|
|||
'SyncHydrationService: Skipping SYNC_IMPORT creation (file-based bootstrap)',
|
||||
);
|
||||
|
||||
// CRITICAL: Reject any local pending ops since they're now based on superseded state.
|
||||
// Without SYNC_IMPORT, SyncImportFilterService won't automatically filter them.
|
||||
// These ops have superseded clocks and payloads that don't match the new snapshot.
|
||||
if (unsyncedOpsToReject.length > 0) {
|
||||
const opIds = unsyncedOpsToReject.map((entry) => entry.op.id);
|
||||
await this.opLogStore.markRejected(opIds);
|
||||
OpLog.normal(
|
||||
`SyncHydrationService: Rejected ${unsyncedOpsToReject.length} local pending op(s) ` +
|
||||
`(superseded after file-based sync snapshot)`,
|
||||
);
|
||||
|
||||
// Notify user that local changes were discarded
|
||||
this.snackService.open({
|
||||
msg: T.F.SYNC.S.LOCAL_CHANGES_DISCARDED_SNAPSHOT,
|
||||
translateParams: {
|
||||
count: unsyncedOpsToReject.length,
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
// Any local pending ops are now based on superseded state and must be
|
||||
// rejected: without a SYNC_IMPORT, SyncImportFilterService won't filter
|
||||
// them automatically. The rejection is deferred into
|
||||
// commitFileSnapshotBaseline() below so it commits atomically with the
|
||||
// state replacement. Rejecting here (a separate transaction) would
|
||||
// strand these ops as permanently non-uploadable if the baseline commit
|
||||
// then failed (e.g. the op-log tail changed) and the old state survived.
|
||||
lastSeq = await this.opLogStore.getLastSeq();
|
||||
}
|
||||
|
||||
|
|
@ -343,12 +332,15 @@ export class SyncHydrationService {
|
|||
// working clock separately on this legacy full-state-import path.
|
||||
await this.opLogStore.setVectorClock(clockForStorage);
|
||||
} else {
|
||||
// Reject superseded local ops atomically with the state replacement.
|
||||
const rejectOpIds = unsyncedOpsToReject.map((entry) => entry.op.id);
|
||||
const appendResult = await this.opLogStore.commitFileSnapshotBaseline({
|
||||
state: dataToLoad,
|
||||
lastAppliedOpSeq: lastSeq,
|
||||
vectorClock: clockForStorage,
|
||||
compactedAt: Date.now(),
|
||||
snapshotIncludedOps: hooks?.snapshotIncludedOps ?? [],
|
||||
...(rejectOpIds.length ? { rejectOpIds } : {}),
|
||||
...(downloadedArchiveYoung ? { archiveYoung: downloadedArchiveYoung } : {}),
|
||||
...(downloadedArchiveOld ? { archiveOld: downloadedArchiveOld } : {}),
|
||||
});
|
||||
|
|
@ -363,10 +355,39 @@ export class SyncHydrationService {
|
|||
? `; skipped ${appendResult.skippedCount} duplicate(s)`
|
||||
: ''),
|
||||
);
|
||||
|
||||
// Notify the user only after the rejection durably committed with the
|
||||
// new baseline — never before, so a failed commit leaves both the old
|
||||
// state and the still-uploadable local ops intact.
|
||||
if (rejectOpIds.length > 0) {
|
||||
OpLog.normal(
|
||||
`SyncHydrationService: Rejected ${rejectOpIds.length} local pending op(s) ` +
|
||||
`(superseded after file-based sync snapshot)`,
|
||||
);
|
||||
this.snackService.open({
|
||||
msg: T.F.SYNC.S.LOCAL_CHANGES_DISCARDED_SNAPSHOT,
|
||||
translateParams: { count: rejectOpIds.length },
|
||||
});
|
||||
}
|
||||
}
|
||||
hooks?.afterSnapshotPersisted?.();
|
||||
OpLog.normal('SyncHydrationService: Committed snapshot persistence baseline');
|
||||
|
||||
// Flush the in-memory tracked-time accumulator into a durable syncTimeSpent
|
||||
// op BEFORE replacing NgRx. The accumulator holds time already applied to
|
||||
// the live state we are about to discard; left in place, that stale delta
|
||||
// later flushes as a LOCAL (non-remote) syncTimeSpent that the reducer
|
||||
// intentionally ignores (task.reducer only applies remote syncTimeSpent),
|
||||
// so the accepted time silently vanishes from live state until the next
|
||||
// op-log replay. Flushing captures it as a pending op (re-applied
|
||||
// additively on replay, and uploaded) and empties the accumulator so
|
||||
// nothing stale survives the replacement. Placed AFTER the baseline commit
|
||||
// so the appended op cannot move the op-log tail past the seq that
|
||||
// commitFileSnapshotBaseline() asserts. Unlike backup import — which
|
||||
// clear()s because it deliberately discards local concurrent state — sync
|
||||
// hydration preserves local edits (cf. snapshot-hydration handling).
|
||||
this.taskTimeSyncService.flush();
|
||||
|
||||
// 10. Dispatch loadAllData to update NgRx
|
||||
hooks?.beforeStateLoad?.();
|
||||
this.store.dispatch(loadAllData({ appDataComplete: dataToLoad }));
|
||||
|
|
|
|||
|
|
@ -280,6 +280,88 @@ describe('ConflictJournalService (store)', () => {
|
|||
expect(doneThen).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('durable clear boundary (privacy fail-safe on profile switch)', () => {
|
||||
const MARKER_KEY = 'SUP_CONFLICT_JOURNAL_CLEARED_BEFORE';
|
||||
|
||||
afterEach(() => localStorage.removeItem(MARKER_KEY));
|
||||
|
||||
it('hides pre-switch entries and zeroes the badge even when the bulk clear fails', async () => {
|
||||
const now = Date.now();
|
||||
await service.record(
|
||||
makeEntry({ id: 'stale-a', resolvedAt: now - 1000, status: 'unreviewed' }),
|
||||
);
|
||||
await service.record(
|
||||
makeEntry({ id: 'stale-b', resolvedAt: now - 500, status: 'kept' }),
|
||||
);
|
||||
expect(service.unreviewedCount()).toBe(1);
|
||||
|
||||
// The exact failure the fix targets: the IndexedDB bulk clear throws.
|
||||
const db = await service['_ensureDb']();
|
||||
spyOn(db, 'clear').and.rejectWith(new Error('idb clear failed'));
|
||||
|
||||
await expectAsync(service.clearAll()).toBeResolved(); // never throws
|
||||
|
||||
// Survivors physically remain but are hidden from every read path.
|
||||
expect(await service.list('history')).toEqual([]);
|
||||
expect(await service.getEntry('stale-a')).toBeUndefined();
|
||||
expect(await service.getEntry('stale-b')).toBeUndefined();
|
||||
expect(service.unreviewedCount()).toBe(0);
|
||||
});
|
||||
|
||||
it('shows only entries recorded after a failed clear (the new profile stays clean)', async () => {
|
||||
await service.record(makeEntry({ id: 'pre', resolvedAt: Date.now() - 1000 }));
|
||||
|
||||
const db = await service['_ensureDb']();
|
||||
spyOn(db, 'clear').and.rejectWith(new Error('idb clear failed'));
|
||||
await service.clearAll();
|
||||
|
||||
// Record relative to the actual boundary so the assertion is clock-agnostic.
|
||||
const marker = Number(localStorage.getItem(MARKER_KEY));
|
||||
await service.record(
|
||||
makeEntry({ id: 'post', resolvedAt: marker + 1000, status: 'unreviewed' }),
|
||||
);
|
||||
|
||||
expect((await service.list('history')).map((e) => e.id)).toEqual(['post']);
|
||||
expect(await service.getEntry('post')).toBeTruthy();
|
||||
expect(service.unreviewedCount()).toBe(1);
|
||||
});
|
||||
|
||||
it('drops the marker on a successful clear so later entries are never wrongly filtered', async () => {
|
||||
await service.record(makeEntry({ id: 'x', resolvedAt: Date.now() - 1000 }));
|
||||
await service.clearAll(); // succeeds → marker removed
|
||||
|
||||
expect(localStorage.getItem(MARKER_KEY)).toBeNull();
|
||||
// An old-timestamped entry is still visible: no stale boundary lingers.
|
||||
await service.record(makeEntry({ id: 'old-but-valid', resolvedAt: 1000 }));
|
||||
expect((await service.list('history')).map((e) => e.id)).toEqual(['old-but-valid']);
|
||||
});
|
||||
|
||||
it('pruneOnStart reclaims hidden survivors but spares post-marker entries, then drops the marker', async () => {
|
||||
const now = Date.now();
|
||||
await service.record(makeEntry({ id: 'survivor', resolvedAt: now - 1000 }));
|
||||
|
||||
const db = await service['_ensureDb']();
|
||||
spyOn(db, 'clear').and.rejectWith(new Error('idb clear failed'));
|
||||
await service.clearAll();
|
||||
const marker = Number(localStorage.getItem(MARKER_KEY));
|
||||
expect(marker).toBeGreaterThan(0);
|
||||
|
||||
// A conflict recorded by the NEW profile (after the boundary) must NOT be
|
||||
// reclaimed by the marker-aware prune — guards against a "prune deletes
|
||||
// everything while a marker is set" regression.
|
||||
await service.record(makeEntry({ id: 'fresh', resolvedAt: marker + 1000 }));
|
||||
|
||||
// Next app start: prune uses delete (not clear), so it reclaims the hidden
|
||||
// survivor, keeps the fresh entry, and drops the now-unnecessary marker.
|
||||
await service.pruneOnStart(now);
|
||||
|
||||
expect(localStorage.getItem(MARKER_KEY)).toBeNull();
|
||||
expect(await service.getEntry('survivor')).toBeUndefined();
|
||||
expect(await service.getEntry('fresh')).toBeTruthy();
|
||||
expect((await service.list('history')).map((e) => e.id)).toEqual(['fresh']);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
|
|
|||
|
|
@ -49,6 +49,27 @@ interface ConflictJournalDB extends DBSchema {
|
|||
|
||||
const DAY_MS = 24 * 60 * 60 * 1000;
|
||||
|
||||
// Durable "cleared before" timestamp written by clearAll() on a profile/dataset
|
||||
// switch and consulted by the read paths. It lives in localStorage on purpose:
|
||||
// it must survive — and be readable — even when the journal's own IndexedDB is
|
||||
// unhealthy, which is exactly the failure clearAll() has to tolerate. Any entry
|
||||
// resolved at or before this instant is hidden, so a swallowed bulk-clear failure
|
||||
// can never surface the previous dataset's titles/values.
|
||||
//
|
||||
// Known limitations (all gated behind the already-rare failed-clear path; the
|
||||
// data is device-local, observe-only review metadata — never task data):
|
||||
// - Timestamp boundary, not a monotonic epoch: a backward wall-clock step
|
||||
// between recording an old entry and the clear could let that entry slip the
|
||||
// filter. A monotonic per-entry epoch would be immune; deferred as low-stakes.
|
||||
// - Assumes localStorage outlives the journal's IndexedDB from clear-time until
|
||||
// the next read — seconds, across the profile-switch reload. A successful clear
|
||||
// or the next pruneOnStart drops the marker well before any sustained
|
||||
// storage-pressure eviction (relevant only to mobile WebViews under load).
|
||||
// - The marker's safety assumes nothing bulk-wipes localStorage while journal
|
||||
// entries survive in IndexedDB. No runtime path does today (`clearLS()` is
|
||||
// unused); a future "reset app data" flow MUST also clear the journal DB.
|
||||
const CONFLICT_JOURNAL_CLEARED_BEFORE_KEY = 'SUP_CONFLICT_JOURNAL_CLEARED_BEFORE';
|
||||
|
||||
@Injectable({
|
||||
providedIn: 'root',
|
||||
})
|
||||
|
|
@ -157,7 +178,7 @@ export class ConflictJournalService {
|
|||
CONFLICT_JOURNAL_STORE,
|
||||
CONFLICT_JOURNAL_INDEX_RESOLVED_AT,
|
||||
);
|
||||
const newestFirst = ascending.reverse();
|
||||
const newestFirst = this._dropInvalidated(ascending).reverse();
|
||||
if (view === 'unreviewed') {
|
||||
return newestFirst.filter((entry) => entry.status === 'unreviewed');
|
||||
}
|
||||
|
|
@ -172,13 +193,60 @@ export class ConflictJournalService {
|
|||
async getEntry(id: string): Promise<ConflictJournalEntry | undefined> {
|
||||
try {
|
||||
const db = await this._ensureDb();
|
||||
return await db.get(CONFLICT_JOURNAL_STORE, id);
|
||||
const entry = await db.get(CONFLICT_JOURNAL_STORE, id);
|
||||
// Hidden behind a durable clear boundary (see _dropInvalidated): read as
|
||||
// "no such entry" so a stale id can never be surfaced or flipped.
|
||||
if (entry && entry.resolvedAt <= this._getClearedBefore()) {
|
||||
return undefined;
|
||||
}
|
||||
return entry;
|
||||
} catch (err) {
|
||||
OpLog.err('ConflictJournalService: getEntry failed (ignored)', err);
|
||||
return undefined;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Hides entries resolved before a durable clear boundary — the fail-safe for a
|
||||
* clearAll() whose bulk delete failed after a profile/dataset switch, so
|
||||
* survivors can never surface the previous dataset's content. No-op when unset.
|
||||
*/
|
||||
private _dropInvalidated(entries: ConflictJournalEntry[]): ConflictJournalEntry[] {
|
||||
const clearedBefore = this._getClearedBefore();
|
||||
// Boundary favors hiding: an entry resolved at the exact clear instant is
|
||||
// treated as pre-clear (hidden), never leaked. Legitimate new entries are
|
||||
// recorded strictly after the clear (forward clock), so they stay visible.
|
||||
return clearedBefore > 0
|
||||
? entries.filter((entry) => entry.resolvedAt > clearedBefore)
|
||||
: entries;
|
||||
}
|
||||
|
||||
private _getClearedBefore(): number {
|
||||
try {
|
||||
const raw = localStorage.getItem(CONFLICT_JOURNAL_CLEARED_BEFORE_KEY);
|
||||
const value = raw === null ? 0 : Number(raw);
|
||||
return Number.isFinite(value) && value > 0 ? value : 0;
|
||||
} catch {
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
|
||||
private _setClearedBefore(ts: number): void {
|
||||
try {
|
||||
localStorage.setItem(CONFLICT_JOURNAL_CLEARED_BEFORE_KEY, String(ts));
|
||||
} catch (err) {
|
||||
OpLog.err('ConflictJournalService: failed to persist clear marker', err);
|
||||
}
|
||||
}
|
||||
|
||||
private _clearClearedBefore(): void {
|
||||
try {
|
||||
localStorage.removeItem(CONFLICT_JOURNAL_CLEARED_BEFORE_KEY);
|
||||
} catch (err) {
|
||||
OpLog.err('ConflictJournalService: failed to remove clear marker', err);
|
||||
}
|
||||
}
|
||||
|
||||
/** User confirmed the auto-resolution. */
|
||||
async markKept(id: string): Promise<void> {
|
||||
await this._setStatus(id, 'kept');
|
||||
|
|
@ -222,6 +290,10 @@ export class ConflictJournalService {
|
|||
try {
|
||||
const db = await this._ensureDb();
|
||||
const deleted = await this._prune(db, now);
|
||||
// _prune has physically removed any survivors hidden behind a failed
|
||||
// clearAll's durable marker, so the marker (and its filtering) is no longer
|
||||
// needed. Only reached when _prune committed, so no survivor can outlive it.
|
||||
this._clearClearedBefore();
|
||||
await this._refreshUnreviewedCount(db);
|
||||
return deleted;
|
||||
} catch (err) {
|
||||
|
|
@ -247,10 +319,16 @@ export class ConflictJournalService {
|
|||
|
||||
const retentionWindowMs = JOURNAL_RETENTION_DAYS * DAY_MS;
|
||||
const cutoff = now - retentionWindowMs;
|
||||
// Also physically drop survivors of a failed clearAll (resolved before the
|
||||
// durable marker); they are already hidden from reads, this reclaims them.
|
||||
const clearedBefore = this._getClearedBefore();
|
||||
const idsToDelete = new Set<string>();
|
||||
|
||||
for (const entry of ascending) {
|
||||
if (entry.resolvedAt < cutoff) {
|
||||
if (
|
||||
entry.resolvedAt < cutoff ||
|
||||
(clearedBefore > 0 && entry.resolvedAt <= clearedBefore)
|
||||
) {
|
||||
idsToDelete.add(entry.id);
|
||||
}
|
||||
}
|
||||
|
|
@ -289,12 +367,29 @@ export class ConflictJournalService {
|
|||
* must not fail after the dataset has already been replaced.
|
||||
*/
|
||||
async clearAll(): Promise<void> {
|
||||
// Persist a durable invalidation boundary FIRST, in localStorage — which does
|
||||
// not depend on the (possibly unhealthy) journal IndexedDB. If the bulk clear
|
||||
// below fails, the read paths hide every entry resolved before this instant,
|
||||
// so the next profile can never see the previous dataset's titles/values.
|
||||
const clearedBefore = Date.now();
|
||||
this._setClearedBefore(clearedBefore);
|
||||
try {
|
||||
const db = await this._ensureDb();
|
||||
await db.clear(CONFLICT_JOURNAL_STORE);
|
||||
// Physical clear succeeded — no stale entries remain, so the marker (and
|
||||
// its read-time filtering) is no longer needed.
|
||||
this._clearClearedBefore();
|
||||
await this._refreshUnreviewedCount(db);
|
||||
} catch (err) {
|
||||
OpLog.err('ConflictJournalService: clearAll failed (ignored)', err);
|
||||
// Clear failed: KEEP the marker so the read paths hide the survivors, and
|
||||
// still reflect the logical clear on the badge. Contract: never throw after
|
||||
// the dataset was replaced.
|
||||
OpLog.err(
|
||||
'ConflictJournalService: clearAll failed; entries hidden via durable marker',
|
||||
err,
|
||||
);
|
||||
this._unreviewedCount.set(0);
|
||||
this._revision.update((r) => r + 1);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -307,6 +402,20 @@ export class ConflictJournalService {
|
|||
// reflects the committed change; `unreviewedCount` catches up once the query
|
||||
// succeeds. Fires on every mutation even when the count is unchanged.
|
||||
this._revision.update((r) => r + 1);
|
||||
const clearedBefore = this._getClearedBefore();
|
||||
if (clearedBefore > 0) {
|
||||
// A prior clearAll left a marker (its bulk delete failed): count only
|
||||
// post-marker unreviewed entries so the badge ignores hidden survivors.
|
||||
const unreviewed = await db.getAllFromIndex(
|
||||
CONFLICT_JOURNAL_STORE,
|
||||
CONFLICT_JOURNAL_INDEX_STATUS,
|
||||
IDBKeyRange.only('unreviewed'),
|
||||
);
|
||||
this._unreviewedCount.set(
|
||||
unreviewed.filter((entry) => entry.resolvedAt > clearedBefore).length,
|
||||
);
|
||||
return;
|
||||
}
|
||||
const count = await db.countFromIndex(
|
||||
CONFLICT_JOURNAL_STORE,
|
||||
CONFLICT_JOURNAL_INDEX_STATUS,
|
||||
|
|
|
|||
|
|
@ -286,6 +286,89 @@ describe('OperationEncryptionService', () => {
|
|||
});
|
||||
});
|
||||
|
||||
// --- project-move footprint (op.entityIds) across the real crypto flow ---
|
||||
// Proves the attack model concretely: entityIds ride OUTSIDE the AES-GCM
|
||||
// ciphertext (tamperable), the authenticated payload footprint stays intact,
|
||||
// and the interim #2 gate catches the tamper on decrypt without rejecting
|
||||
// legitimate or synthetic (envelope-only) ops.
|
||||
|
||||
const createMoveOp = (): SyncOperation => ({
|
||||
...createMockSyncOp({
|
||||
actionPayload: {
|
||||
id: 'task-1',
|
||||
projectMoveSubTaskIds: ['sub-1', 'sub-2'],
|
||||
changes: { projectId: 'project-2' },
|
||||
},
|
||||
entityChanges: [],
|
||||
}),
|
||||
actionType: LWW_TASK,
|
||||
opType: 'UPDATE',
|
||||
entityType: 'TASK',
|
||||
entityId: 'task-1',
|
||||
entityIds: ['task-1', 'sub-1', 'sub-2'],
|
||||
});
|
||||
|
||||
it('round-trips a legitimate encrypted project move (entityIds ride outside the ciphertext)', async () => {
|
||||
const encrypted = await service.encryptOperation(createMoveOp(), TEST_PASSWORD);
|
||||
// entityIds are plaintext envelope metadata; only the payload is encrypted.
|
||||
expect(encrypted.entityIds).toEqual(['task-1', 'sub-1', 'sub-2']);
|
||||
expect(typeof encrypted.payload).toBe('string');
|
||||
|
||||
const decrypted = await service.decryptOperation(encrypted, TEST_PASSWORD);
|
||||
expect(decrypted.entityIds).toEqual(['task-1', 'sub-1', 'sub-2']);
|
||||
expect(
|
||||
(decrypted.payload as { actionPayload: { projectMoveSubTaskIds: string[] } })
|
||||
.actionPayload.projectMoveSubTaskIds,
|
||||
).toEqual(['sub-1', 'sub-2']);
|
||||
});
|
||||
|
||||
it('rejects a decrypted move whose entityIds footprint was tampered (single)', async () => {
|
||||
const encrypted = await service.encryptOperation(createMoveOp(), TEST_PASSWORD);
|
||||
// A compromised server appends a victim task id to the plaintext envelope;
|
||||
// the authenticated projectMoveSubTaskIds ciphertext is untouched.
|
||||
const tampered: SyncOperation = {
|
||||
...encrypted,
|
||||
entityIds: [...(encrypted.entityIds as string[]), 'victim-task'],
|
||||
};
|
||||
await expectAsync(
|
||||
service.decryptOperation(tampered, TEST_PASSWORD),
|
||||
).toBeRejectedWithError(OperationIntegrityError);
|
||||
});
|
||||
|
||||
it('rejects a tampered entityIds footprint via the batch decrypt path too', async () => {
|
||||
const [encrypted] = await service.encryptOperations(
|
||||
[createMoveOp()],
|
||||
TEST_PASSWORD,
|
||||
);
|
||||
const tampered: SyncOperation = {
|
||||
...encrypted,
|
||||
entityIds: [...(encrypted.entityIds as string[]), 'victim-task'],
|
||||
};
|
||||
await expectAsync(
|
||||
service.decryptOperations([tampered], TEST_PASSWORD),
|
||||
).toBeRejectedWithError(OperationIntegrityError);
|
||||
});
|
||||
|
||||
it('accepts a synthetic LWW op that carries entityIds but no authenticated footprint', async () => {
|
||||
// Conflict-resolution synthetic ops legitimately declare entityIds in the
|
||||
// envelope only (no payload projectMoveSubTaskIds); they must NOT be rejected.
|
||||
const syntheticOp: SyncOperation = {
|
||||
...createMockSyncOp({
|
||||
actionPayload: { id: 'task-1', changes: { projectId: 'project-2' } },
|
||||
entityChanges: [],
|
||||
}),
|
||||
actionType: LWW_TASK,
|
||||
opType: 'UPDATE',
|
||||
entityType: 'TASK',
|
||||
entityId: 'task-1',
|
||||
entityIds: ['task-1', 'sub-1'],
|
||||
};
|
||||
const encrypted = await service.encryptOperation(syntheticOp, TEST_PASSWORD);
|
||||
await expectAsync(
|
||||
service.decryptOperation(encrypted, TEST_PASSWORD),
|
||||
).toBeResolved();
|
||||
});
|
||||
|
||||
describe('full-state opType promotion', () => {
|
||||
const createFullStateOp = (
|
||||
opType: OpType.SyncImport | OpType.BackupImport | OpType.Repair,
|
||||
|
|
|
|||
|
|
@ -94,4 +94,122 @@ describe('assertDecryptedOpMetadataIntegrity', () => {
|
|||
).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe('rejects project-move footprint tampering (fail closed)', () => {
|
||||
it('throws when op.entityIds injects a victim id absent from the authenticated footprint', () => {
|
||||
// Valid encrypted move of task-123 (+ subtask sub-1). A compromised server
|
||||
// appended victim-task to the plaintext envelope so the LWW project-repair
|
||||
// reducer would drag it out of its project too. GHSA-8pxh-mgc7-gp3g.
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['task-123', 'sub-1', 'victim-task'],
|
||||
});
|
||||
const authenticatedPayload = {
|
||||
id: 'task-123',
|
||||
projectMoveSubTaskIds: ['sub-1'],
|
||||
changes: { projectId: 'project-2' },
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).toThrowError(OperationIntegrityError);
|
||||
});
|
||||
|
||||
it('throws for the multi-entity-wrapped move payload too', () => {
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['task-123', 'victim-task'],
|
||||
});
|
||||
const authenticatedPayload = {
|
||||
actionPayload: { id: 'task-123', projectMoveSubTaskIds: [] },
|
||||
entityChanges: [],
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).toThrowError(OperationIntegrityError);
|
||||
});
|
||||
|
||||
it('throws when a non-string id is injected into op.entityIds', () => {
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['task-123', 42 as unknown as string],
|
||||
});
|
||||
const authenticatedPayload = {
|
||||
id: 'task-123',
|
||||
projectMoveSubTaskIds: ['sub-1'],
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).toThrowError(OperationIntegrityError);
|
||||
});
|
||||
});
|
||||
|
||||
describe('accepts legitimate project moves (no false positives)', () => {
|
||||
it('passes when op.entityIds exactly equals {entityId} ∪ projectMoveSubTaskIds (flat)', () => {
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['task-123', 'sub-1', 'sub-2'],
|
||||
});
|
||||
const authenticatedPayload = {
|
||||
id: 'task-123',
|
||||
projectMoveSubTaskIds: ['sub-1', 'sub-2'],
|
||||
changes: { projectId: 'project-2' },
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).not.toThrow();
|
||||
});
|
||||
|
||||
it('passes for the multi-entity-wrapped move payload', () => {
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['task-123', 'sub-1'],
|
||||
});
|
||||
const authenticatedPayload = {
|
||||
actionPayload: { id: 'task-123', projectMoveSubTaskIds: ['sub-1'] },
|
||||
entityChanges: [],
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).not.toThrow();
|
||||
});
|
||||
|
||||
it('ignores ordering differences (set, not sequence, equality)', () => {
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['sub-1', 'task-123'],
|
||||
});
|
||||
const authenticatedPayload = {
|
||||
id: 'task-123',
|
||||
projectMoveSubTaskIds: ['sub-1'],
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).not.toThrow();
|
||||
});
|
||||
|
||||
it('does not validate entityIds when the payload carries no authenticated footprint (synthetic LWW op)', () => {
|
||||
// Synthetic conflict-resolution ops carry entityIds in the envelope only;
|
||||
// there is no projectMoveSubTaskIds to bind against, so they must pass
|
||||
// untouched (validating them would reject valid ops and break sync).
|
||||
const op = createOp({
|
||||
entityId: 'task-123',
|
||||
entityIds: ['task-123', 'sub-1', 'anything'],
|
||||
});
|
||||
const authenticatedPayload = { id: 'task-123', changes: {} };
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).not.toThrow();
|
||||
});
|
||||
|
||||
it('does not validate footprint when op.entityIds is absent', () => {
|
||||
const op = createOp({ entityId: 'task-123', entityIds: undefined });
|
||||
const authenticatedPayload = {
|
||||
id: 'task-123',
|
||||
projectMoveSubTaskIds: ['sub-1'],
|
||||
};
|
||||
expect(() =>
|
||||
assertDecryptedOpMetadataIntegrity(op, authenticatedPayload),
|
||||
).not.toThrow();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -153,25 +153,98 @@ export const assertDecryptedOpMetadataIntegrity = (
|
|||
// (unauthenticated) `op.entityId`. Missing / non-string / mismatched all mean
|
||||
// the metadata cannot be trusted — convertOpToAction coerces `id = op.entityId`
|
||||
// in every one of those cases, so anything but a positive match is rejected.
|
||||
if (typeof payloadId === 'string' && payloadId === op.entityId) {
|
||||
if (!(typeof payloadId === 'string' && payloadId === op.entityId)) {
|
||||
// Log ids only — never payload content (op log is exportable). Rule 9.
|
||||
SyncLog.err(
|
||||
'[assertDecryptedOpMetadataIntegrity] encrypted op entityId does not match authenticated payload.id — rejecting (possible sync-server tampering)',
|
||||
{
|
||||
opId: op.id,
|
||||
entityType: op.entityType,
|
||||
opEntityId: op.entityId,
|
||||
payloadId: typeof payloadId === 'string' ? payloadId : `<${typeof payloadId}>`,
|
||||
actionType: op.actionType,
|
||||
},
|
||||
);
|
||||
throw new OperationIntegrityError(
|
||||
`Operation ${op.id} failed metadata integrity check: encrypted payload id ` +
|
||||
`does not match op.entityId (possible sync-server tampering). ` +
|
||||
`GHSA-8pxh-mgc7-gp3g`,
|
||||
);
|
||||
}
|
||||
|
||||
// Second vector on the same LWW update: the multi-task project-move footprint.
|
||||
assertEncryptedProjectMoveFootprintIntegrity(op, actionPayload);
|
||||
};
|
||||
|
||||
/**
|
||||
* A task project-move LWW update declares its multi-task footprint twice: as the
|
||||
* authenticated `payload.projectMoveSubTaskIds` (inside the AES-GCM ciphertext)
|
||||
* and as the plaintext `op.entityIds` envelope field. The LWW project-repair
|
||||
* reducer (task-shared-meta-reducers/lww-update.meta-reducer.ts) trusts
|
||||
* `meta.entityIds` — copied verbatim from that envelope by convertOpToAction —
|
||||
* as the source footprint and moves EVERY declared task out of its current
|
||||
* project. A compromised sync server could therefore append victim task ids to
|
||||
* the envelope of an otherwise-valid encrypted move and orphan those tasks,
|
||||
* without touching (or being able to decrypt) the ciphertext.
|
||||
*
|
||||
* Bind the envelope footprint to the authenticated one: require exact-set
|
||||
* equality between `op.entityIds` and `{op.entityId} ∪ projectMoveSubTaskIds`.
|
||||
*
|
||||
* INTERIM hardening — only enforceable when the authenticated payload actually
|
||||
* carries a `projectMoveSubTaskIds` array. Synthetic LWW ops minted by conflict
|
||||
* resolution legitimately carry `entityIds` WITHOUT that payload field (their
|
||||
* footprint lives only in the plaintext envelope), so they cannot be validated
|
||||
* here and are intentionally left untouched to avoid rejecting valid ops. Fully
|
||||
* closing the envelope-injection vector needs the durable fix that is still
|
||||
* OPEN: bind the complete footprint as GCM AAD behind an envelope-versioned
|
||||
* migration so every producer's footprint is authenticated. GHSA-8pxh-mgc7-gp3g.
|
||||
*
|
||||
* @throws OperationIntegrityError when the declared footprint diverges from the
|
||||
* authenticated one.
|
||||
*/
|
||||
const assertEncryptedProjectMoveFootprintIntegrity = (
|
||||
op: SyncOperation,
|
||||
actionPayload: Record<string, unknown> | undefined,
|
||||
): void => {
|
||||
if (op.entityIds === undefined || !op.entityId) {
|
||||
return;
|
||||
}
|
||||
const subTaskIds = actionPayload?.['projectMoveSubTaskIds'];
|
||||
if (!Array.isArray(subTaskIds)) {
|
||||
// No authenticated footprint to bind against (e.g. synthetic LWW op) — see
|
||||
// the "INTERIM hardening" note above. Leave the op unchanged.
|
||||
return;
|
||||
}
|
||||
|
||||
// Log ids only — never payload content (op log is exportable). Rule 9.
|
||||
const authenticatedFootprint = new Set<string>([
|
||||
op.entityId,
|
||||
...subTaskIds.filter((id): id is string => typeof id === 'string'),
|
||||
]);
|
||||
// Exact-set equality: same size (no extras, no duplicates, no non-strings) and
|
||||
// every declared id is in the authenticated footprint (no injected victim id).
|
||||
const isExactSet =
|
||||
op.entityIds.length === authenticatedFootprint.size &&
|
||||
op.entityIds.every((id) => typeof id === 'string' && authenticatedFootprint.has(id));
|
||||
if (isExactSet) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Log ids/counts only — never payload content (op log is exportable). Rule 9.
|
||||
SyncLog.err(
|
||||
'[assertDecryptedOpMetadataIntegrity] encrypted op entityId does not match authenticated payload.id — rejecting (possible sync-server tampering)',
|
||||
'[assertDecryptedOpMetadataIntegrity] encrypted op entityIds do not match the authenticated project-move footprint — rejecting (possible sync-server tampering)',
|
||||
{
|
||||
opId: op.id,
|
||||
entityType: op.entityType,
|
||||
opEntityId: op.entityId,
|
||||
payloadId: typeof payloadId === 'string' ? payloadId : `<${typeof payloadId}>`,
|
||||
declaredCount: op.entityIds.length,
|
||||
authenticatedCount: authenticatedFootprint.size,
|
||||
actionType: op.actionType,
|
||||
},
|
||||
);
|
||||
throw new OperationIntegrityError(
|
||||
`Operation ${op.id} failed metadata integrity check: encrypted payload id ` +
|
||||
`does not match op.entityId (possible sync-server tampering). ` +
|
||||
`GHSA-8pxh-mgc7-gp3g`,
|
||||
`Operation ${op.id} failed metadata integrity check: encrypted op entityIds do ` +
|
||||
`not match the authenticated project-move footprint (possible sync-server ` +
|
||||
`tampering). GHSA-8pxh-mgc7-gp3g`,
|
||||
);
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -310,4 +310,59 @@ describe('Task-time replay state machine integration', () => {
|
|||
.toEqual(comparableTaskTimes(acceptedState));
|
||||
}
|
||||
});
|
||||
|
||||
it('preserves an unflushed tracked-time delta across a snapshot hydration by flushing it into a durable op (#3)', () => {
|
||||
// Reduced repro of the sync-hydration data-loss scenario at the reducer/
|
||||
// replay level. A tracked-but-unflushed delta lives only in the live store
|
||||
// and the in-memory accumulator when a remote snapshot that never saw it
|
||||
// replaces the live store. SyncHydrationService now flush()es the accumulator
|
||||
// BEFORE loadAllData, so the delta becomes a durable op that re-applies
|
||||
// additively on replay onto the delta-less remote baseline.
|
||||
taskTimeSync.clear();
|
||||
dispatchedEntries = [];
|
||||
|
||||
// 1. Live state after tracking +500ms locally (addTimeSpent already applied).
|
||||
const trackedDelta = 500;
|
||||
let liveState = createState([createTask('task-1', { [DATES[0]]: 1000 })]);
|
||||
liveState = taskReducer(
|
||||
liveState,
|
||||
TimeTrackingActions.addTimeSpent({
|
||||
task: liveState.entities['task-1']!,
|
||||
date: DATES[0],
|
||||
duration: trackedDelta,
|
||||
isFromTrackingReminder: false,
|
||||
}),
|
||||
);
|
||||
taskTimeSync.accumulate('task-1', trackedDelta, DATES[0]);
|
||||
expect(liveState.entities['task-1']!.timeSpentOnDay[DATES[0]]).toBe(1500);
|
||||
|
||||
// 2. The fix: flush BEFORE hydration replaces the live store. The delta is
|
||||
// dispatched as a durable syncTimeSpent op (captured here as an entry).
|
||||
taskTimeSync.flush();
|
||||
expect(dispatchedEntries).toEqual([
|
||||
{ id: 'task-1', date: DATES[0], duration: trackedDelta },
|
||||
]);
|
||||
const durableEvents: ReplayEvent[] = dispatchedEntries.map((entry) => ({
|
||||
kind: 'delta',
|
||||
entry,
|
||||
}));
|
||||
|
||||
// 3. Hydration replaces the live store with a REMOTE snapshot that never saw
|
||||
// the delta (it was never uploaded) — task-1 is back to 1000.
|
||||
const hydratedRemoteState = createState([createTask('task-1', { [DATES[0]]: 1000 })]);
|
||||
|
||||
// 4. Replaying the durable op onto the hydrated baseline restores the delta.
|
||||
const afterReplay = durableEvents.reduce(applyReplayEvent, hydratedRemoteState);
|
||||
expect(afterReplay.entities['task-1']!.timeSpentOnDay[DATES[0]]).toBe(1500);
|
||||
expect(afterReplay.entities['task-1']!.timeSpent).toBe(1500);
|
||||
|
||||
// Contrast: without the flush at hydration time the delta is not yet durable,
|
||||
// so replaying the (empty) durable log leaves the hydrated baseline at 1000 —
|
||||
// the silent under-count this fix closes.
|
||||
const withoutFlush = ([] as ReplayEvent[]).reduce(
|
||||
applyReplayEvent,
|
||||
hydratedRemoteState,
|
||||
);
|
||||
expect(withoutFlush.entities['task-1']!.timeSpentOnDay[DATES[0]]).toBe(1000);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue