1. src/gateway/server-node-events.ts, line 1382-1408 (link)

   Throttle check–set gap allows concurrent double-writes

   There is an await between the throttle read and the map update. Two concurrent handleNodeEvent calls for the same deviceId arriving within the same event-loop tick will both pass the now - lastPersistedAt < NODE_PRESENCE_PERSIST_MIN_INTERVAL_MS check, both call Promise.all([updatePairedNodeMetadata, updatePairedDeviceMetadata]), and both write to the pairing store before either reaches recentNodePresencePersistAt.set(deviceId, now). The extra write is harmless in production (it just re-stamps the same timestamp), but it defeats the throttle's intent and means getRecentNodePresencePersistCountForTests() might not reflect the actual number of DB writes.

   A simple guard is to tentatively mark the device in the throttle map before awaiting, and remove it on failure:

   // Before the Promise.all
   recentNodePresencePersistAt.set(deviceId, now);   // optimistic lock
   // ... await ...
   // On failure path:
   recentNodePresencePersistAt.delete(deviceId);      // roll back if unpaired/error

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/gateway/server-node-events.ts
   Line: 1382-1408
   
   Comment:
   **Throttle check–set gap allows concurrent double-writes**
   
   There is an `await` between the throttle read and the map update. Two concurrent `handleNodeEvent` calls for the same `deviceId` arriving within the same event-loop tick will both pass the `now - lastPersistedAt < NODE_PRESENCE_PERSIST_MIN_INTERVAL_MS` check, both call `Promise.all([updatePairedNodeMetadata, updatePairedDeviceMetadata])`, and both write to the pairing store before either reaches `recentNodePresencePersistAt.set(deviceId, now)`. The extra write is harmless in production (it just re-stamps the same timestamp), but it defeats the throttle's intent and means `getRecentNodePresencePersistCountForTests()` might not reflect the actual number of DB writes.
   
   A simple guard is to tentatively mark the device in the throttle map before awaiting, and remove it on failure:
   
   ```typescript
   // Before the Promise.all
   recentNodePresencePersistAt.set(deviceId, now);   // optimistic lock
   // ... await ...
   // On failure path:
   recentNodePresencePersistAt.delete(deviceId);      // roll back if unpaired/error
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

2. src/gateway/server-node-events.ts, line 1103-1126 (link)

   Fallback eviction can remove in-window throttle entries

   When map.size > maxEntries and no entries are past the TTL, the for loop deletes nothing, and the while loop removes the oldest-inserted key unconditionally. Because the map is keyed by deviceId and values are updated in-place (not re-inserted), the "oldest-inserted" key is whatever device first appeared in the process's lifetime — its stored timestamp could still be within the 60-second throttle window. Removing it allows that device to bypass throttling immediately after eviction.

   With MAX_RECENT_NODE_PRESENCE_KEYS = 1024 this only matters at scale, but worth noting: the fallback could prefer evicting the entry with the largest elapsed time (now - ts) rather than the first insertion, to ensure the still-fresh entry is not mistakenly removed.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/gateway/server-node-events.ts
   Line: 1103-1126
   
   Comment:
   **Fallback eviction can remove in-window throttle entries**
   
   When `map.size > maxEntries` and no entries are past the TTL, the `for` loop deletes nothing, and the `while` loop removes the oldest-inserted key unconditionally. Because the map is keyed by `deviceId` and values are updated in-place (not re-inserted), the "oldest-inserted" key is whatever device first appeared in the process's lifetime — its stored timestamp could still be within the 60-second throttle window. Removing it allows that device to bypass throttling immediately after eviction.
   
   With `MAX_RECENT_NODE_PRESENCE_KEYS = 1024` this only matters at scale, but worth noting: the fallback could prefer evicting the entry with the *largest* elapsed time (`now - ts`) rather than the first insertion, to ensure the still-fresh entry is not mistakenly removed.
   
   How can I resolve this? If you propose a fix, please make it concise.

Description

The NODE_PRESENCE_ALIVE_EVENT handler parses evt.payloadJSON with JSON.parse via parsePayloadObject() without any event-specific size bound.

• The WebSocket connection max payload is set to MAX_PAYLOAD_BYTES = 25 * 1024 * 1024 after connect, so an authenticated client can send very large JSON frames.
• parsePayloadObject() calls JSON.parse(payloadJSON) directly, which can cause significant CPU and memory pressure on large inputs.
• The presence event is expected to be small (e.g., a trigger string), so allowing multi-megabyte payloads provides an unnecessary resource-exhaustion vector.

Vulnerable code:

case NODE_PRESENCE_ALIVE_EVENT: {
  const obj = parsePayloadObject(evt.payloadJSON);
  ...
}

function parsePayloadObject(payloadJSON?: string | null): Record<string, unknown> | null {
  ...
  payload = JSON.parse(payloadJSON) as unknown;
  ...
}

Recommendation

Add a strict maximum size for presence payloads before attempting to parse, and/or enforce it at schema validation.

Example (handler-level guard):

const MAX_PRESENCE_PAYLOAD_BYTES = 4 * 1024; // presence should be tiny

case NODE_PRESENCE_ALIVE_EVENT: {
  if (!evt.payloadJSON || Buffer.byteLength(evt.payloadJSON, "utf8") > MAX_PRESENCE_PAYLOAD_BYTES) {
    return { ok: true, event: evt.event, handled: false, reason: "payload_too_large" };
  }
  const obj = parsePayloadObject(evt.payloadJSON);
  ...
}

Optionally also constrain payloadJSON length in NodeEventParamsSchema for this event (or split into per-event schemas) to reject oversized input earlier.