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.
This commit is contained in:
@@ -73,6 +73,18 @@ def _validate_sort_order(value: object) -> int:
|
|||||||
return value
|
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(
|
async def _warn_on_duplicate_name(
|
||||||
database: NestQuestDatabase,
|
database: NestQuestDatabase,
|
||||||
display_name: str,
|
display_name: str,
|
||||||
@@ -166,6 +178,7 @@ async def edit_child(
|
|||||||
and its readback — the returned record is exactly the one this
|
and its readback — the returned record is exactly the one this
|
||||||
edit produced, not a later writer's.
|
edit produced, not a later writer's.
|
||||||
"""
|
"""
|
||||||
|
_validate_child_id(child_id)
|
||||||
async with _connection_lock(database):
|
async with _connection_lock(database):
|
||||||
dao = ChildrenDao(database)
|
dao = ChildrenDao(database)
|
||||||
existing = await dao.get(child_id)
|
existing = await dao.get(child_id)
|
||||||
@@ -222,6 +235,7 @@ async def set_child_active(
|
|||||||
connection-scoped lock, so a concurrent opposite transition cannot
|
connection-scoped lock, so a concurrent opposite transition cannot
|
||||||
make this call report the other caller's value.
|
make this call report the other caller's value.
|
||||||
"""
|
"""
|
||||||
|
_validate_child_id(child_id)
|
||||||
async with _connection_lock(database):
|
async with _connection_lock(database):
|
||||||
dao = ChildrenDao(database)
|
dao = ChildrenDao(database)
|
||||||
if await dao.get(child_id) is None:
|
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
|
existing child), an unknown id, or a duplicate is rejected with
|
||||||
ValueError before any write, so the current order survives a
|
ValueError before any write, so the current order survives a
|
||||||
rejected call. The DAO enforces this inside its own transaction
|
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
|
wrapper additionally rejects non-integer ids up front, since
|
||||||
``bool`` and floats would silently compare against stored integer
|
``bool`` and floats would silently compare against stored integer
|
||||||
ids at the SQL layer.
|
ids at the SQL layer.
|
||||||
"""
|
"""
|
||||||
for child_id in ordered_ids:
|
for child_id in ordered_ids:
|
||||||
if isinstance(child_id, bool) or not isinstance(child_id, int):
|
_validate_child_id(child_id)
|
||||||
raise ValueError(
|
|
||||||
f"ordered_ids must contain only integer child ids, got "
|
|
||||||
f"{child_id!r}"
|
|
||||||
)
|
|
||||||
await ChildrenDao(database).reorder(ordered_ids)
|
await ChildrenDao(database).reorder(ordered_ids)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -242,6 +242,33 @@ def test_edit_child_missing_raises(tmp_path) -> None:
|
|||||||
_with_db(tmp_path, "edit-missing.db")(_body)
|
_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:
|
def test_edit_child_name_validation(tmp_path) -> None:
|
||||||
async def _body(database):
|
async def _body(database):
|
||||||
child = await create_child(database, "Ada")
|
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)
|
second = await create_child(database, "Bo", sort_order=1)
|
||||||
with pytest.raises(ValueError, match="duplicate child ids"):
|
with pytest.raises(ValueError, match="duplicate child ids"):
|
||||||
await reorder_children(database, [first.id, first.id, second.id])
|
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])
|
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"])
|
await reorder_children(database, [first.id, second.id, "3"])
|
||||||
listed = await list_children(database)
|
listed = await list_children(database)
|
||||||
assert [c.sort_order for c in listed] == [0, 1]
|
assert [c.sort_order for c in listed] == [0, 1]
|
||||||
|
|||||||
Reference in New Issue
Block a user