mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
Only store a device fingerprint against a device that still exists
A player that reconnects after its row was deleted sends the id it still has cached. device_fingerprints.device_id has a foreign key to devices(id), so writing that id back fails the constraint. The throw was caught, which is why this looked harmless, but the catch abandons the whole fingerprint block: last_seen is not updated, the reinstall link is not made, and the settings restore never runs. That restore exists specifically for the post-delete re-pair, so the failure landed exactly where the feature was meant to help and a re-paired panel came back with its orientation, name and playlist reset. Production shows 37 of these, timestamped identically to the "sending unpaired" log lines — the same event seen from the other side. The incoming id is preferred, then whatever is already stored, and only an id that still resolves is written; otherwise NULL, which the column allows and which ON DELETE SET NULL already leaves behind. The INSERT path a few lines below had this guard; the UPDATE was missed, and it is the one that fires. Tests cover the deleted-id reconnect, that last_seen still advances, and that live ids are unaffected. One asserts the raw unguarded statement really does raise FOREIGN KEY constraint failed, and another asserts the guard is present in the handler itself, since the others exercise a mirror of that statement. Also ignores *.sqlite / *.sqlite3, which the existing *.db rules missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
This commit is contained in:
parent
8caf908d3c
commit
a93f65b20a
4
.gitignore
vendored
4
.gitignore
vendored
|
|
@ -55,3 +55,7 @@ shared/Transitions/demo.html
|
|||
audit/
|
||||
.mcp.json
|
||||
**/.mcp.json
|
||||
|
||||
# Local SQLite artifacts (any extension the tooling might produce)
|
||||
*.sqlite
|
||||
*.sqlite3
|
||||
|
|
|
|||
131
server/test/fingerprint-stale-device-id.test.js
Normal file
131
server/test/fingerprint-stale-device-id.test.js
Normal file
|
|
@ -0,0 +1,131 @@
|
|||
'use strict';
|
||||
|
||||
// A device that reconnects after its row was deleted sends the id it still has cached. That id
|
||||
// is gone, and device_fingerprints.device_id has an FK to devices(id) — so writing it back
|
||||
// throws FOREIGN KEY constraint failed. The throw is caught, which is why nothing looked broken,
|
||||
// but the catch abandons the ENTIRE fingerprint block: last_seen is not touched, the reinstall
|
||||
// link is not made, and the #150 settings restore never runs. That restore exists precisely for
|
||||
// the post-delete re-pair, so the failure lands exactly where the feature was supposed to help —
|
||||
// a re-paired panel comes back with its orientation/name/playlist reset.
|
||||
//
|
||||
// Observed on production: 37 occurrences, timestamped identically to the "sending unpaired" log
|
||||
// lines, which is the same event seen from the other side.
|
||||
//
|
||||
// The rule pinned here: only ever store a device_id that still resolves. Prefer the incoming id,
|
||||
// fall back to what is already stored, else NULL — which the column allows, and which is what
|
||||
// ON DELETE SET NULL already leaves behind.
|
||||
|
||||
const { test, before, after } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
const fs = require('node:fs');
|
||||
const crypto = require('node:crypto');
|
||||
|
||||
const DATA_DIR = path.join(os.tmpdir(), 'st-fp-' + crypto.randomBytes(4).toString('hex'));
|
||||
fs.mkdirSync(path.join(DATA_DIR, 'db'), { recursive: true });
|
||||
process.env.DATA_DIR = DATA_DIR;
|
||||
process.env.SELF_HOSTED = 'true';
|
||||
process.env.NODE_ENV = 'test';
|
||||
|
||||
const { db } = require('../db/database');
|
||||
|
||||
const mkDevice = (name) => {
|
||||
const id = crypto.randomUUID();
|
||||
db.prepare(`INSERT INTO devices (id, name, status, created_at)
|
||||
VALUES (?, ?, 'online', strftime('%s','now'))`).run(id, name);
|
||||
return id;
|
||||
};
|
||||
|
||||
// The exact statement the register handler runs, with the guard applied.
|
||||
function writeFingerprint(fingerprint, incomingDeviceId) {
|
||||
const existing = db.prepare('SELECT * FROM device_fingerprints WHERE fingerprint = ?').get(fingerprint);
|
||||
if (!existing) return null;
|
||||
const known = (id) => !!(id && db.prepare('SELECT 1 FROM devices WHERE id = ?').get(id));
|
||||
const fpDeviceId = known(incomingDeviceId) ? incomingDeviceId
|
||||
: (known(existing.device_id) ? existing.device_id : null);
|
||||
db.prepare("UPDATE device_fingerprints SET last_seen = strftime('%s','now'), device_id = ? WHERE fingerprint = ?")
|
||||
.run(fpDeviceId, fingerprint);
|
||||
return db.prepare('SELECT * FROM device_fingerprints WHERE fingerprint = ?').get(fingerprint);
|
||||
}
|
||||
|
||||
before(() => { db.pragma('foreign_keys = ON'); });
|
||||
after(() => { try { fs.rmSync(DATA_DIR, { recursive: true, force: true }); } catch { /* */ } });
|
||||
|
||||
test('foreign keys are actually enforced, or this whole file proves nothing', () => {
|
||||
assert.equal(db.pragma('foreign_keys', { simple: true }), 1);
|
||||
});
|
||||
|
||||
test('the SHIPPED handler carries the guard, not just this test', () => {
|
||||
// writeFingerprint() above mirrors the handler's statement; without this check the whole
|
||||
// file would still pass against an unfixed deviceSocket.js.
|
||||
const src = fs.readFileSync(path.join(__dirname, '..', 'ws', 'deviceSocket.js'), 'utf8');
|
||||
const i = src.indexOf('UPDATE device_fingerprints SET last_seen');
|
||||
assert.notEqual(i, -1, 'the UPDATE still exists');
|
||||
const window = src.slice(Math.max(0, i - 900), i + 300);
|
||||
assert.match(window, /SELECT 1 FROM devices WHERE id = \?/,
|
||||
'the id is validated against devices before being written');
|
||||
assert.doesNotMatch(window, /\.run\(\s*device_id \|\| existing\.device_id/,
|
||||
'the unguarded write is gone');
|
||||
});
|
||||
|
||||
test('THE BUG: a reconnect carrying a DELETED device id must not throw', () => {
|
||||
const fp = 'fp-' + crypto.randomBytes(4).toString('hex');
|
||||
const dev = mkDevice('Panel');
|
||||
db.prepare('INSERT INTO device_fingerprints (fingerprint, device_id) VALUES (?, ?)').run(fp, dev);
|
||||
|
||||
db.prepare('DELETE FROM devices WHERE id = ?').run(dev); // ON DELETE SET NULL clears the link
|
||||
assert.equal(db.prepare('SELECT device_id FROM device_fingerprints WHERE fingerprint = ?').get(fp).device_id, null);
|
||||
|
||||
// The player still has the old id cached and sends it on reconnect.
|
||||
const row = writeFingerprint(fp, dev);
|
||||
assert.ok(row, 'the write completed instead of throwing');
|
||||
assert.equal(row.device_id, null, 'a vanished id is not written back');
|
||||
});
|
||||
|
||||
test('last_seen is still updated — the block is no longer abandoned', () => {
|
||||
const fp = 'fp-' + crypto.randomBytes(4).toString('hex');
|
||||
const dev = mkDevice('Panel2');
|
||||
db.prepare('INSERT INTO device_fingerprints (fingerprint, device_id, last_seen) VALUES (?, ?, 1)').run(fp, dev);
|
||||
db.prepare('DELETE FROM devices WHERE id = ?').run(dev);
|
||||
|
||||
const row = writeFingerprint(fp, dev);
|
||||
assert.ok(row.last_seen > 1, 'the fingerprint is still seen, which is what drives reinstall tracking');
|
||||
});
|
||||
|
||||
test('a LIVE device id is stored, so normal tracking is unaffected', () => {
|
||||
const fp = 'fp-' + crypto.randomBytes(4).toString('hex');
|
||||
const a = mkDevice('A');
|
||||
db.prepare('INSERT INTO device_fingerprints (fingerprint, device_id) VALUES (?, ?)').run(fp, a);
|
||||
const b = mkDevice('B');
|
||||
assert.equal(writeFingerprint(fp, b).device_id, b, 'the incoming id wins when it resolves');
|
||||
});
|
||||
|
||||
test('an absent incoming id keeps the existing link rather than clearing it', () => {
|
||||
const fp = 'fp-' + crypto.randomBytes(4).toString('hex');
|
||||
const a = mkDevice('Keeper');
|
||||
db.prepare('INSERT INTO device_fingerprints (fingerprint, device_id) VALUES (?, ?)').run(fp, a);
|
||||
assert.equal(writeFingerprint(fp, null).device_id, a, 'still linked — this is what reinstall detection reads');
|
||||
});
|
||||
|
||||
test('both ids vanished -> NULL, not a throw', () => {
|
||||
const fp = 'fp-' + crypto.randomBytes(4).toString('hex');
|
||||
const a = mkDevice('Gone');
|
||||
db.prepare('INSERT INTO device_fingerprints (fingerprint, device_id) VALUES (?, ?)').run(fp, a);
|
||||
db.prepare('DELETE FROM devices WHERE id = ?').run(a);
|
||||
const ghost = crypto.randomUUID();
|
||||
assert.equal(writeFingerprint(fp, ghost).device_id, null);
|
||||
});
|
||||
|
||||
test('the UNGUARDED statement really does throw — the bug is what we think it is', () => {
|
||||
const fp = 'fp-' + crypto.randomBytes(4).toString('hex');
|
||||
const dev = mkDevice('Proof');
|
||||
db.prepare('INSERT INTO device_fingerprints (fingerprint, device_id) VALUES (?, ?)').run(fp, dev);
|
||||
db.prepare('DELETE FROM devices WHERE id = ?').run(dev);
|
||||
assert.throws(
|
||||
() => db.prepare("UPDATE device_fingerprints SET last_seen = strftime('%s','now'), device_id = ? WHERE fingerprint = ?")
|
||||
.run(dev, fp),
|
||||
/FOREIGN KEY constraint failed/,
|
||||
'this is the exact error seen 37 times on prod',
|
||||
);
|
||||
});
|
||||
|
|
@ -462,8 +462,19 @@ module.exports = function setupDeviceSocket(io) {
|
|||
try {
|
||||
const existing = db.prepare('SELECT * FROM device_fingerprints WHERE fingerprint = ?').get(fingerprint);
|
||||
if (existing) {
|
||||
// device_id arrives from the client and can name a row that no longer exists (a
|
||||
// reconnect after the device was deleted — the same case that emits device:unpaired).
|
||||
// device_fingerprints.device_id has an FK to devices(id), so writing a stale id
|
||||
// throws, the catch below swallows it, and the WHOLE fingerprint block is abandoned
|
||||
// — including the #150 settings restore that a post-delete re-pair depends on.
|
||||
// Prefer the incoming id, fall back to what is already stored, and only ever write
|
||||
// an id that still resolves. Same guard as the INSERT path below; this UPDATE was
|
||||
// missed, and it is the one that actually fires (37 FK failures on prod).
|
||||
const known = (id) => !!(id && db.prepare('SELECT 1 FROM devices WHERE id = ?').get(id));
|
||||
const fpDeviceId = known(device_id) ? device_id
|
||||
: (known(existing.device_id) ? existing.device_id : null);
|
||||
db.prepare("UPDATE device_fingerprints SET last_seen = strftime('%s','now'), device_id = ? WHERE fingerprint = ?")
|
||||
.run(device_id || existing.device_id, fingerprint);
|
||||
.run(fpDeviceId, fingerprint);
|
||||
// If this fingerprint was previously registered to a different device, block the new registration
|
||||
if (!device_id && existing.device_id && pairing_code) {
|
||||
// Someone reinstalled - link them back to existing device
|
||||
|
|
|
|||
Loading…
Reference in a new issue