From 8bbc11b340cc584975af8f44da0036e0b9bbddc3 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 27 Aug 2026 17:14:32 -0400 Subject: [PATCH] fix(sidebar): reject rev at MAX_SAFE_INTEGER boundary to keep mint headroom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rev parse bound used Number.isSafeInteger, which accepts Number.MAX_SAFE_INTEGER itself. The click path mints rev = maxRev + 1, so an accepted boundary rev overflows to an unsafe value that never advances again — recreating the same-second toggle wedge the bound exists to prevent. Reject rev >= Number.MAX_SAFE_INTEGER (exclusive, normalize to 0) so maxRev + 1 always stays safe. updatedAt keeps its inclusive bound: the click path takes Math.max(now, observed), never increments, so it cannot overflow. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../sidebar/lib/channelMutesStorage.test.mjs | 64 +++++++++++++------ .../sidebar/lib/channelMutesStorage.ts | 19 +++--- .../sidebar/lib/channelStarsStorage.test.mjs | 64 +++++++++++++------ .../sidebar/lib/channelStarsStorage.ts | 19 +++--- 4 files changed, 112 insertions(+), 54 deletions(-) diff --git a/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs b/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs index 4bc5d137b3..289eaa6568 100644 --- a/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs +++ b/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs @@ -42,13 +42,15 @@ test("parseMutePayload: malformed rev (string / negative / non-integer / NaN / u neg: { muted: true, updatedAt: 1, rev: -2 }, frac: { muted: true, updatedAt: 1, rev: 1.5 }, nan: { muted: true, updatedAt: 1, rev: NaN }, - // Above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past it, so a - // malformed entry at this magnitude would wedge later toggles forever - // unless rejected here (Carl P2). + // At or above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past + // Number.MAX_SAFE_INTEGER, so an entry at the boundary itself (or beyond) + // would wedge later toggles forever unless rejected here (Carl P2 / + // Thufir off-by-one). The bound is exclusive, so the boundary normalizes. + boundary: { muted: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER }, huge: { muted: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER + 1 }, }, }); - for (const id of ["str", "neg", "frac", "nan", "huge"]) { + for (const id of ["str", "neg", "frac", "nan", "boundary", "huge"]) { assert.equal(result.channels[id].rev, 0, `${id} rev normalized to 0`); assert.equal(result.channels[id].muted, true, `${id} entry kept`); } @@ -189,24 +191,48 @@ test("mergeStores: same-second old-build click (rev 0) loses to an earlier new-b ); }); -test("mergeStores: a huge (unsafe) rev entry cannot wedge a later false toggle", () => { - // Carl P2 regression. A malformed blob carries a same-second rev far above - // Number.MAX_SAFE_INTEGER on a `muted:true` entry. Before the parse bound, - // this survived as-is and — since a real later click mints rev = maxSeen + 1, - // which cannot advance past an unsafe integer — the true entry won the - // same-second tie forever, suppressing every later unmute. The parser now - // normalizes the unsafe rev to 0, so a same-second later click (rev 1) wins. +test("mergeStores: a boundary (MAX_SAFE_INTEGER) rev cannot wedge later same-second toggles", () => { + // Carl P2 / Thufir off-by-one regression. A malformed blob carries a + // same-second rev of exactly Number.MAX_SAFE_INTEGER on a `muted:true` + // entry — the largest value the pre-fix `Number.isSafeInteger` guard still + // ACCEPTED. Had it survived, the click path (`rev = max(seen) + 1`) would + // mint Number.MAX_SAFE_INTEGER + 1, an unsafe integer that never advances, + // so the true entry would win every same-second tie forever. The parser now + // rejects `rev >= Number.MAX_SAFE_INTEGER` (exclusive), normalizing it to 0 + // and preserving headroom for `maxRev + 1`. Drive the production mint + // (`Math.max(localRev, maxRevSeen) + 1`) through alternating same-second + // toggles and assert each later toggle mints a strictly-advancing rev and wins. const wedged = parseMutePayload({ version: 1, - channels: { c: { muted: true, updatedAt: 100, rev: Number.MAX_VALUE } }, + channels: { + c: { muted: true, updatedAt: 100, rev: Number.MAX_SAFE_INTEGER }, + }, }); - assert.equal(wedged.channels.c.rev, 0, "unsafe rev is normalized to 0"); - const laterUnmute = S(E(false, 100, 1)); - assert.deepEqual( - mergeStores(wedged, laterUnmute).channels.c, - E(false, 100, 1), - "the same-second later unmute (rev 1) wins — no permanent wedge", - ); + assert.equal(wedged.channels.c.rev, 0, "boundary rev is normalized to 0"); + + // Production mint: same fixed second (100), rev = max(local, seen) + 1. + const mint = (store, muted) => { + const local = store.channels.c; + const rev = Math.max(local?.rev ?? 0, 0) + 1; + return { store: S(E(muted, 100, rev)), rev }; + }; + + let state = wedged; + let prevRev = state.channels.c.rev; // 0 + for (const muted of [false, true, false, true]) { + const { store: click, rev } = mint(state, muted); + assert.ok( + rev > prevRev, + `mint rev ${rev} strictly advances past ${prevRev}`, + ); + state = mergeStores(state, click); + assert.deepEqual( + state.channels.c, + E(muted, 100, rev), + `same-second toggle to ${muted} (rev ${rev}) wins — no wedge`, + ); + prevRev = rev; + } }); test("mergeStores: unmute with higher updatedAt overrides mute", () => { diff --git a/desktop/src/features/sidebar/lib/channelMutesStorage.ts b/desktop/src/features/sidebar/lib/channelMutesStorage.ts index 7f85909081..ff81ea7143 100644 --- a/desktop/src/features/sidebar/lib/channelMutesStorage.ts +++ b/desktop/src/features/sidebar/lib/channelMutesStorage.ts @@ -56,19 +56,22 @@ export function parseMutePayload(json: unknown): ChannelMuteStore | null { ((v as Record).updatedAt as number) >= 0 ); }) - // Normalize `rev`: accept a safe non-negative integer, otherwise 0. - // An entry is never dropped solely because `rev` is absent (older - // build) or malformed — absence is a valid mergeable value. `rev` - // above `Number.MAX_SAFE_INTEGER` is rejected here: at that - // magnitude `maxRev + 1` no longer advances, so a malformed entry - // could wedge every later toggle. Treating it as rev 0 lets a real - // later click win the same-second tie instead (Carl P2). + // Normalize `rev`: accept a non-negative integer with headroom for + // one mint, otherwise 0. An entry is never dropped solely because + // `rev` is absent (older build) or malformed — absence is a valid + // mergeable value. `rev >= Number.MAX_SAFE_INTEGER` is rejected: + // the click path mints `maxRev + 1`, so accepting the boundary + // itself would overflow to the same unsafe value and stop + // advancing, wedging every later toggle. Requiring strictly-below + // keeps `maxRev + 1` safe. Treating an out-of-range value as rev 0 + // lets a real later click win the same-second tie instead (Carl P2). .map(([id, v]) => { const rawRev = v.rev; const rev = typeof rawRev === "number" && Number.isSafeInteger(rawRev) && - rawRev >= 0 + rawRev >= 0 && + rawRev < Number.MAX_SAFE_INTEGER ? rawRev : 0; return [ diff --git a/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs b/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs index 1cc25361dd..bc86a8af99 100644 --- a/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs +++ b/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs @@ -42,13 +42,15 @@ test("parseStarPayload: malformed rev (string / negative / non-integer / NaN / u neg: { starred: true, updatedAt: 1, rev: -2 }, frac: { starred: true, updatedAt: 1, rev: 1.5 }, nan: { starred: true, updatedAt: 1, rev: NaN }, - // Above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past it, so a - // malformed entry at this magnitude would wedge later toggles forever - // unless rejected here (Carl P2). + // At or above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past + // Number.MAX_SAFE_INTEGER, so an entry at the boundary itself (or beyond) + // would wedge later toggles forever unless rejected here (Carl P2 / + // Thufir off-by-one). The bound is exclusive, so the boundary normalizes. + boundary: { starred: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER }, huge: { starred: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER + 1 }, }, }); - for (const id of ["str", "neg", "frac", "nan", "huge"]) { + for (const id of ["str", "neg", "frac", "nan", "boundary", "huge"]) { assert.equal(result.channels[id].rev, 0, `${id} rev normalized to 0`); assert.equal(result.channels[id].starred, true, `${id} entry kept`); } @@ -189,24 +191,48 @@ test("mergeStores: same-second old-build click (rev 0) loses to an earlier new-b ); }); -test("mergeStores: a huge (unsafe) rev entry cannot wedge a later false toggle", () => { - // Carl P2 regression. A malformed blob carries a same-second rev far above - // Number.MAX_SAFE_INTEGER on a `starred:true` entry. Before the parse bound, - // this survived as-is and — since a real later click mints rev = maxSeen + 1, - // which cannot advance past an unsafe integer — the true entry won the - // same-second tie forever, suppressing every later unstar. The parser now - // normalizes the unsafe rev to 0, so a same-second later click (rev 1) wins. +test("mergeStores: a boundary (MAX_SAFE_INTEGER) rev cannot wedge later same-second toggles", () => { + // Carl P2 / Thufir off-by-one regression. A malformed blob carries a + // same-second rev of exactly Number.MAX_SAFE_INTEGER on a `starred:true` + // entry — the largest value the pre-fix `Number.isSafeInteger` guard still + // ACCEPTED. Had it survived, the click path (`rev = max(seen) + 1`) would + // mint Number.MAX_SAFE_INTEGER + 1, an unsafe integer that never advances, + // so the true entry would win every same-second tie forever. The parser now + // rejects `rev >= Number.MAX_SAFE_INTEGER` (exclusive), normalizing it to 0 + // and preserving headroom for `maxRev + 1`. Drive the production mint + // (`Math.max(localRev, maxRevSeen) + 1`) through alternating same-second + // toggles and assert each later toggle mints a strictly-advancing rev and wins. const wedged = parseStarPayload({ version: 1, - channels: { c: { starred: true, updatedAt: 100, rev: Number.MAX_VALUE } }, + channels: { + c: { starred: true, updatedAt: 100, rev: Number.MAX_SAFE_INTEGER }, + }, }); - assert.equal(wedged.channels.c.rev, 0, "unsafe rev is normalized to 0"); - const laterUnstar = S(E(false, 100, 1)); - assert.deepEqual( - mergeStores(wedged, laterUnstar).channels.c, - E(false, 100, 1), - "the same-second later unstar (rev 1) wins — no permanent wedge", - ); + assert.equal(wedged.channels.c.rev, 0, "boundary rev is normalized to 0"); + + // Production mint: same fixed second (100), rev = max(local, seen) + 1. + const mint = (store, starred) => { + const local = store.channels.c; + const rev = Math.max(local?.rev ?? 0, 0) + 1; + return { store: S(E(starred, 100, rev)), rev }; + }; + + let state = wedged; + let prevRev = state.channels.c.rev; // 0 + for (const starred of [false, true, false, true]) { + const { store: click, rev } = mint(state, starred); + assert.ok( + rev > prevRev, + `mint rev ${rev} strictly advances past ${prevRev}`, + ); + state = mergeStores(state, click); + assert.deepEqual( + state.channels.c, + E(starred, 100, rev), + `same-second toggle to ${starred} (rev ${rev}) wins — no wedge`, + ); + prevRev = rev; + } }); test("mergeStores: unstar with higher updatedAt overrides star", () => { diff --git a/desktop/src/features/sidebar/lib/channelStarsStorage.ts b/desktop/src/features/sidebar/lib/channelStarsStorage.ts index 61dae1c10a..30ffed627b 100644 --- a/desktop/src/features/sidebar/lib/channelStarsStorage.ts +++ b/desktop/src/features/sidebar/lib/channelStarsStorage.ts @@ -56,19 +56,22 @@ export function parseStarPayload(json: unknown): ChannelStarStore | null { ((v as Record).updatedAt as number) >= 0 ); }) - // Normalize `rev`: accept a safe non-negative integer, otherwise 0. - // An entry is never dropped solely because `rev` is absent (older - // build) or malformed — absence is a valid mergeable value. `rev` - // above `Number.MAX_SAFE_INTEGER` is rejected here: at that - // magnitude `maxRev + 1` no longer advances, so a malformed entry - // could wedge every later toggle. Treating it as rev 0 lets a real - // later click win the same-second tie instead (Carl P2). + // Normalize `rev`: accept a non-negative integer with headroom for + // one mint, otherwise 0. An entry is never dropped solely because + // `rev` is absent (older build) or malformed — absence is a valid + // mergeable value. `rev >= Number.MAX_SAFE_INTEGER` is rejected: + // the click path mints `maxRev + 1`, so accepting the boundary + // itself would overflow to the same unsafe value and stop + // advancing, wedging every later toggle. Requiring strictly-below + // keeps `maxRev + 1` safe. Treating an out-of-range value as rev 0 + // lets a real later click win the same-second tie instead (Carl P2). .map(([id, v]) => { const rawRev = v.rev; const rev = typeof rawRev === "number" && Number.isSafeInteger(rawRev) && - rawRev >= 0 + rawRev >= 0 && + rawRev < Number.MAX_SAFE_INTEGER ? rawRev : 0; return [