From 17ac5c0a6f2a6e0979246ef38933f4d177ea719e Mon Sep 17 00:00:00 2001 From: KoalaDev <6156589+Shik3i@users.noreply.github.com> Date: Sun, 28 Jun 2026 05:54:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(host-control-mode):=20adversarial=20audit?= =?UTF-8?q?=20=E2=80=94=20force-sync=20stall,=20desync/lobby,=20gate=20par?= =?UTF-8?q?ity,=20BC=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit fixes (each verified against the actual code path): H-1 (server): track force-sync initiator on PREPARE; let the demoted initiator's EXECUTE through the host-only gate so mid-sync demotion no longer strands the whole room paused. Clear on EXECUTE/peer-leave. M-1 (background): episode-lobby gate now uses !amController() for parity with CONTENT_EVENT and server gates — co-hosts can drive the room and initiate lobbies, not just the owner. M-2/M-3 (popup/content/background): forceSyncReset respects hcmGuestLocked; desynced guest skips EPISODE_LOBBY so they don't get frozen in pause after lobby completion, and checkEpisodeLobbyCompletion excludes desynced peers from the required count so they don't block the lobby. M-4 (background): getHostSyncTarget clamps extrapolation to 2x heartbeat interval so a stale host heartbeat can't snap the guest tens of seconds past the host's real position. L-1..L-4 (server/content/background/popup): clarify dedup comment re: network-blip window, enforce desync invariant on SW-restore, add forceSyncBtn guest-locked backstop, refresh badge text in place. Backward compatibility (verified by BC-1..BC-4 regression tests): - Old client ↔ new server: server adds fields only, never requires; old heartbeats stripped of desynced; host-only enforced server-side even when the client has no awareness. - New client ↔ old server: empty capabilities → host-control UI hidden, all gates default to everyone, behavior byte-identical to pre-HCM. - Mixed rooms: every pre-HCM event type relays cleanly in both directions. --- extension/background.js | 35 ++++++--- extension/content.js | 19 ++++- extension/popup.js | 6 +- scripts/test-server-ws.mjs | 144 +++++++++++++++++++++++++++++++++++++ server/index.js | 40 +++++++++-- 5 files changed, 226 insertions(+), 18 deletions(-) diff --git a/extension/background.js b/extension/background.js index 836263a..481be64 100644 --- a/extension/background.js +++ b/extension/background.js @@ -1,4 +1,4 @@ -import { EVENTS, CONTROL_MODES, CAPABILITIES, PROTOCOL_VERSION, OFFICIAL_SERVER_URL, OFFICIAL_SERVER_TOKEN, EPISODE_LOBBY_TIMEOUT, FORCE_SYNC_TIMEOUT } from './shared/constants.js'; +import { EVENTS, CONTROL_MODES, CAPABILITIES, PROTOCOL_VERSION, OFFICIAL_SERVER_URL, OFFICIAL_SERVER_TOKEN, EPISODE_LOBBY_TIMEOUT, FORCE_SYNC_TIMEOUT, HEARTBEAT_INTERVAL } from './shared/constants.js'; import { generateUsername } from './shared/names.js'; import { loadLocale, getMessage, getSystemLanguage } from './i18n.js'; import { sameEpisode } from './episode-utils.js'; @@ -93,14 +93,22 @@ const HOST_ONLY_GATED_ACTIONS = [ EVENTS.EPISODE_LOBBY, EVENTS.EPISODE_LOBBY_CANCEL ]; // Best-effort estimate of where the room (host) is right now, for guest snap-back. -// Extrapolates from the host peer's last known state (±~1s). Used by content.js. +// Extrapolates from the host peer's last known state. Used by content.js. function getHostSyncTarget() { if (!currentRoom || !Array.isArray(currentRoom.peers)) return null; const host = currentRoom.peers.find(p => (typeof p === 'object' ? p.peerId : p) === hostPeerId); if (!host || typeof host !== 'object') return null; let targetTime = typeof host.currentTime === 'number' ? host.currentTime : null; if (targetTime !== null && host.playbackState === 'playing' && host.lastHeartbeat) { - targetTime += Math.max(0, (Date.now() - host.lastHeartbeat) / 1000); + // M-4: clamp extrapolation. lastHeartbeat is the *arrival* time of the host's + // last heartbeat — beyond ~2 heartbeat intervals the host's true state is too + // stale (they may have paused without the next heartbeat landing yet) and the + // linear extrapolation would overshoot by tens of seconds. Cap it so the + // guest snaps to a position within plausibility; the next heartbeat corrects. + const elapsedSec = (Date.now() - host.lastHeartbeat) / 1000; + if (elapsedSec > 0 && elapsedSec <= 2 * HEARTBEAT_INTERVAL / 1000) { + targetTime += elapsedSec; + } } return { playbackState: host.playbackState || null, targetTime }; } @@ -167,6 +175,11 @@ function ensureState() { serverCapabilities = Array.isArray(currentRoom.capabilities) ? currentRoom.capabilities : []; } if (data.hcmDesynced !== undefined) hcmDesynced = data.hcmDesynced; + // L-2: enforce the desync invariant on restore — a persisted hcmDesynced=true + // is stale if our restored role is no longer "gated guest" (e.g. we became + // the host, or the room is in 'everyone'). Without this, the first heartbeat + // after SW restart would broadcast a bogus Solo flag for up to 15s. + hcmEnforceDesyncInvariant(); if (data.lastActionState) lastActionState = data.lastActionState; if (data.eventQueue) eventQueue = [...eventQueue, ...data.eventQueue].slice(0, 50); @@ -1482,8 +1495,12 @@ function executeEpisodeLobby() { function checkEpisodeLobbyCompletion() { if (!episodeLobby || !currentRoom) return; - const peerCount = currentRoom.peers ? currentRoom.peers.length : 1; - if (episodeLobby.readyPeers.length >= peerCount) { + const peers = Array.isArray(currentRoom.peers) ? currentRoom.peers : []; + // M-3: desynced peers (watching on their own) sit out the lobby — their content + // script ignores EPISODE_LOBBY and never reports ready. Don't let them block + // completion: count only peers who actually participate. + const participatingCount = peers.filter(p => !(typeof p === 'object' && p.desynced)).length; + if (episodeLobby.readyPeers.length >= participatingCount) { executeEpisodeLobby(); } } @@ -2174,9 +2191,11 @@ async function handleAsyncMessage(message, sender, sendResponse) { // Host Control Mode: a gated guest must NOT initiate an episode lobby — the // server drops the guest's EPISODE_LOBBY, so the lobby would never complete // and the guest would self-pause (PAUSE_FOR_LOBBY) into a 60s freeze. In - // host-only the host drives episode sync; the guest just follows / snaps back. - if (controlMode === CONTROL_MODES.HOST_ONLY && !amHost()) { - addLog(`Episode change ("${newTitle}") — host-only guest, not creating a lobby (host drives).`, 'info'); + // host-only the controllers (owner + co-hosts) drive episode sync; a plain + // guest just follows / snaps back. Use amController() for parity with the + // CONTENT_EVENT gate and the server's controllers-based check. + if (controlMode === CONTROL_MODES.HOST_ONLY && !amController()) { + addLog(`Episode change ("${newTitle}") — host-only guest, not creating a lobby (controller drives).`, 'info'); sendResponse({ status: 'host_only_guest_skip' }); return; } diff --git a/extension/content.js b/extension/content.js index 499ede1..a7ade14 100644 --- a/extension/content.js +++ b/extension/content.js @@ -961,6 +961,15 @@ // Episode Auto-Sync: Lobby notification from background if (message.type === 'EPISODE_LOBBY') { + // Host Control Mode: a desynced guest is watching on their own and must + // not join the lobby flow. Otherwise they'd pause on title match, report + // ready, but then ignore the host's FORCE_SYNC_* (hcmDesynced skip in + // SERVER_COMMAND) and end up frozen in pause. They also can't be counted + // toward lobby completion (background filters them out). + if (hcmDesynced) { + sendResponse({ status: 'ignored_desynced' }); + return true; + } const expectedTitle = message.expectedTitle; if (expectedTitle) { reportLog(`Episode lobby received: waiting for "${expectedTitle}"`, 'info'); @@ -1504,9 +1513,13 @@ chrome.runtime.sendMessage({ type: 'GET_HCM_STRINGS' }, (res) => { if (chrome.runtime.lastError || !res) return; Object.keys(hcmStrings).forEach(k => { if (res[k]) hcmStrings[k] = res[k]; }); - // If the badge is already showing (early desync), re-render it with the - // localized text now that we have it. - if (hcmBadgeHost) { hcmRemoveBadge(); hcmShowBadge(); } + // If the badge is already showing (early desync), refresh its text in place. + // Re-creating the host element nukes the click target mid-poll and can drop a + // click that landed between remove() and the re-create (L-4). + if (hcmBadgeHost) { + const span = hcmBadgeHost.shadowRoot && hcmBadgeHost.shadowRoot.querySelector('span'); + if (span) span.textContent = '● ' + hcmStrings.badge; + } }); })(); diff --git a/extension/popup.js b/extension/popup.js index f2785f7..d1449f1 100644 --- a/extension/popup.js +++ b/extension/popup.js @@ -1494,6 +1494,7 @@ elements.targetTab.addEventListener('change', () => { }); elements.forceSyncBtn.addEventListener('click', async () => { + if (hcmGuestLocked) return; // guest in host-only room — backstop (M-2/L-3) if (elements.forceSyncBtn.disabled) return; const originalText = elements.forceSyncBtn.textContent; @@ -1535,7 +1536,10 @@ elements.forceSyncBtn.addEventListener('click', async () => { const peerCount = (status.peers || []).filter(p => (typeof p === 'object' ? p.peerId : p) !== localPeerId).length; const syncTimeoutMs = peerCount === 0 ? 3000 : 12000; const forceSyncReset = () => { - if (!forceSyncDone) { + // Don't unlock a button that's locked because we became a guest mid-flight — + // hcmGuestLocked is the source of truth for the lock state, and the next + // CONTROL_MODE update restores the correct label. + if (!forceSyncDone && !hcmGuestLocked) { elements.forceSyncBtn.disabled = false; elements.forceSyncBtn.textContent = originalText; } diff --git a/scripts/test-server-ws.mjs b/scripts/test-server-ws.mjs index a3606ef..7e7a0de 100644 --- a/scripts/test-server-ws.mjs +++ b/scripts/test-server-ws.mjs @@ -226,6 +226,150 @@ try { close(); resetConnectionRate(); + // --- H-1: a demoted co-host's FORCE_SYNC_EXECUTE still relays when they + // initiated the in-flight PREPARE. Without the initiator exemption, the + // EXECUTE would be dropped by the host-only gate and every peer would be + // left stuck paused. --- + const h1rid = 'h1-'+Date.now(); + const ho = await c(), hc = await c(), hg = await c(); // owner / co-host / guest + await j(ho, h1rid, 'owner'); await j(hc, h1rid, 'cohost'); await j(hg, h1rid, 'guest'); + ho._m.length = hc._m.length = hg._m.length = 0; + s(ho,'set_control_mode',{controlMode:'host-only'}); + await w(ho,'control_mode'); await w(hc,'control_mode'); await w(hg,'control_mode'); + ho._m.length = hc._m.length = hg._m.length = 0; + // owner promotes co-host; co-host initiates force sync + s(ho,'set_peer_role',{peerId:'cohost',controller:true}); + await w(hc,'control_mode'); + ho._m.length = hc._m.length = hg._m.length = 0; + s(hc,'force_sync_prepare',{targetTime:0}); + await w(ho,'force_sync_prepare'); await w(hg,'force_sync_prepare'); + ho._m.length = hc._m.length = hg._m.length = 0; + // owner demotes the co-host mid-flight — the EXECUTE must still go through. + s(ho,'set_peer_role',{peerId:'cohost',controller:false}); + await w(hc,'control_mode'); + ho._m.length = hc._m.length = hg._m.length = 0; + s(hc,'force_sync_execute',{}); + await w(ho,'force_sync_execute'); await w(hg,'force_sync_execute'); + // After the EXECUTE, the initiator slot is cleared: a fresh EXECUTE from the + // (now plain guest) co-host is gated again, confirming the exemption is scoped. + ho._m.length = hc._m.length = hg._m.length = 0; + s(hc,'force_sync_execute',{}); + let reGated=false; try { await w(ho,'force_sync_execute',500); } catch { reGated=true; } + assert.ok(reGated, 'initiator exemption is cleared after the EXECUTE relayes'); + close(); + resetConnectionRate(); + + // --- A guest's stray EXECUTE (no matching PREPARE they initiated) is still gated --- + const grid = 'h1b-'+Date.now(); + const go = await c(), gg = await c(); + await j(go, grid, 'own'); await j(gg, grid, 'gst'); + go._m.length = gg._m.length = 0; + s(go,'set_control_mode',{controlMode:'host-only'}); + await w(go,'control_mode'); await w(gg,'control_mode'); + go._m.length = gg._m.length = 0; + s(gg,'force_sync_execute',{}); + let uninitGated=false; try { await w(go,'force_sync_execute',500); } catch { uninitGated=true; } + assert.ok(uninitGated, 'guest FORCE_SYNC_EXECUTE without a matching PREPARE is gated'); + close(); + resetConnectionRate(); + + // ===================================================================== + // BACKWARD COMPATIBILITY — old clients (pre-HCM build) against new server. + // These tests simulate an old client by deliberately omitting fields the + // new feature added (desynced in heartbeats, capabilities expectation) + // and by ignoring CONTROL_MODE broadcasts. The wire format for existing + // events must stay byte-compatible: the relay must accept old payloads + // and must not inject new fields old clients would misread. + // ===================================================================== + + // --- BC-1: Old-client heartbeat (no `desynced` field) is accepted and + // relayed without injecting `desynced`. Old clients never sent the + // field; the relay must strip it from the wire so we don't surprise + // them with unexpected keys. --- + const bcrid = 'bc-'+Date.now(); + const bco = await c(), bcn = await c(); // bco = "old", bcn = "new" + await j(bco, bcrid, 'oldp'); await j(bcn, bcrid, 'newp'); + bco._m.length = bcn._m.length = 0; + // Old-client heartbeat: every field an old build would send, NO `desynced`. + s(bco,'peer_status',{ + status:'heartbeat', username:'old', tabTitle:'t', mediaTitle:'m', + playbackState:'playing', currentTime:42, volume:0.5, muted:false + }); + let bcRelay = null; const bcStart = Date.now(); + while (Date.now()-bcStart < 800 && !bcRelay) { + for (let i=0;isetTimeout(r,30)); + } + assert.ok(bcRelay, 'old-style heartbeat relayed'); + assert.ok(bcRelay.desynced === undefined, 'old-client heartbeat has no desynced on the wire (stripped)'); + assert.equal(bcRelay.currentTime, 42, 'old-client currentTime preserved'); + assert.equal(bcRelay.senderId, 'oldp', 'old-client senderId preserved'); + close(); + resetConnectionRate(); + + // --- BC-2: Old client in a host-only room — server still gates its events + // even though the client has no awareness of the mode. This is the + // key guarantee for mixed rooms during rollout: an old client can't + // drive a host-only room just because it ignores CONTROL_MODE. --- + const hmrid = 'hcmix-'+Date.now(); + const hmo = await c(), hmg = await c(); // hmo = host (new), hmg = "old" guest + await j(hmo, hmrid, 'hmixhost'); await j(hmg, hmrid, 'hmixold'); + hmo._m.length = hmg._m.length = 0; + s(hmo,'set_control_mode',{controlMode:'host-only'}); + await w(hmo,'control_mode'); await w(hmg,'control_mode'); // old client's socket still receives it; old client would ignore + hmo._m.length = hmg._m.length = 0; + // "Old" guest tries to drive — server must drop. Host must NOT receive it. + s(hmg,'pause',{currentTime:5}); + let oldGated=false; try { await w(hmo,'pause',600); } catch { oldGated=true; } + assert.ok(oldGated, 'old-client pause dropped in host-only (server enforces regardless of client awareness)'); + // Host's own command still relays to the old client's socket — old client + // applies it via its existing PLAY/PAUSE handler. + s(hmo,'pause',{currentTime:7}); await w(hmg,'pause'); + close(); + resetConnectionRate(); + + // --- BC-3: Mixed room with old + new client in 'everyone' mode — every + // event flows identically to pre-HCM. Confirms no regression in the + // default-mode relay path that could fragment a rolling-update room. --- + const mxrid = 'mix-'+Date.now(); + const mxo = await c(), mxn = await c(); // mxo = old, mxn = new + await j(mxo, mxrid, 'oldmx'); await j(mxn, mxrid, 'newmx'); + mxo._m.length = mxn._m.length = 0; + // Old → new + s(mxo,'play',{currentTime:1}); await w(mxn,'play'); + s(mxo,'seek',{currentTime:99}); await w(mxn,'seek'); + s(mxo,'force_sync_prepare',{targetTime:5}); await w(mxn,'force_sync_prepare'); + s(mxo,'episode_lobby',{expectedTitle:'S1E1'}); await w(mxn,'episode_lobby'); + // New → old + mxo._m.length = mxn._m.length = 0; + s(mxn,'pause',{currentTime:2}); await w(mxo,'pause'); + s(mxn,'seek',{currentTime:50}); await w(mxo,'seek'); + s(mxn,'force_sync_execute',{}); await w(mxo,'force_sync_execute'); + s(mxn,'episode_lobby_cancel',{}); await w(mxo,'episode_lobby_cancel'); + close(); + resetConnectionRate(); + + // --- BC-4: New-client heartbeat WITH `desynced` does not break an old + // client's receive path. The field is appended but old clients ignore + // unknown keys — verify the relay preserves every pre-HCM field and + // only adds `desynced`. --- + const b4rid = 'bc4-'+Date.now(); + const b4old = await c(), b4new = await c(); + await j(b4old, b4rid, 'b4o'); await j(b4new, b4rid, 'b4n'); + b4old._m.length = b4new._m.length = 0; + s(b4new,'peer_status',{status:'heartbeat', desynced:true, currentTime:7, playbackState:'paused', username:'newp'}); + let b4Relay = null; const b4Start = Date.now(); + while (Date.now()-b4Start < 800 && !b4Relay) { + for (let i=0;isetTimeout(r,30)); + } + assert.ok(b4Relay, 'new-client heartbeat relayed to old client'); + assert.equal(b4Relay.desynced, true, 'desynced preserved for new recipients'); + assert.equal(b4Relay.currentTime, 7, 'old fields preserved on the same relay'); + assert.equal(b4Relay.senderId, 'b4n', 'senderId preserved'); + close(); + resetConnectionRate(); + // --- Password room --- const prid = 'pw-'+Date.now(); const pw1 = await c(); await j(pw1, prid, 'admin', 's3cret'); diff --git a/server/index.js b/server/index.js index 42cbc10..43d50b8 100644 --- a/server/index.js +++ b/server/index.js @@ -247,15 +247,21 @@ function removePeerFromRoom(socketId, roomId, reason) { // reassign host to the earliest remaining peer so the feature stays usable. // (v1: immediate fallback, no grace period — see host-control-mode docs.) // Skip while a join for this peerId is in flight (peerJoinLocks holds it): - // that's a reconnect / second tab where the same peerId is being re-added - // right after — covers both the explicit 'dedupe' removal AND the - // 'disconnect' the kicked old socket fires. Demoting there would silently - // unlock the room on every host network blip. + // that's the reconnect / second-tab case where the same peerId is being + // re-added right after the dedupe kicked the old socket. Demoting there + // would silently unlock the room on every second-tab open. + // NOTE: this does *not* cover a true network blip where the old socket's + // 'disconnect' fires before the new socket acquires its join lock — in that + // window peerJoinLocks is empty, so the fallback fires. Documented v1 + // limitation (no host grace period); see KNOWN_LIMITATIONS.md. const peerRejoining = peerJoinLocks.has(peerId); const peerGone = !isPeerStillConnected && !peerRejoining; if (peerGone && room.controllers && room.peers.size > 0) { const wasController = room.controllers.has(peerId); room.controllers.delete(peerId); + // H-1: a leaving initiator strands the room's force-sync — release the + // slot so a future controller's PREPARE can take over cleanly. + if (room.forceSyncInitiator === peerId) room.forceSyncInitiator = null; if (room.hostPeerId === peerId) { // Owner left → reassign owner + fall back to 'everyone' so the room is // never stuck locked, and reset the controller set to just the new owner. @@ -410,7 +416,12 @@ io.on('connection', (socket) => { controlMode: CONTROL_MODES.EVERYONE, lastControlModeChangeAt: 0, // M-4: per-room debounce for control-mode toggles // Co-Host: peers allowed to drive in 'host-only'. Always includes the owner. - controllers: new Set([peerId]) + controllers: new Set([peerId]), + // H-1: peerId of the in-flight force-sync initiator. Lets a demoted + // controller's FORCE_SYNC_EXECUTE through the host-only gate — without + // it, demoting a co-host mid-force-sync would drop their EXECUTE and + // leave every peer stuck paused. + forceSyncInitiator: null }; rooms.set(roomId, room); createdByMe = true; @@ -544,12 +555,29 @@ io.on('connection', (socket) => { // In 'host-only' mode, drop room-moving events from anyone who is not // a controller (the owner + any promoted co-hosts). Robust chokepoint: // independent of client behavior, kills spam. Heartbeats/ACKs pass. - if (room.controlMode === CONTROL_MODES.HOST_ONLY && + // + // H-1 exception: a demoted co-host's FORCE_SYNC_EXECUTE still has to + // land — otherwise their already-relayed PREPARE would leave the whole + // room stuck paused. Track the in-flight initiator on PREPARE and let + // their matching EXECUTE through regardless of current controllers set. + if (eventName === EVENTS.FORCE_SYNC_PREPARE && + room.controlMode === CONTROL_MODES.HOST_ONLY && + room.controllers && room.controllers.has(mapping.peerId)) { + room.forceSyncInitiator = mapping.peerId; + } + const isOwnForceSyncExecute = eventName === EVENTS.FORCE_SYNC_EXECUTE && + room.forceSyncInitiator && mapping.peerId === room.forceSyncInitiator; + if (!isOwnForceSyncExecute && + room.controlMode === CONTROL_MODES.HOST_ONLY && !(room.controllers && room.controllers.has(mapping.peerId)) && HOST_ONLY_GATED_EVENTS.has(eventName)) { log('ROOM', `Dropped ${eventName} from guest ${mapping.peerId} in host-only room ${mapping.roomId.substring(0, 3)}***`); return; } + // Clear initiator tracking once the EXECUTE has been relayed. + if (eventName === EVENTS.FORCE_SYNC_EXECUTE && room.forceSyncInitiator) { + room.forceSyncInitiator = null; + } // --- S-2 & S-3: Sanitize ALL relay fields (strings, numbers, booleans) --- const clamp = (val, max) => typeof val === 'string' ? val.substring(0, max) : undefined;