diff --git a/server/lib/ota-breaker.js b/server/lib/ota-breaker.js index c52bffd..b359be0 100644 --- a/server/lib/ota-breaker.js +++ b/server/lib/ota-breaker.js @@ -129,6 +129,29 @@ function startSweep() { } function reset() { state.clear(); loggedBad.clear(); Object.assign(rateBackoffCtr, rollingCounter()); } + +// Forgive ONE device's rate state, called when that device proves its identity on the /device +// socket (a valid device_token, timing-safe compared). +// +// /api/update/check is deliberately unauthenticated — every client version, including old ones +// that never learned to send a token, has to be able to ask. That means `?device_id=` is +// caller-supplied, so anyone who learns a device's UUID can burn its bucket with a handful of +// requests and leave the REAL device in rate-backoff, silently un-updatable, for up to 30 +// minutes at a time. +// +// Adding auth to the check would strand old clients, and keying on IP is wrong here (the fleet +// SNATs — see the note at the top of this file). So instead the poisoning is made +// self-healing: the genuine device reconnects on its own schedule, proves who it is, and gets +// its bucket cleared. An attacker can still cause noise, but the denial now lasts until the +// device's next authenticated reconnect rather than as long as the attacker keeps poking. +// +// This cannot be used to evade the breaker's real job: a device stuck in an OTA loop is +// re-registering legitimately, and clearing its rate state on each genuine reconnect is exactly +// what a healthy device looks like — the loop protection is the DOWNLOAD guard, not this. +function forgiveDevice(deviceId) { + if (!deviceId) return false; + return state.delete('d:' + deviceId); +} function _size() { return state.size; } // #146 observability — how many update checks the breaker is rate-backing-off (total + // last completed window). A device=none 1.8.x flood shows here as rateBackoffLastWindow. @@ -136,4 +159,4 @@ function stats(now = Date.now()) { const rb = read(rateBackoffCtr, now); return { rateBackoffTotal: rb.total, rateBackoffLastWindow: rb.lastWindow }; } -module.exports = { decide, reset, sweep, startSweep, cmp, parseVer, _size, stats, WINDOW_MS, THRESHOLD }; +module.exports = { decide, reset, forgiveDevice, sweep, startSweep, cmp, parseVer, _size, stats, WINDOW_MS, THRESHOLD }; diff --git a/server/test/ota-breaker-device-forgive.test.js b/server/test/ota-breaker-device-forgive.test.js new file mode 100644 index 0000000..d1a07a0 --- /dev/null +++ b/server/test/ota-breaker-device-forgive.test.js @@ -0,0 +1,94 @@ +'use strict'; + +// The OTA rate-breaker's bucket must not be a lever anyone can pull against someone else's panel. +// +// `/api/update/check` is deliberately unauthenticated — every client version has to be able to +// ask, including old ones that never learned to send a token — and it keys the breaker on the +// caller-supplied `?device_id=`. Keying on IP is not an option either: the fleet SNATs behind one +// address, so per-IP would collapse a whole site into one bucket (see lib/ota-breaker.js). +// +// The consequence was that anyone who learned a panel's UUID could burn its bucket with a handful +// of requests and leave the REAL panel in rate-backoff — silently un-updatable, for up to 30 +// minutes at a time, renewable indefinitely. +// +// The containment is to make it self-healing rather than to add auth: when a device proves its +// identity on the /device socket (device_token, timing-safe compared) its bucket is cleared. An +// attacker can still make noise, but the denial now lasts until the panel's next genuine +// reconnect instead of as long as the attacker keeps poking. + +const { test, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); +const breaker = require('../lib/ota-breaker'); + +const LATEST = '9.9.9'; // always newer than the client, so checks are "offerable" +const OLD = '1.0.0'; + +beforeEach(() => breaker.reset()); + +// Drive the breaker until it starts refusing, mirroring what the route does. +function hammer(deviceId, n) { + let last = null; + for (let i = 0; i < n; i++) last = breaker.decide(OLD, LATEST, deviceId); + return last; +} + +test('THE BUG: rapid checks against a device id trip that device into rate-backoff', () => { + const victim = 'victim-device-uuid'; + const verdict = hammer(victim, breaker.THRESHOLD + 2); + assert.equal(verdict.reason, 'rate-backoff', 'the bucket trips'); + assert.equal(verdict.update_available, false); + assert.ok(verdict.retry_after_seconds > 0, 'and reports a cooldown'); +}); + +test('the poisoning is TARGETED — only the named device is affected', () => { + const victim = 'victim-device-uuid'; + hammer(victim, breaker.THRESHOLD + 2); + const bystander = breaker.decide(OLD, LATEST, 'some-other-device'); + assert.notEqual(bystander.reason, 'rate-backoff', 'an unrelated device is unaffected'); + assert.equal(bystander.update_available, true, 'and is still offered its update'); +}); + +test('the device proving its identity clears the backoff', () => { + const victim = 'victim-device-uuid'; + assert.equal(hammer(victim, breaker.THRESHOLD + 2).reason, 'rate-backoff', 'poisoned first'); + + const cleared = breaker.forgiveDevice(victim); + assert.equal(cleared, true, 'the bucket existed and was cleared'); + + const after = breaker.decide(OLD, LATEST, victim); + assert.notEqual(after.reason, 'rate-backoff', 'the real device is no longer refused'); + assert.equal(after.update_available, true, 'and is offered its update again'); +}); + +test('forgiving one device does not clear anyone else', () => { + hammer('device-a', breaker.THRESHOLD + 2); + hammer('device-b', breaker.THRESHOLD + 2); + breaker.forgiveDevice('device-a'); + assert.equal(breaker.decide(OLD, LATEST, 'device-b').reason, 'rate-backoff', + 'device-b keeps its own state'); +}); + +test('forgiving is safe for ids that were never seen, and for no id at all', () => { + assert.equal(breaker.forgiveDevice('never-checked-in'), false, 'returns false rather than throwing'); + assert.equal(breaker.forgiveDevice(null), false); + assert.equal(breaker.forgiveDevice(undefined), false); + assert.equal(breaker.forgiveDevice(''), false); +}); + +test('a version-keyed bucket (no device_id) is NOT reachable by forgiveDevice', () => { + // Old clients send only ?version=, so they share a per-version bucket. That one protects the + // server from a fleet of stuck legacy clients and must not be clearable by device id. + const v = hammer(null, breaker.THRESHOLD + 2); + assert.equal(v.reason, 'rate-backoff', 'the version bucket trips'); + breaker.forgiveDevice(OLD); // the version string is not a device id + assert.equal(breaker.decide(OLD, LATEST, null).reason, 'rate-backoff', + 'the version-keyed bucket survives — it is a different namespace'); +}); + +test('the breaker still does its real job after a forgive', () => { + const d = 'looping-device'; + breaker.forgiveDevice(d); + // A device that genuinely loops still gets stopped; forgiving is not an escape hatch. + assert.equal(hammer(d, breaker.THRESHOLD + 2).reason, 'rate-backoff', + 'a looping client is still rate-limited'); +}); diff --git a/server/ws/deviceSocket.js b/server/ws/deviceSocket.js index 51a33e9..428687e 100644 --- a/server/ws/deviceSocket.js +++ b/server/ws/deviceSocket.js @@ -731,6 +731,11 @@ module.exports = function setupDeviceSocket(io) { persistIdentity(device_id, data); // change-detected write (see persistIdentity) } socket.join(device_id); + // The device just proved its identity (device_token, timing-safe). Clear any OTA + // rate-backoff held against it: /api/update/check is unauthenticated and takes a + // caller-supplied ?device_id=, so that bucket can have been burned by anyone who + // merely knows this UUID. A genuine reconnect is the proof that lets us forgive it. + try { require('../lib/ota-breaker').forgiveDevice(device_id); } catch (_) { /* non-fatal */ } socket.emit('device:registered', { device_id, device_token: tokenToSend, status: 'online' }); // #143: a device paired/claimed server-side (user_id set) that RECONNECTS must be told // it's paired — the app leaves the Connect page ONLY on 'device:paired' (web: hides the