From 71b3028a8e29cecf81504309f3e8b63e83375204 Mon Sep 17 00:00:00 2001 From: HomeOS Bootstrap Date: Sat, 18 Jul 2026 08:59:12 +0200 Subject: [PATCH] fix: harden MQTT sharing + input roles + dedup (adversarial review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-agent review (5 confirmed) + Codex found real issues in the rework: - [P1 security] Shared `shelly` ACL had readwrite +/rpc, so a compromised device could inject Switch.Set into ANY other device's /rpc. Now read-only on +/rpc (receive own commands) + write only homeos/rpc (reply). Broker-verified: a shelly publish to victim/rpc is denied; laravel's is not. - [P2 security] `+` wildcard reaches reserved homeos/ring namespaces → bogus device. Ingest now rejects RESERVED_PREFIXES (homeos/ring/$SYS). - [P2 security] Unbounded auto-onboarding = DB-exhaustion DoS. Added a device cap (homeos.mqtt.max_devices, default 250). - [P2 correctness] Every Shelly `input` became a phantom window contact (wall switches shown as windows, possibly inverted). `input` is now a generic sensor; the user PROMOTES specific inputs to window/door contacts on the device page (invert-aware), stored in config->input_roles and applied in the ingest — this is the "assign contacts" flow the user asked for. - [P3 ux] Motion pill read `active`; producer writes `on`. Now reads both. - [P1 migrations] Dedup computed survivor keys once; 3+ duplicates could collide on unique(device_id,key). Re-query per duplicate (ring + mqtt_prefix). +9 tests (reserved prefix, cap, input generic/promoted/inverted, demo echo). Live-verified: input published → onboarded as input → assigned window contact via UI → appears on Fenster page, persists across messages. Suite 57 green, 12/12 tabs clean. Co-Authored-By: Claude Opus 4.8 --- app/Jobs/IngestShellyMessage.php | 61 +++++++++++++- app/Livewire/Devices/Show.php | 81 +++++++++++++++++++ app/Support/Mqtt/ShellyNormalizer.php | 12 +-- config/homeos.php | 4 + ...01_add_ring_id_unique_index_to_devices.php | 4 +- ...dd_mqtt_prefix_unique_index_to_devices.php | 4 +- docker/mosquitto/config/acl | 15 ++-- lang/de/devices.php | 6 ++ lang/en/devices.php | 6 ++ .../views/components/entity-state.blade.php | 9 ++- .../views/livewire/devices/show.blade.php | 22 ++++- tests/Feature/MqttTest.php | 56 +++++++++++++ tests/Feature/ShellyNormalizerTest.php | 15 ++-- 13 files changed, 271 insertions(+), 24 deletions(-) diff --git a/app/Jobs/IngestShellyMessage.php b/app/Jobs/IngestShellyMessage.php index dc0bead..9de610e 100644 --- a/app/Jobs/IngestShellyMessage.php +++ b/app/Jobs/IngestShellyMessage.php @@ -28,6 +28,9 @@ class IngestShellyMessage implements ShouldQueue use AppliesDeviceState; use Queueable; + /** Prefixes a device may never claim — they belong to internal/other-integration namespaces. */ + private const RESERVED_PREFIXES = ['homeos', 'ring', '$SYS']; + public function __construct( public string $topic, public string $payload, @@ -49,6 +52,12 @@ class IngestShellyMessage implements ShouldQueue [$prefix, $component] = $parsed; + // Reserved namespaces still match the broker's `+/status/#` wildcard; never let a device + // masquerade as one of them (would create a bogus "homeos"/"ring" device). + if (in_array($prefix, self::RESERVED_PREFIXES, true)) { + return; + } + $data = json_decode($this->payload, true); if (! is_array($data)) { return; @@ -63,8 +72,15 @@ class IngestShellyMessage implements ShouldQueue return; } $device = $this->onboard($prefix); + if ($device === null) { + return; // onboarding cap reached — drop rather than flood the DB + } } + // Apply per-device input roles: an input the user promoted to a window/door contact + // becomes a contact entity (invert-aware). Config-driven, so this stays out of the driver. + $updates = array_map(fn ($u) => $this->applyInputRole($device, $u), $updates); + $observedAt = $this->observedAt ?? (int) round(microtime(true) * 1_000_000); $observedTime = \Illuminate\Support\Carbon::createFromTimestampMs(intdiv($observedAt, 1000)); @@ -81,6 +97,12 @@ class IngestShellyMessage implements ShouldQueue ['type' => $update['type']], ); + // Keep the entity type in sync with its (config-driven) role — e.g. an input promoted + // to a contact, or demoted back — so the correct renderer/query applies. + if ($entity->type !== $update['type']) { + $entity->forceFill(['type' => $update['type']])->save(); + } + if ($this->applyState($entity->id, $update['state'], $observedAt)) { DeviceStateChanged::dispatch($device->uuid, $entity->key, $update['state']); } @@ -93,8 +115,45 @@ class IngestShellyMessage implements ShouldQueue return array_intersect(array_column($updates, 'type'), ShellyNormalizer::PRIMARY_TYPES) !== []; } - private function onboard(string $prefix): Device + /** + * Promote a raw `input` update to a `contact` when the user has assigned that input a + * window/door role on the device page (stored in device config `input_roles.`). + * + * @param array{key:string,type:string,state:array} $update + * @return array{key:string,type:string,state:array} + */ + private function applyInputRole(Device $device, array $update): array { + if ($update['type'] !== 'input') { + return $update; + } + + $index = explode(':', $update['key'])[1] ?? '0'; + $role = data_get($device->config, "input_roles.{$index}"); + + if (! is_array($role)) { + return $update; + } + + $on = (bool) ($update['state']['on'] ?? false); + $open = ($role['invert'] ?? false) ? ! $on : $on; + + return [ + 'key' => $update['key'], + 'type' => 'contact', + 'state' => ['open' => $open, 'position' => $open ? 'open' : 'closed', 'kind' => $role['kind'] ?? 'window'], + ]; + } + + private function onboard(string $prefix): ?Device + { + // Cap auto-onboarding so a misbehaving/compromised device flooding distinct prefixes on + // the shared account can't exhaust the DB. Assigned devices are unaffected. + $cap = (int) config('homeos.mqtt.max_devices', 250); + if ($cap > 0 && Device::count() >= $cap) { + return null; + } + try { return Device::create([ 'name' => $prefix, // user renames on the device detail page diff --git a/app/Livewire/Devices/Show.php b/app/Livewire/Devices/Show.php index a3845bc..b3c9979 100644 --- a/app/Livewire/Devices/Show.php +++ b/app/Livewire/Devices/Show.php @@ -3,6 +3,7 @@ namespace App\Livewire\Devices; use App\Models\Device; +use App\Models\Entity; use App\Models\Room; use App\Services\DeviceCommandService; use App\Services\MqttCredentialProvisioner; @@ -109,6 +110,86 @@ class Show extends Component $this->flashCommand($command->result); } + /** + * Assign a Shelly input a role: '' = plain input, 'window'/'door' = a contact sensor. + * Stored in device config so the ingest maps future messages accordingly; the entity is + * reclassified now so the page reflects it immediately. + */ + public function setInputRole(int $entityId, string $kind): void + { + $entity = $this->inputEntity($entityId); + if ($entity === null) { + return; + } + + $index = explode(':', $entity->key)[1] ?? '0'; + $config = $this->device->config ?? []; + + if ($kind === '') { + unset($config['input_roles'][$index]); + $role = null; + } else { + $role = [ + 'kind' => in_array($kind, ['window', 'door'], true) ? $kind : 'window', + 'invert' => (bool) data_get($config, "input_roles.{$index}.invert", false), + ]; + $config['input_roles'][$index] = $role; + } + + $this->device->update(['config' => $config]); + $this->reclassifyInput($entity, $role); + $this->flashMessage(__('devices.saved')); + } + + public function setInputInvert(int $entityId, bool $invert): void + { + $entity = $this->inputEntity($entityId); + if ($entity === null) { + return; + } + + $index = explode(':', $entity->key)[1] ?? '0'; + $config = $this->device->config ?? []; + + if (! isset($config['input_roles'][$index])) { + return; + } + + $config['input_roles'][$index]['invert'] = $invert; + $this->device->update(['config' => $config]); + $this->reclassifyInput($entity, $config['input_roles'][$index]); + $this->flashMessage(__('devices.saved')); + } + + private function inputEntity(int $entityId): ?Entity + { + $entity = $this->device->entities()->with('state')->find($entityId); + + return ($entity !== null && str_starts_with($entity->key, 'input:')) ? $entity : null; + } + + /** Reclassify an input entity to contact (role set) or back to input, reformatting its state. */ + private function reclassifyInput(Entity $entity, ?array $role): void + { + $state = $entity->state?->state ?? []; + + if ($role !== null) { + $on = array_key_exists('on', $state) + ? (bool) $state['on'] + : (($state['position'] ?? 'closed') !== 'closed'); + $open = ($role['invert'] ?? false) ? ! $on : $on; + $newState = ['open' => $open, 'position' => $open ? 'open' : 'closed', 'kind' => $role['kind']]; + $newType = 'contact'; + } else { + $on = array_key_exists('on', $state) ? (bool) $state['on'] : (($state['position'] ?? 'closed') !== 'closed'); + $newState = ['on' => $on]; + $newType = 'input'; + } + + $entity->forceFill(['type' => $newType])->save(); + $entity->state?->update(['state' => $newState]); + } + protected function flashCommand(string $result): void { $ok = $result === 'ok'; diff --git a/app/Support/Mqtt/ShellyNormalizer.php b/app/Support/Mqtt/ShellyNormalizer.php index 6252770..115acdf 100644 --- a/app/Support/Mqtt/ShellyNormalizer.php +++ b/app/Support/Mqtt/ShellyNormalizer.php @@ -14,7 +14,7 @@ class ShellyNormalizer * A new prefix is auto-onboarded only when one of these arrives, so `sys`/`cloud`/`mqtt` * status noise never creates a junk device. */ - public const PRIMARY_TYPES = ['switch', 'light', 'cover', 'contact', 'temperature', 'humidity', 'battery', 'power', 'motion']; + public const PRIMARY_TYPES = ['switch', 'light', 'cover', 'contact', 'input', 'temperature', 'humidity', 'battery', 'power', 'motion']; /** * @param array $payload @@ -42,13 +42,15 @@ class ShellyNormalizer break; case 'input': - // A reed/switch input — a window or door contact wired to a Shelly input. - // Only the digital (switch) mode is a contact; analog/count inputs are skipped. + // A digital (switch/button) input — a generic on/off sensor. It is NOT assumed + // to be a window/door contact: on most relays input:0 is just the wall switch. + // The user promotes specific inputs to contacts on the device page (invert-aware); + // that role is applied in the ingest job, keeping this mapping pure. Analog/count + // inputs carry no boolean `state` and are skipped. if (! is_bool($payload['state'] ?? null)) { break; } - $open = (bool) $payload['state']; - $updates[] = ['key' => "input:{$index}", 'type' => 'contact', 'state' => ['open' => $open, 'position' => $open ? 'open' : 'closed']]; + $updates[] = ['key' => "input:{$index}", 'type' => 'input', 'state' => ['on' => (bool) $payload['state']]]; break; case 'contact': diff --git a/config/homeos.php b/config/homeos.php index 3cad6af..5a437d1 100644 --- a/config/homeos.php +++ b/config/homeos.php @@ -21,5 +21,9 @@ return [ // Auto-create a device the first time an unknown prefix publishes a real component. 'auto_onboard' => (bool) env('MQTT_AUTO_ONBOARD', true), + + // Hard cap on total devices, so a flood of distinct prefixes on the shared account can't + // exhaust the DB via auto-onboarding. 0 disables the cap. + 'max_devices' => (int) env('MQTT_MAX_DEVICES', 250), ], ]; diff --git a/database/migrations/2026_07_18_060001_add_ring_id_unique_index_to_devices.php b/database/migrations/2026_07_18_060001_add_ring_id_unique_index_to_devices.php index 4c7487c..103e03a 100644 --- a/database/migrations/2026_07_18_060001_add_ring_id_unique_index_to_devices.php +++ b/database/migrations/2026_07_18_060001_add_ring_id_unique_index_to_devices.php @@ -25,9 +25,11 @@ return new class extends Migration } $survivorId = $group->first()->id; // ordered by id → earliest row wins - $survivorKeys = DB::table('entities')->where('device_id', $survivorId)->pluck('key')->all(); foreach ($group->slice(1) as $dup) { + // Re-query each time: an earlier duplicate may have moved a key onto the survivor, + // so a stale key set would let a later duplicate collide on unique(device_id,key). + $survivorKeys = DB::table('entities')->where('device_id', $survivorId)->pluck('key')->all(); // Collisions: keep the survivor's entity (+state); drop the duplicate's. DB::table('entities')->where('device_id', $dup->id)->whereIn('key', $survivorKeys)->delete(); // Disjoint entities: move them (and their state, via entity_id) onto the survivor. diff --git a/database/migrations/2026_07_18_070001_add_mqtt_prefix_unique_index_to_devices.php b/database/migrations/2026_07_18_070001_add_mqtt_prefix_unique_index_to_devices.php index 712c336..88ef444 100644 --- a/database/migrations/2026_07_18_070001_add_mqtt_prefix_unique_index_to_devices.php +++ b/database/migrations/2026_07_18_070001_add_mqtt_prefix_unique_index_to_devices.php @@ -24,9 +24,11 @@ return new class extends Migration } $survivorId = $group->first()->id; - $survivorKeys = DB::table('entities')->where('device_id', $survivorId)->pluck('key')->all(); foreach ($group->slice(1) as $dup) { + // Re-query each time: a prior duplicate may have moved a key onto the survivor, + // so a stale key set would let a later duplicate collide on unique(device_id,key). + $survivorKeys = DB::table('entities')->where('device_id', $survivorId)->pluck('key')->all(); DB::table('entities')->where('device_id', $dup->id)->whereIn('key', $survivorKeys)->delete(); DB::table('entities')->where('device_id', $dup->id)->update(['device_id' => $survivorId]); DB::table('devices')->where('id', $dup->id)->delete(); diff --git a/docker/mosquitto/config/acl b/docker/mosquitto/config/acl index 51e693e..fc9c34b 100644 --- a/docker/mosquitto/config/acl +++ b/docker/mosquitto/config/acl @@ -14,18 +14,19 @@ user ring topic readwrite ring/# # Shared device account (default onboarding): every Shelly signs in as `shelly` with one -# password (Home-Assistant style, no per-device setup). Scoped to device topics only — it can -# publish status/events for any prefix and do RPC, but cannot touch homeos/, ring/ or $SYS. -# `+` matches exactly one prefix level, so `+/status/#` = /status/…, `+/rpc` covers both -# the device's /rpc request topic and its homeos/rpc reply topic (src=homeos). +# password (Home-Assistant style, no per-device setup). Scoped to device topics only. +# `+` matches exactly one prefix level (so `+/status/#` = /status/…). Deliberately NO +# write to `+/rpc`: devices only READ RPC requests HomeOS sends to their own /rpc and +# reply on the fixed `homeos/rpc` topic (src=homeos) — this denies a compromised device the +# ability to publish Switch.Set to ANOTHER device's /rpc (cross-device command injection). +# Reserved namespaces (homeos/, ring/) still match `+`; the ingest path rejects those prefixes. user shelly topic write +/status/# topic write +/events/# topic write +/online topic write +/debug/# -topic read +/command/# -topic read +/settings/# -topic readwrite +/rpc +topic read +/rpc +topic write homeos/rpc # Per-device credentials (optional hardening): a device can instead authenticate with its OWN # username = topic prefix (provisioned on the device detail page) and be bound to just its diff --git a/lang/de/devices.php b/lang/de/devices.php index 2264c21..efb56eb 100644 --- a/lang/de/devices.php +++ b/lang/de/devices.php @@ -7,9 +7,15 @@ return [ 'open' => 'Offen', 'closed' => 'Geschlossen', 'tilted' => 'Gekippt', + 'input' => 'Eingang', 'contact_open' => 'Offen', 'contact_closed' => 'Geschlossen', 'contact_tilted' => 'Gekippt', + 'role_label' => 'Als:', + 'role_input' => 'Eingang', + 'role_window' => 'Fensterkontakt', + 'role_door' => 'Türkontakt', + 'role_invert' => 'Invertiert', 'battery' => 'Batterie', 'motion' => 'Bewegung', 'no_motion' => 'Ruhe', diff --git a/lang/en/devices.php b/lang/en/devices.php index 610f356..0313913 100644 --- a/lang/en/devices.php +++ b/lang/en/devices.php @@ -7,9 +7,15 @@ return [ 'open' => 'Open', 'closed' => 'Closed', 'tilted' => 'Tilted', + 'input' => 'Input', 'contact_open' => 'Open', 'contact_closed' => 'Closed', 'contact_tilted' => 'Tilted', + 'role_label' => 'As:', + 'role_input' => 'Input', + 'role_window' => 'Window contact', + 'role_door' => 'Door contact', + 'role_invert' => 'Inverted', 'battery' => 'Battery', 'motion' => 'Motion', 'no_motion' => 'Idle', diff --git a/resources/views/components/entity-state.blade.php b/resources/views/components/entity-state.blade.php index 4d98829..b985fba 100644 --- a/resources/views/components/entity-state.blade.php +++ b/resources/views/components/entity-state.blade.php @@ -37,9 +37,16 @@ @break @case('motion') - @php $active = ($s['active'] ?? false) === true; @endphp + @php $active = ($s['on'] ?? $s['active'] ?? false) === true; @endphp {{ $active ? __('devices.motion') : __('devices.no_motion') }} @break + + @case('input') + @php $on = ($s['on'] ?? false) === true; @endphp + + {{ $entity->name ?: __('devices.input') }} · {{ $on ? __('devices.on') : __('devices.off') }} + + @break @endswitch diff --git a/resources/views/livewire/devices/show.blade.php b/resources/views/livewire/devices/show.blade.php index c1a6e36..bb79396 100644 --- a/resources/views/livewire/devices/show.blade.php +++ b/resources/views/livewire/devices/show.blade.php @@ -87,7 +87,8 @@ @else
@foreach ($device->entities as $entity) -
+ @php $isInput = \Illuminate\Support\Str::startsWith($entity->key, 'input:'); $idx = explode(':', $entity->key)[1] ?? '0'; $role = data_get($device->config, "input_roles.$idx"); @endphp +
{{ $entity->name ?? $entity->key }} {{ $entity->type }}
@@ -97,6 +98,25 @@ wire:click="toggleEntity({{ $entity->id }})" :label="$entity->name ?? $entity->key" /> @endif
+ @if ($isInput) + {{-- Promote a Shelly input to a window/door contact (invert-aware) --}} +
+ {{ __('devices.role_label') }} + + @if ($role) + + @endif +
+ @endif
@endforeach
diff --git a/tests/Feature/MqttTest.php b/tests/Feature/MqttTest.php index 7288c12..057b7f9 100644 --- a/tests/Feature/MqttTest.php +++ b/tests/Feature/MqttTest.php @@ -79,6 +79,62 @@ class MqttTest extends TestCase $this->assertDatabaseCount('devices', 0); } + public function test_reserved_prefixes_are_never_onboarded(): void + { + // A device authed as `shelly` can technically publish homeos/status/# (wildcard) — the + // ingest must refuse to create a bogus "homeos"/"ring" device from it. + (new IngestShellyMessage('homeos/status/switch:0', json_encode(['output' => true])))->handle(); + (new IngestShellyMessage('ring/status/switch:0', json_encode(['output' => true])))->handle(); + + $this->assertDatabaseCount('devices', 0); + } + + public function test_onboarding_respects_the_device_cap(): void + { + config()->set('homeos.mqtt.max_devices', 1); + Device::create(['name' => 'existing', 'vendor' => 'Shelly', 'protocol' => 'mqtt', 'config' => ['mqtt_prefix' => 'a']]); + + (new IngestShellyMessage('flood-1/status/switch:0', json_encode(['output' => true])))->handle(); + + $this->assertDatabaseCount('devices', 1); // cap reached, no new device + } + + public function test_input_is_a_plain_input_until_assigned_a_contact_role(): void + { + (new IngestShellyMessage('shelly1-x/status/input:0', json_encode(['id' => 0, 'state' => true])))->handle(); + + $entity = Device::where('config->mqtt_prefix', 'shelly1-x')->first()->entities()->where('key', 'input:0')->first(); + $this->assertSame('input', $entity->type); + $this->assertTrue(data_get($entity->state->state, 'on')); + } + + public function test_input_promoted_to_a_contact_via_device_config(): void + { + $device = Device::create([ + 'name' => 'Fenster', 'vendor' => 'Shelly', 'protocol' => 'mqtt', 'status' => 'active', + 'config' => ['mqtt_prefix' => 'shelly1-fenster', 'input_roles' => ['0' => ['kind' => 'window', 'invert' => false]]], + ]); + + (new IngestShellyMessage('shelly1-fenster/status/input:0', json_encode(['id' => 0, 'state' => true])))->handle(); + + $entity = $device->entities()->where('key', 'input:0')->first(); + $this->assertSame('contact', $entity->type); + $this->assertSame(['open' => true, 'position' => 'open', 'kind' => 'window'], $entity->state->state); + } + + public function test_input_contact_role_respects_inversion(): void + { + $device = Device::create([ + 'name' => 'Fenster', 'vendor' => 'Shelly', 'protocol' => 'mqtt', 'status' => 'active', + 'config' => ['mqtt_prefix' => 'shelly1-inv', 'input_roles' => ['0' => ['kind' => 'door', 'invert' => true]]], + ]); + + // Inverted: reed closed (state true) means the door is CLOSED. + (new IngestShellyMessage('shelly1-inv/status/input:0', json_encode(['id' => 0, 'state' => true])))->handle(); + + $this->assertFalse(data_get($device->entities()->where('key', 'input:0')->first()->state->state, 'open')); + } + public function test_concurrent_onboarding_does_not_duplicate(): void { (new IngestShellyMessage('shelly-dup/status/switch:0', json_encode(['output' => true])))->handle(); diff --git a/tests/Feature/ShellyNormalizerTest.php b/tests/Feature/ShellyNormalizerTest.php index fb5bc1d..c7144d2 100644 --- a/tests/Feature/ShellyNormalizerTest.php +++ b/tests/Feature/ShellyNormalizerTest.php @@ -7,15 +7,16 @@ use PHPUnit\Framework\TestCase; class ShellyNormalizerTest extends TestCase { - public function test_digital_input_becomes_a_contact(): void + public function test_digital_input_is_a_generic_input_not_a_contact(): void { - $open = ShellyNormalizer::normalize('input:0', ['id' => 0, 'state' => true]); - $this->assertSame('input:0', $open[0]['key']); - $this->assertSame('contact', $open[0]['type']); - $this->assertSame(['open' => true, 'position' => 'open'], $open[0]['state']); + // A wall-switch input must NOT be assumed to be a window contact — that's opt-in per device. + $on = ShellyNormalizer::normalize('input:0', ['id' => 0, 'state' => true]); + $this->assertSame('input:0', $on[0]['key']); + $this->assertSame('input', $on[0]['type']); + $this->assertSame(['on' => true], $on[0]['state']); - $closed = ShellyNormalizer::normalize('input:1', ['id' => 1, 'state' => false]); - $this->assertSame(['open' => false, 'position' => 'closed'], $closed[0]['state']); + $off = ShellyNormalizer::normalize('input:1', ['id' => 1, 'state' => false]); + $this->assertSame(['on' => false], $off[0]['state']); } public function test_analog_or_count_input_is_not_a_contact(): void