fix(security): patch chat-parser over-read and validate companion WS origin

Two independent issues found during a codebase security review.

1. NINJAM chat message parser (src/core/mpb.cpp)
   mpb_chat_message::parse assigned parms[x] = p before walking for the
   next NUL, and only checked for end-of-buffer AFTER an unconditional
   p++. A malicious server could send a chat message whose final
   parameter had no NUL terminator; parms[x] then pointed at
   unterminated memory. Downstream, njclient.cpp consumes the pointer
   via snprintf(" %s", ...) and ChatMessage_Callback, both of which
   C-string-walk past the buffer until they hit a stray NUL — a
   reliable heap info-disclosure primitive leaking bytes into chat UI
   and logs. The fix only records parms[x] AFTER confirming a NUL was
   seen strictly before endp; unterminated walks now break out of the
   loop and leave that slot null.

2. Video companion WebSocket server (juce/video/VideoCompanion.cpp,.h)
   The ixwebsocket server bound to 127.0.0.1:7170 accepted any
   handshake with no Origin or Host validation. Browsers do NOT apply
   SOP/CORS to raw WebSocket upgrades, so any webpage the user visited
   in another tab could open ws://127.0.0.1:7170 and, on Open, receive
   the config push (VDO.Ninja room ID, local username) plus subsequent
   roster broadcasts — then join the VDO.Ninja room directly to lurk
   on public-server sessions.

   - On Open, require Origin == "https://jamwide.audio" AND Host ==
     "127.0.0.1:<port>" or "localhost:<port>". Failures are closed
     with code 1008 ("origin not allowed") BEFORE any state is sent.
   - A new validatedClients_ set (guarded by wsMutex_) tracks which
     clients passed validation. broadcastRoster, broadcastBufferDelay,
     requestPopout, and the deactivate farewell now iterate
     wsServer_->getClients() but only send when the raw pointer is in
     validatedClients_, so an unvalidated client that ixwebsocket
     briefly keeps in its client list during close cannot receive
     state.
   - Close/Error messages remove the pointer from the set; stop and
     destructor clear the set entirely so stale pointers cannot carry
     across a server restart.

   The legitimate companion page at https://jamwide.audio opens
   ws://127.0.0.1:7170 and both headers match, so the check is
   transparent to users. Host validation also mitigates DNS rebinding.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: mkschulze

juce/video/VideoCompanion.cpp

@@ -56,6 +56,7 @@ VideoCompanion::~VideoCompanion()
             wsServer_->stop();
             wsServer_.reset();
         }
+        validatedClients_.clear();
     }
 }
 
@@ -273,11 +274,61 @@ bool VideoCompanion::startWebSocketServer(const SessionSnapshot& snap)
         {
             if (!aliveFlag->load(std::memory_order_acquire))
                 return;
+
             if (msg->type == ix::WebSocketMessageType::Open)
             {
+                // SECURITY: validate Origin and Host headers before emitting
+                // any state. Browsers do NOT apply SOP/CORS to raw WebSocket
+                // upgrades, so without this check any webpage the user visits
+                // in another tab could connect to ws://127.0.0.1:7170 and
+                // receive the room ID + roster on Open, then join VDO.Ninja
+                // directly to lurk. Also guards against DNS rebinding — only
+                // loopback Host values are accepted.
+                //
+                // The legit companion is loaded from https://jamwide.audio/
+                // and its JS opens ws://127.0.0.1:<port>, so Origin will be
+                // exactly "https://jamwide.audio" and Host will be
+                // "127.0.0.1:<port>" (or "localhost:<port>" with some browsers).
+                const auto& headers = msg->openInfo.headers;
+                auto originIt = headers.find("Origin");
+                auto hostIt   = headers.find("Host");
+                const std::string expectedHost1 = "127.0.0.1:" + std::to_string(snap.wsPort);
+                const std::string expectedHost2 = "localhost:" + std::to_string(snap.wsPort);
+                const bool originOk = (originIt != headers.end()
+                                       && originIt->second == "https://jamwide.audio");
+                const bool hostOk = (hostIt != headers.end()
+                                     && (hostIt->second == expectedHost1
+                                         || hostIt->second == expectedHost2));
+                if (!originOk || !hostOk)
+                {
+                    DBG("VideoCompanion: rejecting WS client — Origin='"
+                        << (originIt != headers.end() ? originIt->second : std::string("<missing>"))
+                        << "' Host='"
+                        << (hostIt   != headers.end() ? hostIt->second   : std::string("<missing>"))
+                        << "'");
+                    // 1008 = policy violation. Not in WebSocketCloseConstants
+                    // but RFC 6455 defines it; ix::WebSocket::close accepts any
+                    // uint16_t.
+                    webSocket.close(1008, "origin not allowed");
+                    return;  // do NOT add to validatedClients_ and do NOT send config
+                }
+
+                // Validation passed — register and send initial state.
+                {
+                    std::lock_guard<std::mutex> lock(wsMutex_);
+                    validatedClients_.insert(&webSocket);
+                }
                 sendConfigToClient(webSocket, snap);
             }
-            // Close: no-op (client disconnected)
+            else if (msg->type == ix::WebSocketMessageType::Close
+                  || msg->type == ix::WebSocketMessageType::Error)
+            {
+                // Remove before the underlying ix::WebSocket is destroyed so
+                // broadcasts never dereference a stale pointer. Error also
+                // triggers cleanup — ixwebsocket tears the connection down.
+                std::lock_guard<std::mutex> lock(wsMutex_);
+                validatedClients_.erase(&webSocket);
+            }
         }
     );
 
@@ -319,6 +370,11 @@ void VideoCompanion::stopWebSocketServer()
         wsServer_->stop();
         serverToStop = std::move(wsServer_);
         // wsServer_ is now null — broadcastRoster/sendConfig will bail early
+        // Clear the validated-clients set: the ix::WebSocket objects behind
+        // those pointers are about to be destroyed with the server. Any stale
+        // pointer lookup in broadcastRoster/broadcastBufferDelay is guarded
+        // by the wsServer_ null check that fires first under the same lock.
+        validatedClients_.clear();
     }
 
     // Destroy the server object off the message thread to avoid DAW state-save timeout.
@@ -451,11 +507,17 @@ void VideoCompanion::broadcastRoster(const std::vector<NJClient::RemoteUserInfo>
 
     json += "]}";
 
-    // Broadcast to all connected clients
+    // Broadcast to validated clients only. Iterate the set copy of raw
+    // pointers captured under the lock — then cross-reference each against
+    // getClients() (which owns shared_ptrs) so we never dereference a stale
+    // pointer. This prevents unvalidated clients from receiving roster state
+    // even if ixwebsocket briefly keeps them around during close.
     auto clients = wsServer_->getClients();
+    const auto payload = json.toStdString();
     for (auto& client : clients)
     {
-        client->send(json.toStdString());
+        if (validatedClients_.count(client.get()) > 0)
+            client->send(payload);
     }
 }
 
@@ -485,8 +547,10 @@ void VideoCompanion::broadcastBufferDelay(float bpm, int bpi)
     if (!wsServer_) return;
     cachedDelayMs_ = computed;
     auto clients = wsServer_->getClients();
+    const auto payload = json.toStdString();
     for (auto& client : clients)
-        client->send(json.toStdString());
+        if (validatedClients_.count(client.get()) > 0)
+            client->send(payload);
 }
 
 // ── Popout / Roster Lookup (Phase 13) ─────────────────────────────────────
@@ -501,8 +565,10 @@ void VideoCompanion::requestPopout(const juce::String& streamId)
     std::lock_guard<std::mutex> lock(wsMutex_);
     if (!wsServer_) return;
     auto clients = wsServer_->getClients();
+    const auto payload = json.toStdString();
     for (auto& client : clients)
-        client->send(json.toStdString());
+        if (validatedClients_.count(client.get()) > 0)
+            client->send(payload);
 }
 
 juce::String VideoCompanion::getStreamIdForUserIndex(int index) const
@@ -539,8 +605,10 @@ void VideoCompanion::deactivate()
         {
             juce::String json = "{\"type\":\"deactivate\"}";
             auto clients = wsServer_->getClients();
+            const auto payload = json.toStdString();
             for (auto& client : clients)
-                client->send(json.toStdString());
+                if (validatedClients_.count(client.get()) > 0)
+                    client->send(payload);
         }
     }
 

juce/video/VideoCompanion.h

@@ -156,9 +156,17 @@ class VideoCompanion
     int wsPort_ = kDefaultWsPort;
 
     std::unique_ptr<ix::WebSocketServer> wsServer_;
-    std::mutex wsMutex_;  // Guards wsServer_ start/stop and broadcast
+    std::mutex wsMutex_;  // Guards wsServer_ start/stop, broadcast, and validatedClients_
     std::future<void> stopFuture_;  // WR-01 fix: joinable handle for async server teardown
 
+    // Security: only clients whose Origin and Host headers matched the companion
+    // URL are added here on Open. Broadcasts iterate this set instead of
+    // wsServer_->getClients() so a rogue webpage cannot receive config/roster
+    // even briefly. Pointers are the address of the ix::WebSocket& passed to the
+    // onClientMessage callback — valid from Open until Close, and Close removes
+    // the entry. Guarded by wsMutex_.
+    std::set<ix::WebSocket*> validatedClients_;
+
     // Current session config (set on launch, sent to each connecting WS client)
     juce::String currentRoom_;
     juce::String currentPush_;

src/core/mpb.cpp

@@ -868,14 +868,24 @@ int mpb_chat_message::parse(Net_Message *msg) // return 0 on success
 
   const char *endp=(char*)msg->get_data()+msg->get_size();
 
+  // Security fix: only record parms[x] AFTER confirming a NUL terminator
+  // was seen strictly before endp. The original loop assigned parms[x]=p
+  // before walking, then tested the end-of-buffer condition only after an
+  // unconditional p++, which left the last parms[] pointing into an
+  // unterminated region whenever the wire payload ran off the end without
+  // a trailing NUL. Downstream consumers (snprintf %s, ChatMessage_Callback)
+  // would then C-string-walk past the message buffer, leaking adjacent heap
+  // contents to the chat UI and log. A malicious server could use this as a
+  // reliable read primitive.
   int x;
   memset(parms,0,sizeof(parms));
   for (x = 0; x < (int) (sizeof(parms)/sizeof(parms[0])); x ++)
   {
-    parms[x]=p;
+    const char *start = p;
     while (p < endp && *p) p++;
-    p++;
-    if (p >= endp) break;
+    if (p >= endp) break;   // unterminated: do NOT record this pointer
+    parms[x] = start;
+    p++;                    // skip the NUL we just confirmed
   }
   return x?0:3;
 }