mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 22:03:13 -06:00
fix(pairing): expire a pairing code on device liveness, not row age
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) <noreply@anthropic.com>
This commit is contained in:
parent
1036333982
commit
090b6c12cb
|
|
@ -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 };
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
119
server/test/pairing-code-liveness.test.js
Normal file
119
server/test/pairing-code-liveness.test.js
Normal file
|
|
@ -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);
|
||||
});
|
||||
Loading…
Reference in a new issue