fix(desktop): stop a future-dated message from marking a channel read

desktop/src/features/channels/readState/readStateFormat.test.mjs

@@ -3,9 +3,12 @@ import test from "node:test";
 
 import {
   isMsgContextKey,
+  isPlausibleReadMarker,
   isThreadContextKey,
+  MAX_READ_MARKER_SKEW_SECONDS,
   maxReadAt,
   msgContextKey,
+  sanitizeContexts,
 } from "./readStateFormat.ts";
 
 const EVENT_ID = "a".repeat(64);
@@ -58,3 +61,51 @@ test("isThreadContextKey_shortId_returnsFalse", () => {
 test("msgContextKey_output_roundTripsThroughValidator", () => {
   assert.equal(isMsgContextKey(msgContextKey(EVENT_ID)), true);
 });
+
+// --- Synced read state carries the same skew policy -----------------------
+//
+// A NIP-RS blob may have been published by another desktop that predates the
+// policy. Admitting a year-ahead context there would poison this device too,
+// and monotonic merging means it would never expire.
+
+test("sanitizeContexts_dropsAnImplausiblyFutureMarker", () => {
+  const now = 1_780_000_000;
+
+  const result = sanitizeContexts(
+    {
+      "channel-real": now - 3_600,
+      "channel-skewed": now + 30,
+      "channel-poisoned": now + 365 * 24 * 60 * 60,
+    },
+    now,
+  );
+
+  assert.deepEqual(result, {
+    "channel-real": now - 3_600,
+    "channel-skewed": now + 30,
+  });
+});
+
+test("sanitizeContexts_stillDropsTheOldMalformedShapes", () => {
+  const now = 1_780_000_000;
+
+  assert.deepEqual(
+    sanitizeContexts(
+      { a: "12", b: 1.5, c: -1, d: 4_294_967_296, e: now - 1 },
+      now,
+    ),
+    { e: now - 1 },
+  );
+});
+
+test("isPlausibleReadMarker_boundaryIsInclusive", () => {
+  const now = 1_780_000_000;
+  assert.equal(
+    isPlausibleReadMarker(now + MAX_READ_MARKER_SKEW_SECONDS, now),
+    true,
+  );
+  assert.equal(
+    isPlausibleReadMarker(now + MAX_READ_MARKER_SKEW_SECONDS + 1, now),
+    false,
+  );
+});

desktop/src/features/channels/readState/readStateFormat.ts

@@ -37,6 +37,41 @@ export const THREAD_PREFIX = "thread:";
 
 const EVENT_ID_PATTERN = /^[0-9a-f]{64}$/;
 
+// How far ahead of this machine's clock a read marker may plausibly land.
+// `created_at` is self-asserted by the sending client and the relay does not
+// bound it for ordinary messages, so an unbounded marker lets one future-dated
+// event mark every later message in the channel as already read — no badge, no
+// divider, no thread resume — until wall-clock time catches up with it.
+//
+// A tolerance rather than a hard `now` ceiling: ordinary skew between two
+// machines is seconds, and rejecting that would drop a marker a sibling device
+// legitimately wrote. 120s matches the relay's own `MAX_COMMAND_SKEW_SECS`
+// (`handlers/moderation_commands.rs`), the house number for "clock difference
+// we accept"; NIP-AB already says clients MUST NOT set `created_at` in the
+// future at all.
+export const MAX_READ_MARKER_SKEW_SECONDS = 120;
+
+export function nowUnixSeconds(): number {
+  return Math.floor(Date.now() / 1_000);
+}
+
+/**
+ * Whether a read marker at `unixSeconds` could have been written by a clock
+ * this one agrees with.
+ *
+ * The single skew policy for read state. Every route a marker can enter by
+ * consults it, because read markers are monotonic and persisted: a marker
+ * accepted once from a live event, from local storage, or from an NIP-RS
+ * event synced by another (possibly unpatched) desktop stays effective, and
+ * a year-ahead one is unrecoverable from the UI.
+ */
+export function isPlausibleReadMarker(
+  unixSeconds: number,
+  now: number = nowUnixSeconds(),
+): boolean {
+  return unixSeconds <= now + MAX_READ_MARKER_SKEW_SECONDS;
+}
+
 export function maxReadAt(...markers: Array<number | null>): number | null {
   return markers.reduce<number | null>((latest, marker) => {
     if (marker === null) return latest;
@@ -94,14 +129,26 @@ export function isValidBlob(obj: unknown): obj is ReadStateBlob {
   return true;
 }
 
+/**
+ * Validate a decrypted blob's context map.
+ *
+ * Implausible markers are dropped rather than clamped. Markers are monotonic
+ * and this blob may have been written by another desktop that predates the
+ * skew policy, so a year-ahead entry admitted here would silently mark every
+ * later message read and never expire. Dropping it restores the channel to
+ * unread, which the user can see and act on; clamping it to the present would
+ * assert a read position nobody ever reached.
+ */
 export function sanitizeContexts(
   contexts: Record<string, unknown>,
+  now: number = nowUnixSeconds(),
 ): Record<string, number> {
   const result: Record<string, number> = {};
   for (const [key, value] of Object.entries(contexts)) {
     if (new TextEncoder().encode(key).length > 256) continue;
     if (typeof value !== "number" || !Number.isInteger(value)) continue;
     if (value < 0 || value > 4294967295) continue;
+    if (!isPlausibleReadMarker(value, now)) continue;
     result[key] = value;
   }
   return result;

desktop/src/features/channels/readState/readStateStorage.test.mjs

@@ -153,3 +153,50 @@ test("writeStoredReadState survives a throwing localStorage.setItem", () => {
     );
   });
 });
+
+// --- A poisoned marker must not survive a restart -------------------------
+//
+// Read markers are monotonic and persisted. A marker written before the skew
+// policy existed — or by a sibling desktop that still lacks it — is year-ahead
+// and unrecoverable from the UI, so hydration is the last place it can be
+// disarmed: the next write persists whatever readStoredReadState loaded.
+
+test("readStoredReadState drops a persisted year-ahead marker", () => {
+  installLocalStorage();
+  const pubkey = "f".repeat(64);
+  const yearAhead = NOW + 365 * 24 * 60 * 60;
+
+  localStorage.setItem(
+    localReadStateKey(pubkey),
+    JSON.stringify({
+      "channel-real": new Date((NOW - 3_600) * 1_000).toISOString(),
+      "channel-poisoned": new Date(yearAhead * 1_000).toISOString(),
+    }),
+  );
+
+  const stored = readStoredReadState(pubkey, NOW);
+
+  assert.equal(stored.contexts.get("channel-real"), NOW - 3_600);
+  assert.equal(
+    stored.contexts.has("channel-poisoned"),
+    false,
+    "an implausible marker must not be hydrated back into effect",
+  );
+});
+
+test("readStoredReadState keeps a marker inside the skew tolerance", () => {
+  installLocalStorage();
+  const pubkey = "1".repeat(64);
+  const slightlyAhead = NOW + 30;
+
+  localStorage.setItem(
+    localReadStateKey(pubkey),
+    JSON.stringify({
+      "channel-skewed": new Date(slightlyAhead * 1_000).toISOString(),
+    }),
+  );
+
+  const stored = readStoredReadState(pubkey, NOW);
+
+  assert.equal(stored.contexts.get("channel-skewed"), slightlyAhead);
+});

desktop/src/features/channels/readState/readStateStorage.ts

@@ -1,11 +1,13 @@
 import {
   isPlainRecord,
+  isPlausibleReadMarker,
   localIsoToUnixSeconds,
   localPublishableContextKey,
   localReadStateKey,
   localSourceCreatedAtKey,
   LOCAL_MAX_PRUNABLE_CONTEXTS,
   MSG_PREFIX,
+  nowUnixSeconds,
   READ_STATE_HORIZON_SECONDS,
   THREAD_PREFIX,
 } from "@/features/channels/readState/readStateFormat";
@@ -20,6 +22,7 @@ export type StoredReadState = {
 function mergeLocalStorageKey(
   contexts: Map<string, number>,
   key: string,
+  now: number,
 ): void {
   const raw = localStorage.getItem(key);
   if (!raw) return;
@@ -31,6 +34,11 @@ function mergeLocalStorageKey(
     for (const [channelId, value] of Object.entries(parsed)) {
       const unixSeconds = localIsoToUnixSeconds(value);
       if (unixSeconds === null) continue;
+      // A marker persisted before the skew policy existed — or written by a
+      // sibling desktop that still lacks it — is dropped on the way in, not
+      // clamped: hydration is the last chance to disarm it, since markers are
+      // monotonic and the next write persists whatever we load here.
+      if (!isPlausibleReadMarker(unixSeconds, now)) continue;
       const current = contexts.get(channelId) ?? 0;
       if (unixSeconds > current) {
         contexts.set(channelId, unixSeconds);
@@ -92,9 +100,12 @@ function readContextSourceCreatedAt(pubkey: string): Map<string, number> {
   return result;
 }
 
-export function readStoredReadState(pubkey: string): StoredReadState {
+export function readStoredReadState(
+  pubkey: string,
+  now: number = nowUnixSeconds(),
+): StoredReadState {
   const contexts = new Map<string, number>();
-  mergeLocalStorageKey(contexts, localReadStateKey(pubkey));
+  mergeLocalStorageKey(contexts, localReadStateKey(pubkey), now);
 
   return {
     contexts,

desktop/src/features/channels/unreadReadMarker.test.mjs

@@ -513,3 +513,119 @@ test("addThreadActivityItems keeps newest items when input is newest-first", ()
   assert.equal(result.items[0].id, "reply-1");
   assert.equal(result.items.at(-1).id, "reply-100");
 });
+
+// --- A future-dated created_at must not mark later messages read ---
+//
+// `created_at` is self-asserted by the sending client and the relay does not
+// bound it for ordinary messages. Before the skew policy, one event dated a
+// year out landed in the read marker verbatim, so every genuinely new message
+// failed `createdAt > readAt` and was classified read: no badge, no divider,
+// no thread resume, until wall-clock time caught up.
+//
+// The tolerance decides whether a timestamp is *plausible*; it is not a value
+// to clamp to. Clamping an outlier to `now + 120` would manufacture a read
+// frontier two minutes into the future and hide every legitimate message that
+// arrives before the clock reaches it. An implausible timestamp is discarded
+// and the marker repaired to the present instead.
+
+const NOW = 1_780_000_000;
+const YEAR_AHEAD = NOW + 365 * 24 * 60 * 60;
+
+test("resolveChannelReadMarker_futureCallerReadAt_repairsToThePresent", () => {
+  const result = resolveChannelReadMarker(
+    new Date(YEAR_AHEAD * 1_000).toISOString(),
+    undefined,
+    NOW,
+  );
+
+  assert.equal(result.markAt, NOW);
+  assert.ok(
+    result.markAt < YEAR_AHEAD,
+    "a future-dated message must not push the marker past it",
+  );
+});
+
+test("resolveChannelReadMarker_repairedMarker_doesNotHideTheNextTwoMinutes", () => {
+  // The regression the ceiling-clamp version had: a correct message arriving a
+  // second from now must still be unread against the repaired marker.
+  const { markAt } = resolveChannelReadMarker(
+    new Date(YEAR_AHEAD * 1_000).toISOString(),
+    undefined,
+    NOW,
+  );
+
+  const marker = computeChannelUnreadMarker(
+    [topLevel("soon", NOW + 1)],
+    markAt,
+  );
+
+  assert.equal(marker.firstUnreadMessageId, "soon");
+  assert.equal(marker.unreadCount, 1);
+});
+
+test("resolveChannelReadMarker_futureObservedLatest_repairsAndKeepsObserved", () => {
+  const result = resolveChannelReadMarker(null, YEAR_AHEAD, NOW);
+
+  assert.equal(result.markAt, NOW);
+  // The observed refs must survive: that event really is still unread, so
+  // clearing them would drop the sidebar dot the policy exists to preserve.
+  assert.equal(result.clearObserved, false);
+});
+
+test("resolveChannelReadMarker_keepsThePlausibleInputWhenTheOtherIsPoison", () => {
+  // Discarding is per-input, not on the max: a real caller position must not be
+  // thrown away just because the observed timestamp beside it is implausible.
+  const realRead = NOW - 3_600;
+
+  const result = resolveChannelReadMarker(
+    new Date(realRead * 1_000).toISOString(),
+    YEAR_AHEAD,
+    NOW,
+  );
+
+  assert.equal(result.markAt, realRead);
+  assert.equal(result.clearObserved, false);
+});
+
+test("resolveChannelReadMarker_afterRepair_aLaterRealMessageIsStillUnread", () => {
+  // The end-to-end shape of the bug, through the same comparison the divider
+  // uses: poison arrives, the channel is marked read, then a genuine message.
+  const { markAt } = resolveChannelReadMarker(
+    new Date(YEAR_AHEAD * 1_000).toISOString(),
+    undefined,
+    NOW,
+  );
+  const genuineMessage = topLevel("later", NOW + 300);
+
+  const marker = computeChannelUnreadMarker([genuineMessage], markAt);
+
+  assert.equal(marker.firstUnreadMessageId, "later");
+  assert.equal(marker.unreadCount, 1);
+});
+
+test("resolveChannelReadMarker_ordinarySkewInsideTolerance_isKept", () => {
+  // A sender 30s ahead of this machine is normal. Discarding that would move
+  // the marker back to now and leave a message the user just read unread.
+  const slightlyAhead = NOW + 30;
+
+  const result = resolveChannelReadMarker(
+    new Date(slightlyAhead * 1_000).toISOString(),
+    undefined,
+    NOW,
+  );
+
+  assert.equal(result.markAt, slightlyAhead);
+});
+
+test("resolveChannelReadMarker_pastReadAt_isUnaffectedByTheCeiling", () => {
+  // Comfortably before NOW: the ceiling must only ever bite upwards.
+  const readAt = new Date((NOW - 86_400) * 1_000).toISOString();
+  const expected = NOW - 86_400;
+
+  assert.equal(
+    resolveChannelReadMarker(readAt, undefined, NOW).markAt,
+    expected,
+  );
+  assert.equal(resolveChannelReadMarker(null, 200, NOW).markAt, 200);
+  assert.equal(resolveChannelReadMarker(null, undefined, NOW).markAt, null);
+});