Maniacs_Mansion/NestQuest
11
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reject coerced and out-of-range skip_on_away values

Browse Source 
Review remediation for PR #194:
- Admin create/edit request models use StrictBool so JSON 0/1 and
  "true"/"false" are 422 instead of being coerced to a bool.
- Fresh-schema quest_definitions gains CHECK (skip_on_away IN (0, 1));
  migrated databases (column added without the CHECK) are guarded by a
  DAO validator applied at every write path that takes the flag.
- QuestDefinitionsDao.update() accepts skip_on_away (None leaves it
  unchanged), validated like the other write paths.

Task: ded75ab8-0219-4d52-aefa-82c09e0b5e6b

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
overseer
2026-09-23 20:40:22 -04:00
co-authored by Claude Opus 5.5
parent ab0a320f3f
commit e8ae25e5eb
6 changed files with 191 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/routes_admin.py
+3 -3
View File
@@ -251,7 +251,7 @@ import datetime
from typing import Annotated, Literal, NoReturn
from fastapi import APIRouter, Depends, HTTPException, Request, Response
from pydantic import BaseModel, field_validator, model_validator
from pydantic import BaseModel, StrictBool, field_validator, model_validator
from api import transitions as api_transitions
from api.auth import require_admin
@@ -464,7 +464,7 @@ class AdminQuestDefinitionCreateRequest(BaseModel):
windows: list[str | tuple[str, str | None]]
description: str | None = None
icon: str | None = None
skip_on_away: bool = True
skip_on_away: StrictBool = True
class AdminQuestDefinitionEditRequest(BaseModel):
@@ -488,7 +488,7 @@ class AdminQuestDefinitionEditRequest(BaseModel):
icon: str | None = None
rule: AdminRuleRequest | None = None
windows: list[str | tuple[str, str | None]] | None = None
skip_on_away: bool | None = None
skip_on_away: StrictBool | None = None
class AdminQuestDefinitionActiveRequest(BaseModel):
custom_components/nestquest/core/dao_rules.py
+27 -4
View File
@@ -105,6 +105,19 @@ def _validate_window(window: str) -> None:
)
def _validate_skip_on_away(value: object) -> int:
"""Return ``value`` as 0/1, raising ValueError for anything else.
The fresh-schema CHECK constraint enforces this in SQLite, but a
migrated database gained the column through ``ADD COLUMN`` without
it, so every DAO write path validates here instead. Accepts a bool
or the ints 0/1; floats, strings and other ints are rejected.
"""
if not isinstance(value, int) or value not in (0, 1):
raise ValueError(f"skip_on_away must be 0 or 1, got {value!r}")
return int(value)
@dataclass(frozen=True)
class ScheduleRuleRecord:
"""One row of ``schedule_rules``."""
@@ -647,6 +660,7 @@ class QuestDefinitionsDao:
empty list or None creates an unassigned definition, which
callers may fill via :meth:`add_assignee`.
"""
skip_on_away_value = _validate_skip_on_away(skip_on_away)
async with _connection_lock(self._database):
async with self._database.transaction():
await self._validate_rule_exists(schedule_rule_id)
@@ -665,7 +679,7 @@ class QuestDefinitionsDao:
due_time,
int(is_active),
created_at,
int(skip_on_away),
skip_on_away_value,
),
)
definition_id = result.lastrowid
@@ -719,6 +733,7 @@ class QuestDefinitionsDao:
_validate_date(schedule_rule.start_date, "start_date")
if schedule_rule.end_date is not None:
_validate_date(schedule_rule.end_date, "end_date")
skip_on_away_value = _validate_skip_on_away(skip_on_away)
async with _connection_lock(self._database):
async with self._database.transaction():
for child_id in assignee_child_ids:
@@ -737,7 +752,7 @@ class QuestDefinitionsDao:
None,
1,
created_at,
int(skip_on_away),
skip_on_away_value,
),
)
definition_id = result.lastrowid
@@ -810,6 +825,8 @@ class QuestDefinitionsDao:
does not exist. Edits change future instances only: no
``quest_instances`` or ``completion_events`` row is touched.
"""
if skip_on_away is not _UNSET:
skip_on_away = _validate_skip_on_away(skip_on_away)
async with _connection_lock(self._database):
async with self._database.transaction():
definition = await self.get(definition_id)
@@ -829,7 +846,7 @@ class QuestDefinitionsDao:
parameters.append(value)
if skip_on_away is not _UNSET:
assignments.append("skip_on_away = ?")
parameters.append(int(skip_on_away))
parameters.append(skip_on_away)
if assignments:
parameters.append(definition_id)
await self._database.execute(
@@ -1030,13 +1047,16 @@ class QuestDefinitionsDao:
description: str | None | object = _UNSET,
icon: str | None | object = _UNSET,
due_time: str | None | object = _UNSET,
skip_on_away: bool | None = None,
) -> int:
"""Update the given fields; returns rows updated (0 if absent).
Arguments default to the module sentinel ``_UNSET`` meaning
"leave this column alone"; passing ``None`` explicitly writes
SQL NULL, so callers can remove optional metadata
(description, icon, due_time). Assignment and activation are
(description, icon, due_time). ``skip_on_away`` is NOT NULL,
so its ``None`` default leaves it unchanged; a supplied value
must be 0/1 (a bool). Assignment and activation are
deliberately NOT settable here: they have their own explicit
operations (:meth:`add_assignee`, :meth:`remove_assignee`,
:meth:`set_active`) so they stay separately loggable and
@@ -1053,6 +1073,9 @@ class QuestDefinitionsDao:
if value is not _UNSET:
assignments.append(f"{column} = ?")
parameters.append(value)
if skip_on_away is not None:
assignments.append("skip_on_away = ?")
parameters.append(_validate_skip_on_away(skip_on_away))
if not assignments:
return 0
parameters.append(definition_id)
custom_components/nestquest/core/schema.py
+1 -1
View File
@@ -258,7 +258,7 @@ QUEST_DEFINITIONS_TABLE_SQL = """(
due_time TEXT,
is_active INTEGER NOT NULL DEFAULT 1 CHECK (typeof(is_active) = 'integer' AND is_active IN (0, 1)),
created_at TEXT NOT NULL,
skip_on_away INTEGER NOT NULL DEFAULT 1
skip_on_away INTEGER NOT NULL DEFAULT 1 CHECK (skip_on_away IN (0, 1))
)"""
SCHEMA_V1_QUEST_DEFINITIONS_DDL: list[str] = [
tests/test_api_admin_questdef.py
+69
View File
@@ -523,6 +523,75 @@ async def test_edit_rejections_are_422_and_change_nothing(
}
@pytest.mark.parametrize("coercible", [0, 1, "true", "false"])
async def test_create_rejects_coercible_skip_on_away_and_persists_nothing(
admin_questdefs: SimpleNamespace, coercible: object
) -> None:
"""A numeric or string skip_on_away is 422, not coerced to a bool."""
client = admin_questdefs.client
dao = _dao_rules.QuestDefinitionsDao(admin_questdefs.database)
ada = (await _create_children(client, ("Ada",)))[0]
existing = await client.post(
"/api/v1/admin/quest-definitions",
json={
"title": "Feed the cat",
"rule": _weekly_rule_body(),
"assignee_child_ids": [ada],
"windows": ["morning"],
"skip_on_away": False,
},
headers=_admin_headers(),
)
assert existing.status_code == 201
existing_id = existing.json()["id"]
response = await client.post(
"/api/v1/admin/quest-definitions",
json={
"title": "Tidy the den",
"rule": _weekly_rule_body(),
"assignee_child_ids": [ada],
"windows": ["morning"],
"skip_on_away": coercible,
},
headers=_admin_headers(),
)
assert response.status_code == 422
assert set(await _definitions_by_id(admin_questdefs.database)) == {
existing_id
}
assert (await dao.get(existing_id)).skip_on_away is False
@pytest.mark.parametrize("coercible", [0, 1, "true", "false"])
async def test_edit_rejects_coercible_skip_on_away_and_changes_nothing(
admin_questdefs: SimpleNamespace, coercible: object
) -> None:
"""A numeric or string skip_on_away edit is 422; the flag is kept."""
client = admin_questdefs.client
dao = _dao_rules.QuestDefinitionsDao(admin_questdefs.database)
ada = (await _create_children(client, ("Ada",)))[0]
definition_id = (await _create_definition(client, [ada])).json()["id"]
# Start from each stored value so a coerced write would be visible.
for stored in (True, False):
set_flag = await client.patch(
f"/api/v1/admin/quest-definitions/{definition_id}",
json={"skip_on_away": stored},
headers=_admin_headers(),
)
assert set_flag.status_code == 200
response = await client.patch(
f"/api/v1/admin/quest-definitions/{definition_id}",
json={"skip_on_away": coercible, "title": "Renamed"},
headers=_admin_headers(),
)
assert response.status_code == 422
stored_definition = await dao.get(definition_id)
assert stored_definition.skip_on_away is stored
assert stored_definition.title == "Tidy the den"
# --- set_active -------------------------------------------------------------
tests/test_dao_rules.py
+67
View File
@@ -847,6 +847,7 @@ def test_definition_update_cannot_change_assignee_or_active(
"description",
"icon",
"due_time",
"skip_on_away",
}
assert await definitions.update(
definition.id, title="A2"
@@ -862,6 +863,72 @@ def test_definition_update_cannot_change_assignee_or_active(
_with_db(tmp_path, "definition-update-shape.db")(_body)
def test_definition_update_toggles_skip_on_away_and_omission_keeps_it(
tmp_path,
) -> None:
async def _body(database, rules, definitions, children, child):
rule = await rules.create("daily", "2026-09-14")
definition = await definitions.create("A", rule.id, NOW)
assert definition.skip_on_away is True
assert await definitions.update(
definition.id, skip_on_away=False
) == 1
assert (await definitions.get(definition.id)).skip_on_away is False
# Omitting the flag (None) leaves the stored False untouched.
assert await definitions.update(definition.id, title="A2") == 1
kept = await definitions.get(definition.id)
assert kept.title == "A2"
assert kept.skip_on_away is False
assert await definitions.update(definition.id) == 0
assert await definitions.update(
definition.id, skip_on_away=True
) == 1
assert (await definitions.get(definition.id)).skip_on_away is True
_with_db(tmp_path, "definition-update-skip-on-away.db")(_body)
@pytest.mark.parametrize("bad", [2, -1, 0.0, 1.5, "1", "true", b"\x01"])
def test_skip_on_away_rejected_unless_zero_or_one_at_every_write(
tmp_path, bad
) -> None:
"""Every DAO entry point taking the flag rejects non-0/1 values.
This is the only guard on migrated databases, whose column was
added without the fresh-schema CHECK constraint.
"""
async def _body(database, rules, definitions, children, child):
rule = await rules.create("daily", "2026-09-14")
definition = await definitions.create("A", rule.id, NOW)
with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"):
await definitions.create("B", rule.id, NOW, skip_on_away=bad)
with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"):
await definitions.create_with_rule_and_windows(
"C",
schedule_rule_to_storage(
ScheduleRule(
rule_type=RuleType.DAILY, start_date="2026-09-14"
)
),
NOW,
[child.id],
[("morning", None)],
skip_on_away=bad,
)
with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"):
await definitions.edit_definition(
definition.id, title="A2", skip_on_away=bad
)
with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"):
await definitions.update(
definition.id, title="A2", skip_on_away=bad
)
stored = await definitions.list_all()
assert [(d.title, d.skip_on_away) for d in stored] == [("A", True)]
_with_db(tmp_path, "definition-skip-on-away-invalid.db")(_body)
def test_definition_update_no_fields_returns_zero(tmp_path) -> None:
async def _body(database, rules, definitions, children, child):
rule = await rules.create("daily", "2026-09-14")
tests/test_schema.py
+24
View File
@@ -634,6 +634,30 @@ def test_schedule_rules_rule_type_check_enforced(tmp_path) -> None:
_run(database.close())
@pytest.mark.parametrize("bad_flag", [2, -1])
def test_quest_definitions_skip_on_away_check_enforced(
tmp_path, bad_flag
) -> None:
database = _open_db(tmp_path / f"skip-on-away-{bad_flag}.db")
try:
_apply(database)
_child(database)
_insert_rule(database)
with pytest.raises(sqlite3.IntegrityError, match="CHECK"):
_insert_definition(database, skip_on_away=bad_flag)
definition_id = _insert_definition(database, skip_on_away=0)
with pytest.raises(sqlite3.IntegrityError, match="CHECK"):
_run(
database.execute(
"UPDATE quest_definitions SET skip_on_away = ? "
"WHERE id = ?",
(bad_flag, definition_id),
)
)
finally:
_run(database.close())
def test_schedule_rules_interval_zero_fails(tmp_path) -> None:
database = _open_db(tmp_path / "interval-zero.db")
try: