From 090b6c12cb5054b32c07d8c49790acf58a229f23 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Mon, 27 Jul 2026 10:59:38 -0500 Subject: [PATCH] fix(pairing): expire a pairing code on device liveness, not row age MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A screen that was still connected and still displaying its pairing code could not be paired. Reloading the player produced the same code, and the on-screen instruction ("restart the display to get a new code") could not help. devices.created_at is written once, at first registration, and the row is never recreated: a player persists its device_id and its pairing code in local storage and re-registers with them forever. Expiry was measured from created_at, so 15 minutes after first boot the row became permanently unclaimable while the device kept heartbeating — and a restart reused the stored identity and reproduced the same code, so there was no way out. Observed in production: an unclaimed web player, still online and heartbeating, whose row was created 4 days 20 hours earlier and had been unpairable for all but its first 15 minutes. Prod is carrying several such rows; alpha has some 13 days old. Key expiry on last_heartbeat instead, falling back to created_at for a row that has never checked in. That answers the question the operator actually has — is this screen still there showing me this code? — while keeping the property the expiry exists for: a device that has genuinely gone away still expires. Trade-off, taken deliberately: a code stays claimable while its screen is connected rather than for a fixed 15 minutes. That is what the product implies, since the code is on the screen the whole time, and guessing is bounded by lib/pair-lockout (5 failures per IP per 15 min) and the 5/min route limit rather than by this TTL. SERVER-ONLY. The player's device:registered handler reads only device_id and device_token and has no way to display a server-issued code, so reissuing one would have left fielded players showing a stale code — strictly worse. This fix needs no player update and un-strands every already-affected device in the field on deploy. Co-Authored-By: Claude Opus 5 (1M context) --- server/lib/pair-lockout.js | 13 ++- server/server.js | 23 ++++- server/test/pairing-code-liveness.test.js | 119 ++++++++++++++++++++++ 3 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 server/test/pairing-code-liveness.test.js diff --git a/server/lib/pair-lockout.js b/server/lib/pair-lockout.js index a20a7f5..b2441d7 100644 --- a/server/lib/pair-lockout.js +++ b/server/lib/pair-lockout.js @@ -30,10 +30,15 @@ function recordFailure(ip, now = Date.now()) { // A successful pair (or any reason to forgive an IP) clears its failure record. function reset(ip) { failures.delete(ip); } -// A provisioning code is stale once it is older than the TTL (devices.created_at is the -// register time for a provisioning device). -function isCodeExpired(createdAtSec, now = Date.now()) { - return Math.floor(now / 1000) - createdAtSec > PAIRING_TTL_SEC; +// A provisioning code is stale once the device has not been seen for longer than the TTL. +// +// The caller passes a LIVENESS timestamp (devices.last_heartbeat, falling back to +// created_at for a row that has never checked in) — NOT the row's creation time. A player +// keeps its device_id and its code across restarts and re-registers with them forever, so +// created_at never advances; keying on it made a still-connected screen permanently +// unpairable 15 minutes after first boot. See the comment at the call site in server.js. +function isCodeExpired(lastSeenSec, now = Date.now()) { + return Math.floor(now / 1000) - lastSeenSec > PAIRING_TTL_SEC; } module.exports = { isLocked, recordFailure, reset, isCodeExpired, MAX_FAILS, LOCKOUT_MS, PAIRING_TTL_SEC }; diff --git a/server/server.js b/server/server.js index da91f26..26ce7ac 100644 --- a/server/server.js +++ b/server/server.js @@ -953,7 +953,28 @@ app.post('/api/provision/pair', requireAuth, resolveTenancy, checkDeviceLimit, ( // An EXPIRED code is a legitimate-but-stale code (a slow rollout, not an attack), so it // does NOT count toward the lockout - it just asks the display to regenerate. This keeps // a bulk rollout from one office/NAT IP from locking itself out on expired codes. - if (pairLockout.isCodeExpired(device.created_at)) { + // Expiry is keyed on LIVENESS, not on when the row was first created. + // + // devices.created_at is written once, at the device's first registration, and the row is + // never recreated: a player persists its device_id and its pairing code and re-registers + // with them forever. Keying expiry on created_at therefore made a screen permanently + // unclaimable 15 minutes after first boot, while it kept heartbeating and kept showing + // the code — and "restart the display to get a new code" could not help, because a + // restart reuses the stored identity and produces the same code. Seen in production on a + // web player whose row was 4 days old, still online, unpairable for all but its first 15 + // minutes. + // + // last_heartbeat answers the question the operator actually cares about: is this screen + // still there showing me this code? A device that has gone away for longer than the TTL + // still expires, which is what the expiry is for. Fall back to created_at for a row that + // has never checked in. + // + // Trade-off, deliberately taken: a code stays claimable while its screen is connected, + // rather than for a fixed 15 minutes. That is the behaviour the product implies (the code + // is displayed on the screen the whole time), and guessing is bounded by lib/pair-lockout + // (5 failures per IP per 15 min) plus the 5/min route limit, not by this TTL. + const lastSeen = device.last_heartbeat || device.created_at; + if (pairLockout.isCodeExpired(lastSeen)) { return res.status(410).json({ error: 'Pairing code expired - restart the display to get a new code' }); } pairLockout.reset(ip); // a valid claim forgives prior failed attempts from this IP diff --git a/server/test/pairing-code-liveness.test.js b/server/test/pairing-code-liveness.test.js new file mode 100644 index 0000000..9a6bd39 --- /dev/null +++ b/server/test/pairing-code-liveness.test.js @@ -0,0 +1,119 @@ +'use strict'; + +// A pairing code must be claimable for as long as the screen is DISPLAYING it. +// +// Expiry was measured from `devices.created_at`, but the row is created once, on the +// device's first registration, and is never recreated: a player persists its device_id and +// its code and re-registers with them forever. So 15 minutes after first boot the row was +// permanently unclaimable, while the screen kept showing a code and the device kept +// heartbeating. The on-screen instruction ("restart the display to get a new code") could +// not help, because restarting reuses the stored identity and produces the same code. +// +// Observed in production: an unclaimed web player, still heartbeating, whose row was +// created 4 days 20 hours earlier and had been unpairable for all but the first 15 minutes +// of that. +// +// The fix keys expiry on LIVENESS instead: a device that has checked in recently is +// pairable; one that has been gone longer than the TTL is not. That matches what an +// operator sees — if the code is on the screen, typing it works — while still killing the +// code for a device that has actually gone away. + +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 Database = require('better-sqlite3'); + +const { freePort } = require('./helpers/free-port'); +let PORT, BASE, proc, db; +const DATA_DIR = path.join(os.tmpdir(), 'st-pairlive-' + crypto.randomBytes(4).toString('hex')); +const LOG = path.join(os.tmpdir(), 'st-pairlive-' + crypto.randomBytes(4).toString('hex') + '.log'); +const PW = 'Passw0rd123'; +const S = {}; +const TTL_SEC = require('../lib/pair-lockout').PAIRING_TTL_SEC; + +const jfetch = async (p, opts = {}) => { + const res = await fetch(BASE + p, opts); + let body = null; try { body = await res.json(); } catch { /* */ } + return { status: res.status, body }; +}; +const post = (tok, obj) => ({ + method: 'POST', + headers: { ...(tok ? { Authorization: 'Bearer ' + tok } : {}), 'Content-Type': 'application/json' }, + body: JSON.stringify(obj), +}); + +// Seed an unclaimed device exactly as a registered-but-unpaired player leaves it. +// createdMinAgo models how long ago the row first appeared; seenMinAgo models the last +// heartbeat, i.e. whether the screen is still alive and showing the code. +function seedDevice({ createdMinAgo, seenMinAgo }) { + const id = crypto.randomUUID(); + const code = String(100000 + Math.floor(Math.random() * 900000)); + const now = Math.floor(Date.now() / 1000); + db.prepare(`INSERT INTO devices (id, pairing_code, status, app_version, created_at, last_heartbeat) + VALUES (?, ?, 'provisioning', '1.1.0-web', ?, ?)`) + .run(id, code, now - createdMinAgo * 60, seenMinAgo === null ? null : now - seenMinAgo * 60); + return { id, code }; +} +// NOTE: /api/provision is rate-limited to 5 requests/minute per IP (server.js), so this +// suite deliberately spends at most five pair attempts. +const pair = (code) => jfetch('/api/provision/pair', post(S.token, { pairing_code: code, name: 'Test Screen' })); + +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' }, + 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 new Promise(r => setTimeout(r, 250)); + } + if (!up) throw new Error('server did not boot:\n' + fs.readFileSync(LOG, 'utf8').slice(-2000)); + db = new Database(path.join(DATA_DIR, 'db', 'remote_display.db')); + + const email = 'u' + crypto.randomBytes(5).toString('hex') + '@x.local'; + const reg = await jfetch('/api/auth/register', post(null, { email, password: PW })); + S.token = reg.body.token; + assert.ok(S.token, 'registered an operator to pair as'); +}); +after(() => { try { db && db.close(); } catch { /* */ } try { proc.kill('SIGKILL'); } catch { /* */ } }); + +test('a freshly-registered screen pairs', async () => { + const d = seedDevice({ createdMinAgo: 1, seenMinAgo: 0 }); + const r = await pair(d.code); + assert.equal(r.status, 200, `a new screen must pair (got ${r.status} ${JSON.stringify(r.body)})`); +}); + +test('THE BUG: a screen that is still alive and showing its code pairs, however old the row is', async () => { + // The exact production shape: row created days ago, device heartbeating right now. + const d = seedDevice({ createdMinAgo: 60 * 24 * 5, seenMinAgo: 0 }); + const r = await pair(d.code); + assert.equal(r.status, 200, + `a live screen displaying its code must be pairable regardless of row age (got ${r.status} ${JSON.stringify(r.body)})`); +}); + +test('a screen that has been gone longer than the TTL is refused', async () => { + // The property the expiry exists for: an abandoned code must not stay claimable. + const d = seedDevice({ createdMinAgo: 60, seenMinAgo: Math.ceil(TTL_SEC / 60) + 5 }); + const r = await pair(d.code); + assert.equal(r.status, 410, 'an abandoned code must expire'); + assert.match(r.body.error, /expired/i); +}); + +test('a device that never checked in falls back to its creation time', async () => { + const stale = seedDevice({ createdMinAgo: Math.ceil(TTL_SEC / 60) + 5, seenMinAgo: null }); + assert.equal((await pair(stale.code)).status, 410, 'never-seen and old -> expired'); +}); + +test('an unknown code is still a 404, not an expiry', async () => { + const r = await pair('000001'); + assert.equal(r.status, 404); +});