fix(read-state): close four Thufir pass-3 findings in readStateFormat/Snapshot

Finding 1 — effective frontier for liveness: computeOverrideLiveness now takes effectiveFrontier as a required third argument (was reading merged.frontiers.get(rawCtxId) internally). NIP-RS :169-196/:510-514 requires the hierarchical effective frontier, not the context's own stored value. Callers supply max(own, parent) before calling. Finding 2 — d-tag slot regex: isValidReadStateDTag now validates against /^[0-9a-f]{32}$/ (was any 1-64 ASCII characters). Nonconforming coordinates are now correctly rejected per NIP-RS :55/:68. Updated all test fixtures (slot1/slot2 → valid 32-hex constants; read-state:test in communityUnreadObserver → valid slot). Removed the now-dead isAscii helper. Finding 3 — null-prototype wire projection: decodeContexts builds wire with Object.create(null) instead of {}, so a legitimate __proto__ context ID is stored as an own property in both the wire and frontiers projections rather than silently dropped from wire. Finding 4 — decode once in parseReadStateEvent: decodeContexts is now exported. parseReadStateEvent calls it once and derives both blob.contexts (wire) and contexts (frontiers/overrides) from the single result, eliminating the double traversal of up to 10k entries. Deleted the unused exported ValidatedOverrideGroup type. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
2026-11-01 06:51:17 +00:00 · 2026-07-31 15:13:59 -04:00
parent 50cc45a3c3
commit dc63d2b88d
4 changed files with 239 additions and 49 deletions
--- a/desktop/src/features/channels/readState/readStateFormat.ts
+++ b/desktop/src/features/channels/readState/readStateFormat.ts
@@ -213,15 +213,6 @@ export interface ParsedContexts {
  readonly overrides: ReadonlyMap<string, OverrideRegister>;
 }

-/**
- * A validated override group ready for merge.
- * `kind === "live"` → three-key group with all counters present.
- * `kind === "floor"` → tombstone, only `c` is meaningful (s=0, b=0).
- */
-export type ValidatedOverrideGroup =
-  | { kind: "live"; reg: OverrideRegister }
-  | { kind: "floor"; c: number };
-
 // ---------------------------------------------------------------------------
 // Internal single decode path — the only place validation logic lives
 // ---------------------------------------------------------------------------
@@ -231,9 +222,12 @@ export type ValidatedOverrideGroup =
 *
 * Returns three projections of the same validated data:
 *
- * - `wire`      — flat `Record<string, number>` with `ov_*` keys preserved
- *                 in wire form (for `ReadStateBlob.contexts` backward compat).
+ * - `wire`      — null-prototype `Record<string, number>` with `ov_*` keys
+ *                 preserved in wire form (for `ReadStateBlob.contexts` compat).
 *                 Escaped frontier keys are stored as-is (e.g. `esc:ov_s:x`).
+ *                 Built with `Object.create(null)` so that context IDs that
+ *                 collide with `Object.prototype` properties (e.g. `__proto__`)
+ *                 survive as own-property entries in both projections.
 * - `frontiers` — `Map<rawCtx, number>` — frontier keys unescaped; same raw
 *                 domain as `overrides`.
 * - `overrides` — `Map<rawCtx, OverrideRegister>` — registers keyed by the
@@ -248,8 +242,11 @@ export type ValidatedOverrideGroup =
 *    siblings alike.  One oversized sibling drops its whole group; an
 *    oversized frontier key drops only that entry.
 * 4. Unescape frontier wire keys (`esc:…` → raw ctx) for `frontiers`.
+ *
+ * Exported so `parseReadStateEvent` can call once and derive both
+ * `blob.contexts` and the structured `ParsedContexts` from one result.
 */
-function decodeContexts(raw: Record<string, unknown>): {
+export function decodeContexts(raw: Record<string, unknown>): {
  wire: Record<string, number>;
  frontiers: Map<string, number>;
  overrides: Map<string, OverrideRegister>;
@@ -287,7 +284,13 @@ function decodeContexts(raw: Record<string, unknown>): {

  // ── Pass 2: validate each suffix as a group ───────────────────────────────
  const overrides = new Map<string, OverrideRegister>();
-  const wire: Record<string, number> = {};
+  // Use a null-prototype object so context IDs that coincide with
+  // Object.prototype property names (e.g. `__proto__`, `toString`) survive
+  // as own-property entries rather than silently colliding.
+  const wire: Record<string, number> = Object.create(null) as Record<
+    string,
+    number
+  >;
  const suffixes = new Set([...sWire.keys(), ...cWire.keys(), ...bWire.keys()]);

  for (const suffix of suffixes) {
@@ -443,12 +446,15 @@ export function sanitizeContexts(
  return decodeContexts(contexts).wire;
 }

+// Exactly 32 lowercase hexadecimal characters as required by NIP-RS :55/:68.
+const READ_STATE_SLOT_ID_PATTERN = /^[0-9a-f]{32}$/;
+
 export function isValidReadStateDTag(
  value: string | undefined,
 ): value is string {
  if (!value?.startsWith(READ_STATE_D_TAG_PREFIX)) return false;
  const slotId = value.slice(READ_STATE_D_TAG_PREFIX.length);
-  return slotId.length > 0 && slotId.length <= 64 && isAscii(slotId);
+  return READ_STATE_SLOT_ID_PATTERN.test(slotId);
 }

 export function localExtraSlotIdsKey(pubkey: string): string {
@@ -463,12 +469,3 @@ export function localIsoToUnixSeconds(value: unknown): number | null {
  const ms = Date.parse(value);
  return Number.isNaN(ms) ? null : Math.floor(ms / 1_000);
 }
-
-function isAscii(value: string): boolean {
-  for (let index = 0; index < value.length; index += 1) {
-    if (value.charCodeAt(index) > 0x7f) {
-      return false;
-    }
-  }
-  return true;
-}
--- a/desktop/src/features/channels/readState/readStateOverride.test.mjs
+++ b/desktop/src/features/channels/readState/readStateOverride.test.mjs
@@ -6,6 +6,7 @@ import {
  escapeFrontierKey,
  isOverrideActive,
  isOverrideKey,
+  isValidReadStateDTag,
  mergeOverrideRegisters,
  OV_B_PREFIX,
  OV_C_PREFIX,
@@ -26,6 +27,10 @@ import {
 // Helpers for event-level integration tests
 // ---------------------------------------------------------------------------

+// Valid 32-lowercase-hex slot IDs per NIP-RS spec :55/:68.
+const SLOT_A = "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6";
+const SLOT_B = "1234567890abcdef1234567890abcdef";
+
 /**
 * Build a minimal fake RelayEvent whose content decrypts to a ReadStateBlob
 * JSON string.  The decrypt function is provided by the caller so no real
@@ -958,7 +963,7 @@ test("parseContexts_liveGroup_multibyte_exactly257Bytes_dropped", () => {

 test("parseReadStateEvent_singleEvent_frontierAndOverrideBothPresent", async () => {
  const pubkey = "c".repeat(64);
-  const parsed = await parseFakeEvent(pubkey, "slot1", {
+  const parsed = await parseFakeEvent(pubkey, SLOT_A, {
    "ch:alpha": 5000,
    "ov_s:ch:alpha": 2,
    "ov_c:ch:alpha": 1,
@@ -980,7 +985,7 @@ test("mergeReadStateEventsStructured_twoBlobs_mergesBothFrontiersAndRegisters",
  // Blob 2: ch:alpha frontier=6000, register s=3/c=1/b=5500 (higher frontier, higher S+B)
  const merged = await mergeFakeEvents(pubkey, [
    {
-      dTagSlot: "slot1",
+      dTagSlot: SLOT_A,
      blobContexts: {
        "ch:alpha": 5000,
        "ov_s:ch:alpha": 2,
@@ -989,7 +994,7 @@ test("mergeReadStateEventsStructured_twoBlobs_mergesBothFrontiersAndRegisters",
      },
    },
    {
-      dTagSlot: "slot2",
+      dTagSlot: SLOT_B,
      blobContexts: {
        "ch:alpha": 6000,
        "ov_s:ch:alpha": 3,
@@ -1013,7 +1018,7 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessProperly_evaluated", async
  // Liveness: S(1)>0, F(100)>B(95) → inactive (frontier exceeded baseline)
  const merged = await mergeFakeEvents(pubkey, [
    {
-      dTagSlot: "slot1",
+      dTagSlot: SLOT_A,
      blobContexts: {
        "ch:beta": 100,
        "ov_s:ch:beta": 1,
@@ -1022,7 +1027,7 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessProperly_evaluated", async
      },
    },
    {
-      dTagSlot: "slot2",
+      dTagSlot: SLOT_B,
      blobContexts: {
        "ch:beta": 80,
        "ov_s:ch:beta": 1,
@@ -1035,7 +1040,11 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessProperly_evaluated", async
  assert.equal(merged.frontiers.get("ch:beta"), 100);
  assert.deepEqual(merged.overrides.get("ch:beta"), { s: 1, c: 0, b: 95 });

-  const liveness = computeOverrideLiveness(merged, "ch:beta");
+  const liveness = computeOverrideLiveness(
+    merged,
+    "ch:beta",
+    merged.frontiers.get("ch:beta") ?? 0,
+  );
  // F(100) > B(95) → inactive
  assert.equal(liveness.active, false);
  assert.equal(liveness.frontier, 100);
@@ -1053,7 +1062,7 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessActive_afterMerge", async
  // Liveness: S(1)>0, F(55)<=B(60), S(1)>C(0) → active
  const merged = await mergeFakeEvents(pubkey, [
    {
-      dTagSlot: "slot1",
+      dTagSlot: SLOT_A,
      blobContexts: {
        "ch:gamma": 50,
        "ov_s:ch:gamma": 1,
@@ -1062,7 +1071,7 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessActive_afterMerge", async
      },
    },
    {
-      dTagSlot: "slot2",
+      dTagSlot: SLOT_B,
      blobContexts: {
        "ch:gamma": 55, // just the frontier, no override keys
      },
@@ -1072,7 +1081,11 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessActive_afterMerge", async
  assert.equal(merged.frontiers.get("ch:gamma"), 55);
  assert.deepEqual(merged.overrides.get("ch:gamma"), { s: 1, c: 0, b: 60 });

-  const liveness = computeOverrideLiveness(merged, "ch:gamma");
+  const liveness = computeOverrideLiveness(
+    merged,
+    "ch:gamma",
+    merged.frontiers.get("ch:gamma") ?? 0,
+  );
  // S(1)>0, F(55)<=B(60), S(1)>C(0) → active
  assert.equal(liveness.active, true);
  assert.equal(liveness.frontier, 55);
@@ -1081,7 +1094,7 @@ test("mergeReadStateEventsStructured_twoBlobs_livenessActive_afterMerge", async
 test("mergeReadStateEventsStructured_unknownPubkey_returnsEmpty", async () => {
  const pubkey = "aa".repeat(32);
  const wrongPubkey = "bb".repeat(32);
-  const { event, decrypt } = makeEvent(pubkey, "slot1", { "ch:x": 100 });
+  const { event, decrypt } = makeEvent(pubkey, SLOT_A, { "ch:x": 100 });
  // Pass event but query with wrong pubkey — parseReadStateEvent returns null for each
  const merged = await mergeReadStateEventsStructured(
    [event],
@@ -1107,7 +1120,7 @@ test("mergeReadStateEventsStructured_fullCollisionWitness_distinctNamespaces", a

  const merged = await mergeFakeEvents(pubkey, [
    {
-      dTagSlot: "slot1",
+      dTagSlot: SLOT_A,
      blobContexts: {
        // raw ctx `ov_s:evil`: frontier published escaped; register uses raw suffix
        "esc:ov_s:evil": 7,
@@ -1122,7 +1135,7 @@ test("mergeReadStateEventsStructured_fullCollisionWitness_distinctNamespaces", a
      },
    },
    {
-      dTagSlot: "slot2",
+      dTagSlot: SLOT_B,
      blobContexts: {
        // second device advances `evil` frontier and register
        evil: 120,
@@ -1163,7 +1176,11 @@ test("mergeReadStateEventsStructured_fullCollisionWitness_distinctNamespaces", a
  );

  // Liveness for `ov_s:evil`: F(7), s=1, c=0, b=6 → F(7)>B(6) → inactive
-  const ovsEvilLiveness = computeOverrideLiveness(merged, "ov_s:evil");
+  const ovsEvilLiveness = computeOverrideLiveness(
+    merged,
+    "ov_s:evil",
+    merged.frontiers.get("ov_s:evil") ?? 0,
+  );
  assert.equal(
    ovsEvilLiveness.active,
    false,
@@ -1172,10 +1189,177 @@ test("mergeReadStateEventsStructured_fullCollisionWitness_distinctNamespaces", a
  assert.equal(ovsEvilLiveness.frontier, 7);

  // Liveness for `evil`: F(120), s=4, c=2, b=110 → F(120)>B(110) → inactive
-  const evilLiveness = computeOverrideLiveness(merged, "evil");
+  const evilLiveness = computeOverrideLiveness(
+    merged,
+    "evil",
+    merged.frontiers.get("evil") ?? 0,
+  );
  assert.equal(evilLiveness.active, false, "evil: F(120)>B(110) → inactive");
  assert.equal(evilLiveness.frontier, 120);

  // Wire keys never polluted: no `esc:` key survives in frontiers map
  assert.equal(merged.frontiers.has("esc:ov_s:evil"), false);
 });
+
+// ---------------------------------------------------------------------------
+// isValidReadStateDTag — spec-conformant d-tag validation (NIP-RS :55/:68)
+// ---------------------------------------------------------------------------
+
+test("isValidReadStateDTag_valid32HexSlot_returnsTrue", () => {
+  assert.equal(
+    isValidReadStateDTag("read-state:a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6"),
+    true,
+  );
+});
+
+test("isValidReadStateDTag_uppercaseHex_returnsFalse", () => {
+  // Uppercase letters are NOT valid — spec requires [0-9a-f]{32} only.
+  assert.equal(
+    isValidReadStateDTag("read-state:A1B2C3D4E5F6A7B8C9D0E1F2A3B4C5D6"),
+    false,
+  );
+});
+
+test("isValidReadStateDTag_nonHexChars_returnsFalse", () => {
+  // 'g' and 'z' are not hexadecimal.
+  assert.equal(
+    isValidReadStateDTag("read-state:gggggggggggggggggggggggggggggggg"),
+    false,
+  );
+});
+
+test("isValidReadStateDTag_31HexChars_returnsFalse", () => {
+  // One char short — must be exactly 32.
+  assert.equal(isValidReadStateDTag("read-state:" + "a".repeat(31)), false);
+});
+
+test("isValidReadStateDTag_33HexChars_returnsFalse", () => {
+  // One char over — must be exactly 32.
+  assert.equal(isValidReadStateDTag("read-state:" + "a".repeat(33)), false);
+});
+
+test("isValidReadStateDTag_missingPrefix_returnsFalse", () => {
+  assert.equal(isValidReadStateDTag("a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6"), false);
+});
+
+test("isValidReadStateDTag_emptySlot_returnsFalse", () => {
+  assert.equal(isValidReadStateDTag("read-state:"), false);
+});
+
+test("isValidReadStateDTag_undefined_returnsFalse", () => {
+  assert.equal(isValidReadStateDTag(undefined), false);
+});
+
+test("isValidReadStateDTag_legacySlot1_returnsFalse", () => {
+  // Non-conforming opaque slot names that were used in test fixtures before
+  // this fix must now be correctly rejected.
+  assert.equal(isValidReadStateDTag("read-state:slot1"), false);
+});
+
+// ---------------------------------------------------------------------------
+// decodeContexts — __proto__ context ID survives in both projections
+// ---------------------------------------------------------------------------
+
+test("decodeContexts_protoContextId_survivesBothProjections", () => {
+  // JSON.parse produces an object whose `__proto__` key is handled as a
+  // regular property by V8's JSON parser (it sets the value but does NOT
+  // change the prototype chain).  Without a null-prototype `wire` dict,
+  // assigning `wire[wireKey] = value` where wireKey === "__proto__" is a
+  // no-op (it modifies Object.prototype, not the object itself), so the
+  // flat projection silently loses the context while the `frontiers` Map
+  // retains it — the two claimed-equivalent views disagree.
+  //
+  // With `Object.create(null)` for `wire`, the assignment becomes an
+  // own-property write and both projections agree.
+  const raw = JSON.parse('{"__proto__":7,"normal":8}');
+
+  // Structured projection via parseContexts.
+  const { frontiers } = parseContexts(raw);
+
+  // Flat projection via sanitizeContexts.
+  const wire = sanitizeContexts(raw);
+
+  // frontiers Map must contain __proto__ with value 7.
+  assert.equal(frontiers.get("__proto__"), 7, "__proto__ must be in frontiers");
+  assert.equal(frontiers.get("normal"), 8, "normal must be in frontiers");
+
+  // wire must have __proto__ as an own property with value 7 — NOT inherited.
+  assert.equal(
+    Object.hasOwn(wire, "__proto__"),
+    true,
+    "__proto__ must be an own property of wire",
+  );
+  assert.equal(wire.__proto__, 7, "__proto__ wire value must be 7");
+  assert.equal(wire.normal, 8, "normal wire value must be 8");
+});
+
+// ---------------------------------------------------------------------------
+// computeOverrideLiveness — hierarchical effective-frontier witness
+// (NIP-RS :169-196/:510-514)
+// ---------------------------------------------------------------------------
+
+test("computeOverrideLiveness_hierarchicalFrontier_overrideBecomesInactive", async () => {
+  // Wire state:
+  //   channel ctx: frontier=100
+  //   thread ctx:  frontier=50, register s=1/c=0/b=60
+  //
+  // Thread own frontier: 50 → override would be active (F(50)<=B(60), S(1)>C(0)).
+  // Effective frontier applying hierarchical rule:
+  //   effective(thread) = max(merged[thread], merged[channel]) = max(50, 100) = 100
+  // With F_eff=100 > B(60) → override is INACTIVE.
+  //
+  // A caller that reads merged.frontiers.get("thread") and passes only 50
+  // would incorrectly see the override as active.  This test verifies that
+  // passing the correct effective frontier (100) gives the right answer.
+  const pubkey = "e1".repeat(32);
+  const channelCtx = "aabbccdd-channel-uuid";
+  const threadCtx =
+    "thread:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef";
+
+  const merged = await mergeFakeEvents(pubkey, [
+    {
+      dTagSlot: SLOT_A,
+      blobContexts: {
+        [channelCtx]: 100,
+        [threadCtx]: 50,
+        [`ov_s:${threadCtx}`]: 1,
+        [`ov_c:${threadCtx}`]: 0,
+        [`ov_b:${threadCtx}`]: 60,
+      },
+    },
+  ]);
+
+  assert.equal(merged.frontiers.get(channelCtx), 100);
+  assert.equal(merged.frontiers.get(threadCtx), 50);
+  assert.deepEqual(merged.overrides.get(threadCtx), { s: 1, c: 0, b: 60 });
+
+  // Own frontier only — this would INCORRECTLY show active.
+  const ownFrontierLiveness = computeOverrideLiveness(
+    merged,
+    threadCtx,
+    merged.frontiers.get(threadCtx) ?? 0,
+  );
+  assert.equal(
+    ownFrontierLiveness.active,
+    true,
+    "with own frontier F(50)<=B(60) the override looks active",
+  );
+
+  // Hierarchical effective frontier — correct answer is INACTIVE.
+  const channelFrontier = merged.frontiers.get(channelCtx) ?? 0;
+  const threadFrontier = merged.frontiers.get(threadCtx) ?? 0;
+  const effectiveFrontier = Math.max(threadFrontier, channelFrontier);
+  assert.equal(effectiveFrontier, 100);
+
+  const hierarchicalLiveness = computeOverrideLiveness(
+    merged,
+    threadCtx,
+    effectiveFrontier,
+  );
+  assert.equal(
+    hierarchicalLiveness.active,
+    false,
+    "with effective frontier F(100)>B(60) the override is correctly inactive",
+  );
+  assert.equal(hierarchicalLiveness.frontier, 100);
+});
--- a/desktop/src/features/channels/readState/readStateSnapshot.ts
+++ b/desktop/src/features/channels/readState/readStateSnapshot.ts
@@ -1,12 +1,11 @@
 import { nip44DecryptFromSelf } from "@/shared/api/tauri";
 import type { RelayEvent } from "@/shared/api/types";
 import {
+  decodeContexts,
  isOverrideActive,
  isValidBlob,
  isValidReadStateDTag,
  mergeOverrideRegisters,
-  parseContexts,
-  sanitizeContexts,
  type OverrideLiveness,
  type OverrideRegister,
  type ParsedContexts,
@@ -55,15 +54,15 @@ export async function parseReadStateEvent(
    const plaintext = await decrypt(event.content);
    const parsed = JSON.parse(plaintext);
    if (!isValidBlob(parsed)) return null;
-    const sanitized = sanitizeContexts(parsed.contexts);
+    const { wire, frontiers, overrides } = decodeContexts(parsed.contexts);
    return {
      dTag,
      blob: {
        v: 1,
        client_id: parsed.client_id,
-        contexts: sanitized,
+        contexts: wire,
      },
-      contexts: parseContexts(parsed.contexts),
+      contexts: { frontiers, overrides },
      createdAt: event.created_at,
    };
  } catch (error) {
@@ -134,17 +133,27 @@ export async function mergeReadStateEventsStructured(
 * frontier used — so the caller does not have to apply `isOverrideActive`
 * manually.  Call after `mergeReadStateEventsStructured`.
 *
- * @param merged   Result of `mergeReadStateEventsStructured`.
- * @param rawCtxId Raw (unescaped) context ID to evaluate.
+ * @param merged          Result of `mergeReadStateEventsStructured`.
+ * @param rawCtxId        Raw (unescaped) context ID to evaluate.
+ * @param effectiveFrontier
+ *   The merged **effective** frontier for `rawCtxId`, after applying the
+ *   hierarchical parent rule from NIP-RS `:169–196`/`:510–514`:
+ *   ```
+ *   effective(ctx) = max(merged[ctx], effective(parent(ctx)))
+ *   ```
+ *   For a channel context the effective frontier equals its own merged value.
+ *   For a thread or message context the caller must fold in the parent channel
+ *   frontier before passing the value here.  The caller owns this computation
+ *   because only the caller knows the context hierarchy.
 */
 export function computeOverrideLiveness(
  merged: MergedReadState,
  rawCtxId: string,
+  effectiveFrontier: number,
 ): OverrideLiveness {
-  const frontier = merged.frontiers.get(rawCtxId) ?? 0;
  const reg = merged.overrides.get(rawCtxId);
-  const active = reg !== undefined && isOverrideActive(reg, frontier);
-  return { active, frontier };
+  const active = reg !== undefined && isOverrideActive(reg, effectiveFrontier);
+  return { active, frontier: effectiveFrontier };
 }

 /**
--- a/desktop/src/features/communities/communityUnreadObserver.test.mjs
+++ b/desktop/src/features/communities/communityUnreadObserver.test.mjs
@@ -230,7 +230,7 @@ test("fetchCommunityUnread ignores self-authored and read thread/message events"
        pubkey: PUBKEY,
        created_at: 80,
        tags: [
-          ["d", "read-state:test"],
+          ["d", "read-state:a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6"],
          ["t", "read-state"],
        ],
        content: JSON.stringify({
@@ -708,7 +708,7 @@ function quietRelayWithReadState(readAtSeconds) {
        pubkey: PUBKEY,
        created_at: 200,
        tags: [
-          ["d", "read-state:test"],
+          ["d", "read-state:a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6"],
          ["t", "read-state"],
        ],
        content: JSON.stringify({