knacky/Metamorph
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(m6): post-review pass — cache prefix, snapshot lock, perm-before-parse, LIKE escape

Browse Source 
Addresses spec-reviewer + code-reviewer feedback on the M6 bundle:

Critical:
- frontend/src/lib/missions.ts: add `listPrefix()` so TanStack invalidation
  catches every filtered list variant; the previous `list()` returned
  `['missions','list',{}]` and only matched the exact empty-filter cache,
  leaving filtered tables stale after create/transition/delete.
- backend/app/services/missions.py: acquire the same per-scenario
  `pg_advisory_xact_lock` key used by `set_scenario_tests` before
  snapshotting; without it a concurrent M5 reorder could freeze a torn
  snapshot under READ COMMITTED. Sorted by key to avoid deadlocks with
  another snapshotter.

Important:
- backend/app/api/missions.py: `@require_perm("mission.update",
  "mission.archive")` on the transition endpoint so users without either
  perm get 403 before the body is parsed (no shape leak via 400).
- backend/app/services/missions.py: escape `%` / `_` / `\` in user-typed
  `q` / `client` LIKE search; users can no longer trigger wildcard
  semantics by typing literal `%`. Added `escape='\\'` arg on every .like().
- backend/app/services/missions.py: filter `MissionTest.deleted_at` and
  `MissionScenario.deleted_at` in the list-item and detail counts so M7+
  soft-deletes don't drift the totals silently.

Nits:
- backend/app/api/users.py: order `/users/roster` by email for stable
  rendering + deterministic e2e selectors.
- frontend/src/pages/MissionDetailPage.tsx: distinct accent per
  transition target (cyan/orange/green/teal) matching the status legend.
- e2e/tests/m6-missions.spec.ts: switch fragile `getByRole(name=/In
  Progress/i)` to the stable `mission-transition-in_progress` data-testid.

New tests:
- test_create_mission_rejects_soft_deleted_scenario
- test_transition_perm_gate_runs_before_payload_parse
- test_search_treats_wildcards_as_literals

Suite: 106 pytest passing (was 103), 43 Playwright passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Knacky
2026-05-13 15:14:57 +02:00
parent 00b7557e30
commit e1b51db25f
9 changed files with 149 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
backend/app/api/missions.py
View File

@@ -430,10 +430,14 @@ def set_members(mission_id: str):
@bp.post("/<mission_id>/transition")
@require_auth
@require_perm("mission.update", "mission.archive")
def transition(mission_id: str):
"""Status transition. Gate logic mirrors the perm seed: `mission.archive`
"""Status transition. The outer decorator gates the endpoint on holding
EITHER `mission.update` or `mission.archive` — so a request with neither
perm sees 403 before its body is even parsed (no shape leak via 400).
The inner refinement then enforces the per-target rule: `mission.archive`
is required when the target is `archived`; `mission.update` covers the
other transitions. Admins bypass both checks.
other transitions. Admins bypass via the decorator's `is_admin` check.
"""
mid = _parse_uuid_or_400(mission_id)
if mid is None:

7
backend/app/api/users.py
View File

@@ -69,6 +69,7 @@ def list_roster():
"""
q = request.args.get("q") or None
rows = users_svc.list_users(q=q, is_active=True, limit=200, offset=0)[0]
# Sort by email for predictable rendering and stable e2e selectors.
return jsonify(
{
"items": [
@@ -77,8 +78,10 @@ def list_roster():
"email": u.email,
"display_name": u.display_name,
}
for u in rows
if u.deleted_at is None
for u in sorted(
(u for u in rows if u.deleted_at is None),
key=lambda x: x.email,
)
]
}
)

64
backend/app/services/missions.py
View File

@@ -13,12 +13,13 @@ allowed (perm codes); this service enforces *which mission* is visible.
from __future__ import annotations
import hashlib
import uuid
from dataclasses import dataclass
from datetime import date, datetime, timezone
from typing import Any, Iterable
from sqlalchemy import func, or_, select
from sqlalchemy import func, or_, select, text
from sqlalchemy.orm import Session, selectinload
from app.db.session import session_scope
@@ -216,6 +217,40 @@ def _validate_role_hint(value: str) -> str:
return value
def _escape_like(raw: str) -> str:
"""Escape LIKE wildcards so user-typed `%` / `_` / `\\` stay literal."""
return raw.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
def _lock_scenario_ids_for_snapshot(s: Session, scenario_ids: list[uuid.UUID]) -> None:
"""Acquire a per-scenario `pg_advisory_xact_lock` for every source scenario
we're about to snapshot.
Why: a concurrent admin invoking `set_scenario_tests(scenario_id)` (M5)
deletes-then-reinserts the `scenario_template_tests` join rows mid-transaction.
Under READ COMMITTED, `_snapshot_scenarios` could observe a partial view
(selectinload re-queries) and freeze a torn snapshot. Sharing the same lock
key as `app.services.scenario_templates.set_scenario_tests` makes the
snapshot wait until the reorder commits (and vice versa).
The lock keys are derived deterministically from the scenario UUIDs via
blake2b (cf. lessons: `hash()` is randomised per-worker). We sort the keys
before acquiring to avoid deadlocks with another snapshotter that holds
them in a different order.
"""
if not scenario_ids:
return
keys: list[int] = []
for sid in scenario_ids:
digest = hashlib.blake2b(sid.bytes, digest_size=8).digest()
keys.append(int.from_bytes(digest, "big", signed=True))
for key in sorted(keys):
s.execute(
text("SELECT pg_advisory_xact_lock(CAST(:key AS bigint))"),
{"key": key},
)
def _is_member(s: Session, mission_id: uuid.UUID, viewer_id: uuid.UUID) -> bool:
return (
s.scalar(
@@ -390,6 +425,7 @@ def _snapshot_scenarios(
if not scenario_ids:
return
_lock_scenario_ids_for_snapshot(s, scenario_ids)
sc_by_id = _load_scenario_templates_for_snapshot(s, scenario_ids)
# Collect the underlying test_template ids in stable order.
@@ -495,10 +531,16 @@ def _member_views(s: Session, mission: Mission) -> list[MissionMemberView]:
def _scenario_views(scenarios: list[MissionScenario]) -> list[MissionScenarioView]:
"""Assemble scenario views. `mission_scenarios` and `mission_tests` both
carry `SoftDeleteMixin`; M6 doesn't surface soft-deletion of those rows in
any endpoint, but the filter is applied here so future deletions (M7+)
don't drift the rendered list silently."""
views: list[MissionScenarioView] = []
for sc in sorted(scenarios, key=lambda s_: s_.position):
live = [sc for sc in scenarios if sc.deleted_at is None]
for sc in sorted(live, key=lambda s_: s_.position):
test_views: list[MissionTestView] = []
for t in sorted(sc.tests, key=lambda t_: t_.position):
live_tests = [t for t in sc.tests if t.deleted_at is None]
for t in sorted(live_tests, key=lambda t_: t_.position):
tag_views = [
MissionMitreTagView(
kind=tag.mitre_kind,
@@ -571,8 +613,12 @@ def _to_detail_view(s: Session, m: Mission) -> MissionView:
def _to_list_item(m: Mission) -> MissionListItemView:
# Cheap counts via the loaded relationships (selectinloaded by the caller).
# We filter soft-deleted children consistently with `_scenario_views` so
# the list and the detail page agree.
live_scenarios = [sc for sc in m.scenarios if sc.deleted_at is None]
tests_count = sum(len(sc.tests) for sc in live_scenarios)
tests_count = sum(
len([t for t in sc.tests if t.deleted_at is None]) for sc in live_scenarios
)
return MissionListItemView(
id=m.id,
name=m.name,
@@ -629,15 +675,15 @@ def list_missions(
stmt = stmt.where(Mission.status == status)
count_stmt = count_stmt.where(Mission.status == status)
if client:
like = f"%{client.lower()}%"
cond = func.lower(Mission.client_target).like(like)
like = f"%{_escape_like(client.lower())}%"
cond = func.lower(Mission.client_target).like(like, escape="\\")
stmt = stmt.where(cond)
count_stmt = count_stmt.where(cond)
if q:
like = f"%{q.lower()}%"
like = f"%{_escape_like(q.lower())}%"
cond = or_(
func.lower(Mission.name).like(like),
func.lower(Mission.description_md).like(like),
func.lower(Mission.name).like(like, escape="\\"),
func.lower(Mission.description_md).like(like, escape="\\"),
)
stmt = stmt.where(cond)
count_stmt = count_stmt.where(cond)

55
backend/tests/test_missions.py
View File

@@ -368,6 +368,32 @@ def test_create_mission_rejects_unknown_scenario(client, admin_token):
assert r.get_json()["error"] == "unknown_scenario_template"
def test_create_mission_rejects_soft_deleted_scenario(client, admin_token):
# Build a scenario, soft-delete it, then try to snapshot — must 400 with
# `unknown_scenario_template` so we don't silently freeze a tombstoned
# template into a new mission.
t = _make_test_template(client, admin_token, name="sd-rejection-t")
sc = client.post(
"/api/v1/scenario-templates",
headers=_bearer(admin_token),
json={"name": "sd-rejection-sc", "test_template_ids": [t["id"]]},
).get_json()
del_r = client.delete(
f"/api/v1/scenario-templates/{sc['id']}", headers=_bearer(admin_token)
)
assert del_r.status_code == 200
r = client.post(
"/api/v1/missions",
headers=_bearer(admin_token),
json={
"name": "sd-rejection-mission",
"scenario_template_ids": [sc["id"]],
},
)
assert r.status_code == 400
assert r.get_json()["error"] == "unknown_scenario_template"
def test_create_mission_validates_dates(client, admin_token):
r = client.post(
"/api/v1/missions",
@@ -459,6 +485,35 @@ def test_list_requires_mission_read_perm(client, noperm_user):
assert r.status_code == 403
def test_transition_perm_gate_runs_before_payload_parse(client, blue_user):
"""A user without `mission.update` or `mission.archive` should see 403,
not 400, even when posting a malformed body — otherwise the endpoint's
shape leaks via the validation error message."""
# blue_user only has mission.read + mission.write_blue_fields, so neither
# mission.update nor mission.archive is held.
r = client.post(
"/api/v1/missions/00000000-0000-0000-0000-000000000000/transition",
headers=_bearer(blue_user["token"]),
json={"status": "garbage-not-a-valid-shape"},
)
assert r.status_code == 403
def test_search_treats_wildcards_as_literals(client, admin_token):
"""User-typed `%` and `_` must NOT act as SQL LIKE wildcards."""
client.post(
"/api/v1/missions",
headers=_bearer(admin_token),
json={"name": "no-wildcards-here"},
)
# Without escaping, `?q=%` would match every mission. With escaping, it
# only matches names that literally contain `%`.
r = client.get("/api/v1/missions?q=%25", headers=_bearer(admin_token))
assert r.status_code == 200
names = [it["name"] for it in r.get_json()["items"]]
assert "no-wildcards-here" not in names
def test_archive_requires_mission_archive_not_just_update(client, admin_token):
"""A user with mission.update but no mission.archive cannot archive."""
# blue_user only has mission.read + mission.write_blue_fields — no update either.