Description

The new node.presence.alive event handling introduces an in-memory throttle map (recentNodePresencePersistAt) keyed by presenceKey = deviceId ?? nodeId. This key ultimately derives from the connecting client's connect.client.id / connect.device.id and can be made high-cardinality by repeatedly reconnecting with new IDs.

Impact:

• Unbounded memory growth: recentNodePresencePersistAt is never pruned and will grow with every new presenceKey observed.
• Disk/CPU I/O amplification even for unknown/unpaired IDs: the handler calls updatePairedNodeMetadata() (and optionally updatePairedDeviceMetadata()), which load pairing state from disk before discovering the ID is not paired. Because the throttle is per-presenceKey, an attacker can bypass the 60s interval by using many distinct IDs and force repeated JSON reads/lock contention.

Vulnerable code:

const presenceKey = opts?.deviceId ?? nodeId;
const lastPersistedAt = recentNodePresencePersistAt.get(presenceKey) ?? 0;
...
await Promise.all([
  updatePairedNodeMetadata(nodeId, { ... }),
  ...(opts?.deviceId ? [updatePairedDeviceMetadata(opts.deviceId, { ... })] : []),
]);
recentNodePresencePersistAt.set(presenceKey, receivedAtMs);

Recommendation

Mitigate high-cardinality key abuse and avoid expensive work for unknown IDs:

1. Bound and prune recentNodePresencePersistAt (e.g., LRU with a max size + TTL cleanup).
2. Only record throttle entries after confirming the node/device exists and was updated, and skip loading/persisting for unpaired IDs.
3. Add rate limiting for node.event / node.presence.alive at the transport layer (per-connection/per-IP) to prevent reconnect+flood patterns.

Example (sketch):

​// 1) Maintain a bounded LRU/TTL cache
const recentNodePresencePersistAt = new LruCache<string, number>({ max: 10_000, ttl: 10 * 60_000 });

​// 2) Verify paired node/device first (or provide a lightweight exists check)
const pairedNode = await getPairedNode(nodeId);
if (!pairedNode) return;

​// 3) Throttle and persist
const key = opts?.deviceId ?? nodeId;
const last = recentNodePresencePersistAt.get(key) ?? 0;
if (Date.now() - last < NODE_PRESENCE_PERSIST_MIN_INTERVAL_MS) return;
await updatePairedNodeMetadata(nodeId, patch);
recentNodePresencePersistAt.set(key, Date.now());

Description

The gateway handles node.presence.alive events by extracting obj.trigger from attacker-controlled payloadJSON and persisting it as lastSeenReason for both the node and (optionally) the paired device.

• Input: evt.payloadJSON comes from the node via node.event.
• Transformation: normalizeOptionalString(obj.trigger) only trims whitespace; it does not enforce an allowlist or maximum length.
• Sink: the value is persisted to the pairing state via updatePairedNodeMetadata(...lastSeenReason: trigger) / updatePairedDeviceMetadata(...lastSeenReason: trigger), which writes to disk.

A malicious/compromised node can send a very large trigger string (e.g., megabytes). Even though persistence is throttled to once per minute, the gateway will still write the oversized value to disk, potentially causing:

• disk usage bloat (pairing JSON state file grows dramatically)
• increased memory/CPU usage on subsequent loads/serializations
• degraded performance or denial of service

Vulnerable code:

const trigger = normalizeOptionalString(obj.trigger) ?? "background";
...
updatePairedNodeMetadata(nodeId, { lastSeenReason: trigger })
...
updatePairedDeviceMetadata(opts.deviceId, { lastSeenReason: trigger })

Recommendation

Constrain trigger before persisting:

1. Enforce a strict allowlist (preferred) or a conservative max length.
2. Optionally, normalize to a fixed set of known reasons.

Example fix (allowlist + fallback):

const rawTrigger = normalizeOptionalString(obj.trigger) ?? "background";
const allowed = new Set(["background","silent_push","bg_app_refresh","significant_location","connect"]);
const trigger = allowed.has(rawTrigger) ? rawTrigger : "background";

If you must accept arbitrary strings, cap length:

const trigger = (normalizeOptionalString(obj.trigger) ?? "background").slice(0, 64);

This prevents oversized payloads from bloating persisted state and reduces downstream risk if the value is later displayed in UIs/logs.

No security concerns identified. The new node.presence.alive event path reuses the existing node-auth node.event contract and does not require operator privileges. The trigger value written to persistent storage is passed through normalizeOptionalString before use. No secrets, tokens, or new network attack surfaces are introduced.

1. src/infra/device-pairing.ts, line 738 (link)

   Write key not normalized

   The read on line 732 uses normalizeDeviceId(deviceId) (which trims whitespace), but the write on line 738 uses the raw deviceId. If deviceId ever contains leading/trailing whitespace the read would find the existing entry under the trimmed key but the write would create a new orphaned entry under the un-normalized key, leaving the original record stale.

   Compare the parallel updatePairedNodeMetadata in node-pairing.ts, which correctly keeps a single normalized variable and uses it for both the read and the write.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/infra/device-pairing.ts
   Line: 738
   
   Comment:
   **Write key not normalized**
   
   The read on line 732 uses `normalizeDeviceId(deviceId)` (which trims whitespace), but the write on line 738 uses the raw `deviceId`. If `deviceId` ever contains leading/trailing whitespace the read would find the existing entry under the trimmed key but the write would create a new orphaned entry under the un-normalized key, leaving the original record stale.
   
   Compare the parallel `updatePairedNodeMetadata` in `node-pairing.ts`, which correctly keeps a single `normalized` variable and uses it for both the read and the write.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

Best possible solution:

Close this PR as superseded by #73330.

Do we have a high-confidence way to reproduce the issue?

Yes for the review findings: the PR diff shows the freeform trigger persistence and unbounded throttle map directly. I did not run a live iOS/gateway scenario in this read-only cleanup review.

Is this the best way to solve the issue?

No, this branch is no longer the best way to solve the issue. The merged replacement is narrower and safer because it uses authenticated device identity, closed reason normalization, bounded throttling, docs, and regression tests.

Security review:

Security review needs attention: The stale branch still has concrete resource-exhaustion concerns in the node-authenticated presence persistence path; the merged replacement addresses the same class of issues.

• [medium] Freeform Presence Reason Persistence — src/gateway/server-node-events.ts:695
  The PR branch can persist arbitrary node-controlled trigger strings into pairing metadata as lastSeenReason, which can bloat durable state and downstream loads.
  Confidence: 0.89
• [medium] Unbounded Presence Throttle Tracking — src/gateway/server-node-events.ts:43
  The PR branch keeps recentNodePresencePersistAt as an unbounded module-level map, allowing high-cardinality authenticated node traffic to grow memory and drive repeated metadata lookups.
  Confidence: 0.86

AGENTS.md: found and applied where relevant.

What I checked:

• linked superseding PR: feat: add authenticated iOS background presence beacon #73330 (feat: add authenticated iOS background presence beacon) is merged at 2026-04-28T07:10:36Z.
• cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
• no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

• steipete: Authored the merged replacement PR that carried forward this PR's useful behavior with tighter protocol and persistence boundaries, and left the maintainer review explaining the remaining hardening issues on this branch. (role: replacement implementer and reviewer; confidence: high; commits: 8234825675d8, 23ef816dc468, bdba90a20b2b; files: src/gateway/server-node-events.ts, src/shared/node-presence.ts, apps/ios/Sources/Push/BackgroundAliveBeacon.swift)
• ngutman: Authored this PR's branch commits and is credited as co-author on the merged replacement commit, making them relevant to original intent and source provenance. (role: original source contributor and replacement co-author; confidence: high; commits: c496da5a8518, 89351872e04f, 8234825675d8; files: apps/ios/Sources/Model/NodeAppModel.swift, src/gateway/server-node-events.ts, src/gateway/node-catalog.ts)

Codex review notes: model internal, reasoning high; reviewed against 010b61746379.