From 5fe6cb72abab9f29c3db4867667b9c409c20cb65 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:45:36 -0400 Subject: [PATCH] Review fix: strict non-bool int child_id validation on all three calls Review round 1 (Codex gpt-5.6-terra): SQLite binds bools as integers, so edit_child(True, ...) would silently edit child 1 and set_child_active had the same hole (reorder already validated its list). A shared _validate_child_id guard now runs before lookup in edit_child, set_child_active and reorder_children's list scan, with a coercible-id test matrix for both call shapes (True/False/1.0/"1"/ None) asserting the refused call leaves the profile untouched. Suite: 875 passed. --- custom_components/nestquest/children.py | 22 +++++++++++++----- tests/test_children_registry.py | 31 +++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 0633774..f4d8c3e 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -73,6 +73,18 @@ def _validate_sort_order(value: object) -> int: return value +def _validate_child_id(value: object) -> int: + """Reject non-int child ids (bools included) before any lookup. + + SQLite binds Python bools as integers, so ``True`` would silently + address child 1 and a float would round — malformed service input + must never mutate the wrong profile. + """ + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"child_id must be an integer, got {value!r}") + return value + + async def _warn_on_duplicate_name( database: NestQuestDatabase, display_name: str, @@ -166,6 +178,7 @@ async def edit_child( and its readback — the returned record is exactly the one this edit produced, not a later writer's. """ + _validate_child_id(child_id) async with _connection_lock(database): dao = ChildrenDao(database) existing = await dao.get(child_id) @@ -222,6 +235,7 @@ async def set_child_active( connection-scoped lock, so a concurrent opposite transition cannot make this call report the other caller's value. """ + _validate_child_id(child_id) async with _connection_lock(database): dao = ChildrenDao(database) if await dao.get(child_id) is None: @@ -246,17 +260,13 @@ async def reorder_children( existing child), an unknown id, or a duplicate is rejected with ValueError before any write, so the current order survives a rejected call. The DAO enforces this inside its own transaction - (check and write are atomic under the connection lock); this + (check and write are atomic under the connection lock); this wrapper additionally rejects non-integer ids up front, since ``bool`` and floats would silently compare against stored integer ids at the SQL layer. """ for child_id in ordered_ids: - if isinstance(child_id, bool) or not isinstance(child_id, int): - raise ValueError( - f"ordered_ids must contain only integer child ids, got " - f"{child_id!r}" - ) + _validate_child_id(child_id) await ChildrenDao(database).reorder(ordered_ids) diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index cfc0799..6b84a18 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -242,6 +242,33 @@ def test_edit_child_missing_raises(tmp_path) -> None: _with_db(tmp_path, "edit-missing.db")(_body) +@pytest.mark.parametrize("bad", [True, False, 1.0, "1", None]) +def test_edit_child_non_int_id_rejected(tmp_path, bad) -> None: + """SQLite binds bools as integers: True must not address child 1, + and floats must not round onto a real profile.""" + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="child_id must be an integer"): + await edit_child(database, bad, display_name="X") + # The refused edit left the profile untouched. + assert (await list_children(database))[0].display_name == "Ada" + return None + + _with_db(tmp_path, "edit-bad-id.db")(_body) + + +@pytest.mark.parametrize("bad", [True, False, 1.0, "1", None]) +def test_set_child_active_non_int_id_rejected(tmp_path, bad) -> None: + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="child_id must be an integer"): + await set_child_active(database, bad, False) + assert (await list_children(database))[0].is_active is True + return None + + _with_db(tmp_path, "set-active-bad-id.db")(_body) + + def test_edit_child_name_validation(tmp_path) -> None: async def _body(database): child = await create_child(database, "Ada") @@ -518,9 +545,9 @@ def test_reorder_children_duplicate_and_non_int_rejected(tmp_path) -> None: second = await create_child(database, "Bo", sort_order=1) with pytest.raises(ValueError, match="duplicate child ids"): await reorder_children(database, [first.id, first.id, second.id]) - with pytest.raises(ValueError, match="only integer child ids"): + with pytest.raises(ValueError, match="child_id must be an integer"): await reorder_children(database, [first.id, second.id, True]) - with pytest.raises(ValueError, match="only integer child ids"): + with pytest.raises(ValueError, match="child_id must be an integer"): await reorder_children(database, [first.id, second.id, "3"]) listed = await list_children(database) assert [c.sort_order for c in listed] == [0, 1]