mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
fix(pairing): close deferred-offline reclaim race + idempotent same-code adopt (#192)
A fresh unclaimed player that reconnects (same fingerprint) INSIDE the server's ~5s deferred-offline grace hit a false 'active on another connection' reclaim reject, then collided on UNIQUE(devices.pairing_code) on the fall-through INSERT and wedged unclaimed with no content. Real trial customer (web player) hit it. server/ws/deviceSocket.js: - Fix A (guard): gate the liveConn reclaim reject on !inDeferredOffline (pendingOfflines.has(id)). A device mid-deferred-offline is a zombie, not live, so a same-fingerprint reconnect is a legit reconnect, not a hijack. A genuinely live socket (never disconnected -> no pending-offline) still rejects a cloned fingerprint -> anti-hijack boundary preserved (documented). - Fix B (idempotency): when the unclaimed old row holds the SAME pairing_code the reconnecting player presents, ADOPT/refresh it (mirror the claimed-reclaim path, but no device:paired) instead of INSERT-colliding. Differing-code case unchanged. - deferOffline is NOT shrunk (it exists to prevent transient-blip flapping). server/player/index.html: - The cold-boot flap source: an unfiltered pageshow handler ran verifyLivenessSoon() on every load, opening+registering a socket early, which the boot connect() then tore down and rebuilt (connect->register->disconnect->reconnect). Guard it with ev.persisted (mirror the pagehide guard) so only real bfcache restores trigger it. server/test/pairing-race.test.js: - Forces the race against the real socket server (log-gated reconnect inside the deferred-offline window), asserts no false reject / no UNIQUE collision / single claimable row; + a hijack case asserting a cloned fingerprint on a genuinely-live display is still rejected. Web- and android-shaped fingerprints. Fails 2/4 on pre-fix code, 4/4 with the fix. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
screentinker
GitHub
parent
87d5c5e1d7
commit
ba0663edc1
|
|
@ -2612,7 +2612,14 @@
|
|||
// tear down a live socket on the hidden->visible gap.
|
||||
if (document.visibilityState === 'visible') { requestWakeLock(); verifyLivenessSoon(); }
|
||||
});
|
||||
window.addEventListener('pageshow', verifyLivenessSoon); // sleep/resume via bfcache restore
|
||||
// pairing-race fix: pageshow fires on EVERY load (persisted=false), not only bfcache restores.
|
||||
// Unfiltered it ran verifyLivenessSoon() on the initial cold load, which opened the socket EARLY
|
||||
// (if !socket -> connect) and registered a pairing code — then the intended boot path (tap-overlay
|
||||
// / connectBtnFunc) called connect() again, tearing that socket down and recreating it (the
|
||||
// connect->register->disconnect->reconnect flap that collided with the server's offline grace and
|
||||
// produced the UNIQUE pairing_code collision). Only act on a REAL bfcache restore (persisted=true),
|
||||
// mirroring the pagehide guard above; the cold-boot connect is owned solely by the boot path.
|
||||
window.addEventListener('pageshow', (ev) => { if (ev && ev.persisted) verifyLivenessSoon(); }); // sleep/resume via bfcache restore
|
||||
window.addEventListener('online', verifyLivenessSoon); // network switch (wifi<->cellular)
|
||||
|
||||
// feat/offline-cause-log: display sleep / backgrounding proxy — screen off/on on a TV.
|
||||
|
|
|
|||
214
server/test/pairing-race.test.js
Normal file
214
server/test/pairing-race.test.js
Normal file
|
|
@ -0,0 +1,214 @@
|
|||
'use strict';
|
||||
|
||||
// Pairing-race (fix/pairing-race-1.9.9) — PROVES the deferred-offline zombie race is closed.
|
||||
//
|
||||
// THE BUG (now fixed in ws/deviceSocket.js): on socket disconnect the server defers
|
||||
// heartbeat.removeConnection() by OFFLINE_DEBOUNCE_MS (~5s) via `pendingOfflines`, so
|
||||
// heartbeat.getConnection() keeps returning a "live" ZOMBIE for the grace window. A
|
||||
// same-fingerprint reconnect INSIDE that window used to hit the fingerprint-reclaim
|
||||
// guard and false-reject with device:auth-error, and for an UNCLAIMED row the retry then
|
||||
// INSERTed the on-screen pairing_code the zombie still held -> UNIQUE constraint failed:
|
||||
// devices.pairing_code -> wedged, unclaimed player.
|
||||
//
|
||||
// WHAT WE VALIDATE:
|
||||
// CASE 1 — a same-fingerprint + same-code reconnect INSIDE the deferred-offline window is
|
||||
// NOT rejected, does NOT collide on UNIQUE(pairing_code), and settles to exactly ONE
|
||||
// still-claimable row (Fix A gate `!inDeferredOffline` + Fix B same-code adopt).
|
||||
// CASE 2 — a genuinely-live display (never disconnected, so NO pending-offline armed) STILL
|
||||
// rejects a cloned-fingerprint takeover with device:auth-error (Fix A did not open a hole).
|
||||
//
|
||||
// Both cases run for a web-style fingerprint AND an android-style (SHA-256 hex) fingerprint.
|
||||
//
|
||||
// HARNESS: spawn the real server.js against a temp DATA_DIR + socket.io-client, mirroring
|
||||
// fingerprint-reclaim.test.js. DETERMINISM: to guarantee socket B reconnects AFTER socket A's
|
||||
// server-side disconnect has ARMED the pending-offline timer (and BEFORE the 5s timer fires),
|
||||
// we poll the server log for the exact line the disconnect handler prints
|
||||
// ("Device disconnected: <id> (offline transition deferred ...)"). That line is emitted on the
|
||||
// same synchronous tick, immediately before pendingOfflines.set(...), so observing it proves the
|
||||
// zombie window is open — no wall-clock sleep, no reliance on a fixed reconnect budget.
|
||||
|
||||
const { test, before, after } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawn } = require('node:child_process');
|
||||
const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
const fs = require('node:fs');
|
||||
const crypto = require('node:crypto');
|
||||
const ioClient = require('socket.io-client');
|
||||
const Database = require('better-sqlite3');
|
||||
|
||||
const { freePort } = require('./helpers/free-port');
|
||||
|
||||
let PORT, BASE;
|
||||
const DATA_DIR = path.join(os.tmpdir(), 'st-prace-' + crypto.randomBytes(4).toString('hex'));
|
||||
const LOG = path.join(os.tmpdir(), 'st-prace-' + crypto.randomBytes(4).toString('hex') + '.log');
|
||||
const DB_PATH = path.join(DATA_DIR, 'db', 'remote_display.db');
|
||||
let proc, tdb;
|
||||
|
||||
const sleep = (ms) => new Promise((r) => setTimeout(r, ms));
|
||||
|
||||
before(async () => {
|
||||
PORT = await freePort();
|
||||
BASE = `http://127.0.0.1:${PORT}`;
|
||||
const logFd = fs.openSync(LOG, 'w');
|
||||
proc = spawn('node', ['server.js'], {
|
||||
cwd: path.join(__dirname, '..'),
|
||||
env: {
|
||||
...process.env,
|
||||
DATA_DIR,
|
||||
SELF_HOSTED: 'true',
|
||||
PORT: String(PORT),
|
||||
NODE_ENV: 'test',
|
||||
// Raise the flap/connect-rate ceiling so rapid connect/disconnect/reconnect of the same
|
||||
// fingerprint in these tests can never be soft-refused as flapping (would mask the result).
|
||||
CONNECT_RATE_MAX: '1000',
|
||||
CONNECT_RATE_ANON_MAX: '1000',
|
||||
CONNECT_RATE_QUARANTINE_TRIPS: '0',
|
||||
},
|
||||
stdio: ['ignore', logFd, logFd],
|
||||
});
|
||||
let up = false;
|
||||
for (let i = 0; i < 80; i++) {
|
||||
try { const r = await fetch(BASE + '/api/status'); if (r.ok) { up = true; break; } } catch { /* */ }
|
||||
await sleep(250);
|
||||
}
|
||||
if (!up) throw new Error('server did not boot:\n' + fs.readFileSync(LOG, 'utf8').slice(-2000));
|
||||
tdb = new Database(DB_PATH); tdb.pragma('busy_timeout = 3000'); tdb.pragma('foreign_keys = OFF');
|
||||
});
|
||||
|
||||
after(() => {
|
||||
try { tdb && tdb.close(); } catch { /* */ }
|
||||
try { proc.kill('SIGKILL'); } catch { /* */ }
|
||||
});
|
||||
|
||||
// ---- helpers -------------------------------------------------------------
|
||||
|
||||
const rndCode = () => String(crypto.randomInt(100000, 1000000));
|
||||
const readLog = () => { try { return fs.readFileSync(LOG, 'utf8'); } catch { return ''; } };
|
||||
|
||||
// Poll the server log until `substr` appears (event-driven proof a server-side step ran).
|
||||
async function waitForLog(substr, timeoutMs = 4000) {
|
||||
const deadline = Date.now() + timeoutMs;
|
||||
while (Date.now() < deadline) {
|
||||
if (readLog().includes(substr)) return true;
|
||||
await sleep(25);
|
||||
}
|
||||
throw new Error(`timed out waiting for log line: ${substr}`);
|
||||
}
|
||||
|
||||
function openSocket() {
|
||||
return ioClient(`${BASE}/device`, { transports: ['websocket'], reconnection: false, forceNew: true });
|
||||
}
|
||||
|
||||
// Emit device:register on `sock` and resolve on the FIRST terminal server verdict.
|
||||
// Also records whether device:paired arrived (settle-checked by callers) on sock._paired.
|
||||
function register(sock, payload) {
|
||||
return new Promise((resolve) => {
|
||||
let done = false;
|
||||
const fin = (o) => { if (!done) { done = true; resolve(o); } };
|
||||
sock._paired = false;
|
||||
sock.on('device:paired', () => { sock._paired = true; });
|
||||
sock.once('device:registered', (d) => fin({ outcome: 'registered', device_id: d.device_id, token: d.device_token, status: d.status }));
|
||||
sock.once('device:auth-error', (e) => fin({ outcome: 'auth-error', error: (e && e.error) || '' }));
|
||||
sock.once('device:throttled', (e) => fin({ outcome: 'throttled', reason: (e && e.reason) || '' }));
|
||||
if (sock.connected) sock.emit('device:register', payload);
|
||||
else sock.once('connect', () => sock.emit('device:register', payload));
|
||||
setTimeout(() => fin({ outcome: 'timeout' }), 4000);
|
||||
});
|
||||
}
|
||||
|
||||
// Fingerprint shapes: a short web-player id and an android SHA-256 hardware hash. Fresh each call.
|
||||
const SHAPES = [
|
||||
{ label: 'web-style id', fp: () => `web-${crypto.randomBytes(3).toString('hex')}-${crypto.randomBytes(1).toString('hex')}` },
|
||||
{ label: 'android-style hash', fp: () => crypto.createHash('sha256').update('android|' + crypto.randomUUID()).digest('hex') },
|
||||
];
|
||||
|
||||
// ---- CASE 1 — the race is closed ----------------------------------------
|
||||
|
||||
for (const shape of SHAPES) {
|
||||
test(`CASE 1 [${shape.label}]: same-fingerprint+same-code reconnect INSIDE the deferred-offline window is adopted (no false reject, no UNIQUE collision, exactly one claimable row)`, async () => {
|
||||
const F = shape.fp();
|
||||
const C = rndCode();
|
||||
|
||||
// 1. Socket A: fresh unclaimed web-player-style registration (fingerprint + pairing_code, NO device_id).
|
||||
const sockA = openSocket();
|
||||
const rA = await register(sockA, { fingerprint: F, pairing_code: C, device_info: {} });
|
||||
assert.equal(rA.outcome, 'registered', 'socket A provisions cleanly');
|
||||
const deviceId = rA.device_id;
|
||||
assert.ok(deviceId, 'A got a device_id');
|
||||
|
||||
// Exactly one row for fingerprint F, holding code C, unclaimed.
|
||||
const fpRow = tdb.prepare('SELECT device_id FROM device_fingerprints WHERE fingerprint = ?').get(F);
|
||||
assert.equal(fpRow && fpRow.device_id, deviceId, 'fingerprint F is linked to A\'s new device row');
|
||||
let rowsForCode = tdb.prepare('SELECT id, user_id, pairing_code FROM devices WHERE pairing_code = ?').all(C);
|
||||
assert.equal(rowsForCode.length, 1, 'exactly one device row holds pairing_code C after A');
|
||||
assert.equal(rowsForCode[0].id, deviceId, 'that row IS A\'s row');
|
||||
assert.equal(rowsForCode[0].user_id, null, 'row is unclaimed (user_id NULL)');
|
||||
|
||||
// 2. Disconnect A. Wait ONLY until the server has ARMED the deferred-offline timer (log proof),
|
||||
// which is far inside the 5s window — the zombie connection is now live.
|
||||
sockA.close();
|
||||
await waitForLog(`Device disconnected: ${deviceId} (offline transition deferred`);
|
||||
|
||||
// Snapshot log length so the UNIQUE-collision check is scoped to the reconnect that follows.
|
||||
const logBefore = readLog();
|
||||
assert.ok(!logBefore.includes('UNIQUE constraint failed: devices.pairing_code'), 'no UNIQUE collision before the reconnect');
|
||||
|
||||
// 3. Socket B: SAME fingerprint F, SAME pairing_code C, no device_id — reconnect inside the window.
|
||||
const sockB = openSocket();
|
||||
const rB = await register(sockB, { fingerprint: F, pairing_code: C, device_info: {} });
|
||||
|
||||
// No false-positive reject / throttle — B is adopted.
|
||||
assert.notEqual(rB.outcome, 'auth-error', `B must NOT be false-rejected (got ${rB.outcome}${rB.error ? ': ' + rB.error : ''})`);
|
||||
assert.notEqual(rB.outcome, 'throttled', 'B must NOT be throttled');
|
||||
assert.equal(rB.outcome, 'registered', 'B resolves to device:registered (adopted the existing unclaimed row)');
|
||||
assert.equal(rB.device_id, deviceId, 'B adopted the SAME row (no new device_id)');
|
||||
|
||||
// Settle briefly to catch any late device:paired (there must be none — the row is unclaimed).
|
||||
await sleep(200);
|
||||
assert.equal(sockB._paired, false, 'no device:paired for an UNCLAIMED adopt (still on the pairing screen)');
|
||||
|
||||
// No UNIQUE(pairing_code) collision was hit by the reconnect.
|
||||
assert.ok(!readLog().includes('UNIQUE constraint failed: devices.pairing_code'), 'no UNIQUE constraint failed on devices.pairing_code');
|
||||
|
||||
// After settling: EXACTLY ONE row for fingerprint F, still code C, still claimable.
|
||||
const fpRow2 = tdb.prepare('SELECT device_id FROM device_fingerprints WHERE fingerprint = ?').get(F);
|
||||
assert.equal(fpRow2 && fpRow2.device_id, deviceId, 'fingerprint F still maps to the same single row');
|
||||
rowsForCode = tdb.prepare('SELECT id, user_id, pairing_code FROM devices WHERE pairing_code = ?').all(C);
|
||||
assert.equal(rowsForCode.length, 1, 'still EXACTLY ONE device row holds pairing_code C (no duplicate insert)');
|
||||
assert.equal(rowsForCode[0].id, deviceId, 'it is the same original row');
|
||||
assert.equal(rowsForCode[0].user_id, null, 'still claimable — user_id NULL');
|
||||
assert.equal(rowsForCode[0].pairing_code, C, 'still holds the on-screen code C');
|
||||
|
||||
sockB.close();
|
||||
});
|
||||
}
|
||||
|
||||
// ---- CASE 2 — hijack boundary intact ------------------------------------
|
||||
|
||||
for (const shape of SHAPES) {
|
||||
test(`CASE 2 [${shape.label}]: a genuinely-live display (no pending-offline armed) STILL rejects a cloned-fingerprint takeover`, async () => {
|
||||
const F = shape.fp();
|
||||
const C = rndCode();
|
||||
|
||||
// Socket A registers and STAYS CONNECTED — it never disconnects, so NO pending-offline timer is armed.
|
||||
const sockA = openSocket();
|
||||
const rA = await register(sockA, { fingerprint: F, pairing_code: C, device_info: {} });
|
||||
assert.equal(rA.outcome, 'registered', 'live socket A registers');
|
||||
const deviceId = rA.device_id;
|
||||
|
||||
// A second socket presents the SAME fingerprint F (a clone) while A is genuinely live.
|
||||
const sockB = openSocket();
|
||||
const rB = await register(sockB, { fingerprint: F, pairing_code: rndCode(), device_info: {} });
|
||||
|
||||
assert.equal(rB.outcome, 'auth-error', `cloned-fingerprint takeover of a LIVE display must be rejected (got ${rB.outcome})`);
|
||||
assert.match(rB.error, /active on another connection/i, 'rejected with the "active on another connection" auth-error');
|
||||
|
||||
// No duplicate row was minted for the clone's code, and the fingerprint still points at A's row.
|
||||
const fpRow = tdb.prepare('SELECT device_id FROM device_fingerprints WHERE fingerprint = ?').get(F);
|
||||
assert.equal(fpRow && fpRow.device_id, deviceId, 'fingerprint still linked to the live device (takeover gained nothing)');
|
||||
|
||||
sockA.close();
|
||||
sockB.close();
|
||||
});
|
||||
}
|
||||
|
|
@ -482,7 +482,27 @@ module.exports = function setupDeviceSocket(io) {
|
|||
// was cleared by hand. The real security boundary is the operator claiming the
|
||||
// code in the dashboard; a cloned fingerprint that falls through only gets an
|
||||
// unclaimed, tokenless, content-less row — harmless.
|
||||
if (liveConn) {
|
||||
// Pairing-race (1.9.9): a device mid-DEFERRED-OFFLINE is NOT genuinely live.
|
||||
// On disconnect we defer heartbeat.removeConnection() by OFFLINE_DEBOUNCE_MS
|
||||
// (anti-flap), so heartbeat.getConnection() keeps returning a ZOMBIE entry for the
|
||||
// just-closed socket for the whole grace window. A same-fingerprint reconnect inside
|
||||
// that window (this branch is only reached via a device_fingerprints match, so it
|
||||
// inherently IS the same physical display) would otherwise hit a FALSE-POSITIVE
|
||||
// "active on another connection" reject, and for an unclaimed row the retry then
|
||||
// collided on UNIQUE(devices.pairing_code) and wedged the player. Gate the reject on
|
||||
// there being NO pending-offline timer for this device.
|
||||
//
|
||||
// ANTI-HIJACK BOUNDARY (unchanged): a DIFFERENT physical device presenting a CLONED
|
||||
// fingerprint while the REAL device is actively connected never disconnected -> no
|
||||
// pending-offline timer is armed -> inDeferredOffline is false -> the liveConn reject
|
||||
// STILL fires. We ONLY relax the reject when the matched row's OWN connection is in
|
||||
// its deferred-offline grace (i.e. it just dropped) — a legitimate reconnect of that
|
||||
// same display. Even then, a CLAIMED row is reclaimed only because liveConn (the real
|
||||
// anti-hijack check) already passed, and an UNCLAIMED fall-through only ever yields a
|
||||
// tokenless, content-less row: the operator claiming the on-screen code stays the
|
||||
// real trust boundary, so relaxing this reject grants an attacker nothing.
|
||||
const inDeferredOffline = pendingOfflines.has(existing.device_id);
|
||||
if (liveConn && !inDeferredOffline) {
|
||||
// Log at most once per device per window so a retrying/stuck device can't flood stdout.
|
||||
const nowMs = Date.now();
|
||||
if (nowMs - (lastReclaimRejectLogAt.get(existing.device_id) || 0) >= config.reclaimRejectLogWindowMs) {
|
||||
|
|
@ -552,6 +572,50 @@ module.exports = function setupDeviceSocket(io) {
|
|||
// Fall through to the pairing_code path below, which provisions a fresh row with the
|
||||
// on-screen code and (#150) relinks the fingerprint to it. reclaimSettleSeconds no
|
||||
// longer gates this path — a genuinely live socket (above) is the only rejection.
|
||||
//
|
||||
// IDEMPOTENCY (pairing-race 1.9.9): EXCEPT when the reconnecting player presents the
|
||||
// SAME pairing_code this unclaimed row already holds. That is exactly the zombie case
|
||||
// Fix A now lets through: the row is mid-deferred-offline and STILL holds the on-screen
|
||||
// code, so the fall-through INSERT below collides on UNIQUE(devices.pairing_code) and
|
||||
// wedges the player unclaimed with no content. The stale-code hazard in the comment
|
||||
// above applies ONLY when the codes DIFFER; when they MATCH there is no stale code to
|
||||
// strand, so ADOPT the existing row (mirror the claimed-reclaim refresh above) instead
|
||||
// of re-INSERTing. Deliberately NO device:paired — the row is unclaimed and the
|
||||
// operator must still claim the on-screen code (the real trust boundary is untouched).
|
||||
if (pairing_code === oldDevice.pairing_code) {
|
||||
const newToken = generateDeviceToken();
|
||||
db.prepare('UPDATE devices SET device_token = ? WHERE id = ?').run(newToken, existing.device_id);
|
||||
console.log(`Fingerprint match: adopting UNCLAIMED same-code row ${existing.device_id} (code ${pairing_code}) instead of re-provisioning`);
|
||||
authenticated = true;
|
||||
// Cancel any pending offline timer - device is back in the grace window
|
||||
if (pendingOfflines.has(existing.device_id)) {
|
||||
clearTimeout(pendingOfflines.get(existing.device_id));
|
||||
pendingOfflines.delete(existing.device_id);
|
||||
}
|
||||
evictPriorSocket(existing.device_id, socket.id);
|
||||
db.prepare("UPDATE devices SET status = 'online', last_heartbeat = strftime('%s','now'), ip_address = ?, updated_at = strftime('%s','now'), offline_reason = NULL, offline_reason_at = NULL, offline_detail = NULL WHERE id = ?")
|
||||
.run(getClientIp(socket), existing.device_id);
|
||||
socket.emit('device:registered', { device_id: existing.device_id, device_token: newToken, status: 'online' });
|
||||
// No device:paired — the row is unclaimed; the player stays on the pairing screen
|
||||
// showing its (still-valid) code for the operator to claim.
|
||||
currentDeviceId = existing.device_id;
|
||||
heartbeat.registerConnection(existing.device_id, socket.id);
|
||||
heartbeat.recordReconnect(existing.device_id);
|
||||
persistIdentity(existing.device_id, data);
|
||||
socket.join(existing.device_id);
|
||||
logDeviceStatus(existing.device_id, 'online');
|
||||
emitToDeviceWorkspace(dashboardNs, existing.device_id, 'dashboard:device-status', { device_id: existing.device_id, status: 'online', liveness: heartbeat.livenessFor(existing.device_id) });
|
||||
// Flush any commands/playlist-updates queued while this device was offline.
|
||||
commandQueue.flushQueue(deviceNs, existing.device_id, buildPlaylistPayload);
|
||||
// Send playlist
|
||||
const access = checkDeviceAccess(existing.device_id);
|
||||
if (!access.allowed) {
|
||||
socket.emit('device:playlist-update', { assignments: [], suspended: true, message: access.message, detail: access.detail });
|
||||
} else {
|
||||
socket.emit('device:playlist-update', buildPlaylistPayload(existing.device_id));
|
||||
}
|
||||
return;
|
||||
}
|
||||
}
|
||||
}
|
||||
} else if (device_id || pairing_code) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue