diff --git a/custom_components/houseplan/websocket_api.py b/custom_components/houseplan/websocket_api.py index 1361cf12..75eecc60 100755 --- a/custom_components/houseplan/websocket_api.py +++ b/custom_components/houseplan/websocket_api.py @@ -1294,14 +1294,22 @@ async def ws_config_set(hass: HomeAssistant, connection, msg: dict[str, Any]) -> data = await rt.config_store.async_load() or {} current_rev = data.get("rev", 0) if "expected_rev" not in msg and current_rev: - # audit B4: expected_rev stays optional for old cards mid-upgrade, - # but a blind overwrite of a non-empty store is worth a warning — - # it is exactly how a stale client silently discards someone's work. + # #340: a request without the revision it read is indistinguishable + # from a stale writer. Keep the schema field optional only for an + # empty-store bootstrap and so this path can return the same stable + # domain error as an explicit stale revision. Accepting it over a + # saved document would bypass optimistic locking entirely. _LOGGER.warning( "House Plan: config/set without expected_rev over rev %s — " - "the client bypasses conflict detection (outdated card?)", + "write rejected (outdated client?)", current_rev, ) + connection.send_error( + msg["id"], "conflict", + f"Configuration revision is required; reload the configuration " + f"(current rev {current_rev})", + ) + return if "expected_rev" in msg and msg["expected_rev"] != current_rev: connection.send_error( msg["id"], "conflict", diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f0111149..0c5992ae 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -863,7 +863,7 @@ transmit light is the separate `zero_wall_style` policy. | `houseplan/virtual_light/toggle` | `marker_id` | `{marker_id,on,rev}` / err `not_toggleable`; event `houseplan_virtual_light_updated` | | `houseplan/trail/get` | — | `{trails: {marker: {current, previous}}}` — vacuum runs, raw robot coords | | `houseplan/trail/delete` | `marker_id` | `{ok, removed}` — erase current/previous runs after marker deletion | -| `houseplan/config/set` | `config`, `expected_rev?` | `{ok, rev}` / err `conflict`; event `houseplan_config_updated` | +| `houseplan/config/set` | `config`, `expected_rev` | `{ok, rev}` / err `conflict`; event `houseplan_config_updated` | | `houseplan/plan/optimize` | `config`, `layout`, both expected revisions | crash-resumable two-store commit + one-deep backup | | `houseplan/plan/optimize_undo` | both expected revisions | restores backup only before any later edit | | `houseplan/plan/set` | `space_id`, `ext` (svg/png/jpg/webp), `data` (b64, ≤8 MB) | `{ok, url}` — writes `..`, deletes nothing | @@ -878,6 +878,13 @@ transmit light is the separate `zero_wall_style` policy. | `houseplan/import/revalidate` | preview `token`, `duplicate_policy?` | refreshed bounded preview and current expected revisions | | `houseplan/import/apply` | token, both expected revisions, content confirmation | crash-resumable paired config/layout commit; full import gets one-deep undo | +`config/set.expected_rev` is semantically mandatory once a document exists. +The wire schema permits omission only for the first empty-store bootstrap at +revision zero, so the endpoint can return the stable `conflict` domain error +instead of a generic format error. A revision-less write over `rev > 0` is +rejected under the same `write_lock` before validation, no-op detection, +backup cleanup, file collection or update events (#340). + The normal frontend reaches `houseplan/plan/optimize` only after the exact preview candidate passes `src/plan-geometry-preflight.ts`. That pure barrier uses the same room/open-span/ordinary+hosted-opening projection, diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4021a036..4dc15f1c 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +- An outdated tab or third-party client can no longer silently replace a newer + saved plan by omitting its configuration revision. House Plan rejects that + write as a conflict and keeps the server copy intact; revision-less bootstrap + remains available only while the configuration store is empty + ([#340](https://github.com/Matysh/houseplan-card/issues/340)). + - A very acute apex between walls of different thickness (say 15 and 30 cm) on legacy plans renders as an honest tip again instead of the "trident": the inner-face convergence accounts for both thicknesses, not just the diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index 806603a8..92d29282 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -8,6 +8,12 @@ ## Не выпущено +- Устаревшая вкладка или сторонний клиент больше не может молча заменить новый + сохранённый план, не передав ревизию конфигурации. House Plan отклоняет такую + запись как конфликт и сохраняет серверную копию; инициализация без ревизии + разрешена только пока хранилище конфигурации пусто + ([#340](https://github.com/Matysh/houseplan-card/issues/340)). + - Очень острая вершина между стенами разной толщины (например, 15 и 30 см) на старых планах снова рисуется честным остриём, а не «трезубцем»: расчёт смыкания внутренних граней учитывает обе толщины, а не одну наибольшую diff --git a/docs/CONFIG-COMPATIBILITY.md b/docs/CONFIG-COMPATIBILITY.md index d1fc5e83..865c7727 100644 --- a/docs/CONFIG-COMPATIBILITY.md +++ b/docs/CONFIG-COMPATIBILITY.md @@ -48,6 +48,21 @@ Unknown future fields remain outside this report and continue to follow the backend's forward-compatibility policy. Absence from the report is therefore not permission to delete a field. +## Revision-less config writers (#340) + +The current frontend has sent `expected_rev` with every `config/set` since +v1.4.4. A client without that field may initialise an empty configuration store +at revision zero, where no newer work exists to overwrite. Once a document has +been saved, omission is a `conflict` and leaves the config, revision, backup and +events unchanged — including when the submitted document would otherwise be a +semantic no-op. + +There is no version-based compatibility window for writes over a non-zero +revision. Without a server-issued token, an old client and a stale concurrent +writer produce the same request; accepting either would reopen last-writer-wins +data loss. This changes only the WebSocket write contract. Stored config, +model/store versions, exports and read compatibility are unchanged. + ## Stable wall identity — model v8 (#282) Model v8 adds `space.wall_segments[]`, ordered `rooms[].wall_ids[]`, IDs on diff --git a/docs/TESTING.md b/docs/TESTING.md index 710ffb2b..292b2a22 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -1064,8 +1064,12 @@ separately promised workflows: duplicate outline is still NOT containment [auto: smoke_inert_openings] - [ ] Backend hardening (v1.43.1, audit B2-B5): the admin check fails closed when the entry is unavailable; layout/set honours expected_rev; a - config/set without expected_rev over a non-empty store logs a warning; - NaN/Infinity coordinates and oversized collections are rejected [auto: unit: logic.test] + config/set without expected_rev may bootstrap revision zero, but over a + non-empty store returns `conflict` without changing config/rev/backup or + firing an event (including a no-op body); the production frontend writer + inventory requires expected_rev; + NaN/Infinity coordinates and oversized collections are rejected + [auto: tests_backend/test_ha_websocket.py + coordinate-write-barrier-guard.test] - [ ] Save race (v1.43.0, audit L2): make a markup edit, then press Save in any dialog within 500 ms (or let another client save) — the markup edit must survive and reach the server; a failed reload now shows a toast [auto: unit: tests_backend] diff --git a/docs/USER-GUIDE.md b/docs/USER-GUIDE.md index f2d1a1c7..9b70be30 100644 --- a/docs/USER-GUIDE.md +++ b/docs/USER-GUIDE.md @@ -904,6 +904,12 @@ valid choice is made. - avoid editing the same object in two browsers: the second save may need a refresh and manual reapplication. +Every client that saves the shared configuration must return the revision from +`houseplan/config/get`. Omitting it is allowed only while the configuration +store is still empty; afterwards House Plan rejects the save as a conflict +instead of risking another client's work. If an old cached card repeatedly +reports conflicts, update House Plan and refresh the dashboard. + ### Files and quotas Plan files accept SVG/PNG/JPG/WebP up to 8 MB, with a 200-file/256-MB total. diff --git a/docs/USER-GUIDE.ru.md b/docs/USER-GUIDE.ru.md index 8d11de6c..ef3b9b41 100644 --- a/docs/USER-GUIDE.ru.md +++ b/docs/USER-GUIDE.ru.md @@ -1657,6 +1657,12 @@ JSON хранит названия, идентификаторы HA и точн - Масштаб просмотра, последнее открытое пространство и киоск-размеры локальны браузеру. - Если открыт несохранённый диалог и Lovelace быстро пересобирает карточку, текущая версия пытается восстановить черновик в течение короткого окна. +Любой клиент, который сохраняет общую конфигурацию, обязан вернуть ревизию из +`houseplan/config/get`. Не передавать её можно только при первом создании ещё +пустого хранилища; после этого House Plan отклоняет запись как конфликт, а не +рискует чужой работой. Если старая закэшированная карточка постоянно сообщает о +конфликте, обновите House Plan и перезагрузите дашборд. + ### Несколько карточек и стартовые пространства Если разные экземпляры карточки должны всегда оставаться на разных diff --git a/scripts/coordinate-write-barrier-guard.mjs b/scripts/coordinate-write-barrier-guard.mjs index 76142a37..9a6c9ee6 100644 --- a/scripts/coordinate-write-barrier-guard.mjs +++ b/scripts/coordinate-write-barrier-guard.mjs @@ -36,7 +36,7 @@ export function checkCoordinateWriteBarriers(root = defaultRoot) { if (configWrites.length !== 1) errors.push(`frontend config writer inventory: ${configWrites.length}`); for (const index of configWrites) requireWindow( errors, frontend, index, - /const canonicalCandidate = canonicalizeConfigGeometry\(candidate\);[\s\S]*config: canonicalCandidate/, + /const canonicalCandidate = canonicalizeConfigGeometry\(candidate\);[\s\S]*config: canonicalCandidate,[\s\S]*expected_rev: this\._cfgRev/, 'frontend config/set', 600, ); if (occurrences(frontend, '._sendConfigCandidate(candidate)').length !== 2) { diff --git a/test/coordinate-write-barrier-guard.test.mjs b/test/coordinate-write-barrier-guard.test.mjs index c7b1c1c0..15436d06 100644 --- a/test/coordinate-write-barrier-guard.test.mjs +++ b/test/coordinate-write-barrier-guard.test.mjs @@ -1,4 +1,9 @@ import assert from 'node:assert/strict'; +import { + mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; import test from 'node:test'; import { fileURLToPath } from 'node:url'; @@ -9,3 +14,38 @@ const repoRoot = fileURLToPath(new URL('..', import.meta.url)); test('all frontend and backend coordinate writers converge on one boundary (#291)', () => { assert.deepEqual(checkCoordinateWriteBarriers(repoRoot), []); }); + +test('the canonical config writer cannot drop its expected revision (#340)', () => { + const fixtureRoot = mkdtempSync(join(tmpdir(), 'houseplan-config-rev-')); + try { + mkdirSync(resolve(fixtureRoot, 'src'), { recursive: true }); + mkdirSync(resolve(fixtureRoot, 'custom_components/houseplan'), { recursive: true }); + for (const name of ['houseplan-card.ts', 'houseplan-editor-runtime.ts']) { + writeFileSync( + resolve(fixtureRoot, 'src', name), + readFileSync(resolve(repoRoot, 'src', name), 'utf8'), + ); + } + const componentRoot = resolve(repoRoot, 'custom_components/houseplan'); + for (const name of readdirSync(componentRoot).filter((entry) => entry.endsWith('.py'))) { + writeFileSync( + resolve(fixtureRoot, 'custom_components/houseplan', name), + readFileSync(resolve(componentRoot, name), 'utf8'), + ); + } + + const cardPath = resolve(fixtureRoot, 'src/houseplan-card.ts'); + const productionCard = readFileSync(cardPath, 'utf8'); + const withoutRevision = productionCard + .replace(', expected_rev: this._cfgRev', ''); + assert.notEqual(withoutRevision, productionCard, 'fixture must remove the revision field'); + writeFileSync(cardPath, withoutRevision); + + assert.ok( + checkCoordinateWriteBarriers(fixtureRoot) + .includes('frontend config/set bypasses the canonical boundary'), + ); + } finally { + rmSync(fixtureRoot, { recursive: true, force: true }); + } +}); diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index 36fc0c01..73153d04 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -1,6 +1,7 @@ """WebSocket API tests (CI): layout ops, config rev conflict, not_ready gate.""" import copy import json +import logging from pathlib import Path import pytest @@ -454,6 +455,91 @@ async def test_config_rev_conflict(hass: HomeAssistant, hass_ws_client: WebSocke assert resp["result"]["rev"] == 1 +async def test_issue_340_config_set_without_revision_is_bootstrap_only( + hass: HomeAssistant, + hass_ws_client: WebSocketGenerator, + caplog: pytest.LogCaptureFixture, +) -> None: + """A missing revision may initialise an empty store, never replace it.""" + from custom_components.houseplan.store import OPTIMIZE_BACKUP, get_data + + await _setup(hass) + first_client = await hass_ws_client(hass) + stale_client = await hass_ws_client(hass) + first_config = { + "spaces": [], + "markers": [{"id": "first", "binding": "virtual", "name": "First"}], + "settings": {}, + } + stale_config = { + "spaces": [], + "markers": [{"id": "stale-secret", "binding": "virtual", "name": "Stale"}], + "settings": {}, + } + config_events: list[dict] = [] + hass.bus.async_listen( + "houseplan_config_updated", lambda event: config_events.append(event.data) + ) + + # The sole legacy compatibility path: before any document exists there is + # no newer work to overwrite. The write becomes rev 1 under write_lock. + await first_client.send_json_auto_id({ + "type": "houseplan/config/set", "config": copy.deepcopy(first_config), + }) + bootstrap = await first_client.receive_json() + await hass.async_block_till_done() + assert bootstrap["success"] and bootstrap["result"]["rev"] == 1 + assert config_events == [{"rev": 1}] + + runtime = get_data(hass) + assert runtime is not None + stored_before = copy.deepcopy(await runtime.config_store.async_load()) + backup = { + "kind": "optimize", + "after_config_rev": 1, + "after_layout_rev": 7, + "sentinel": "keep", + } + await runtime.store.async_save({ + "layout": {}, "rev": 7, OPTIMIZE_BACKUP: copy.deepcopy(backup), + }) + config_events.clear() + + with caplog.at_level(logging.WARNING, logger="custom_components.houseplan.websocket_api"): + await stale_client.send_json_auto_id({ + "type": "houseplan/config/set", "config": copy.deepcopy(stale_config), + }) + rejected = await stale_client.receive_json() + await hass.async_block_till_done() + assert not rejected["success"] + assert rejected["error"]["code"] == "conflict" + assert "revision is required" in rejected["error"]["message"].lower() + assert await runtime.config_store.async_load() == stored_before + assert (await runtime.store.async_load())[OPTIMIZE_BACKUP] == backup + assert config_events == [] + assert "write rejected" in caplog.text + assert "stale-secret" not in caplog.text + + # Even an exact semantic no-op may not be used to bypass the CAS guard. + await stale_client.send_json_auto_id({ + "type": "houseplan/config/set", "config": copy.deepcopy(first_config), + }) + noop_without_revision = await stale_client.receive_json() + await hass.async_block_till_done() + assert not noop_without_revision["success"] + assert noop_without_revision["error"]["code"] == "conflict" + assert await runtime.config_store.async_load() == stored_before + assert config_events == [] + + # The same client succeeds after reading and returning the current rev. + await stale_client.send_json_auto_id({ + "type": "houseplan/config/set", "config": copy.deepcopy(stale_config), + "expected_rev": 1, + }) + retried = await stale_client.receive_json() + assert retried["success"] and retried["result"]["rev"] == 2 + + async def test_canonical_rewrites_are_noops_without_events_or_undo_loss( hass: HomeAssistant, hass_ws_client: WebSocketGenerator ) -> None: