diff --git a/CHANGELOG.md b/CHANGELOG.md index f53477f..f645e49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to this project will be documented here. Format: [Keep a Cha ## [Unreleased] +### Fixed (post-M6 review pass — spec-reviewer + code-reviewer) +- **SPA cache invalidation only refreshed the empty-filter list** (`frontend/src/lib/missions.ts:136`): `missionKeys.list()` returns `['missions','list',{}]`. TanStack v5's `invalidateQueries({queryKey})` is prefix-based, but `{}` is treated as an atomic final element — so create / transition / delete called with that key only invalidated the *exact* empty-filter list, leaving any filtered variant stale until manual refetch. Added `missionKeys.listPrefix()` returning `['missions','list']` and switched all three mutation `onSuccess` paths to it. +- **Snapshot lacked the per-scenario advisory lock** (`backend/app/services/missions.py:467`): a concurrent `PUT /scenario-templates/{id}/tests` (M5 reorder, which deletes-then-reinserts join rows) running while `_snapshot_scenarios` walked `sc.tests` could freeze a torn snapshot — `selectinload` re-queries under READ COMMITTED so a partial view was possible. Added `_lock_scenario_ids_for_snapshot` that acquires the same `pg_advisory_xact_lock` key used by `set_scenario_tests` (blake2b digest of the scenario UUID, sorted to avoid deadlocks). Snapshot and reorder now serialise per scenario. +- **Transition endpoint leaked its body shape via 400 before the perm gate** (`backend/app/api/missions.py:441`): a user without `mission.update` or `mission.archive` POSTing `{"status":"x"}` got a Pydantic 400 instead of 403. Added `@require_perm("mission.update", "mission.archive")` so the gate fires before the parse; the inner refinement still enforces the per-target perm. Test `test_transition_perm_gate_runs_before_payload_parse`. +- **LIKE wildcards in user-typed search were honoured as SQL wildcards** (`backend/app/services/missions.py:632,637`): `?q=%` matched every mission. Added `_escape_like` that pre-escapes `%`, `_`, `\` and a matching `escape='\\'` argument on every `.like(...)` call. Test `test_search_treats_wildcards_as_literals`. +- **Counts ignored soft-deleted mission children** (`backend/app/services/missions.py:587,597`): `tests_count` and the detail view summed `len(sc.tests)` without filtering `MissionTest.deleted_at`. Harmless today (M6 doesn't soft-delete mission tests), but would drift silently once M7+ surfaces `state=skipped/blocked`. Added the filter in both `_to_list_item` and `_scenario_views`. +- **`/users/roster` was unordered** (`backend/app/api/users.py:73`): the wizard's member list shuffled rows on every refetch. Sorted by `email` for predictable rendering + stable e2e selectors. +- **Frontend transition button accent collapsed `in_progress` and `completed` into one colour** (`frontend/src/pages/MissionDetailPage.tsx:97`): both rendered cyan, so the status legend in the list didn't match the transition button. Added a `TRANSITION_BUTTON_ACCENT` map mirroring `MISSION_STATUS_ACCENT` (cyan/orange/green/teal). +- **Soft-deleted source scenario was a silent foot-gun**: `_load_scenario_templates_for_snapshot` already rejected it, but no test pinned the behaviour. Added `test_create_mission_rejects_soft_deleted_scenario` so future refactors can't regress to "freeze a tombstoned scenario into a fresh mission". +- **E2E wizard assertion used `getByRole('button', { name: /In Progress/i })`** (`e2e/tests/m6-missions.spec.ts:287`): the accessible name is `→ In Progress` and the arrow Unicode is brittle. Switched to `getByTestId('mission-transition-in_progress')`. + ### Added — M6 (Missions & snapshot) - **CRUD `missions`** (`app/services/missions.py` + `app/api/missions.py`): - Fields: name, client_target, date_start, date_end, status (`draft/in_progress/completed/archived`), description (markdown), visibility_mode (frozen to `whitebox` in v1). diff --git a/backend/app/api/missions.py b/backend/app/api/missions.py index 2d15383..3358b89 100644 --- a/backend/app/api/missions.py +++ b/backend/app/api/missions.py @@ -430,10 +430,14 @@ def set_members(mission_id: str): @bp.post("//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: diff --git a/backend/app/api/users.py b/backend/app/api/users.py index af8ef95..7efbfb2 100644 --- a/backend/app/api/users.py +++ b/backend/app/api/users.py @@ -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, + ) ] } ) diff --git a/backend/app/services/missions.py b/backend/app/services/missions.py index 28d904f..3ace038 100644 --- a/backend/app/services/missions.py +++ b/backend/app/services/missions.py @@ -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) diff --git a/backend/tests/test_missions.py b/backend/tests/test_missions.py index 587aee1..a45b87f 100644 --- a/backend/tests/test_missions.py +++ b/backend/tests/test_missions.py @@ -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. diff --git a/e2e/tests/m6-missions.spec.ts b/e2e/tests/m6-missions.spec.ts index 8aadb31..c6cbc1e 100644 --- a/e2e/tests/m6-missions.spec.ts +++ b/e2e/tests/m6-missions.spec.ts @@ -284,7 +284,7 @@ test.describe('M6 — Missions', () => { // Should land on the detail page await expect(page).toHaveURL(/\/missions\/[0-9a-f-]+$/); - await expect(page.getByRole('button', { name: /In Progress/i })).toBeVisible(); + await expect(page.getByTestId('mission-transition-in_progress')).toBeVisible(); await expect(page.getByTestId('mission-tab-tests')).toBeVisible(); // Tests tab renders 3 snapshotted tests await expect(page.getByText('spa-wizard-t1')).toBeVisible(); diff --git a/frontend/src/lib/missions.ts b/frontend/src/lib/missions.ts index e47dd9c..6f3f95b 100644 --- a/frontend/src/lib/missions.ts +++ b/frontend/src/lib/missions.ts @@ -133,6 +133,11 @@ export interface TransitionPayload { } export const missionKeys = { + /** Prefix-only key — pass this to `invalidateQueries` to refresh every + * filtered variant. Matching is prefix-based: `['missions','list',{q:'x'}]` + * also gets invalidated. + */ + listPrefix: () => ['missions', 'list'] as const, list: (filters?: MissionFilters) => ['missions', 'list', filters ?? {}] as const, detail: (id: string) => ['missions', 'detail', id] as const, }; diff --git a/frontend/src/pages/MissionDetailPage.tsx b/frontend/src/pages/MissionDetailPage.tsx index 53aa146..30e3392 100644 --- a/frontend/src/pages/MissionDetailPage.tsx +++ b/frontend/src/pages/MissionDetailPage.tsx @@ -27,6 +27,13 @@ const ALLOWED_TRANSITIONS: Record = { archived: [], }; +const TRANSITION_BUTTON_ACCENT: Record = { + draft: 'cyan', + in_progress: 'orange', + completed: 'green', + archived: 'teal', +}; + function useMission(id: string) { return useQuery({ queryKey: missionKeys.detail(id), @@ -55,14 +62,14 @@ export function MissionDetailPage() { apiPost(`/missions/${missionId}/transition`, body), onSuccess: () => { qc.invalidateQueries({ queryKey: missionKeys.detail(missionId) }); - qc.invalidateQueries({ queryKey: missionKeys.list() }); + qc.invalidateQueries({ queryKey: missionKeys.listPrefix() }); }, }); const remove = useMutation({ mutationFn: () => apiDelete<{ ok: true }>(`/missions/${missionId}`), onSuccess: () => { - qc.invalidateQueries({ queryKey: missionKeys.list() }); + qc.invalidateQueries({ queryKey: missionKeys.listPrefix() }); navigate('/missions'); }, }); @@ -94,7 +101,7 @@ export function MissionDetailPage() { {allowedNext.map((target) => (